From 682428f1a1153ce7f612646d8188e6b567bd9236 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 23 Aug 2026 14:21:18 -0700 Subject: [PATCH 1/2] fix(apply_patch): read raw file content for Update File matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_patch's Update File op read the target via the real read_file tool, whose output is cat -n formatted (line numbers prefixed). That numbered text was then matched against the patch's raw context lines, which can never succeed — every Update op with context lines failed. Add a readRawFile callback (plain fs read, no display formatting) used only for the Update leg's content-matching input; write_file still goes through the full posixTools pipeline for the actual mutation. --- src/agent/apply-patch-diff.test.ts | 57 ++++++++++++------ src/agent/codex-read-raw-file.ts | 32 ++++++++++ src/agent/codex-tool-mount.test.ts | 3 + src/agent/codex-tool-proxies.test.ts | 90 ++++++++++++++++++---------- src/agent/codex-tool-proxies.ts | 38 ++++++++++-- src/agent/tools.ts | 8 ++- src/subagent/run.ts | 2 + 7 files changed, 173 insertions(+), 57 deletions(-) create mode 100644 src/agent/codex-read-raw-file.ts diff --git a/src/agent/apply-patch-diff.test.ts b/src/agent/apply-patch-diff.test.ts index af6b3dffe..e39997c27 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 { mkdtemp, rm, 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"; @@ -36,23 +37,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 +46,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), 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 { diff --git a/src/agent/codex-read-raw-file.ts b/src/agent/codex-read-raw-file.ts new file mode 100644 index 000000000..9b3ad2fb1 --- /dev/null +++ b/src/agent/codex-read-raw-file.ts @@ -0,0 +1,32 @@ +/** + * Raw (non-`cat -n`) file reads for apply_patch's Update File leg (CL-6966). + * Mirrors the error mapping in @intx/tools-posix's read-file.js so failures + * read the same way `read_file` would report them. + */ + +import { readFile } from "node:fs/promises"; +import { resolve } from "node:path"; +import { hasCode } from "@intx/types"; +import type { CodexReadRawFile } from "./codex-tool-proxies.js"; + +/** `path` is a workspace-relative path (apply_patch rejects absolute paths). */ +export function createCodexReadRawFile(cwd: string): CodexReadRawFile { + return async (path) => { + const absolutePath = resolve(cwd, path); + 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 5c3cea10b..2c5e978c9 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 71bc6ded9..002c63222 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 f2313846b..14716d2a4 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 dcfecbbef..e6bff287b 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 4f0c265f4..f59b98929 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), runManageTasks, allowDelete: allowDeleteFromCapabilities(params.capabilities), allowShell: allowShellFromCapabilities(params.capabilities), From b5585e03c2e08691edd0ab036bfabac1fd156020 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 23 Aug 2026 14:47:07 -0700 Subject: [PATCH 2/2] fix(apply_patch): enforce workspace containment and secret-guard on raw reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readRawFile bypassed every ToolPlugin (pathEscapePlugin, secretGuardPlugin, authzPlugin, permissionPlugin) since apply_patch's applyOp calls it outside the posixTools middleware chain entirely. requireRelativePath only rejects absolute paths, not `../` traversal, so an Update File op naming a path like ../../.env with an insertion-only hunk (no context match required) could read a secret and hand it to write_file via Move to — exfiltration, not just an unauthorized read. readRawFile now reuses the same containment and secret-file authorities the plugin chain already uses instead of reimplementing them: resolveWorkspacePath (symlink-aware realpath containment, from path-restriction.ts) and isSensitivePath (the secret-guard denylist). Containment honors the same skipPermissions/yolo escape hatch pathEscapePlugin does; the secret check never bypasses, matching secretGuardPlugin's own unconditional behavior. Both are hard failures with a clear error, not a silent fallthrough. Added regression tests for the attack shapes: ../ traversal out of the workspace, a symlinked directory leading outside the workspace, a secret-guard path (.env) under skipPermissions, and ../ traversal to a secret file — all via the insertion-only hunk shape that made the unauthorized read exploitable. --- src/agent/apply-patch-diff.test.ts | 106 +++++++++++++++++++++++++++-- src/agent/codex-read-raw-file.ts | 67 ++++++++++++++++-- src/agent/tools.ts | 2 +- src/subagent/run.ts | 2 +- 4 files changed, 166 insertions(+), 11 deletions(-) diff --git a/src/agent/apply-patch-diff.test.ts b/src/agent/apply-patch-diff.test.ts index e39997c27..d73bc2b75 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, 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"; @@ -24,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, }); @@ -51,7 +54,7 @@ async function makeApplyPatch(cwd: string): Promise { return createCodexToolProxies({ isCodex: true, runTool, - readRawFile: createCodexReadRawFile(cwd), + readRawFile: createCodexReadRawFile(cwd, gate), runManageTasks: async () => ({ content: "ok" }), }); } @@ -148,3 +151,98 @@ describe("apply_patch Update File matches raw content, not read_file's numbered } }); }); + +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 index 9b3ad2fb1..dfc7110f6 100644 --- a/src/agent/codex-read-raw-file.ts +++ b/src/agent/codex-read-raw-file.ts @@ -1,18 +1,75 @@ /** * Raw (non-`cat -n`) file reads for apply_patch's Update File leg (CL-6966). - * Mirrors the error mapping in @intx/tools-posix's read-file.js so failures - * read the same way `read_file` would report them. + * + * `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 a workspace-relative path (apply_patch rejects absolute paths). */ -export function createCodexReadRawFile(cwd: string): CodexReadRawFile { +/** `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) => { - const absolutePath = resolve(cwd, 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)) { diff --git a/src/agent/tools.ts b/src/agent/tools.ts index e6bff287b..c7435c261 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -388,7 +388,7 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise { ...createCodexToolProxies({ isCodex: isCodexProviderName(params.provider.providerName), runTool, - readRawFile: createCodexReadRawFile(params.cwd), + readRawFile: createCodexReadRawFile(params.cwd, permissionGate), runManageTasks, allowDelete: allowDeleteFromCapabilities(params.capabilities), allowShell: allowShellFromCapabilities(params.capabilities),