From edf4aca7bc275541e226fc5c8725d93bfb2f42be Mon Sep 17 00:00:00 2001 From: Yuval Dinodia <102706514+yetval@users.noreply.github.com> Date: Wed, 29 Jul 2026 08:17:54 -0400 Subject: [PATCH] fix(agents): apply_patch destroys an existing file when a patch creates that path (#114911) * fix(agents): stop apply_patch from silently overwriting existing files An "*** Add File:" hunk wrote its target unconditionally. When the path already existed, apply_patch replaced the entire file, returned Success, and listed the path under "added", so neither the model nor the UI got any signal that existing content had been destroyed. The "*** Move to:" destination of an update hunk had the same gap and reported the clobbered path as merely modified. The add and move-to branches now check the destination through the patch file ops before writing and fail closed when it exists. Routing the check through fileOps keeps it correct on all three backends (workspace-scoped fs-safe root, raw fs, sandbox bridge). The check runs per hunk in patch order, so deleting a path earlier in the same patch and recreating it still works. * fix(agents): make apply_patch destination creation atomic The previous guard checked that an add or move-to destination was absent and then wrote it. A competing writer could create the path in that gap, after which the write still replaced it, so the no-clobber guarantee did not hold under contention. Destination creation now goes through a single exclusive create-if-absent operation on every patch backend: Root.create for the workspace-scoped default, an O_EXCL write for the raw filesystem, and a new pinned create operation in the sandbox mutation helper that opens the target with O_CREAT|O_EXCL and reports a reserved exit code when it already exists. PatchFileOps drops its separate existence check. Resolving the host ops behind an early return removes the repeated workspaceOnly branch inside each operation and the optional-call dance that let a missing root silently skip a write. * fix(agents): complete atomic apply-patch creation * fix(agents): preserve raced create replacements * fix(agents): handle fs-safe patch collisions * fix(agents): publish sandbox creates atomically * test(agents): cover exclusive create provenance rollback * fix(agents): use typed exclusive-create signal --------- Co-authored-by: Vincent Koc --- docs/tools/apply-patch.md | 1 + extensions/mxc/src/fs-bridge.ts | 29 ++- extensions/mxc/test/mxc-backend.test.ts | 19 ++ extensions/openshell/src/fs-bridge.ts | 37 +++ .../openshell/src/openshell-core.test.ts | 55 +++++ src/agents/agent-tools-agent-config.test.ts | 1 + ...tools.create-openclaw-coding-tools.test.ts | 17 +- src/agents/apply-patch-file-ops.ts | 205 +++++++++++++++++ src/agents/apply-patch.test.ts | 210 +++++++++++++++++- src/agents/apply-patch.ts | 138 ++---------- .../sandbox/fs-bridge-mutation-helper.test.ts | 208 +++++++++++++++++ .../sandbox/fs-bridge-mutation-helper.ts | 51 +++++ .../fs-bridge-native-mutation-python.ts | 137 ++++++++++++ .../sandbox/fs-bridge.anchored-ops.test.ts | 12 + src/agents/sandbox/fs-bridge.ts | 45 ++++ src/agents/sandbox/fs-bridge.types.ts | 13 ++ src/agents/sandbox/remote-fs-bridge.test.ts | 68 ++++++ src/agents/sandbox/remote-fs-bridge.ts | 47 +++- .../test-helpers/host-sandbox-fs-bridge.ts | 19 ++ 19 files changed, 1192 insertions(+), 120 deletions(-) create mode 100644 src/agents/apply-patch-file-ops.ts create mode 100644 src/agents/sandbox/fs-bridge-native-mutation-python.ts diff --git a/docs/tools/apply-patch.md b/docs/tools/apply-patch.md index 0ecab50067dd..f790c1e70681 100644 --- a/docs/tools/apply-patch.md +++ b/docs/tools/apply-patch.md @@ -32,6 +32,7 @@ The tool accepts a single `input` string that wraps one or more file operations: - Patch paths support relative paths (from the workspace directory) and absolute paths. - `tools.exec.applyPatch.workspaceOnly` defaults to `true` (workspace-contained). Set it to `false` only if you intentionally want `apply_patch` to write/delete outside the workspace directory. +- `*** Add File:` and a non-self `*** Move to:` require the destination path to be absent. To intentionally replace a path, delete it earlier in the same patch before adding or moving the replacement. - Use `*** Move to:` within an `*** Update File:` hunk to rename files. - `*** End of File` marks an EOF-only insert when needed. - Enabled by default for every model. Set `tools.exec.applyPatch.enabled: false` diff --git a/extensions/mxc/src/fs-bridge.ts b/extensions/mxc/src/fs-bridge.ts index eed8bfa83c23..68c5673d6567 100644 --- a/extensions/mxc/src/fs-bridge.ts +++ b/extensions/mxc/src/fs-bridge.ts @@ -7,7 +7,7 @@ import { type SandboxFsStat, type SandboxResolvedPath, } from "openclaw/plugin-sdk/sandbox"; -import { isPathInside } from "openclaw/plugin-sdk/security-runtime"; +import { FsSafeError, isPathInside } from "openclaw/plugin-sdk/security-runtime"; import { resolveMxcReadOnlySkillMounts, type MxcReadOnlySkillMount, @@ -86,6 +86,33 @@ class MxcFsBridge implements SandboxFsBridge { }); } + async createFileExclusive(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + }): Promise<"created" | "exists"> { + const target = this.resolveTarget(params); + this.ensureWritable(target, "create files"); + const buffer = Buffer.isBuffer(params.data) + ? params.data + : Buffer.from(params.data, params.encoding ?? "utf8"); + try { + await ( + await fsRoot(target.mount.hostRoot) + ).create(target.mountRelativePath, buffer, { + mkdir: params.mkdir !== false, + }); + return "created"; + } catch (error) { + if (error instanceof FsSafeError && error.code === "already-exists") { + return "exists"; + } + throw error; + } + } + async mkdirp(params: { filePath: string; cwd?: string }): Promise { const target = this.resolveTarget(params); this.ensureWritable(target, "create directories"); diff --git a/extensions/mxc/test/mxc-backend.test.ts b/extensions/mxc/test/mxc-backend.test.ts index 7dacdb3b6549..3c7f63898b88 100644 --- a/extensions/mxc/test/mxc-backend.test.ts +++ b/extensions/mxc/test/mxc-backend.test.ts @@ -737,6 +737,25 @@ describeOnWindows("createMxcSandboxBackendHandle (Windows-only MXC backend tests }); expect(bridge).toBeDefined(); + const createFileExclusive = bridge?.createFileExclusive?.bind(bridge); + expect(createFileExclusive).toBeTypeOf("function"); + await expect( + createFileExclusive!({ filePath: "notes/exclusive.txt", data: "first", cwd: workdir }), + ).resolves.toBe("created"); + await expect( + createFileExclusive!({ + filePath: "notes/exclusive.txt", + data: "replacement", + cwd: workdir, + }), + ).resolves.toBe("exists"); + expect(readFileSync(path.join(workdir, "notes", "exclusive.txt"), "utf-8")).toBe("first"); + const raceOutcomes = await Promise.all([ + createFileExclusive!({ filePath: "notes/race.txt", data: "one", cwd: workdir }), + createFileExclusive!({ filePath: "notes/race.txt", data: "two", cwd: workdir }), + ]); + expect(raceOutcomes.toSorted()).toEqual(["created", "exists"]); + await bridge?.writeFile({ filePath: "notes/one.txt", data: "hello mxc", cwd: workdir }); expect(await bridge?.readFile({ filePath: "notes/one.txt", cwd: workdir })).toEqual( Buffer.from("hello mxc"), diff --git a/extensions/openshell/src/fs-bridge.ts b/extensions/openshell/src/fs-bridge.ts index 4764cc584a11..405dc3e47d56 100644 --- a/extensions/openshell/src/fs-bridge.ts +++ b/extensions/openshell/src/fs-bridge.ts @@ -117,6 +117,43 @@ class OpenShellFsBridge implements SandboxFsBridge { await this.backend.syncLocalPathToRemote(hostPath, target.containerPath); } + async createFileExclusive(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise<"created" | "exists"> { + const target = this.resolveTarget(params); + const hostPath = this.requireHostPath(target); + this.ensureWritable(target, "create files"); + await assertLocalPathSafety({ + target, + root: target.mountHostRoot, + allowMissingLeaf: true, + allowFinalSymlinkForUnlink: false, + }); + const buffer = Buffer.isBuffer(params.data) + ? params.data + : Buffer.from(params.data, params.encoding ?? "utf8"); + const root = await fsRoot(target.mountHostRoot); + try { + await root.create(path.relative(target.mountHostRoot, hostPath), buffer, { + mkdir: params.mkdir !== false, + }); + } catch (error) { + if (error instanceof FsSafeError && error.code === "already-exists") { + return "exists"; + } + throw error; + } + // Mirror mode treats local state as canonical. Syncing may fail, but must + // never downgrade the exclusive local create to an overwriting write. + await this.backend.syncLocalPathToRemote(hostPath, target.containerPath); + return "created"; + } + async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveTarget(params); const hostPath = this.requireHostPath(target); diff --git a/extensions/openshell/src/openshell-core.test.ts b/extensions/openshell/src/openshell-core.test.ts index 0f5ec291c84e..970e2a07c81e 100644 --- a/extensions/openshell/src/openshell-core.test.ts +++ b/extensions/openshell/src/openshell-core.test.ts @@ -945,6 +945,61 @@ describe("openshell fs bridges", () => { ); }); + it("creates mirror files exclusively before syncing them", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const createFileExclusive = bridge.createFileExclusive?.bind(bridge); + expect(createFileExclusive).toBeTypeOf("function"); + + await expect( + createFileExclusive!({ filePath: "nested/file.txt", data: "first" }), + ).resolves.toBe("created"); + await expect( + createFileExclusive!({ filePath: "nested/file.txt", data: "replacement" }), + ).resolves.toBe("exists"); + await expect(fs.readFile(path.join(workspaceDir, "nested", "file.txt"), "utf8")).resolves.toBe( + "first", + ); + expect(backend["syncLocalPathToRemote"]).toHaveBeenCalledTimes(1); + }); + + it("keeps the canonical local exclusive create when mirror sync fails", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const backend = createMirrorBackendMock(); + backend["syncLocalPathToRemote"] = vi.fn().mockRejectedValue(new Error("remote rejected")); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const createFileExclusive = bridge.createFileExclusive?.bind(bridge); + expect(createFileExclusive).toBeTypeOf("function"); + + await expect(createFileExclusive!({ filePath: "file.txt", data: "canonical" })).rejects.toThrow( + "remote rejected", + ); + await expect(fs.readFile(path.join(workspaceDir, "file.txt"), "utf8")).resolves.toBe( + "canonical", + ); + }); + it("creates remote mirror directories through the pinned backend operation", async () => { const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); const backend = createMirrorBackendMock(); diff --git a/src/agents/agent-tools-agent-config.test.ts b/src/agents/agent-tools-agent-config.test.ts index cd905a827f06..0d689eedbae5 100644 --- a/src/agents/agent-tools-agent-config.test.ts +++ b/src/agents/agent-tools-agent-config.test.ts @@ -37,6 +37,7 @@ describe("Agent-specific tool filtering", () => { }), readFile: async () => Buffer.from(""), writeFile: async () => {}, + createFileExclusive: async () => "created", mkdirp: async () => {}, remove: async () => {}, rename: async () => {}, diff --git a/src/agents/agent-tools.create-openclaw-coding-tools.test.ts b/src/agents/agent-tools.create-openclaw-coding-tools.test.ts index fd136f871593..c1a000bf7b92 100644 --- a/src/agents/agent-tools.create-openclaw-coding-tools.test.ts +++ b/src/agents/agent-tools.create-openclaw-coding-tools.test.ts @@ -2022,8 +2022,9 @@ describe("createOpenClawCodingTools", () => { it("records ordinary write, edit, and apply_patch memory provenance from turn taint", async () => { const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-memory-write-taint-")); + const rollback = vi.fn(async () => {}); const recordWriteProvenance = vi.fn>( - async () => {}, + async () => rollback, ); registerMemoryCapability("memory-core", { flushPlanResolver: () => ({ @@ -2078,6 +2079,20 @@ describe("createOpenClawCodingTools", () => { contentAfter: "network project note\n", }), ); + await expect( + applyPatch("patch-existing-memory", { + input: [ + "*** Begin Patch", + "*** Add File: memory/project.md", + "+replacement", + "*** End Patch", + ].join("\n"), + }), + ).rejects.toThrow(/file already exists/i); + expect(rollback).toHaveBeenCalledOnce(); + await expect(fs.readFile(path.join(workspaceDir, "memory/project.md"), "utf8")).resolves.toBe( + "network project note\n", + ); } finally { await fs.rm(workspaceDir, { recursive: true, force: true }); } diff --git a/src/agents/apply-patch-file-ops.ts b/src/agents/apply-patch-file-ops.ts new file mode 100644 index 000000000000..33dbdd453cec --- /dev/null +++ b/src/agents/apply-patch-file-ops.ts @@ -0,0 +1,205 @@ +import syncFs from "node:fs"; +import fs from "node:fs/promises"; +import { openRootFile, type RootFileOpenResult } from "../infra/boundary-file-read.js"; +import { FsSafeError, root as fsRoot } from "../infra/fs-safe.js"; +import { + type MemoryWriteProvenanceObserver, + withMemoryWriteProvenance, +} from "./memory-write-provenance.js"; +import { toRelativeSandboxPath } from "./path-policy.js"; +import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; +import { decodeUtf8File } from "./utf8-file.js"; + +export type SandboxApplyPatchConfig = { + root: string; + bridge: SandboxFsBridge; +}; + +export type ApplyPatchFileOptions = { + cwd: string; + sandbox?: SandboxApplyPatchConfig; + /** Restrict patch paths to the workspace root (cwd). Default: true. Set false to opt out. */ + workspaceOnly?: boolean; + memoryWriteProvenance?: MemoryWriteProvenanceObserver; +}; + +type PatchCreateOutcome = "created" | "exists"; + +export type PatchFileOps = { + readFile: (filePath: string) => Promise; + writeFile: (filePath: string, content: string) => Promise; + createFileExclusive: (filePath: string, content: string) => Promise; + remove: (filePath: string) => Promise; + mkdirp: (dir: string) => Promise; +}; + +export async function createPatchTarget(params: { + target: { resolved: string; display: string }; + contents: string; + ops: PatchFileOps; + hint: string; +}) { + const outcome = await params.ops.createFileExclusive(params.target.resolved, params.contents); + if (outcome === "exists") { + throw new Error( + `Cannot create ${params.target.display}: the file already exists. ${params.hint}`, + ); + } +} + +export function resolvePatchFileOps(options: ApplyPatchFileOptions): PatchFileOps { + if (options.sandbox) { + const { root, bridge } = options.sandbox; + return withPatchMemoryWriteProvenance({ + observer: options.memoryWriteProvenance, + operations: { + readFile: async (filePath) => { + const buf = await bridge.readFile({ filePath, cwd: root }); + return decodeUtf8File(buf, filePath); + }, + writeFile: (filePath, content) => bridge.writeFile({ filePath, cwd: root, data: content }), + createFileExclusive: (filePath, content) => { + if (!bridge.createFileExclusive) { + throw new Error( + "Sandbox filesystem bridge does not support atomic file creation; refusing to overwrite an existing path.", + ); + } + return bridge.createFileExclusive({ filePath, cwd: root, data: content }); + }, + remove: (filePath) => bridge.remove({ filePath, cwd: root, force: false }), + mkdirp: (dir) => bridge.mkdirp({ filePath: dir, cwd: root }), + }, + }); + } + + if (options.workspaceOnly === false) { + return withPatchMemoryWriteProvenance({ + observer: options.memoryWriteProvenance, + operations: { + readFile: async (filePath) => decodeUtf8File(await fs.readFile(filePath), filePath), + writeFile: async (filePath, content) => { + await fs.writeFile(filePath, content, "utf8"); + }, + createFileExclusive: async (filePath, content) => { + try { + await fs.writeFile(filePath, content, { encoding: "utf8", flag: "wx" }); + return "created"; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") { + return "exists"; + } + throw error; + } + }, + remove: (filePath) => fs.rm(filePath), + mkdirp: async (dir) => { + await fs.mkdir(dir, { recursive: true }); + }, + }, + }); + } + + const rootPromise = fsRoot(options.cwd); + return withPatchMemoryWriteProvenance({ + observer: options.memoryWriteProvenance, + operations: { + readFile: async (filePath) => { + const opened = await openRootFile({ + absolutePath: filePath, + rootPath: options.cwd, + boundaryLabel: "workspace root", + }); + assertBoundaryRead(opened, filePath); + try { + return decodeUtf8File(syncFs.readFileSync(opened.fd), filePath); + } finally { + syncFs.closeSync(opened.fd); + } + }, + writeFile: async (filePath, content) => { + const relative = toRelativeSandboxPath(options.cwd, filePath); + await (await rootPromise).write(relative, content, { encoding: "utf8" }); + }, + createFileExclusive: async (filePath, content) => { + const relative = toRelativeSandboxPath(options.cwd, filePath); + try { + await (await rootPromise).create(relative, content, { encoding: "utf8" }); + return "created"; + } catch (error) { + // fs-safe opens an existing destination before its O_EXCL commit. A final + // symlink is rejected during that probe, but for create semantics it is + // still an occupied destination and must fail closed. + if ( + error instanceof FsSafeError && + (error.code === "already-exists" || error.code === "symlink") + ) { + return "exists"; + } + throw error; + } + }, + remove: async (filePath) => { + const relative = toRelativeSandboxPath(options.cwd, filePath); + await (await rootPromise).remove(relative); + }, + mkdirp: async (dir) => { + const relative = toRelativeSandboxPath(options.cwd, dir, { allowRoot: true }); + const root = await rootPromise; + if (relative === "" || relative === ".") { + await root.ensureRoot(); + return; + } + await root.mkdir(relative); + }, + }, + }); +} + +class PatchCreateExistsSignal extends Error {} + +function withPatchMemoryWriteProvenance(params: { + operations: PatchFileOps; + observer: MemoryWriteProvenanceObserver | undefined; +}): PatchFileOps { + const operations = withMemoryWriteProvenance(params.operations, params.observer); + if (!params.observer) { + return operations; + } + return { + ...operations, + createFileExclusive: async (filePath, content) => { + if (!params.observer?.classifies(filePath)) { + return params.operations.createFileExclusive(filePath, content); + } + try { + await params.observer.write({ + absolutePath: filePath, + contentBefore: "", + contentAfter: content, + commit: async () => { + if ((await params.operations.createFileExclusive(filePath, content)) === "exists") { + throw new PatchCreateExistsSignal(); + } + }, + }); + return "created"; + } catch (error) { + if (error instanceof PatchCreateExistsSignal) { + return "exists"; + } + throw error; + } + }, + }; +} + +function assertBoundaryRead( + opened: RootFileOpenResult, + targetPath: string, +): asserts opened is Extract { + if (opened.ok) { + return; + } + const reason = opened.reason === "validation" ? "unsafe path" : "path not found"; + throw new Error(`Failed boundary read for ${targetPath} (${reason})`); +} diff --git a/src/agents/apply-patch.test.ts b/src/agents/apply-patch.test.ts index 4ac8d6a1623e..36c31cbdeafc 100644 --- a/src/agents/apply-patch.test.ts +++ b/src/agents/apply-patch.test.ts @@ -40,13 +40,24 @@ function buildAddFilePatch(targetPath: string): string { *** End Patch`; } -function createMemoryPatchSandbox(initialFiles: Record = {}) { +function createMemoryPatchSandbox( + initialFiles: Record = {}, + options: { supportsExclusiveCreate?: boolean } = {}, +) { const files = new Map( Object.entries(initialFiles).map(([filePath, contents]) => [`/sandbox/${filePath}`, contents]), ); const writeFile = vi.fn(async ({ filePath, data }) => { files.set(filePath, Buffer.isBuffer(data) ? Buffer.from(data) : data); }); + const createFileExclusive = vi.fn(async ({ filePath, data }) => { + if (files.has(filePath)) { + return "exists" as const; + } + files.set(filePath, Buffer.isBuffer(data) ? Buffer.from(data) : data); + return "created" as const; + }); + const mkdirp = vi.fn(async () => {}); const bridge: SandboxFsBridge = { resolvePath: ({ filePath }) => ({ relativePath: filePath, @@ -59,6 +70,7 @@ function createMemoryPatchSandbox(initialFiles: Record : Buffer.from(contents ?? ""); }, writeFile, + ...(options.supportsExclusiveCreate === false ? {} : { createFileExclusive }), remove: async ({ filePath }) => { files.delete(filePath); }, @@ -75,12 +87,14 @@ function createMemoryPatchSandbox(initialFiles: Record ? null : { type: "file", size: Buffer.byteLength(contents), mtimeMs: 0 }; }, - mkdirp: async () => {}, + mkdirp, }; return { files, bridge, writeFile, + createFileExclusive, + mkdirp, options: { cwd: "/local/workspace", sandbox: { @@ -187,6 +201,198 @@ describe("applyPatch", () => { expect(result.summary.added).toEqual(["hello.txt"]); }); + it("rejects an add hunk that targets an existing file", async () => { + const memory = createMemoryPatchSandbox({ "notes.txt": "keep me\n" }); + const patch = `*** Begin Patch +*** Add File: notes.txt ++replacement +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).rejects.toThrow( + /Cannot create notes\.txt: the file already exists/, + ); + expect(memory.files.get("/sandbox/notes.txt")).toBe("keep me\n"); + expect(memory.writeFile.mock.calls).toHaveLength(0); + }); + + it.each([ + { name: "workspace-confined host", workspaceOnly: true }, + { name: "unconfined host", workspaceOnly: false }, + ])( + "keeps existing contents in $name when an add hunk targets them", + async ({ workspaceOnly }) => { + await withWorkspaceTempDir(async (dir) => { + const target = path.join(dir, "notes.txt"); + await fs.writeFile(target, "IMPORTANT USER DATA\nsecond line\n", "utf8"); + const tool = createApplyPatchTool({ cwd: dir, workspaceOnly }); + const patch = `*** Begin Patch +*** Add File: notes.txt ++replacement +*** End Patch`; + + await expect( + tool.execute("call-add-existing", { input: patch }, undefined), + ).rejects.toThrow(/Cannot create notes\.txt: the file already exists/); + expect(await fs.readFile(target, "utf8")).toBe("IMPORTANT USER DATA\nsecond line\n"); + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "refuses existing symlinks in both host modes without changing their targets", + async () => { + for (const workspaceOnly of [true, false]) { + await withWorkspaceTempDir(async (dir) => { + const target = path.join(dir, "target.txt"); + const link = path.join(dir, "notes.txt"); + await fs.writeFile(target, "keep me\n", "utf8"); + await fs.symlink("target.txt", link); + const patch = `*** Begin Patch +*** Add File: notes.txt ++replacement +*** End Patch`; + + await expect(applyPatch(patch, { cwd: dir, workspaceOnly })).rejects.toThrow( + /Cannot create notes\.txt: the file already exists/, + ); + await expect(fs.readFile(target, "utf8")).resolves.toBe("keep me\n"); + await expect(fs.readlink(link)).resolves.toBe("target.txt"); + }); + } + }, + ); + + it("refuses an add hunk when a competing writer creates the target mid-patch", async () => { + const memory = createMemoryPatchSandbox(); + memory.mkdirp.mockImplementation(async () => { + memory.files.set("/sandbox/notes.txt", "written by another writer\n"); + }); + const patch = `*** Begin Patch +*** Add File: notes.txt ++replacement +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).rejects.toThrow( + /Cannot create notes\.txt: the file already exists/, + ); + expect(memory.files.get("/sandbox/notes.txt")).toBe("written by another writer\n"); + expect(memory.writeFile.mock.calls).toHaveLength(0); + }); + + it("refuses a move hunk when a competing writer creates the destination mid-patch", async () => { + const memory = createMemoryPatchSandbox({ "source.txt": "foo\nbar\n" }); + memory.mkdirp.mockImplementation(async () => { + memory.files.set("/sandbox/dest.txt", "written by another writer\n"); + }); + const patch = `*** Begin Patch +*** Update File: source.txt +*** Move to: dest.txt +@@ + foo +-bar ++baz +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).rejects.toThrow( + /Cannot create dest\.txt: the file already exists/, + ); + expect(memory.files.get("/sandbox/dest.txt")).toBe("written by another writer\n"); + expect(memory.files.get("/sandbox/source.txt")).toBe("foo\nbar\n"); + }); + + it("allows an add hunk after the same path is deleted in the patch", async () => { + const memory = createMemoryPatchSandbox({ "notes.txt": "old\n" }); + const patch = `*** Begin Patch +*** Delete File: notes.txt +*** Add File: notes.txt ++new +*** End Patch`; + + const result = await applyPatch(patch, memory.options); + + expect(memory.files.get("/sandbox/notes.txt")).toBe("new\n"); + expect(result.summary.added).toEqual(["notes.txt"]); + }); + + it("rejects a move hunk that targets an existing file", async () => { + const memory = createMemoryPatchSandbox({ + "source.txt": "foo\nbar\n", + "dest.txt": "keep me\n", + }); + const patch = `*** Begin Patch +*** Update File: source.txt +*** Move to: dest.txt +@@ + foo +-bar ++baz +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).rejects.toThrow( + /Cannot create dest\.txt: the file already exists/, + ); + expect(memory.files.get("/sandbox/dest.txt")).toBe("keep me\n"); + expect(memory.files.get("/sandbox/source.txt")).toBe("foo\nbar\n"); + }); + + it.each([ + { name: "workspace-confined host", workspaceOnly: true }, + { name: "unconfined host", workspaceOnly: false }, + ])( + "preserves source and destination when a move target exists in $name", + async ({ workspaceOnly }) => { + await withWorkspaceTempDir(async (dir) => { + await fs.writeFile(path.join(dir, "source.txt"), "foo\nbar\n", "utf8"); + await fs.writeFile(path.join(dir, "dest.txt"), "keep me\n", "utf8"); + const patch = `*** Begin Patch +*** Update File: source.txt +*** Move to: dest.txt +@@ + foo +-bar ++baz +*** End Patch`; + + await expect(applyPatch(patch, { cwd: dir, workspaceOnly })).rejects.toThrow( + /Cannot create dest\.txt: the file already exists/, + ); + await expect(fs.readFile(path.join(dir, "source.txt"), "utf8")).resolves.toBe("foo\nbar\n"); + await expect(fs.readFile(path.join(dir, "dest.txt"), "utf8")).resolves.toBe("keep me\n"); + }); + }, + ); + + it("fails closed on sandbox adds when atomic create is unavailable", async () => { + const memory = createMemoryPatchSandbox({}, { supportsExclusiveCreate: false }); + const patch = `*** Begin Patch +*** Add File: notes.txt ++new +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).rejects.toThrow( + /does not support atomic file creation/, + ); + expect(memory.files.has("/sandbox/notes.txt")).toBe(false); + }); + + it("still permits sandbox updates when atomic create is unavailable", async () => { + const memory = createMemoryPatchSandbox( + { "source.txt": "before\n" }, + { supportsExclusiveCreate: false }, + ); + const patch = `*** Begin Patch +*** Update File: source.txt +@@ +-before ++after +*** End Patch`; + + await expect(applyPatch(patch, memory.options)).resolves.toMatchObject({ + summary: { modified: ["source.txt"] }, + }); + expect(memory.files.get("/sandbox/source.txt")).toBe("after\n"); + }); + it("updates and moves a file", async () => { const memory = createMemoryPatchSandbox({ "source.txt": "foo\nbar\n", diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index a10e9b3b261e..f76d345e11f9 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -3,28 +3,27 @@ * Parses OpenAI-style patch envelopes and applies add/update/delete/move hunks * through guarded host or sandbox filesystem operations. */ -import syncFs from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { Type } from "typebox"; import { createAbortError } from "../infra/abort-signal.js"; -import { openRootFile, type RootFileOpenResult } from "../infra/boundary-file-read.js"; -import { root as fsRoot } from "../infra/fs-safe.js"; import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../infra/path-alias-guards.js"; -import { applyUpdateHunk } from "./apply-patch-update.js"; import { - type MemoryWriteProvenanceObserver, - withMemoryWriteProvenance, -} from "./memory-write-provenance.js"; -import { toRelativeSandboxPath, resolvePathFromInput } from "./path-policy.js"; + type ApplyPatchFileOptions, + createPatchTarget, + type PatchFileOps, + resolvePatchFileOps, + type SandboxApplyPatchConfig, +} from "./apply-patch-file-ops.js"; +import { applyUpdateHunk } from "./apply-patch-update.js"; +import type { MemoryWriteProvenanceObserver } from "./memory-write-provenance.js"; +import { resolvePathFromInput } from "./path-policy.js"; import type { AgentTool } from "./runtime/index.js"; import { assertSandboxPath } from "./sandbox-paths.js"; -import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; import { withFileMutationQueue, withFileMutationQueues, } from "./sessions/tools/file-mutation-queue.js"; -import { decodeUtf8File } from "./utf8-file.js"; const BEGIN_PATCH_MARKER = "*** Begin Patch"; const END_PATCH_MARKER = "*** End Patch"; @@ -87,17 +86,7 @@ function normalizeUpdateComparison(content: string): string { return `${normalized}\n`; } -type SandboxApplyPatchConfig = { - root: string; - bridge: SandboxFsBridge; -}; - -type ApplyPatchOptions = { - cwd: string; - sandbox?: SandboxApplyPatchConfig; - /** Restrict patch paths to the workspace root (cwd). Default: true. Set false to opt out. */ - workspaceOnly?: boolean; - memoryWriteProvenance?: MemoryWriteProvenanceObserver; +type ApplyPatchOptions = ApplyPatchFileOptions & { signal?: AbortSignal; }; @@ -197,7 +186,12 @@ async function applyPatch(input: string, options: ApplyPatchOptions): Promise { await assertPatchParentPath(hunk.path, options); await ensureDir(target.resolved, fileOps); - await fileOps.writeFile(target.resolved, hunk.contents); + await createPatchTarget({ + target, + contents: hunk.contents, + ops: fileOps, + hint: `Use "*** Update File: ${target.display}" to change it, or delete it earlier in the same patch.`, + }); }); recordSummary(summary, seen, "added", target.display); continue; @@ -224,20 +218,22 @@ async function applyPatch(input: string, options: ApplyPatchOptions): Promise Promise; - writeFile: (filePath: string, content: string) => Promise; - remove: (filePath: string) => Promise; - mkdirp: (dir: string) => Promise; -}; - -function resolvePatchFileOps(options: ApplyPatchOptions): PatchFileOps { - let operations: PatchFileOps; - if (options.sandbox) { - const { root, bridge } = options.sandbox; - operations = { - readFile: async (filePath) => { - const buf = await bridge.readFile({ filePath, cwd: root }); - return decodeUtf8File(buf, filePath); - }, - writeFile: (filePath, content) => bridge.writeFile({ filePath, cwd: root, data: content }), - remove: (filePath) => bridge.remove({ filePath, cwd: root, force: false }), - mkdirp: (dir) => bridge.mkdirp({ filePath: dir, cwd: root }), - }; - } else { - const workspaceOnly = options.workspaceOnly !== false; - const rootPromise = workspaceOnly ? fsRoot(options.cwd) : undefined; - operations = { - readFile: async (filePath) => { - if (!workspaceOnly) { - return decodeUtf8File(await fs.readFile(filePath), filePath); - } - const opened = await openRootFile({ - absolutePath: filePath, - rootPath: options.cwd, - boundaryLabel: "workspace root", - }); - assertBoundaryRead(opened, filePath); - try { - return decodeUtf8File(syncFs.readFileSync(opened.fd), filePath); - } finally { - syncFs.closeSync(opened.fd); - } - }, - writeFile: async (filePath, content) => { - if (!workspaceOnly) { - await fs.writeFile(filePath, content, "utf8"); - return; - } - const relative = toRelativeSandboxPath(options.cwd, filePath); - await (await rootPromise)?.write(relative, content, { encoding: "utf8" }); - }, - remove: async (filePath) => { - if (!workspaceOnly) { - await fs.rm(filePath); - return; - } - const relative = toRelativeSandboxPath(options.cwd, filePath); - await (await rootPromise)?.remove(relative); - }, - mkdirp: async (dir) => { - if (!workspaceOnly) { - await fs.mkdir(dir, { recursive: true }); - return; - } - const relative = toRelativeSandboxPath(options.cwd, dir, { allowRoot: true }); - const root = await rootPromise; - if (!root) { - return; - } - if (relative === "" || relative === ".") { - await root.ensureRoot(); - return; - } - await root.mkdir(relative); - }, - }; - } - return withMemoryWriteProvenance(operations, options.memoryWriteProvenance); -} - async function ensureDir(filePath: string, ops: PatchFileOps) { const parent = path.dirname(filePath); if (!parent || parent === ".") { @@ -477,17 +396,6 @@ async function resolvePatchPath( }; } -function assertBoundaryRead( - opened: RootFileOpenResult, - targetPath: string, -): asserts opened is Extract { - if (opened.ok) { - return; - } - const reason = opened.reason === "validation" ? "unsafe path" : "path not found"; - throw new Error(`Failed boundary read for ${targetPath} (${reason})`); -} - function toDisplayPath(resolved: string, cwd: string): string { const relative = path.relative(cwd, resolved); if (!relative || relative === "") { diff --git a/src/agents/sandbox/fs-bridge-mutation-helper.test.ts b/src/agents/sandbox/fs-bridge-mutation-helper.test.ts index 2c9c4e058e75..128f0b6aa5f7 100644 --- a/src/agents/sandbox/fs-bridge-mutation-helper.test.ts +++ b/src/agents/sandbox/fs-bridge-mutation-helper.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it } from "vitest"; import { withTempDir } from "../../test-helpers/temp-dir.js"; import { buildPinnedWritePlan, + SANDBOX_CREATE_EXISTS_EXIT_CODE, SANDBOX_PINNED_MUTATION_PYTHON, } from "./fs-bridge-mutation-helper.js"; @@ -76,6 +77,63 @@ const FORCED_COPY_FAILURE_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.repla " raise OSError(errno.ENOSPC, 'forced copy failure')\n copy_completed = True", ); +const FORCED_CREATE_FAILURE_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.replace( + " # exclusive create payload is durable before publication", + " raise OSError(errno.ENOSPC, 'forced create failure')\n # exclusive create payload is durable before publication", +); + +const FORCED_CREATE_FAILURE_WITH_REPLACEMENT_MUTATION_PYTHON = + SANDBOX_PINNED_MUTATION_PYTHON.replace( + " # Publish with a native atomic no-replace rename.", + [ + " replacement_fd = os.open(basename, WRITE_FLAGS, 0o600, dir_fd=parent_fd)", + " try:", + " os.write(replacement_fd, b'replacement')", + " finally:", + " os.close(replacement_fd)", + " # Publish with a native atomic no-replace rename.", + ].join("\n"), + ); + +const FORCED_CREATE_TEMP_SUBSTITUTION_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.replace( + " # Publish with a native atomic no-replace rename.", + [ + " os.unlink(temp_name, dir_fd=staging_fd)", + " replacement_fd = os.open(temp_name, WRITE_FLAGS, 0o600, dir_fd=staging_fd)", + " try:", + " os.write(replacement_fd, b'replacement')", + " finally:", + " os.close(replacement_fd)", + " # Publish with a native atomic no-replace rename.", + ].join("\n"), +); + +const FORCED_MISSING_RENAMEAT2_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.replace( + " is_linux = sys.platform.startswith('linux')", + " is_linux = True", +).replace(" rename_fn = getattr(libc, 'renameat2', None)", " rename_fn = None"); + +const FORCED_UNSUPPORTED_RENAMEAT2_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.replace( + " is_linux = sys.platform.startswith('linux')", + " is_linux = True", +).replace( + " rename_fn = getattr(libc, 'renameat2', None)", + [ + " class UnsupportedRename:", + " argtypes = None", + " restype = None", + " def __call__(self, *_args):", + " ctypes.set_errno(errno.ENOSYS)", + " return -1", + " rename_fn = UnsupportedRename()", + ].join("\n"), +); + +const FORCED_STAGING_OPEN_FAILURE_MUTATION_PYTHON = SANDBOX_PINNED_MUTATION_PYTHON.replace( + " staging_fd = open_dir(candidate, dir_fd=parent_fd)", + " raise OSError(errno.EMFILE, 'forced staging open failure')\n staging_fd = open_dir(candidate, dir_fd=parent_fd)", +); + const FORCED_EXDEV_WITH_LATE_SOURCE_WRITE_MUTATION_PYTHON = FORCED_EXDEV_MUTATION_PYTHON.replace( " remove_copied_entry(src_parent_fd, src_basename, ('dir', entry_identity(src_stat), copied_children))", [ @@ -128,6 +186,156 @@ describe("sandbox pinned mutation helper", () => { }); }); + it("creates a new file through a pinned directory fd", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutation(["create", workspace, "nested", "note.txt", "1"], "hello"); + + expect(result.status).toBe(0); + await expect(fs.readFile(path.join(workspace, "nested", "note.txt"), "utf8")).resolves.toBe( + "hello", + ); + }); + }); + + it("creates a file whose basename approaches the filesystem component limit", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const basename = "n".repeat(240); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutation(["create", workspace, "", basename, "0"], "hello"); + + expect(result.status).toBe(0); + await expect(fs.readFile(path.join(workspace, basename), "utf8")).resolves.toBe("hello"); + }); + }); + + it("falls back when Linux libc does not export renameat2", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_MISSING_RENAMEAT2_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "hello", + ); + + expect(result.status).toBe(0); + await expect(fs.readFile(filePath, "utf8")).resolves.toBe("hello"); + await expect(fs.readdir(workspace)).resolves.toStrictEqual(["note.txt"]); + }); + }); + + it("falls back when Linux renameat2 is unsupported at runtime", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_UNSUPPORTED_RENAMEAT2_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "hello", + ); + + expect(result.status).toBe(0); + await expect(fs.readFile(filePath, "utf8")).resolves.toBe("hello"); + await expect(fs.readdir(workspace)).resolves.toStrictEqual(["note.txt"]); + }); + }); + + it("removes a staging directory when opening it fails", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_STAGING_OPEN_FAILURE_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "hello", + ); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("forced staging open failure"); + await expect(fs.readdir(workspace)).resolves.toStrictEqual([]); + }); + }); + + it("refuses to create over an existing file and leaves it untouched", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + await fs.writeFile(filePath, "keep me", "utf8"); + + const result = runMutation(["create", workspace, "", "note.txt", "0"], "replacement"); + + expect(result.status).toBe(SANDBOX_CREATE_EXISTS_EXIT_CODE); + await expect(fs.readFile(filePath, "utf8")).resolves.toBe("keep me"); + }); + }); + + it("removes private staging when writing fails before publication", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_CREATE_FAILURE_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "partial", + ); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("forced create failure"); + await expectPathMissing(filePath); + await expect(fs.readdir(workspace)).resolves.toStrictEqual([]); + }); + }); + + it("preserves a destination raced into a staged exclusive create", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_CREATE_FAILURE_WITH_REPLACEMENT_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "partial", + ); + + expect(result.status).toBe(SANDBOX_CREATE_EXISTS_EXIT_CODE); + await expect(fs.readFile(filePath, "utf8")).resolves.toBe("replacement"); + await expect(fs.readdir(workspace)).resolves.toStrictEqual(["note.txt"]); + }); + }); + + it("fails when the staged exclusive-create pathname is substituted", async () => { + await withTempDir({ prefix: "openclaw-mutation-helper-" }, async (root) => { + const workspace = path.join(root, "workspace"); + const filePath = path.join(workspace, "note.txt"); + await fs.mkdir(workspace, { recursive: true }); + + const result = runMutationWithSource( + FORCED_CREATE_TEMP_SUBSTITUTION_MUTATION_PYTHON, + ["create", workspace, "", "note.txt", "0"], + "expected", + ); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("exclusive publication source changed"); + await expect(fs.readFile(filePath, "utf8")).resolves.toBe("replacement"); + await expect(fs.readdir(workspace)).resolves.toStrictEqual(["note.txt"]); + }); + }); + it.runIf(process.platform !== "win32")( "preserves existing target file mode while writing", async () => { diff --git a/src/agents/sandbox/fs-bridge-mutation-helper.ts b/src/agents/sandbox/fs-bridge-mutation-helper.ts index 9aefcec53c48..6a717a4f34ea 100644 --- a/src/agents/sandbox/fs-bridge-mutation-helper.ts +++ b/src/agents/sandbox/fs-bridge-mutation-helper.ts @@ -4,6 +4,11 @@ * Performs symlink-resistant create/replace/delete operations inside a previously validated sandbox boundary. */ import { PATH_ALIAS_POLICIES } from "../../infra/path-alias-guards.js"; +import { + SANDBOX_CREATE_EXCLUSIVE_PYTHON, + SANDBOX_CREATE_STAGING_PYTHON, + SANDBOX_RENAME_NO_REPLACE_PYTHON, +} from "./fs-bridge-native-mutation-python.js"; import type { PathSafetyCheck, PinnedSandboxDirectoryEntry, @@ -18,7 +23,14 @@ const SANDBOX_PINNED_MUTATION_PYTHON_CANDIDATES = [ "/bin/python3", ] as const; +// Exit code the pinned helper reserves for "create target already exists" so +// callers can tell a lost exclusive-create race from a real failure. Any other +// nonzero exit stays an error. +export const SANDBOX_CREATE_EXISTS_EXIT_CODE = 17; + export const SANDBOX_PINNED_MUTATION_PYTHON = [ + `SANDBOX_CREATE_EXISTS_EXIT_CODE = ${SANDBOX_CREATE_EXISTS_EXIT_CODE}`, + "import ctypes", "import errno", "import os", "import secrets", @@ -94,6 +106,8 @@ export const SANDBOX_PINNED_MUTATION_PYTHON = [ " continue", " raise RuntimeError('failed to allocate sandbox temp directory')", "", + SANDBOX_CREATE_STAGING_PYTHON, + "", "def existing_regular_file_mode(parent_fd, basename):", " try:", " target_stat = os.lstat(basename, dir_fd=parent_fd)", @@ -111,6 +125,8 @@ export const SANDBOX_PINNED_MUTATION_PYTHON = [ " raise OSError(errno.EIO, 'failed to write copied file')", " view = view[written:]", "", + SANDBOX_RENAME_NO_REPLACE_PYTHON, + "", "def copy_regular_file(src_parent_fd, src_basename, dst_parent_fd, dst_basename):", " src_fd = os.open(src_basename, READ_FLAGS, dir_fd=src_parent_fd)", " dst_fd = None", @@ -201,6 +217,8 @@ export const SANDBOX_PINNED_MUTATION_PYTHON = [ " except FileNotFoundError:", " pass", "", + SANDBOX_CREATE_EXCLUSIVE_PYTHON, + "", "def read_file_impl(parent_fd, basename, max_bytes):", " file_fd = os.open(basename, READ_FLAGS, dir_fd=parent_fd)", " try:", @@ -255,6 +273,9 @@ export const SANDBOX_PINNED_MUTATION_PYTHON = [ " getattr(entry_stat, 'st_ctime_ns', int(entry_stat.st_ctime * 1000000000)),", " )", "", + "def inode_identity(entry_stat):", + " return (entry_stat.st_dev, entry_stat.st_ino)", + "", "def same_identity(expected, entry_stat):", " return expected == entry_identity(entry_stat)", "", @@ -425,6 +446,19 @@ export const SANDBOX_PINNED_MUTATION_PYTHON = [ " if parent_fd is not None:", " os.close(parent_fd)", " os.close(root_fd)", + "elif operation == 'create':", + " root_fd = open_dir(sys.argv[2])", + " parent_fd = None", + " try:", + " parent_fd = walk_dir(root_fd, sys.argv[3], sys.argv[5] == '1')", + " try:", + " create_exclusive(parent_fd, sys.argv[4], sys.stdin.buffer)", + " except FileExistsError:", + " sys.exit(SANDBOX_CREATE_EXISTS_EXIT_CODE)", + " finally:", + " if parent_fd is not None:", + " os.close(parent_fd)", + " os.close(root_fd)", "elif operation == 'read':", " root_fd = open_dir(sys.argv[2])", " parent_fd = None", @@ -536,6 +570,23 @@ export function buildPinnedWritePlan(params: { }); } +export function buildPinnedCreatePlan(params: { + check: PathSafetyCheck; + pinned: PinnedSandboxEntry; + mkdir: boolean; +}): SandboxFsCommandPlan { + return buildPinnedMutationPlan({ + checks: [params.check], + args: [ + "create", + params.pinned.mountRootPath, + params.pinned.relativeParentPath, + params.pinned.basename, + params.mkdir ? "1" : "0", + ], + }); +} + export function buildPinnedCopyPlan(params: { sourceCheck: PathSafetyCheck; destinationCheck: PathSafetyCheck; diff --git a/src/agents/sandbox/fs-bridge-native-mutation-python.ts b/src/agents/sandbox/fs-bridge-native-mutation-python.ts new file mode 100644 index 000000000000..ecb2ebed761f --- /dev/null +++ b/src/agents/sandbox/fs-bridge-native-mutation-python.ts @@ -0,0 +1,137 @@ +export const SANDBOX_CREATE_STAGING_PYTHON = [ + "def create_staging_dir(parent_fd):", + " # This helper guarantees descriptor-relative confinement and no-replace", + " # publication, not content integrity against same-UID peers; they can", + " # also rewrite the destination immediately after publication.", + " prefix = '.openclaw-create-'", + " for _ in range(128):", + " candidate = prefix + secrets.token_hex(6)", + " try:", + " os.mkdir(candidate, 0o700, dir_fd=parent_fd)", + " except FileExistsError:", + " continue", + " created_identity = entry_identity(os.lstat(candidate, dir_fd=parent_fd))", + " staging_fd = None", + " try:", + " staging_fd = open_dir(candidate, dir_fd=parent_fd)", + " if not same_identity(created_identity, os.fstat(staging_fd)):", + " raise OSError(errno.ESTALE, 'create staging directory changed', candidate)", + " return candidate, staging_fd", + " except Exception:", + " if staging_fd is not None:", + " os.close(staging_fd)", + " try:", + " current = os.lstat(candidate, dir_fd=parent_fd)", + " if same_identity(created_identity, current):", + " os.rmdir(candidate, dir_fd=parent_fd)", + " os.fsync(parent_fd)", + " except FileNotFoundError:", + " pass", + " raise", + " raise RuntimeError('failed to allocate sandbox create staging directory')", +].join("\n"); + +export const SANDBOX_RENAME_NO_REPLACE_PYTHON = [ + "def rename_no_replace(src_parent_fd, src_basename, dst_parent_fd, dst_basename):", + " libc = ctypes.CDLL(None, use_errno=True)", + " is_linux = sys.platform.startswith('linux')", + " if is_linux:", + " rename_fn = getattr(libc, 'renameat2', None)", + " if rename_fn is None:", + " os.link(", + " src_basename,", + " dst_basename,", + " src_dir_fd=src_parent_fd,", + " dst_dir_fd=dst_parent_fd,", + " follow_symlinks=False,", + " )", + " return", + " flags = 1 # RENAME_NOREPLACE", + " elif sys.platform == 'darwin':", + " rename_fn = getattr(libc, 'renameatx_np', None)", + " flags = 0x00000004 # RENAME_EXCL", + " else:", + " rename_fn = None", + " flags = 0", + " if rename_fn is None:", + " raise OSError(errno.ENOSYS, 'atomic no-replace rename is unavailable')", + " rename_fn.argtypes = [ctypes.c_int, ctypes.c_char_p, ctypes.c_int, ctypes.c_char_p, ctypes.c_uint]", + " rename_fn.restype = ctypes.c_int", + " result = rename_fn(", + " src_parent_fd,", + " os.fsencode(src_basename),", + " dst_parent_fd,", + " os.fsencode(dst_basename),", + " flags,", + " )", + " if result != 0:", + " error_code = ctypes.get_errno()", + " unsupported_codes = {errno.ENOSYS, errno.EINVAL, errno.ENOTSUP}", + " if hasattr(errno, 'EOPNOTSUPP'):", + " unsupported_codes.add(errno.EOPNOTSUPP)", + " if is_linux and error_code in unsupported_codes:", + " os.link(", + " src_basename,", + " dst_basename,", + " src_dir_fd=src_parent_fd,", + " dst_dir_fd=dst_parent_fd,", + " follow_symlinks=False,", + " )", + " return", + " raise OSError(error_code, os.strerror(error_code), dst_basename)", +].join("\n"); + +export const SANDBOX_CREATE_EXCLUSIVE_PYTHON = [ + "def create_exclusive(parent_fd, basename, stdin_buffer):", + " staging_fd = None", + " staging_name = None", + " temp_fd = None", + " temp_name = 'payload'", + " temp_inode = None", + " try:", + " try:", + " os.lstat(basename, dir_fd=parent_fd)", + " except FileNotFoundError:", + " pass", + " else:", + " raise FileExistsError(errno.EEXIST, os.strerror(errno.EEXIST), basename)", + " staging_name, staging_fd = create_staging_dir(parent_fd)", + " temp_fd = os.open(temp_name, WRITE_FLAGS, 0o600, dir_fd=staging_fd)", + " while True:", + " chunk = stdin_buffer.read(65536)", + " if not chunk:", + " break", + " write_all(temp_fd, chunk)", + " # exclusive create payload is durable before publication", + " os.fsync(temp_fd)", + " temp_inode = inode_identity(os.fstat(temp_fd))", + " # Publish with a native atomic no-replace rename.", + " rename_no_replace(staging_fd, temp_name, parent_fd, basename)", + " target_stat = os.lstat(basename, dir_fd=parent_fd)", + " if temp_inode != inode_identity(target_stat):", + " raise OSError(errno.ESTALE, 'exclusive publication source changed', basename)", + " os.fsync(parent_fd)", + " finally:", + " if temp_fd is not None:", + " os.close(temp_fd)", + " if staging_fd is not None:", + " # Cleanup stays relative to the private pinned directory, so a", + " # parent-path substitution cannot redirect payload deletion.", + " try:", + " os.unlink(temp_name, dir_fd=staging_fd)", + " except FileNotFoundError:", + " pass", + " staging_identity = entry_identity(os.fstat(staging_fd))", + " os.close(staging_fd)", + " staging_removed = False", + " if staging_name is not None:", + " try:", + " current_staging = os.lstat(staging_name, dir_fd=parent_fd)", + " if same_identity(staging_identity, current_staging):", + " os.rmdir(staging_name, dir_fd=parent_fd)", + " staging_removed = True", + " except FileNotFoundError:", + " pass", + " if staging_removed:", + " os.fsync(parent_fd)", +].join("\n"); diff --git a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts index 03575556ca84..38952f85c6aa 100644 --- a/src/agents/sandbox/fs-bridge.anchored-ops.test.ts +++ b/src/agents/sandbox/fs-bridge.anchored-ops.test.ts @@ -149,6 +149,18 @@ describe("sandbox fs bridge anchored ops", () => { }); const pinnedCases = [ + { + name: "exclusive create pins canonical parent + basename", + invoke: (bridge: ReturnType) => { + const createFileExclusive = bridge.createFileExclusive?.bind(bridge); + if (!createFileExclusive) { + throw new Error("expected exclusive-create capability"); + } + return createFileExclusive({ filePath: "nested/new.txt", data: "created" }); + }, + expectedArgs: ["create", "/workspace", "nested", "new.txt", "1"], + forbiddenArgs: ["/workspace/nested/new.txt"], + }, { name: "write pins canonical parent + basename", invoke: (bridge: ReturnType) => diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 1a8c616931a4..d8b7f42d0175 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -12,6 +12,8 @@ import type { } from "./backend-handle.types.js"; import { runDockerSandboxShellCommand } from "./docker-backend.js"; import { + buildPinnedCreatePlan, + SANDBOX_CREATE_EXISTS_EXIT_CODE, buildPinnedCopyPlan, buildPinnedMkdirpPlan, buildPinnedRemovePlan, @@ -149,6 +151,49 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); } + async createFileExclusive(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise<"created" | "exists"> { + const target = this.resolveResolvedPath(params); + this.ensureWriteAccess(target, "create files"); + const createCheck = { + target, + options: { action: "create files", requireWritable: true } as const, + }; + await this.pathGuard.assertPathSafety(target, createCheck.options); + const buffer = Buffer.isBuffer(params.data) + ? params.data + : Buffer.from(params.data, params.encoding ?? "utf8"); + const pinnedCreateTarget = await this.pathGuard.resolveAnchoredPinnedEntry( + target, + "create files", + ); + const result = await this.runCheckedCommand({ + ...buildPinnedCreatePlan({ + check: createCheck, + pinned: pinnedCreateTarget, + mkdir: params.mkdir !== false, + }), + allowFailure: true, + stdin: buffer, + signal: params.signal, + }); + if (result.code === SANDBOX_CREATE_EXISTS_EXIT_CODE) { + return "exists"; + } + if (result.code !== 0) { + throw new Error( + `sandbox create failed for ${target.containerPath}: ${result.stderr.toString("utf8").trim()}`, + ); + } + return "created"; + } + async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "create directories"); diff --git a/src/agents/sandbox/fs-bridge.types.ts b/src/agents/sandbox/fs-bridge.types.ts index e86280715321..d611ac3d8b2c 100644 --- a/src/agents/sandbox/fs-bridge.types.ts +++ b/src/agents/sandbox/fs-bridge.types.ts @@ -44,6 +44,19 @@ export type SandboxFsBridge = { mkdir?: boolean; signal?: AbortSignal; }): Promise; + /** + * Atomically creates a file only when no entry already exists at the path. + * Backends without this capability must omit it rather than emulate it with + * a check followed by writeFile. + */ + createFileExclusive?(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise<"created" | "exists">; mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise; remove(params: { filePath: string; diff --git a/src/agents/sandbox/remote-fs-bridge.test.ts b/src/agents/sandbox/remote-fs-bridge.test.ts index eb636122d8dc..31e56f156826 100644 --- a/src/agents/sandbox/remote-fs-bridge.test.ts +++ b/src/agents/sandbox/remote-fs-bridge.test.ts @@ -100,6 +100,74 @@ function createWorkspaceReadBridge(workspaceDir: string) { } describe("remote sandbox fs bridge", () => { + it.runIf(process.platform !== "win32")( + "creates files exclusively and preserves existing entries", + async () => { + await withTempDir("openclaw-remote-fs-create-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(workspaceDir, { recursive: true }); + const { runtime } = createLocalRemoteRuntime({ + remoteWorkspaceDir: workspaceDir, + remoteAgentWorkspaceDir: workspaceDir, + }); + const bridge = createRemoteShellSandboxFsBridge({ + sandbox: createSandbox({ workspaceDir, agentWorkspaceDir: workspaceDir }), + runtime, + }); + const createFileExclusive = bridge.createFileExclusive?.bind(bridge); + expect(createFileExclusive).toBeTypeOf("function"); + + await expect( + createFileExclusive!({ filePath: "nested/note.txt", data: "first" }), + ).resolves.toBe("created"); + await expect( + createFileExclusive!({ filePath: "nested/note.txt", data: "replacement" }), + ).resolves.toBe("exists"); + await expect( + fs.readFile(path.join(workspaceDir, "nested", "note.txt"), "utf8"), + ).resolves.toBe("first"); + + const outcomes = await Promise.all([ + createFileExclusive!({ filePath: "race.txt", data: "one" }), + createFileExclusive!({ filePath: "race.txt", data: "two" }), + ]); + expect(outcomes.toSorted()).toEqual(["created", "exists"]); + await expect(fs.readFile(path.join(workspaceDir, "race.txt"), "utf8")).resolves.toMatch( + /^(one|two)$/u, + ); + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "treats a symlink destination as existing without changing its target", + async () => { + await withTempDir("openclaw-remote-fs-create-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "target.txt"), "keep", "utf8"); + await fs.symlink("target.txt", path.join(workspaceDir, "link.txt")); + const { runtime } = createLocalRemoteRuntime({ + remoteWorkspaceDir: workspaceDir, + remoteAgentWorkspaceDir: workspaceDir, + }); + const bridge = createRemoteShellSandboxFsBridge({ + sandbox: createSandbox({ workspaceDir, agentWorkspaceDir: workspaceDir }), + runtime, + }); + const createFileExclusive = bridge.createFileExclusive?.bind(bridge); + expect(createFileExclusive).toBeTypeOf("function"); + + await expect( + createFileExclusive!({ filePath: "link.txt", data: "replacement" }), + ).resolves.toBe("exists"); + await expect(fs.readFile(path.join(workspaceDir, "target.txt"), "utf8")).resolves.toBe( + "keep", + ); + }); + }, + ); + it.runIf(process.platform !== "win32")( "reads files with the pinned mutation helper", async () => { diff --git a/src/agents/sandbox/remote-fs-bridge.ts b/src/agents/sandbox/remote-fs-bridge.ts index 4ab69a854482..29253547c103 100644 --- a/src/agents/sandbox/remote-fs-bridge.ts +++ b/src/agents/sandbox/remote-fs-bridge.ts @@ -11,7 +11,10 @@ import type { SandboxBackendCommandResult, SandboxFsBridgeContext, } from "./backend-handle.types.js"; -import { SANDBOX_PINNED_MUTATION_PYTHON } from "./fs-bridge-mutation-helper.js"; +import { + SANDBOX_CREATE_EXISTS_EXIT_CODE, + SANDBOX_PINNED_MUTATION_PYTHON, +} from "./fs-bridge-mutation-helper.js"; import { createWritableRenameTargetResolver } from "./fs-bridge-rename-targets.js"; import { parseSandboxStatMtimeMs, parseSandboxStatSize } from "./fs-bridge-stat-parse.js"; import type { SandboxFsBridge, SandboxFsStat, SandboxResolvedPath } from "./fs-bridge.types.js"; @@ -191,6 +194,48 @@ class RemoteShellSandboxFsBridge implements SandboxFsBridge { }); } + async createFileExclusive(params: { + filePath: string; + cwd?: string; + data: Buffer | string; + encoding?: BufferEncoding; + mkdir?: boolean; + signal?: AbortSignal; + }): Promise<"created" | "exists"> { + const target = this.resolveTarget(params); + await this.ensureRemoteWritable(target, "create files", params.signal); + const pinned = await this.resolvePinnedParent({ + containerPath: target.containerPath, + action: "create files", + requireWritable: true, + signal: params.signal, + }); + const buffer = Buffer.isBuffer(params.data) + ? params.data + : Buffer.from(params.data, params.encoding ?? "utf8"); + const result = await this.runMutation({ + args: [ + "create", + pinned.mountRootPath, + pinned.relativeParentPath, + pinned.basename, + params.mkdir !== false ? "1" : "0", + ], + stdin: buffer, + allowFailure: true, + signal: params.signal, + }); + if (result.code === SANDBOX_CREATE_EXISTS_EXIT_CODE) { + return "exists"; + } + if (result.code !== 0) { + throw new Error( + `Sandbox create failed for ${target.containerPath}: ${result.stderr.toString("utf8").trim()}`, + ); + } + return "created"; + } + async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveTarget(params); await this.ensureRemoteWritable(target, "create directories", params.signal); diff --git a/src/agents/test-helpers/host-sandbox-fs-bridge.ts b/src/agents/test-helpers/host-sandbox-fs-bridge.ts index 61a4fed0e5b4..2547236a4f02 100644 --- a/src/agents/test-helpers/host-sandbox-fs-bridge.ts +++ b/src/agents/test-helpers/host-sandbox-fs-bridge.ts @@ -45,6 +45,25 @@ export function createSandboxFsBridgeFromResolver( const buffer = Buffer.isBuffer(data) ? data : Buffer.from(data); await fs.writeFile(target.hostPath, buffer); }, + createFileExclusive: async ({ filePath, cwd, data, mkdir = true }) => { + const target = resolvePath(filePath, cwd); + if (!target.hostPath) { + throw new Error(`Expected hostPath for ${target.containerPath}`); + } + if (mkdir) { + await fs.mkdir(path.dirname(target.hostPath), { recursive: true }); + } + const buffer = Buffer.isBuffer(data) ? data : Buffer.from(data); + try { + await fs.writeFile(target.hostPath, buffer, { flag: "wx" }); + return "created"; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") { + return "exists"; + } + throw error; + } + }, mkdirp: async ({ filePath, cwd }) => { const target = resolvePath(filePath, cwd); if (!target.hostPath) {