diff --git a/src/agent/apply-patch-diff.test.ts b/src/agent/apply-patch-diff.test.ts index af6b3dff..d73bc2b7 100644 --- a/src/agent/apply-patch-diff.test.ts +++ b/src/agent/apply-patch-diff.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { createPosixTools } from "@intx/tools-posix"; @@ -7,6 +7,7 @@ import { createToolRunner } from "@intx/agent"; import type { AgentTool } from "@intx/agent"; import { createCodexToolProxies, type CodexRunTool } from "./codex-tool-proxies.js"; +import { createCodexReadRawFile } from "./codex-read-raw-file.js"; import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js"; import { createPermissionGate } from "../permission/gate.js"; @@ -23,11 +24,14 @@ async function invokeApplyPatch(tools: AgentTool[], input: string) { ); } -async function makeApplyPatch(cwd: string): Promise { +async function makeApplyPatch( + cwd: string, + options: { skipPermissions?: boolean } = {}, +): Promise { const gate = createPermissionGate({ approvals: [], interactive: false, - skipPermissions: true, + skipPermissions: options.skipPermissions ?? true, auto: false, cwd, }); @@ -36,23 +40,6 @@ async function makeApplyPatch(cwd: string): Promise { plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }), }); const runTool: CodexRunTool = async (name, args) => { - // read_file's real tool output is cat -n formatted (line numbers), which - // is not the raw content applyUpdateHunks needs — in production this - // means Update File hunks generally fail to match context (filed as - // CL-6966, Urgent; not this issue's bug to fix). This stub bypasses that - // known defect by returning raw content, so the "Update File shows the - // diff" test below is NOT proof that apply_patch Update works end to end - // — it only proves the diff-surfacing added here is correct once the op - // succeeds. Add File / Delete File below do not depend on read_file and - // are real, unstubbed coverage. - if (name === "read_file") { - const path = String((args as { path?: unknown }).path ?? ""); - try { - return { content: await readFile(join(cwd, path), "utf8") }; - } catch (err) { - return { content: err instanceof Error ? err.message : String(err), isError: true }; - } - } const result = await posixTools.run( { id: "codex-proxy", name, arguments: args }, new AbortController().signal, @@ -62,14 +49,49 @@ async function makeApplyPatch(cwd: string): Promise { ...(result.isError === true ? { isError: true } : {}), }; }; + // Real production path (CL-6966): Update File matches patch context against + // raw file content, not read_file's cat -n formatted output. return createCodexToolProxies({ isCodex: true, runTool, + readRawFile: createCodexReadRawFile(cwd, gate), runManageTasks: async () => ({ content: "ok" }), }); } -describe("apply_patch surfaces the changed region", () => { +describe("apply_patch Update File matches raw content, not read_file's numbered output (CL-6966)", () => { + test("multi-hunk Update File succeeds through the real production path", async () => { + const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-")); + try { + await writeFile( + join(cwd, "app.py"), + "def greet():\n print('hi')\n\n\ndef farewell():\n print('bye')\n", + ); + const tools = await makeApplyPatch(cwd); + const input = [ + "*** Begin Patch", + "*** Update File: app.py", + "@@ def greet():", + "- print('hi')", + "+ print('hello')", + "@@ def farewell():", + "- print('bye')", + "+ print('goodbye')", + "*** End Patch", + ].join("\n"); + + const result = await invokeApplyPatch(tools, input); + + expect(result.isError).not.toBe(true); + const written = await Bun.file(join(cwd, "app.py")).text(); + expect(written).toBe( + "def greet():\n print('hello')\n\n\ndef farewell():\n print('goodbye')\n", + ); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + test("Update File shows the diff", async () => { const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-")); try { @@ -129,3 +151,98 @@ describe("apply_patch surfaces the changed region", () => { } }); }); + +describe("apply_patch Update File refuses reads outside the sanctioned workspace (CL-6966 follow-up)", () => { + // Insertion-only hunk (no context to match): the shape that makes an + // unauthorized raw read exploitable rather than merely wrong, since it + // requires no content match to "succeed" and hands the read content + // straight to write_file via Move to. + const insertionOnlyMoveInput = (path: string, moveTo: string) => + [ + "*** Begin Patch", + `*** Update File: ${path}`, + `*** Move to: ${moveTo}`, + "@@", + "+", + "*** End Patch", + ].join("\n"); + + test("../ traversal out of the workspace is refused", async () => { + const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-parent-")); + const cwd = join(parent, "workspace"); + await mkdir(cwd); + try { + await writeFile(join(parent, "victim.txt"), "outside secret\n"); + const tools = await makeApplyPatch(cwd, { skipPermissions: false }); + + const result = await invokeApplyPatch( + tools, + insertionOnlyMoveInput("../victim.txt", "leaked.txt"), + ); + + expect(result.isError).toBe(true); + expect(String(result.content)).toMatch(/escapes working directory/i); + } finally { + await rm(parent, { recursive: true, force: true }); + } + }); + + test("a symlinked directory leading outside the workspace is refused", async () => { + const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-symlink-")); + const cwd = join(parent, "workspace"); + const outside = join(parent, "outside"); + await mkdir(cwd); + await mkdir(outside); + try { + await writeFile(join(outside, "victim.txt"), "outside secret\n"); + await symlink(outside, join(cwd, "escape-link")); + const tools = await makeApplyPatch(cwd, { skipPermissions: false }); + + const result = await invokeApplyPatch( + tools, + insertionOnlyMoveInput("escape-link/victim.txt", "leaked.txt"), + ); + + expect(result.isError).toBe(true); + expect(String(result.content)).toMatch(/escapes working directory/i); + } finally { + await rm(parent, { recursive: true, force: true }); + } + }); + + test("a secret-guard path (.env) is refused even with skipPermissions", async () => { + const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-secret-")); + try { + await writeFile(join(cwd, ".env"), "API_KEY=super-secret\n"); + // skipPermissions: true (yolo) — secret-guard has no bypass, unlike containment. + const tools = await makeApplyPatch(cwd, { skipPermissions: true }); + + const result = await invokeApplyPatch(tools, insertionOnlyMoveInput(".env", "leaked.txt")); + + expect(result.isError).toBe(true); + expect(String(result.content)).toMatch(/sensitive file/i); + // The secret must never have reached the workspace under a new name. + expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("../ traversal to a secret file is refused (secret-guard applies to relative paths too)", async () => { + const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-secret-parent-")); + const cwd = join(parent, "workspace"); + await mkdir(cwd); + try { + await writeFile(join(parent, ".env"), "API_KEY=super-secret\n"); + const tools = await makeApplyPatch(cwd, { skipPermissions: false }); + + const result = await invokeApplyPatch(tools, insertionOnlyMoveInput("../.env", "leaked.txt")); + + expect(result.isError).toBe(true); + expect(String(result.content)).toMatch(/sensitive file|escapes working directory/i); + expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false); + } finally { + await rm(parent, { recursive: true, force: true }); + } + }); +}); diff --git a/src/agent/codex-read-raw-file.ts b/src/agent/codex-read-raw-file.ts new file mode 100644 index 00000000..dfc7110f --- /dev/null +++ b/src/agent/codex-read-raw-file.ts @@ -0,0 +1,89 @@ +/** + * Raw (non-`cat -n`) file reads for apply_patch's Update File leg (CL-6966). + * + * `applyOp` calls this directly from outside the posixTools middleware chain + * — no ToolPlugin ever sees `op.path` here, unlike the write leg which still + * goes through the full pathEscapePlugin / secretGuardPlugin / authzPlugin / + * permissionPlugin stack (see buildCorePosixToolPlugins in + * posix-tool-plugins.ts). `requireRelativePath` in codex-apply-patch.ts only + * rejects absolute paths — it does nothing about `../` traversal — so this + * reader must apply the same containment and secret-file checks itself, or + * an Update File op naming e.g. `../../.env` (with a no-op insertion hunk, + * which requires no context match) can read a secret and hand it straight to + * write_file as an exfiltration primitive. + * + * Reuses the existing containment and secret-file authorities rather than + * reimplementing them: `resolveWorkspacePath` (symlink-aware realpath + * containment, the same function pathEscapePlugin calls) and `isSensitivePath` + * (the secretGuardPlugin denylist). Both are hard denials — the latter has no + * yolo/allowOutside bypass, matching secretGuardPlugin's own unconditional + * behavior. + */ + +import { readFile } from "node:fs/promises"; +import { resolve } from "node:path"; +import { hasCode } from "@intx/types"; +import { resolveWorkspacePath } from "../permission/path-restriction.js"; +import { createWorktreeRootsProvider } from "../permission/worktree-roots.js"; +import { isSensitivePath } from "../plugins/secret-guard-plugin.js"; +import type { PermissionGate } from "../permission/gate.js"; +import type { CodexReadRawFile } from "./codex-tool-proxies.js"; + +/** `path` is workspace-relative (apply_patch's parser rejects absolute paths). */ +export function createCodexReadRawFile( + cwd: string, + permissionGate: PermissionGate, +): CodexReadRawFile { + const rootsProvider = createWorktreeRootsProvider(cwd); + const allowOutside = (): boolean => permissionGate.getSkipPermissions(); + + return async (path) => { + // Secret-file denylist first, on the raw path — this must hold regardless + // of containment or yolo, exactly like secretGuardPlugin's hard deny. + if (isSensitivePath(path)) { + return { + content: `Access to sensitive file blocked by policy: ${path}`, + isError: true, + }; + } + + const resolved = resolveWorkspacePath(cwd, path, rootsProvider); + let absolutePath: string; + if (resolved !== undefined) { + absolutePath = resolved; + } else if (allowOutside()) { + // Mirrors pathEscapePlugin's own allowOutside fallback (yolo mode). + absolutePath = resolve(cwd, path); + } else { + return { + content: `Path escapes working directory: ${path}`, + isError: true, + }; + } + + // Re-check the resolved, symlink-realpath'd form too: a symlink can name + // something innocuous while pointing at a sensitive real path. + if (isSensitivePath(absolutePath)) { + return { + content: `Access to sensitive file blocked by policy: ${path}`, + isError: true, + }; + } + + try { + const buf = await readFile(absolutePath); + if (buf.includes(0)) { + return { content: `refusing to read binary file: ${path}`, isError: true }; + } + return { content: buf.toString("utf8") }; + } catch (err) { + if (hasCode(err)) { + if (err.code === "ENOENT") return { content: `file not found: ${path}`, isError: true }; + if (err.code === "EACCES") return { content: `permission denied: ${path}`, isError: true }; + if (err.code === "EISDIR") + return { content: `path is a directory: ${path}`, isError: true }; + } + return { content: err instanceof Error ? err.message : String(err), isError: true }; + } + }; +} diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index 5c3cea10..2c5e978c 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -130,6 +130,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), + readRawFile: async () => ({ content: "ok" }), runManageTasks: async () => ({ content: "ok" }), }); expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); @@ -152,6 +153,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), + readRawFile: async () => ({ content: "ok" }), runManageTasks: async () => ({ content: "ok" }), allowDelete: allowDeleteFromCapabilities(docsCapabilities), allowShell: allowShellFromCapabilities(docsCapabilities), @@ -167,6 +169,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: false, runTool: async () => ({ content: "ok" }), + readRawFile: async () => ({ content: "ok" }), runManageTasks: async () => ({ content: "ok" }), allowDelete: allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), allowShell: allowShellFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 71bc6ded..002c6322 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -6,6 +6,7 @@ import { allowDeleteFromCapabilities, allowShellFromCapabilities, createCodexToolProxies, + type CodexReadRawFile, type CodexRunManageTasks, type CodexRunTool, } from "./codex-tool-proxies.js"; @@ -26,9 +27,17 @@ function makeRecorder(initial: Record = {}): { calls: Call[]; files: Map; runTool: CodexRunTool; + readRawFile: CodexReadRawFile; } { const files = new Map(Object.entries(initial)); const calls: Call[] = []; + const readRawFile: CodexReadRawFile = async (path) => { + const content = files.get(path); + if (content === undefined) { + return { content: `File not found: ${path}`, isError: true }; + } + return { content }; + }; const runTool: CodexRunTool = async (name, args) => { calls.push({ name, args }); if (name === "read_file") { @@ -54,10 +63,11 @@ function makeRecorder(initial: Record = {}): { } return { content: `unknown tool: ${name}`, isError: true }; }; - return { calls, files, runTool }; + return { calls, files, runTool, readRawFile }; } const unusedManageTasks: CodexRunManageTasks = async () => ({ content: "unused" }); +const unusedReadRawFile: CodexReadRawFile = async () => ({ content: "unused" }); // A real manage_tasks dispatch: parses with the actual arktype schema and // mutates a real Task[] with the actual applyManageTasks reducer from @@ -103,6 +113,7 @@ describe("createCodexToolProxies", () => { const tools = createCodexToolProxies({ isCodex: false, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks: unusedManageTasks, }); expect(tools).toEqual([]); @@ -112,6 +123,7 @@ describe("createCodexToolProxies", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks: unusedManageTasks, }); expect(tools.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); @@ -122,10 +134,11 @@ describe("createCodexToolProxies", () => { }); test("add forwards write_file with Codex trailing newline", async () => { - const { calls, files, runTool } = makeRecorder(); + const { calls, files, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -149,10 +162,11 @@ describe("createCodexToolProxies", () => { }); test("delete forwards delete_file", async () => { - const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "obsolete.txt": "gone" }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -169,10 +183,11 @@ describe("createCodexToolProxies", () => { }); test("allowDelete false refuses Delete without calling delete_file", async () => { - const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "obsolete.txt": "gone" }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, allowDelete: false, runManageTasks: unusedManageTasks, }); @@ -194,10 +209,11 @@ describe("createCodexToolProxies", () => { const original = `def greet(): print("Hi") `; - const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "src/app.py": original }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, allowDelete: false, runManageTasks: unusedManageTasks, }); @@ -223,10 +239,11 @@ print("Hi") const original = `def greet(): print("Hi") `; - const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "src/app.py": original }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, allowDelete: false, runManageTasks: unusedManageTasks, }); @@ -241,7 +258,7 @@ print("Hi") `, ); expect(result.isError).toBeFalsy(); - expect(calls.map((c) => c.name)).toEqual(["read_file", "write_file"]); + expect(calls.map((c) => c.name)).toEqual(["write_file"]); expect(files.get("src/app.py")).toBe(`def greet(): print("Hello, world!") `); @@ -252,10 +269,11 @@ print("Hello, world!") print("Hi") print("bye") `; - const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "src/app.py": original }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -269,9 +287,8 @@ print("bye") `, ); expect(result.isError).toBeFalsy(); - expect(calls.map((c) => c.name)).toEqual(["read_file", "write_file"]); - expect(calls[0]!.args).toEqual({ path: "src/app.py" }); - expect(calls[1]!.args.path).toBe("src/app.py"); + expect(calls.map((c) => c.name)).toEqual(["write_file"]); + expect(calls[0]!.args.path).toBe("src/app.py"); expect(files.get("src/app.py")).toBe(`def greet(): print("Hello, world!") print("bye") @@ -282,10 +299,11 @@ print("bye") const original = `def greet(): print("Hi") `; - const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const { calls, files, runTool, readRawFile } = makeRecorder({ "src/app.py": original }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -300,13 +318,12 @@ print("Hi") `, ); expect(result.isError).toBeFalsy(); - expect(calls.map((c) => c.name)).toEqual(["read_file", "write_file", "delete_file"]); - expect(calls[0]!.args).toEqual({ path: "src/app.py" }); - expect(calls[1]!.args.path).toBe("src/main.py"); - expect(calls[1]!.args.content).toBe(`def greet(): + expect(calls.map((c) => c.name)).toEqual(["write_file", "delete_file"]); + expect(calls[0]!.args.path).toBe("src/main.py"); + expect(calls[0]!.args.content).toBe(`def greet(): print("Hello, world!") `); - expect(calls[2]!.args).toEqual({ path: "src/app.py" }); + expect(calls[1]!.args).toEqual({ path: "src/app.py" }); expect(files.has("src/app.py")).toBe(false); expect(files.get("src/main.py")).toBe(`def greet(): print("Hello, world!") @@ -314,13 +331,14 @@ print("Hello, world!") }); test("multi-op patch runs each op in order", async () => { - const { calls, files, runTool } = makeRecorder({ + const { calls, files, runTool, readRawFile } = makeRecorder({ "src/app.py": "old\n", "obsolete.txt": "x", }); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -337,22 +355,18 @@ print("Hello, world!") `, ); expect(result.isError).toBeFalsy(); - expect(calls.map((c) => c.name)).toEqual([ - "write_file", - "read_file", - "write_file", - "delete_file", - ]); + expect(calls.map((c) => c.name)).toEqual(["write_file", "write_file", "delete_file"]); expect(files.get("hello.txt")).toBe("Hello world\n"); expect(files.get("src/app.py")).toBe("new\n"); expect(files.has("obsolete.txt")).toBe(false); }); test("parse failure surfaces as tool error (isError)", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -371,6 +385,7 @@ print("Hello, world!") const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks: unusedManageTasks, }); const runner = createToolRunner(tools); @@ -383,10 +398,11 @@ print("Hello, world!") }); test("runTool isError aborts the patch with isError", async () => { - const { runTool } = makeRecorder(); + const { runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeApplyPatch( @@ -406,10 +422,11 @@ print("Hello, world!") describe("shell proxy", () => { test("string command forwards to run_shell", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeTool(tools, "shell", { command: "ls -la" }); @@ -418,10 +435,11 @@ describe("shell proxy", () => { }); test("bash -lc argv triple unwraps to the script", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); await invokeTool(tools, "shell", { command: ["bash", "-lc", "echo 'hi there'"] }); @@ -429,10 +447,11 @@ describe("shell proxy", () => { }); test("other argv arrays are shell-quoted and joined", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); await invokeTool(tools, "shell", { command: ["echo", "hello world"] }); @@ -440,10 +459,11 @@ describe("shell proxy", () => { }); test("workdir and timeout_ms translate to cwd and timeout", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); await invokeTool(tools, "shell", { @@ -457,10 +477,11 @@ describe("shell proxy", () => { }); test("missing command surfaces as tool error", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeTool(tools, "shell", {}); @@ -470,10 +491,11 @@ describe("shell proxy", () => { }); test("allowShell false refuses without calling run_shell", async () => { - const { calls, runTool } = makeRecorder(); + const { calls, runTool, readRawFile } = makeRecorder(); const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile, allowShell: false, runManageTasks: unusedManageTasks, }); @@ -488,6 +510,7 @@ describe("shell proxy", () => { const tools = createCodexToolProxies({ isCodex: true, runTool, + readRawFile: unusedReadRawFile, runManageTasks: unusedManageTasks, }); const result = await invokeTool(tools, "shell", { command: "ls" }); @@ -512,6 +535,7 @@ describe("update_plan proxy", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks, }); const result = await invokeTool(tools, "update_plan", { @@ -547,6 +571,7 @@ describe("update_plan proxy", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks, }); const result = await invokeTool(tools, "update_plan", { @@ -562,6 +587,7 @@ describe("update_plan proxy", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + readRawFile: unusedReadRawFile, runManageTasks, }); const result = await invokeTool(tools, "update_plan", {}); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index f2313846..14716d2a 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -22,6 +22,15 @@ export type CodexRunTool = ( args: Record, ) => Promise<{ content: string; isError?: boolean }>; +/** + * Reads a file's raw content (no `cat -n` line-number prefixes) for the + * Update File leg of apply_patch. `read_file` — both the guard plugin and + * the underlying @intx/tools-posix implementation — always numbers its + * output for model display, so it cannot supply the raw text + * `applyUpdateHunks` needs to match a patch's context lines against (CL-6966). + */ +export type CodexReadRawFile = (path: string) => Promise<{ content: string; isError?: boolean }>; + /** * Dispatches update_plan's translated call onto the real manage_tasks * handler. `manage_tasks` is not a posix tool — it has no handler in the @@ -36,6 +45,12 @@ export type CodexRunManageTasks = ( export interface CreateCodexToolProxiesOpts { isCodex: boolean; runTool: CodexRunTool; + /** + * Reads raw file content for apply_patch's Update File leg (CL-6966). Kept + * separate from `runTool` because there is no tool name that returns raw + * content — `read_file` always numbers its output. + */ + readRawFile: CodexReadRawFile; /** Dispatches update_plan's translated manage_tasks(action="create") call. */ runManageTasks: CodexRunManageTasks; /** @@ -152,7 +167,7 @@ export function createCodexToolProxies(opts: CreateCodexToolProxiesOpts): AgentT const allowDelete = opts.allowDelete !== false; const allowShell = opts.allowShell !== false; return [ - createApplyPatchProxy(opts.runTool, allowDelete), + createApplyPatchProxy(opts.runTool, opts.readRawFile, allowDelete), createShellProxy(opts.runTool, allowShell), createUpdatePlanProxy(opts.runManageTasks), ]; @@ -188,7 +203,11 @@ export function allowShellFromCapabilities( return !capabilities.tools.includes("run_shell"); } -function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): AgentTool { +function createApplyPatchProxy( + runTool: CodexRunTool, + readRawFile: CodexReadRawFile, + allowDelete: boolean, +): AgentTool { return stringTool({ definition: applyPatchDefinition, handler: async (rawArgs: Record): Promise => { @@ -208,7 +227,7 @@ function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): Age const lines: string[] = []; for (const op of patch.ops) { - const result = await applyOp(op, runTool, allowDelete); + const result = await applyOp(op, runTool, readRawFile, allowDelete); lines.push(result); } if (lines.length === 0) return "apply_patch: no file operations in envelope."; @@ -217,7 +236,12 @@ function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): Age }); } -async function applyOp(op: PatchOp, runTool: CodexRunTool, allowDelete: boolean): Promise { +async function applyOp( + op: PatchOp, + runTool: CodexRunTool, + readRawFile: CodexReadRawFile, + allowDelete: boolean, +): Promise { if (op.type === "add") { return requireOk( await runTool("write_file", { path: op.path, content: op.content }), @@ -242,7 +266,11 @@ async function applyOp(op: PatchOp, runTool: CodexRunTool, allowDelete: boolean) ); } - const read = await runTool("read_file", { path: op.path }); + // read_file (both the guard plugin and the underlying tools-posix impl) + // numbers its output for model display, so it cannot supply the raw text + // applyUpdateHunks needs to match context lines against (CL-6966). + // readRawFile reads the file directly instead. + const read = await readRawFile(op.path); const original = requireOk(read, `read ${op.path}`); let updated: string; diff --git a/src/agent/tools.ts b/src/agent/tools.ts index dcfecbbe..c7435c26 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -50,6 +50,7 @@ import { type CodexRunManageTasks, type CodexRunTool, } from "./codex-tool-proxies.js"; +import { createCodexReadRawFile } from "./codex-read-raw-file.js"; import type { ReactorEmittedEvent } from "@intx/inference"; const AskOperatorArgs = type({ @@ -384,7 +385,12 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise tool.definition.name !== "apply_patch"); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 4f0c265f..d357edbd 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -46,6 +46,7 @@ import { type CodexRunManageTasks, type CodexRunTool, } from "../agent/codex-tool-proxies.js"; +import { createCodexReadRawFile } from "../agent/codex-read-raw-file.js"; import { isCodexProviderName } from "../config/codex-providers.js"; import { createCompositeBlobReader } from "../agent/lazy-blob-reader.js"; @@ -376,6 +377,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { ...createCodexToolProxies({ isCodex: isCodexProviderName(params.provider.providerName), runTool, + readRawFile: createCodexReadRawFile(params.cwd, permissionGate), runManageTasks, allowDelete: allowDeleteFromCapabilities(params.capabilities), allowShell: allowShellFromCapabilities(params.capabilities),