Skip to content

Commit 43ca9a0

Browse files
vincentkocBartok9
vincentkoc
authored and
Bartok9
committed
fix(plugins): isolate git clone staging per install
1 parent 1cf263a commit 43ca9a0

4 files changed

Lines changed: 259 additions & 173 deletions

File tree

‎src/infra/install-source-utils.ts‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -135,16 +135,7 @@ export type NpmIntegrityDrift = {
135135
actualIntegrity: string;
136136
};
137137

138-
/**
139-
* Runs a callback in a private temp directory and removes it afterward.
140-
*
141-
* By default the temp directory is created under `os.tmpdir()`. Callers that
142-
* later move the staged result into a persistent location with an atomic
143-
* `rename()` should pass `rootDir` pointing at (a directory on) the same
144-
* filesystem as that destination. Otherwise the rename can fail with `EXDEV`
145-
* ("cross-device link not permitted") when `/tmp` and the destination live on
146-
* different mounts — common in containers with bind-mounted state dirs.
147-
*/
138+
/** Runs a callback in a private temp directory and removes it afterward. */
148139
export async function withTempDir<T>(
149140
prefix: string,
150141
fn: (tmpDir: string) => Promise<T>,

‎src/plugins/git-install.test.ts‎

Lines changed: 88 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import os from "node:os";
55
import path from "node:path";
66
import { redactSensitiveUrlLikeString } from "@openclaw/net-policy/redact-sensitive-url";
77
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
8+
import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js";
89
import type { DiagnosticSecurityEvent } from "../infra/diagnostic-events.js";
910

1011
const runCommandWithTimeoutMock = vi.fn();
@@ -145,6 +146,7 @@ describe("isImmutableGitCommitRef", () => {
145146

146147
describe("installPluginFromGitSpec", () => {
147148
const tempDirs: string[] = [];
149+
const trackedTempDirs = useAutoCleanupTempDirTracker(afterEach);
148150

149151
beforeEach(async () => {
150152
runCommandWithTimeoutMock.mockReset();
@@ -486,8 +488,8 @@ describe("installPluginFromGitSpec", () => {
486488
}
487489
});
488490

489-
it("stages the clone under the managed git dir so replacement never crosses filesystems (#99885)", async () => {
490-
const gitDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-git-install-stage-"));
491+
it("stages the clone beside the managed repo so replacement stays on one filesystem (#99885)", async () => {
492+
const gitDir = trackedTempDirs.make("openclaw-git-install-stage-");
491493
try {
492494
runCommandWithTimeoutMock
493495
.mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" })
@@ -515,18 +517,97 @@ describe("installPluginFromGitSpec", () => {
515517
throw new Error(result.error);
516518
}
517519

518-
// The clone destination must live under the resolved managed git dir, not
519-
// os.tmpdir(). Staging elsewhere makes the final atomic rename into the
520-
// persistent repo cross filesystems and fail with EXDEV in containers.
521520
const cloneArgv = commandArgvAt(0);
522521
const cloneDest = cloneArgv[cloneArgv.length - 1];
523-
const resolvedGitDir = path.resolve(gitDir);
524-
expect(path.resolve(cloneDest).startsWith(resolvedGitDir)).toBe(true);
522+
const persistentRepoDir = expectedGitRepoDir({
523+
gitDir,
524+
normalizedSpec: "git:https://github.com/acme/demo.git",
525+
});
526+
const cloneParent = await fs.realpath(path.dirname(path.dirname(path.resolve(cloneDest))));
527+
const targetParent = await fs.realpath(path.dirname(persistentRepoDir));
528+
expect(cloneParent).toBe(targetParent);
525529
} finally {
526530
await fs.rm(gitDir, { recursive: true, force: true });
527531
}
528532
});
529533

534+
it("falls back to OS temp when target workspace creation fails", async () => {
535+
const gitDir = trackedTempDirs.make("openclaw-git-install-stage-fallback-");
536+
runCommandWithTimeoutMock
537+
.mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" })
538+
.mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" })
539+
.mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" });
540+
installPluginFromInstalledPackageDirMock.mockImplementation(
541+
async (params: { packageDir: string }) => {
542+
await fs.mkdir(params.packageDir, { recursive: true });
543+
return {
544+
ok: true,
545+
pluginId: "demo",
546+
targetDir: params.packageDir,
547+
version: "1.2.3",
548+
extensions: ["index.js"],
549+
};
550+
},
551+
);
552+
const mkdtempSpy = vi
553+
.spyOn(fs, "mkdtemp")
554+
.mockRejectedValueOnce(Object.assign(new Error("read-only filesystem"), { code: "EROFS" }));
555+
556+
try {
557+
const result = await installPluginFromGitSpec({
558+
spec: "git:github.com/acme/demo",
559+
gitDir,
560+
});
561+
562+
expect(result.ok).toBe(true);
563+
expect(mkdtempSpy).toHaveBeenCalledTimes(2);
564+
const targetPrefix = mkdtempSpy.mock.calls[0]?.[0];
565+
const fallbackPrefix = mkdtempSpy.mock.calls[1]?.[0];
566+
const persistentRepoDir = expectedGitRepoDir({
567+
gitDir,
568+
normalizedSpec: "git:https://github.com/acme/demo.git",
569+
});
570+
expect(path.dirname(targetPrefix)).toBe(await fs.realpath(path.dirname(persistentRepoDir)));
571+
expect(path.dirname(fallbackPrefix)).toBe(await fs.realpath(os.tmpdir()));
572+
expect(runCommandWithTimeoutMock).toHaveBeenCalledTimes(3);
573+
} finally {
574+
mkdtempSpy.mockRestore();
575+
}
576+
});
577+
578+
it("keeps dry-run clone staging out of managed state", async () => {
579+
const caseDir = trackedTempDirs.make("openclaw-git-dry-run-stage-");
580+
const gitDir = path.join(caseDir, "git");
581+
try {
582+
runCommandWithTimeoutMock
583+
.mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" })
584+
.mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" });
585+
installPluginFromInstalledPackageDirMock.mockImplementation(
586+
async (params: { packageDir: string }) => ({
587+
ok: true,
588+
pluginId: "demo",
589+
targetDir: params.packageDir,
590+
version: "1.2.3",
591+
extensions: ["index.js"],
592+
}),
593+
);
594+
595+
const result = await installPluginFromGitSpec({
596+
spec: "git:github.com/acme/demo",
597+
gitDir,
598+
dryRun: true,
599+
});
600+
601+
expect(result.ok).toBe(true);
602+
const cloneArgv = commandArgvAt(0);
603+
const cloneDest = path.resolve(cloneArgv[cloneArgv.length - 1]);
604+
expect(path.relative(gitDir, cloneDest).split(path.sep)[0]).toBe("..");
605+
await expect(fs.access(gitDir)).rejects.toThrow();
606+
} finally {
607+
await fs.rm(caseDir, { recursive: true, force: true });
608+
}
609+
});
610+
530611
it("uses a credential-free managed repo path for authenticated git URLs", async () => {
531612
const gitDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-git-install-path-"));
532613
try {

0 commit comments

Comments
 (0)