From 30c87bde99774fdbd0b3b31c1921e3750a1d61e4 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sun, 2 Aug 2026 17:58:40 +0800 Subject: [PATCH] fix(agents): reject false success from delegated file mutations (#117938) * fix(agents): verify delegated writes before reporting success * fix(agents): verify delegated edits before success --------- Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> --- .../agent-tools.read.host-operations.test.ts | 42 ++++++++++- src/agents/agent-tools.read.ts | 7 ++ src/agents/sessions/tools/edit.test.ts | 52 +++++++++++++ src/agents/sessions/tools/edit.ts | 72 +++++++++--------- .../sessions/tools/file-write-verification.ts | 30 ++++++++ src/agents/sessions/tools/write.test.ts | 75 +++++++++++++++---- src/agents/sessions/tools/write.ts | 33 ++++---- 7 files changed, 241 insertions(+), 70 deletions(-) create mode 100644 src/agents/sessions/tools/file-write-verification.ts diff --git a/src/agents/agent-tools.read.host-operations.test.ts b/src/agents/agent-tools.read.host-operations.test.ts index ec86c12568df..fc7f8d006fa8 100644 --- a/src/agents/agent-tools.read.host-operations.test.ts +++ b/src/agents/agent-tools.read.host-operations.test.ts @@ -8,15 +8,19 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { withEnvAsync } from "../test-utils/env.js"; +import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; type CapturedEditOperations = { readFile: (absolutePath: string) => Promise; writeFile: (absolutePath: string, content: string) => Promise; + statFile: (absolutePath: string) => Promise; access: (absolutePath: string) => Promise; }; type CapturedWriteOperations = { mkdir: (dir: string) => Promise; + readFile: (absolutePath: string) => Promise; + statFile: (absolutePath: string) => Promise; writeFile: (absolutePath: string, content: string) => Promise; }; @@ -50,8 +54,12 @@ vi.mock("./sessions/index.js", async () => { }; }); -const { createHostWorkspaceEditTool, createHostWorkspaceWriteTool } = - await import("./agent-tools.read.js"); +const { + createHostWorkspaceEditTool, + createHostWorkspaceWriteTool, + createSandboxedEditTool, + createSandboxedWriteTool, +} = await import("./agent-tools.read.js"); const osHome = () => process.env.HOME ?? os.homedir(); const toTildePath = (absolutePath: string) => absolutePath.replace(osHome(), "~"); @@ -228,3 +236,33 @@ describe("createHostWorkspaceEditTool host access mapping", () => { }, ); }); + +describe("file mutation verification operations", () => { + it("provides readback and stat operations to host writes and edits", () => { + createHostWorkspaceWriteTool("/workspace", { workspaceOnly: false }); + + expect(readWriteOps().readFile).toBeTypeOf("function"); + expect(readWriteOps().statFile).toBeTypeOf("function"); + + createHostWorkspaceEditTool("/workspace", { workspaceOnly: false }); + + expect(readEditOps().readFile).toBeTypeOf("function"); + expect(readEditOps().statFile).toBeTypeOf("function"); + }); + + it("provides readback and stat operations to sandbox writes and edits", () => { + const params = { + root: "/workspace", + bridge: {} as SandboxFsBridge, + }; + createSandboxedWriteTool(params); + + expect(readWriteOps().readFile).toBeTypeOf("function"); + expect(readWriteOps().statFile).toBeTypeOf("function"); + + createSandboxedEditTool(params); + + expect(readEditOps().readFile).toBeTypeOf("function"); + expect(readEditOps().statFile).toBeTypeOf("function"); + }); +}); diff --git a/src/agents/agent-tools.read.ts b/src/agents/agent-tools.read.ts index 7924089dc914..53568473102b 100644 --- a/src/agents/agent-tools.read.ts +++ b/src/agents/agent-tools.read.ts @@ -1077,6 +1077,8 @@ function createSandboxEditOperations(params: SandboxToolParams) { params.bridge.readFile({ filePath: absolutePath, cwd: params.root }), writeFile: (absolutePath: string, content: string) => params.bridge.writeFile({ filePath: absolutePath, cwd: params.root, data: content }), + statFile: (absolutePath: string) => + params.bridge.stat({ filePath: absolutePath, cwd: params.root }), access: (absolutePath: string) => assertSandboxFileExists(params, absolutePath), } as const, params.memoryWriteProvenance, @@ -1213,6 +1215,7 @@ function createHostEditOperations( return await fs.readFile(resolveHostPath(absolutePath)); }, writeFile: writeHostFile, + statFile: (absolutePath: string) => statHostFile(resolveHostPath(absolutePath)), access: async (absolutePath: string) => { await fs.access(resolveHostPath(absolutePath)); }, @@ -1236,6 +1239,10 @@ function createHostEditOperations( }, writeFile: (absolutePath: string, content: string) => writeWorkspaceFile(root, getRoot, absolutePath, content), + statFile: async (absolutePath: string) => { + const relative = toRelativeWorkspacePath(root, absolutePath); + return statHostFile(path.resolve(root, relative)); + }, access: async (absolutePath: string) => { let relative: string; try { diff --git a/src/agents/sessions/tools/edit.test.ts b/src/agents/sessions/tools/edit.test.ts index 4c70fed73e83..a2e0aed4ffdd 100644 --- a/src/agents/sessions/tools/edit.test.ts +++ b/src/agents/sessions/tools/edit.test.ts @@ -34,6 +34,22 @@ describe("edit tool", () => { return filePath; } + async function statEditFile(absolutePath: string) { + try { + const stat = await fs.stat(absolutePath); + return { + type: stat.isFile() ? "file" : stat.isDirectory() ? "directory" : "other", + size: stat.size, + mtimeMs: stat.mtimeMs, + } as const; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return null; + } + throw error; + } + } + it("preserves a valid UTF-8 BOM when editing a real file", async () => { const filePath = await createTempFile(Buffer.from("\uFEFFheading\nprice: 5\n", "utf-8")); const tool = createEditTool(tmpDir); @@ -107,6 +123,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile: async () => Buffer.from(original), + statFile: async () => null, writeFile, }; const tool = createEditTool("/remote/workspace", { operations }); @@ -167,6 +184,7 @@ describe("edit tool", () => { await fs.access(absolutePath); }, readFile: (absolutePath) => fs.readFile(absolutePath), + statFile: statEditFile, writeFile: async (absolutePath, content) => { await fs.writeFile(absolutePath, content, "utf-8"); throw new Error("Simulated post-write failure"); @@ -202,6 +220,7 @@ describe("edit tool", () => { await fs.access(absolutePath); }, readFile: (absolutePath) => fs.readFile(absolutePath), + statFile: statEditFile, writeFile: async () => { throw new Error("Simulated write failure"); }, @@ -220,6 +239,31 @@ describe("edit tool", () => { ).rejects.toThrow("Simulated write failure"); }); + it("rejects false success when a delegated write resolves without persisting", async () => { + const filePath = await createTempFile("old\n"); + const operations: EditOperations = { + access: async (absolutePath) => { + await fs.access(absolutePath); + }, + readFile: (absolutePath) => fs.readFile(absolutePath), + statFile: statEditFile, + writeFile: async () => {}, + }; + const tool = createEditTool(tmpDir, { operations }); + + await expect( + tool.execute( + "call-1", + { + path: filePath, + edits: [{ oldText: "old", newText: "new" }], + }, + undefined, + ), + ).rejects.toThrow("Edit verification failed"); + await expect(fs.readFile(filePath, "utf-8")).resolves.toBe("old\n"); + }); + it("recovers multi-edit post-write failures", async () => { const filePath = await createTempFile("alpha beta gamma delta\n"); const operations: EditOperations = { @@ -227,6 +271,7 @@ describe("edit tool", () => { await fs.access(absolutePath); }, readFile: (absolutePath) => fs.readFile(absolutePath), + statFile: statEditFile, writeFile: async (absolutePath, content) => { await fs.writeFile(absolutePath, content, "utf-8"); throw new Error("Simulated post-write failure"); @@ -352,6 +397,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -392,6 +438,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -431,6 +478,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -472,6 +520,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -529,6 +578,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -564,6 +614,7 @@ describe("edit tool", () => { const operations: EditOperations = { access: async () => {}, readFile, + statFile: async () => null, writeFile: async () => {}, }; const tool = createEditToolDefinition("/workspace", { operations }); @@ -617,6 +668,7 @@ describe("edit tool", () => { await fs.access(absolutePath); }, readFile: (absolutePath) => fs.readFile(absolutePath), + statFile: statEditFile, writeFile: async () => { throw new Error("No changes made to the disk because it is full"); }, diff --git a/src/agents/sessions/tools/edit.ts b/src/agents/sessions/tools/edit.ts index ef41623f2cf5..84c4d15bf820 100644 --- a/src/agents/sessions/tools/edit.ts +++ b/src/agents/sessions/tools/edit.ts @@ -7,6 +7,7 @@ import { constants } from "node:fs"; import { access as fsAccess, readFile as fsReadFile, + stat as fsStat, writeFile as fsWriteFile, } from "node:fs/promises"; import { Box, Container, Spacer, Text } from "@earendil-works/pi-tui"; @@ -32,6 +33,7 @@ import { validateNoOpEditTargets, } from "./edit-diff.js"; import { withFileMutationQueue } from "./file-mutation-queue.js"; +import { type PersistedFileStat, verifyPersistedUtf8File } from "./file-write-verification.js"; import { resolveToCwd } from "./path-utils.js"; import { invalidArgText, shortenPath, str } from "./render-utils.js"; import type { EditToolDetails, EditToolInput } from "./tool-contracts.js"; @@ -98,6 +100,8 @@ export interface EditOperations { readFile: (absolutePath: string) => Promise; /** Write content to a file */ writeFile: (absolutePath: string, content: string) => Promise; + /** Stat the target before reporting success */ + statFile: (absolutePath: string) => Promise; /** Check if file is readable and writable (throw if not) */ access: (absolutePath: string) => Promise; } @@ -105,6 +109,26 @@ export interface EditOperations { const defaultEditOperations: EditOperations = { readFile: (path) => fsReadFile(path), writeFile: (path, content) => fsWriteFile(path, content, "utf-8"), + statFile: async (path) => { + try { + const stat = await fsStat(path); + return { + type: stat.isFile() ? "file" : stat.isDirectory() ? "directory" : "other", + size: stat.size, + mtimeMs: stat.mtimeMs, + } as const; + } catch (error) { + if ( + error && + typeof error === "object" && + "code" in error && + (error as { code?: unknown }).code === "ENOENT" + ) { + return null; + } + throw error; + } + }, access: (path) => fsAccess(path, constants.R_OK | constants.W_OK), }; @@ -161,38 +185,6 @@ function validateEditInput(input: EditToolInput): { return { path: input.path, edits: input.edits }; } -function removeExactOccurrences(content: string, needle: string): string { - return needle.length > 0 ? content.split(needle).join("") : content; -} - -function didEditLikelyApply(params: { - originalContent: string; - currentContent: string; - edits: Edit[]; -}): boolean { - if (params.edits.length === 0) { - return false; - } - const normalizedOriginal = normalizeToLF(params.originalContent); - const normalizedCurrent = normalizeToLF(params.currentContent); - if (normalizedOriginal === normalizedCurrent) { - return false; - } - - let withoutInsertedNewText = normalizedCurrent; - for (const edit of params.edits) { - const normalizedNew = normalizeToLF(edit.newText); - if (normalizedNew.length > 0 && !normalizedCurrent.includes(normalizedNew)) { - return false; - } - withoutInsertedNewText = removeExactOccurrences(withoutInsertedNewText, normalizedNew); - } - - return params.edits.every( - (edit) => !withoutInsertedNewText.includes(normalizeToLF(edit.oldText)), - ); -} - function appendMismatchHint(error: Error, currentContent: string): Error { const snippet = currentContent.length <= EDIT_MISMATCH_HINT_LIMIT @@ -422,6 +414,7 @@ export function createEditToolDefinition( } let realEdits: Edit[] = []; + let expectedContent: string | undefined; try { await ops.access(absolutePath); @@ -462,10 +455,16 @@ export function createEditToolDefinition( realEdits, path, ); - await ops.writeFile(absolutePath, bom + finalContent); + expectedContent = bom + finalContent; + await ops.writeFile(absolutePath, expectedContent); if (signal?.aborted) { throw new Error("Operation aborted"); } + if (!(await verifyPersistedUtf8File(absolutePath, expectedContent, ops))) { + throw new Error( + `Edit verification failed for ${path}: the persisted regular file does not match the requested content. Inspect the target and retry.`, + ); + } const diffResult = generateDiffString(baseContent, newContent); const patch = generateUnifiedPatch(path, baseContent, newContent); @@ -492,11 +491,8 @@ export function createEditToolDefinition( .then((current) => current.toString("utf-8")) .catch(() => rawContent); if ( - didEditLikelyApply({ - originalContent: rawContent, - currentContent, - edits: realEdits, - }) + expectedContent !== undefined && + (await verifyPersistedUtf8File(absolutePath, expectedContent, ops)) ) { return { content: [ diff --git a/src/agents/sessions/tools/file-write-verification.ts b/src/agents/sessions/tools/file-write-verification.ts new file mode 100644 index 000000000000..13ae4cf045ab --- /dev/null +++ b/src/agents/sessions/tools/file-write-verification.ts @@ -0,0 +1,30 @@ +export type PersistedFileStat = { + type: "file" | "directory" | "other"; + size: number; + mtimeMs?: number; +}; + +type PersistedUtf8FileOperations = { + readFile: (absolutePath: string) => Promise; + statFile: (absolutePath: string) => Promise; +}; + +export async function verifyPersistedUtf8File( + absolutePath: string, + content: string, + operations: PersistedUtf8FileOperations, +): Promise { + // Success receipts must prove the same regular-file bytes across local and delegated writes. + // Compare encoded bytes because UTF-8 writes normalize invalid surrogate code units. + const expectedContent = Buffer.from(content, "utf8"); + const stat = await operations.statFile(absolutePath).catch(() => null); + if (!stat || stat.type !== "file" || stat.size !== expectedContent.byteLength) { + return false; + } + const readback = await operations.readFile(absolutePath).catch(() => undefined); + if (readback === undefined) { + return false; + } + const persistedContent = Buffer.isBuffer(readback) ? readback : Buffer.from(readback, "utf8"); + return persistedContent.equals(expectedContent); +} diff --git a/src/agents/sessions/tools/write.test.ts b/src/agents/sessions/tools/write.test.ts index 2870352dbc7c..18ff56d6303d 100644 --- a/src/agents/sessions/tools/write.test.ts +++ b/src/agents/sessions/tools/write.test.ts @@ -113,6 +113,58 @@ describe("write tool", () => { expect(result.content[0]?.type).toBe("text"); }); + it("rejects a delegated write that resolves without creating the file", async () => { + const filePath = await createTempPath("missing.txt"); + const tool = createWriteTool(tmpDir, { + operations: createRecoverableOperations(async () => {}), + }); + + await expect( + tool.execute("call-1", { path: filePath, content: "expected\n" }, undefined), + ).rejects.toThrow("Write verification failed"); + await expect(fs.stat(filePath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("rejects a delegated write that leaves stale same-size content", async () => { + const filePath = await createTempPath("stale.txt"); + await fs.writeFile(filePath, "stale\n", "utf-8"); + const tool = createWriteTool(tmpDir, { + operations: createRecoverableOperations(async () => {}), + }); + + await expect( + tool.execute("call-1", { path: filePath, content: "fresh\n" }, undefined), + ).rejects.toThrow("Write verification failed"); + await expect(fs.readFile(filePath, "utf-8")).resolves.toBe("stale\n"); + }); + + it("rejects a delegated write that leaves a non-file target", async () => { + const filePath = await createTempPath("directory"); + await fs.mkdir(filePath); + const tool = createWriteTool(tmpDir, { + operations: createRecoverableOperations(async () => {}), + }); + + await expect( + tool.execute("call-1", { path: filePath, content: "expected\n" }, undefined), + ).rejects.toThrow("Write verification failed"); + }); + + it("verifies delegated writes by their persisted UTF-8 bytes", async () => { + const filePath = await createTempPath("surrogate.txt"); + const content = "unpaired \ud800 surrogate\n"; + const tool = createWriteTool(tmpDir, { + operations: createRecoverableOperations((absolutePath, requestedContent) => + fs.writeFile(absolutePath, requestedContent, "utf-8"), + ), + }); + + await expect( + tool.execute("call-1", { path: filePath, content }, undefined), + ).resolves.toMatchObject({ details: { changed: true, created: true } }); + await expect(fs.readFile(filePath)).resolves.toEqual(Buffer.from(content, "utf8")); + }); + it("writes file URL paths through the shared session path resolver", async () => { const filePath = await createTempPath("notes.md"); const tool = createWriteTool(tmpDir); @@ -271,13 +323,13 @@ describe("write tool", () => { it("reports an overwrite without a fabricated diff when the old file is too large", async () => { const filePath = await createTempPath("large.txt"); await fs.writeFile(filePath, "x".repeat(1024 * 1024 + 1), "utf-8"); - let readCalled = false; + let readCount = 0; const operations = createRecoverableOperations((absolutePath, content) => fs.writeFile(absolutePath, content, "utf-8"), ); - operations.readFile = async () => { - readCalled = true; - throw new Error("oversized pre-write read"); + operations.readFile = async (absolutePath) => { + readCount += 1; + return fs.readFile(absolutePath); }; const tool = createWriteTool(tmpDir, { operations }); @@ -287,7 +339,7 @@ describe("write tool", () => { undefined, ); - expect(readCalled).toBe(false); + expect(readCount).toBe(1); expect(result.details).toEqual({ changed: true, created: false }); }); @@ -306,23 +358,20 @@ describe("write tool", () => { expect(result.details).toEqual({ changed: true, created: false }); }); - it("does not guess creation status when the pre-write stat is unavailable", async () => { + it("rejects success when the post-write stat is unavailable", async () => { await createTempPath("unknown.txt"); const operations: WriteOperations = { mkdir: (dir) => fs.mkdir(dir, { recursive: true }).then(() => {}), writeFile: (absolutePath, content) => fs.writeFile(absolutePath, content, "utf-8"), + readFile: (absolutePath) => fs.readFile(absolutePath), statFile: async () => { throw new Error("remote stat unavailable"); }, }; const tool = createWriteTool(tmpDir, { operations }); - const result = await tool.execute( - "call-1", - { path: "unknown.txt", content: "new\n" }, - undefined, - ); - - expect(result.details).toEqual({ changed: true }); + await expect( + tool.execute("call-1", { path: "unknown.txt", content: "new\n" }, undefined), + ).rejects.toThrow("Write verification failed"); }); }); diff --git a/src/agents/sessions/tools/write.ts b/src/agents/sessions/tools/write.ts index fdec18f5df21..933348fc5742 100644 --- a/src/agents/sessions/tools/write.ts +++ b/src/agents/sessions/tools/write.ts @@ -20,6 +20,7 @@ import { textResult } from "../../tools/common.js"; import type { ToolDefinition, ToolRenderResultOptions } from "../extensions/types.js"; import { generateDiffString, generateUnifiedPatch } from "./edit-diff.js"; import { withFileMutationQueue } from "./file-mutation-queue.js"; +import { type PersistedFileStat, verifyPersistedUtf8File } from "./file-write-verification.js"; import { resolveToCwd } from "./path-utils.js"; import { invalidArgText, @@ -74,10 +75,10 @@ export interface WriteOperations { writeFile: (absolutePath: string, content: string) => Promise; /** Create directory recursively */ mkdir: (dir: string) => Promise; - /** Optional readback used to recover when a write succeeded but the tool aborted before returning */ - readFile?: (absolutePath: string) => Promise; - /** Optional stat used to avoid reporting success for files that already matched before execution */ - statFile?: (absolutePath: string) => Promise; + /** Read persisted content before reporting success */ + readFile: (absolutePath: string) => Promise; + /** Stat the target for prechecks and persisted-file verification */ + statFile: (absolutePath: string) => Promise; } const defaultWriteOperations: WriteOperations = { @@ -111,15 +112,9 @@ export interface WriteToolOptions { operations?: WriteOperations; } -type WriteToolFileStat = { - type: "file" | "directory" | "other"; - size: number; - mtimeMs?: number; -}; - type WriteToolPrecheck = { state: "different" | "same" | "unknown"; - beforeStat?: WriteToolFileStat | null; + beforeStat?: PersistedFileStat | null; beforeText?: string; readAttempted?: boolean; }; @@ -337,7 +332,7 @@ async function readOriginalWriteState( if (!ops.statFile) { return { state: "unknown" }; } - let stat: WriteToolFileStat | null; + let stat: PersistedFileStat | null; try { stat = await ops.statFile(absolutePath); } catch (error) { @@ -465,7 +460,7 @@ async function resolveWriteDetails(params: { async function didWriteMetadataChange( absolutePath: string, - beforeStat: WriteToolFileStat | null | undefined, + beforeStat: PersistedFileStat | null | undefined, ops: WriteOperations, ): Promise { if (!beforeStat || !ops.statFile) { @@ -511,16 +506,15 @@ async function recoverSuccessfulWrite(params: { details: WriteToolDetails; signal?: AbortSignal; }) { - if (!params.ops.readFile || !isWriteRecoveryCandidate(params.error, params.signal)) { + if (!isWriteRecoveryCandidate(params.error, params.signal)) { return null; } - const readback = await params.ops.readFile(params.absolutePath).catch(() => undefined); - const currentContent = Buffer.isBuffer(readback) ? readback.toString("utf8") : readback; + const verified = await verifyPersistedUtf8File(params.absolutePath, params.content, params.ops); const changed = params.precheck.state === "different" || (params.precheck.state === "unknown" && (await didWriteMetadataChange(params.absolutePath, params.precheck.beforeStat, params.ops))); - if (currentContent !== params.content || !changed) { + if (!verified || !changed) { return null; } return successfulWriteResult(params.path, params.content, params.details); @@ -575,6 +569,11 @@ export function createWriteToolDefinition( if (signal?.aborted) { throw new Error("Operation aborted"); } + if (!(await verifyPersistedUtf8File(absolutePath, content, ops))) { + throw new Error( + `Write verification failed for ${path}: the persisted regular file does not match the requested content. Inspect the target and retry.`, + ); + } return successfulWriteResult(path, content, details); } catch (error: unknown) { const recovered = await recoverSuccessfulWrite({