From 00ff4835d45232f850fd2bae3469bd14e26074e6 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 13:24:38 -0700 Subject: [PATCH 1/6] Proxy Codex apply_patch onto Corbits file tools Why: Codex-trained models call apply_patch from pinned instructions, but Corbits only advertised write_file/edit_file/delete_file. That dialect mismatch hurts Codex evals versus Codex-native harnesses. What changed: Codex-only apply_patch proxy (parse envelope, forward through posix write/edit/delete), shared product-mutation ownership, primary deny unchanged, IMPLEMENT/DOCS allowlists, docs leaves refuse Delete/Move via allowDelete. Bridge text left for the next stacked change. Test plan: bun test on codex-apply-patch, codex-tool-proxies, codex-tool-mount, product-mutation-tools, tool-sets; bun run typecheck. --- docs/IMPLEMENTATION.md | 2 + src/agent/codex-apply-patch.test.ts | 306 ++++++++++++++++ src/agent/codex-apply-patch.ts | 404 ++++++++++++++++++++++ src/agent/codex-tool-mount.test.ts | 95 +++++ src/agent/codex-tool-proxies.test.ts | 331 ++++++++++++++++++ src/agent/codex-tool-proxies.ts | 243 +++++++++++++ src/agent/directors/build/package.test.ts | 1 + src/agent/directors/registry.test.ts | 2 +- src/agent/directors/tool-sets.test.ts | 12 +- src/agent/directors/tool-sets.ts | 5 +- src/agent/product-mutation-tools.test.ts | 64 ++++ src/agent/product-mutation-tools.ts | 55 +++ src/agent/tool-search.test.ts | 2 + src/agent/tool-search.ts | 2 + src/agent/tools.ts | 37 +- src/exec/runner.ts | 3 +- src/permission/classify.ts | 34 +- src/permission/gate.ts | 31 +- src/permission/write-path-policy.ts | 3 +- src/subagent/run.ts | 34 ++ src/subagent/thrash.test.ts | 25 ++ src/subagent/thrash.ts | 18 +- src/subagent/tool-preview.test.ts | 9 + src/subagent/tool-preview.ts | 9 +- src/tui/runner.ts | 1 + tests/unit/tui/agent-tools.test.ts | 2 + 26 files changed, 1699 insertions(+), 31 deletions(-) create mode 100644 src/agent/codex-apply-patch.test.ts create mode 100644 src/agent/codex-apply-patch.ts create mode 100644 src/agent/codex-tool-mount.test.ts create mode 100644 src/agent/codex-tool-proxies.test.ts create mode 100644 src/agent/codex-tool-proxies.ts create mode 100644 src/agent/product-mutation-tools.test.ts create mode 100644 src/agent/product-mutation-tools.ts diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index 660eb4fba..0d270a65e 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -159,6 +159,8 @@ Sixteen packages under `src/agent/directors//` register in `DIRECTOR_REGISTR 3. Nested spawn: packages with `spawn.allowlist` forward that list into nested `task` (`spawnAllowlist` on nestedDispatch). Off-list `agent` is refused. `task(agent=skywalker)` is refused (primary is not a spawned worker). Primary omits the list so plugin profiles stay reachable. 4. `directorProfiles()` is the spawn catalog (`default-agents.ts`) — closed set minus skywalker; plugin agent profiles still load and can override by id. 5. Primary chat role is Skywalker: `buildChatRole()` → `createSkywalkerSystemPrompt()`. Product mutation tools (`write_file` / `edit_file` / `delete_file`) live in CORE (and `SKYWALKER_TOOLS`) so they are advertised on the primary without a `tool_search` round-trip. DIY tiny/bounded edits on the parent; spawn build/docs directors for substantial work — a prompt judgment call, not a toolset strip. `PRIMARY_DENIED_PRODUCT_TOOLS` is gone. Shell file-writes stay denied; MCP tools are not re-filtered by a product-write deny list. Optional `writePaths` (when a profile sets it) only gate path-keyed product tools. + + **Codex `apply_patch` proxy.** When the active provider is Codex (`isCodexProviderName`), `createAgentToolset` and `runSubAgent` mount an `apply_patch` stringTool from `createCodexToolProxies` that parses the Codex envelope and forwards each op through the posix `ToolRunner` (`write_file` / `delete_file` / `read_file`) so permission plugins still apply. Primary strips `apply_patch` after mount (Corbits DIY stays on `write_file` / `edit_file` / `delete_file`); build and docs leaf allowlists (`BUILD_TOOLS` / `DOCS_TOOLS`) include it so Codex workers keep the proxy after the capability filter. `CORE_TOOL_NAMES` does not list it. 6. Shipped directors omit `writePaths`. The optional field is still enforced in the permission gate via ALS identity (`identity-context.ts` + `write-path-policy.ts`) when a plugin/custom profile sets it. 7. Spawn effort: pin > package `modelRole` default (`defaultEffortForDirector`; intern=low; plan/review/orchestrator=high; implement/explore/docs/test=medium) > orchestrator/worker binary > parent inheritance. Optional skills are listed in the identity header for awareness; workers do not mount `use_skill` (guidance is baked into package system prompts). Primary mounts `use_skill` for its own skill list. diff --git a/src/agent/codex-apply-patch.test.ts b/src/agent/codex-apply-patch.test.ts new file mode 100644 index 000000000..fdf64bde9 --- /dev/null +++ b/src/agent/codex-apply-patch.test.ts @@ -0,0 +1,306 @@ +import { describe, expect, test } from "bun:test"; +import { + CodexApplyPatchError, + applyUpdateHunks, + extractAffectedPaths, + parseCodexApplyPatch, +} from "./codex-apply-patch.js"; + +describe("parseCodexApplyPatch", () => { + test("parses Add File with Codex trailing newlines", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Add File: hello.txt ++Hello world ++second line +*** End Patch +`); + expect(patch.ops).toEqual([ + { + type: "add", + path: "hello.txt", + content: "Hello world\nsecond line\n", + }, + ]); + }); + + test("Add File with no + lines yields empty content", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Add File: empty.txt +*** End Patch +`); + expect(patch.ops).toEqual([{ type: "add", path: "empty.txt", content: "" }]); + }); + + test("parses Delete File", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Delete File: obsolete.txt +*** End Patch +`); + expect(patch.ops).toEqual([{ type: "delete", path: "obsolete.txt" }]); + }); + + test("parses Update File with Move to", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +*** Move to: src/main.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`); + expect(patch.ops).toHaveLength(1); + const op = patch.ops[0]!; + expect(op.type).toBe("update"); + if (op.type !== "update") throw new Error("unreachable"); + expect(op.path).toBe("src/app.py"); + expect(op.moveTo).toBe("src/main.py"); + expect(op.hunks).toHaveLength(1); + expect(op.hunks[0]!.header).toBe("def greet():"); + expect(op.hunks[0]!.lines).toEqual([ + { kind: "-", text: 'print("Hi")' }, + { kind: "+", text: 'print("Hello, world!")' }, + ]); + }); + + test("parses stacked multi-@@ context anchors as header-only then change hunk", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +@@ class BaseClass +@@ def method(): +-old_line ++new_line +*** End Patch +`); + const op = patch.ops[0]!; + expect(op.type).toBe("update"); + if (op.type !== "update") throw new Error("unreachable"); + expect(op.hunks).toHaveLength(2); + expect(op.hunks[0]).toEqual({ header: "class BaseClass", lines: [] }); + expect(op.hunks[1]!.header).toBe(" def method():"); + expect(op.hunks[1]!.lines).toEqual([ + { kind: "-", text: "old_line" }, + { kind: "+", text: "new_line" }, + ]); + }); + + test("rejects bare empty @@ without lines", () => { + expect(() => + parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +@@ +*** End Patch +`), + ).toThrow(/empty hunk/); + }); + + test("rejects malformed envelope (missing Begin)", () => { + expect(() => + parseCodexApplyPatch(`*** Add File: a.txt ++hi +*** End Patch +`), + ).toThrow(CodexApplyPatchError); + expect(() => + parseCodexApplyPatch(`*** Add File: a.txt ++hi +*** End Patch +`), + ).toThrow(/Begin Patch/); + }); + + test("rejects malformed envelope (missing End)", () => { + expect(() => + parseCodexApplyPatch(`*** Begin Patch +*** Add File: a.txt ++hi +`), + ).toThrow(/End Patch/); + }); + + test("rejects absolute paths", () => { + expect(() => + parseCodexApplyPatch(`*** Begin Patch +*** Add File: /etc/passwd ++x +*** End Patch +`), + ).toThrow(/relative/); + + expect(() => + parseCodexApplyPatch(`*** Begin Patch +*** Delete File: /tmp/x +*** End Patch +`), + ).toThrow(/absolute/); + + expect(() => + parseCodexApplyPatch(`*** Begin Patch +*** Update File: C:\\Windows\\system32\\x +@@ +-a ++b +*** End Patch +`), + ).toThrow(/absolute/); + }); +}); + +describe("extractAffectedPaths", () => { + test("multi-file path extraction", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Add File: hello.txt ++Hello +*** Update File: src/app.py +@@ +-old ++new +*** Delete File: obsolete.txt +*** End Patch +`); + expect(extractAffectedPaths(patch)).toEqual([ + "hello.txt", + "src/app.py", + "obsolete.txt", + ]); + }); + + test("move path extraction includes source and destination", () => { + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +*** Move to: src/main.py +@@ +-a ++b +*** End Patch +`); + expect(extractAffectedPaths(patch)).toEqual(["src/app.py", "src/main.py"]); + }); +}); + +describe("applyUpdateHunks", () => { + test("applies a simple replacement hunk", () => { + const original = `def greet(): +print("Hi") +print("bye") +`; + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`); + const op = patch.ops[0]!; + expect(op.type).toBe("update"); + if (op.type !== "update") throw new Error("unreachable"); + const updated = applyUpdateHunks(original, op.hunks); + expect(updated).toBe(`def greet(): +print("Hello, world!") +print("bye") +`); + }); + + test("applies stacked multi-@@ anchors then replacement", () => { + const original = `class BaseClass + def method(): + old_line + keep +`; + const patch = parseCodexApplyPatch(`*** Begin Patch +*** Update File: src/app.py +@@ class BaseClass +@@ def method(): +- old_line ++ new_line +*** End Patch +`); + const op = patch.ops[0]!; + expect(op.type).toBe("update"); + if (op.type !== "update") throw new Error("unreachable"); + expect(applyUpdateHunks(original, op.hunks)).toBe(`class BaseClass + def method(): + new_line + keep +`); + }); + + test("applies context-aware multi-line hunk", () => { + const original = `line1 +line2 +target +line4 +`; + const hunks = [ + { + lines: [ + { kind: " " as const, text: "line2" }, + { kind: "-" as const, text: "target" }, + { kind: "+" as const, text: "replaced" }, + { kind: " " as const, text: "line4" }, + ], + }, + ]; + expect(applyUpdateHunks(original, hunks)).toBe(`line1 +line2 +replaced +line4 +`); + }); + + test("fuzzy match: rstrip then trim after exact fail", () => { + const original = `foo +bar +baz +`; + const updated = applyUpdateHunks(original, [ + { + lines: [ + { kind: "-", text: "bar" }, + { kind: "+", text: "qux" }, + ], + }, + ]); + expect(updated).toBe(`foo +qux +baz +`); + + const padded = applyUpdateHunks(` foo \nbar\n`, [ + { + lines: [ + { kind: "-", text: "foo" }, + { kind: "+", text: "FOO" }, + ], + }, + ]); + expect(padded).toBe(`FOO +bar +`); + }); + + test("NormalizeToLf: non-empty update result ends with newline", () => { + const updated = applyUpdateHunks("a\nb", [ + { + lines: [ + { kind: "-", text: "b" }, + { kind: "+", text: "c" }, + ], + }, + ]); + expect(updated).toBe("a\nc\n"); + expect(updated.endsWith("\n")).toBe(true); + }); + + test("throws when context cannot be found", () => { + expect(() => + applyUpdateHunks("a\nb\n", [ + { + lines: [ + { kind: "-", text: "missing" }, + { kind: "+", text: "x" }, + ], + }, + ]), + ).toThrow(/failed to find expected lines/); + }); +}); diff --git a/src/agent/codex-apply-patch.ts b/src/agent/codex-apply-patch.ts new file mode 100644 index 000000000..1aa98ca55 --- /dev/null +++ b/src/agent/codex-apply-patch.ts @@ -0,0 +1,404 @@ +/** + * Pure parser/applier for Codex `apply_patch` envelopes. + * + * Grammar (subset of codex-rs apply-patch): + * Patch := "*** Begin Patch" NEWLINE { FileOp } "*** End Patch" [NEWLINE] + * FileOp := AddFile | DeleteFile | UpdateFile + * AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE } + * DeleteFile := "*** Delete File: " path NEWLINE + * UpdateFile := "*** Update File: " path NEWLINE [ "*** Move to: " path NEWLINE ] { Hunk } + * Hunk := "@@" [ " " header ] NEWLINE { (" "|"-"|"+") text NEWLINE } [ "*** End of File" NEWLINE ] + * + * Stacked `@@` anchors (class → method) are accepted as consecutive header-only + * hunks that advance the apply cursor before a hunk with +/- lines. + * + * No filesystem I/O, no shell, no dependencies — parse + string apply only. + */ + +import { isAbsolute } from "node:path"; + +const BEGIN_PATCH = "*** Begin Patch"; +const END_PATCH = "*** End Patch"; +const ADD_FILE = "*** Add File: "; +const DELETE_FILE = "*** Delete File: "; +const UPDATE_FILE = "*** Update File: "; +const MOVE_TO = "*** Move to: "; +const END_OF_FILE = "*** End of File"; + +export type HunkLineKind = " " | "-" | "+"; + +export type PatchHunkLine = { + kind: HunkLineKind; + text: string; +}; + +export type PatchHunk = { + /** Optional text after `@@` (class/method anchor). */ + header?: string; + lines: PatchHunkLine[]; + endOfFile?: boolean; +}; + +export type PatchAddOp = { + type: "add"; + path: string; + /** + * File body reconstructed from `+` lines. Each `+` line contributes + * `text + "\n"` (Codex-rs parity), so non-empty adds end with a trailing newline. + * An Add File with no `+` lines yields `""`. + */ + content: string; +}; + +export type PatchDeleteOp = { + type: "delete"; + path: string; +}; + +export type PatchUpdateOp = { + type: "update"; + path: string; + moveTo?: string; + hunks: PatchHunk[]; +}; + +export type PatchOp = PatchAddOp | PatchDeleteOp | PatchUpdateOp; + +export type ParsedPatch = { + ops: PatchOp[]; +}; + +export class CodexApplyPatchError extends Error { + constructor(message: string) { + super(message); + this.name = "CodexApplyPatchError"; + } +} + +/** Parse a full `*** Begin Patch` … `*** End Patch` envelope into file ops. */ +export function parseCodexApplyPatch(input: string): ParsedPatch { + const rawLines = splitLines(input); + if (rawLines.length === 0) { + throw new CodexApplyPatchError("empty patch: expected '*** Begin Patch'"); + } + + // Tolerate a single trailing blank from a final newline after End Patch. + let lines = rawLines; + if (lines.length > 0 && lines[lines.length - 1] === "") { + lines = lines.slice(0, -1); + } + + if (lines[0]?.trim() !== BEGIN_PATCH) { + throw new CodexApplyPatchError("malformed envelope: first line must be '*** Begin Patch'"); + } + if (lines[lines.length - 1]?.trim() !== END_PATCH) { + throw new CodexApplyPatchError("malformed envelope: last line must be '*** End Patch'"); + } + + const body = lines.slice(1, -1); + const ops: PatchOp[] = []; + let i = 0; + + while (i < body.length) { + const line = body[i]!; + if (line.startsWith(ADD_FILE)) { + const path = requireRelativePath(line.slice(ADD_FILE.length), "Add File"); + i += 1; + const contentLines: string[] = []; + while (i < body.length && body[i]!.startsWith("+")) { + contentLines.push(body[i]!.slice(1)); + i += 1; + } + if (i < body.length && !isFileOpHeader(body[i]!)) { + throw new CodexApplyPatchError( + `malformed Add File '${path}': expected '+' content lines or next file op, got: ${body[i]}`, + ); + } + // Codex-rs: each '+' line contributes text + "\n". + const content = + contentLines.length === 0 ? "" : contentLines.map((l) => `${l}\n`).join(""); + ops.push({ type: "add", path, content }); + continue; + } + + if (line.startsWith(DELETE_FILE)) { + const path = requireRelativePath(line.slice(DELETE_FILE.length), "Delete File"); + i += 1; + ops.push({ type: "delete", path }); + continue; + } + + if (line.startsWith(UPDATE_FILE)) { + const path = requireRelativePath(line.slice(UPDATE_FILE.length), "Update File"); + i += 1; + let moveTo: string | undefined; + if (i < body.length && body[i]!.startsWith(MOVE_TO)) { + moveTo = requireRelativePath(body[i]!.slice(MOVE_TO.length), "Move to"); + i += 1; + } + const hunks: PatchHunk[] = []; + while (i < body.length && isHunkStart(body[i]!)) { + const { hunk, next } = parseHunk(body, i); + hunks.push(hunk); + i = next; + } + if (i < body.length && !isFileOpHeader(body[i]!)) { + throw new CodexApplyPatchError( + `malformed Update File '${path}': expected hunk ('@@') or next file op, got: ${body[i]}`, + ); + } + ops.push(moveTo === undefined ? { type: "update", path, hunks } : { type: "update", path, moveTo, hunks }); + continue; + } + + throw new CodexApplyPatchError( + `malformed envelope: expected file op header (Add/Delete/Update File), got: ${line}`, + ); + } + + return { ops }; +} + +/** + * Relative paths touched by the patch, in encounter order. + * Update-with-move contributes both source and destination. + */ +export function extractAffectedPaths(patch: ParsedPatch): string[] { + const out: string[] = []; + const seen = new Set(); + const push = (p: string) => { + if (seen.has(p)) return; + seen.add(p); + out.push(p); + }; + for (const op of patch.ops) { + if (op.type === "add" || op.type === "delete") { + push(op.path); + } else { + push(op.path); + if (op.moveTo !== undefined) push(op.moveTo); + } + } + return out; +} + +/** + * Apply update hunks to an in-memory file body. Returns the updated string. + * NormalizeToLf-ish: non-empty results end with `\n` (Codex default update mode). + */ +export function applyUpdateHunks(original: string, hunks: PatchHunk[]): string { + let lines = original === "" ? [] : original.replace(/\n$/, "").split("\n"); + // `split` on a lone "\n" yields [""]; treat that as empty content. + if (lines.length === 1 && lines[0] === "" && original === "\n") { + lines = []; + } + + let cursor = 0; + for (const hunk of hunks) { + if (hunk.header !== undefined && hunk.header.length > 0) { + const idx = findLineFrom(lines, hunk.header, cursor); + if (idx === -1) { + throw new CodexApplyPatchError( + `failed to find hunk context header '${hunk.header}'`, + ); + } + cursor = idx + 1; + } + + const oldLines: string[] = []; + const newLines: string[] = []; + for (const hl of hunk.lines) { + if (hl.kind === " " || hl.kind === "-") oldLines.push(hl.text); + if (hl.kind === " " || hl.kind === "+") newLines.push(hl.text); + } + + if (oldLines.length === 0) { + // Context-only @@ anchor (no +/-): cursor already advanced via header. + if (newLines.length === 0) continue; + // Pure insertion (e.g. append). Place at EOF when endOfFile, else at cursor. + const at = hunk.endOfFile ? lines.length : cursor; + lines = [...lines.slice(0, at), ...newLines, ...lines.slice(at)]; + cursor = at + newLines.length; + continue; + } + + const start = findSequence(lines, oldLines, cursor, hunk.endOfFile === true); + if (start === -1) { + throw new CodexApplyPatchError( + `failed to find expected lines in file:\n${oldLines.join("\n")}`, + ); + } + lines = [...lines.slice(0, start), ...newLines, ...lines.slice(start + oldLines.length)]; + cursor = start + newLines.length; + } + + if (lines.length === 0) { + // Empty file: preserve empty; a prior lone newline becomes "\n" only when + // original had content-as-newline — NormalizeToLf leaves truly empty as "". + return original.length > 0 ? "\n" : ""; + } + return `${lines.join("\n")}\n`; +} + +/** Content for an Add File op (already on the op; helper for call sites). */ +export function contentFromAddOp(op: PatchAddOp): string { + return op.content; +} + +function parseHunk( + body: string[], + start: number, +): { hunk: PatchHunk; next: number } { + const headerLine = body[start]!; + let header: string | undefined; + if (headerLine === "@@") { + header = undefined; + } else if (headerLine.startsWith("@@ ")) { + header = headerLine.slice(3); + } else if (headerLine.startsWith("@@")) { + header = headerLine.slice(2).trimStart(); + } else { + throw new CodexApplyPatchError(`expected hunk start '@@', got: ${headerLine}`); + } + + let i = start + 1; + const lines: PatchHunkLine[] = []; + while (i < body.length) { + const raw = body[i]!; + if (raw === END_OF_FILE) { + i += 1; + return { + hunk: header === undefined + ? { lines, endOfFile: true } + : { header, lines, endOfFile: true }, + next: i, + }; + } + if (isHunkStart(raw) || isFileOpHeader(raw)) break; + if (raw.startsWith("***")) { + throw new CodexApplyPatchError(`unexpected marker inside hunk: ${raw}`); + } + const kind = raw[0]; + if (kind !== " " && kind !== "-" && kind !== "+") { + throw new CodexApplyPatchError( + `invalid hunk line (must start with ' ', '-', or '+'): ${raw}`, + ); + } + lines.push({ kind, text: raw.slice(1) }); + i += 1; + } + + // Allow header-only hunks so stacked `@@ class` / `@@ method` anchors can + // precede a hunk that carries the +/- lines (Codex multi-@@ grammar). + if (lines.length === 0) { + if (header === undefined || header.length === 0) { + throw new CodexApplyPatchError( + "empty hunk: expected at least one ' '/'+'/'-' line after '@@'", + ); + } + } + + return { + hunk: header === undefined ? { lines } : { header, lines }, + next: i, + }; +} + +function isFileOpHeader(line: string): boolean { + return ( + line.startsWith(ADD_FILE) || + line.startsWith(DELETE_FILE) || + line.startsWith(UPDATE_FILE) + ); +} + +function isHunkStart(line: string): boolean { + return line === "@@" || line.startsWith("@@"); +} + +function requireRelativePath(raw: string, label: string): string { + const path = raw.trim(); + if (path.length === 0) { + throw new CodexApplyPatchError(`${label}: path must be non-empty`); + } + if (isAbsolute(path) || isWindowsAbsolute(path)) { + throw new CodexApplyPatchError( + `${label}: path must be relative, never absolute (got '${path}')`, + ); + } + return path; +} + +function isWindowsAbsolute(path: string): boolean { + return /^[A-Za-z]:[\\/]/.test(path) || path.startsWith("\\\\"); +} + +function splitLines(input: string): string[] { + // Preserve empty trailing segment so callers can detect a final newline. + return input.split("\n"); +} + +function findLineFrom(lines: string[], target: string, from: number): number { + for (let i = from; i < lines.length; i++) { + if (lines[i] === target) return i; + } + // Soften header seek the same way as hunk body matching. + for (let i = from; i < lines.length; i++) { + if (lines[i]!.trimEnd() === target.trimEnd()) return i; + } + for (let i = from; i < lines.length; i++) { + if (lines[i]!.trim() === target.trim()) return i; + } + return -1; +} + +/** + * Codex seek_sequence subset: exact, then rstrip, then trim. + * Unicode punctuation normalize is intentionally omitted. + */ +function findSequence( + lines: string[], + pattern: string[], + from: number, + endOfFile: boolean, +): number { + if (pattern.length === 0) return from; + if (pattern.length > lines.length) return -1; + + const searchStart = + endOfFile && lines.length >= pattern.length + ? lines.length - pattern.length + : from; + + const tryFrom = (start: number, eq: (a: string, b: string) => boolean): number => { + for (let i = start; i <= lines.length - pattern.length; i++) { + let ok = true; + for (let j = 0; j < pattern.length; j++) { + if (!eq(lines[i + j]!, pattern[j]!)) { + ok = false; + break; + } + } + if (ok) return i; + } + return -1; + }; + + // When eof, try the eof-aligned window first, then fall through from `from`. + const starts = + endOfFile && searchStart !== from ? [searchStart, from] : [searchStart]; + + for (const start of starts) { + const exact = tryFrom(start, (a, b) => a === b); + if (exact !== -1) return exact; + } + for (const start of starts) { + const rstrip = tryFrom(start, (a, b) => a.trimEnd() === b.trimEnd()); + if (rstrip !== -1) return rstrip; + } + for (const start of starts) { + const trimmed = tryFrom(start, (a, b) => a.trim() === b.trim()); + if (trimmed !== -1) return trimmed; + } + return -1; +} diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts new file mode 100644 index 000000000..884ce73f8 --- /dev/null +++ b/src/agent/codex-tool-mount.test.ts @@ -0,0 +1,95 @@ +/** + * Mount coverage for Codex apply_patch proxies: primary strip, allowlists, + * and build-shaped capability filter retention. + */ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, test, spyOn } from "bun:test"; +import * as posixModule from "@intx/tools-posix"; + +import { createCodexToolProxies } from "./codex-tool-proxies.js"; +import { BUILD_TOOLS, DOCS_TOOLS } from "./directors/tool-sets.js"; +import { CORE_TOOL_NAMES } from "./tool-search.js"; + +afterEach(() => { + spyOn(posixModule, "createPosixTools").mockRestore(); +}); + +describe("Codex apply_patch mount", () => { + test("non-Codex createAgentToolset does not advertise apply_patch on primary", async () => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); + spyOn(posixModule, "createPosixTools").mockReturnValue({ + definitions: [], + run: async () => ({ id: "x", content: "" }), + dispose: async () => {}, + } as unknown as ReturnType); + + const { createAgentToolset } = await import("./tools.js"); + const permissionGate = { + check: async () => ({ allowed: true }), + getSkipPermissions: () => false, + } as never; + + const toolset = await createAgentToolset({ + cwd, + permissionGate, + onOperatorGate: async () => ({ kind: "option", index: 0 }), + isCodex: false, + }); + const names = toolset.dynamicRunner.currentDefinitions().map((d) => d.name); + expect(names).not.toContain("apply_patch"); + await toolset.dispose(); + }); + + test("Codex createAgentToolset strips apply_patch on primary after mount", async () => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); + spyOn(posixModule, "createPosixTools").mockReturnValue({ + definitions: [], + run: async () => ({ id: "x", content: "" }), + dispose: async () => {}, + } as unknown as ReturnType); + + const { createAgentToolset } = await import("./tools.js"); + const permissionGate = { + check: async () => ({ allowed: true }), + getSkipPermissions: () => false, + } as never; + + const toolset = await createAgentToolset({ + cwd, + permissionGate, + onOperatorGate: async () => ({ kind: "option", index: 0 }), + isCodex: true, + }); + const names = toolset.dynamicRunner.currentDefinitions().map((d) => d.name); + expect(names).not.toContain("apply_patch"); + // Primary DIY product writes remain mounted. + expect(names).toContain("write_file"); + expect(names).toContain("edit_file"); + expect(names).toContain("delete_file"); + await toolset.dispose(); + }); + + test("BUILD_TOOLS and DOCS_TOOLS include apply_patch; CORE_TOOL_NAMES does not", () => { + expect(BUILD_TOOLS).toContain("apply_patch"); + expect(DOCS_TOOLS).toContain("apply_patch"); + expect(CORE_TOOL_NAMES).not.toContain("apply_patch"); + }); + + test("capability include-filter keeps apply_patch for build-shaped allowlists", () => { + const proxies = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "ok" }), + }); + expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch"]); + + const allow = new Set(BUILD_TOOLS); + const kept = proxies.filter((t) => allow.has(t.definition.name)); + expect(kept.map((t) => t.definition.name)).toContain("apply_patch"); + + const docsAllow = new Set(DOCS_TOOLS); + const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); + expect(docsKept.map((t) => t.definition.name)).toContain("apply_patch"); + }); +}); diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts new file mode 100644 index 000000000..9881483eb --- /dev/null +++ b/src/agent/codex-tool-proxies.test.ts @@ -0,0 +1,331 @@ +import { describe, expect, test } from "bun:test"; +import { createToolRunner } from "@intx/agent"; +import type { AgentTool } from "@intx/agent"; + +import { + allowDeleteFromCapabilities, + createCodexToolProxies, + type CodexRunTool, +} from "./codex-tool-proxies.js"; +import { DOCS_TOOLS, BUILD_TOOLS } from "./directors/tool-sets.js"; + +type Call = { name: string; args: Record }; + +function makeRecorder(initial: Record = {}): { + calls: Call[]; + files: Map; + runTool: CodexRunTool; +} { + const files = new Map(Object.entries(initial)); + const calls: Call[] = []; + const runTool: CodexRunTool = async (name, args) => { + calls.push({ name, args }); + if (name === "read_file") { + const path = String(args.path ?? ""); + const content = files.get(path); + if (content === undefined) { + return { content: `File not found: ${path}`, isError: true }; + } + return { content }; + } + if (name === "write_file") { + const path = String(args.path ?? ""); + files.set(path, String(args.content ?? "")); + return { content: `Wrote file: ${path}` }; + } + if (name === "delete_file") { + const path = String(args.path ?? ""); + files.delete(path); + return { content: `Deleted file: ${path}` }; + } + return { content: `unknown tool: ${name}`, isError: true }; + }; + return { calls, files, runTool }; +} + +async function invokeApplyPatch(tools: AgentTool[], input: string) { + const runner = createToolRunner(tools); + return runner.run( + { id: "call-1", name: "apply_patch", arguments: { input } }, + new AbortController().signal, + ); +} + +describe("createCodexToolProxies", () => { + test("returns [] when not Codex", () => { + const tools = createCodexToolProxies({ + isCodex: false, + runTool: async () => ({ content: "unused" }), + }); + expect(tools).toEqual([]); + }); + + test("returns apply_patch stringTool when Codex", () => { + const tools = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "unused" }), + }); + expect(tools).toHaveLength(1); + expect(tools[0]!.definition.name).toBe("apply_patch"); + expect(tools[0]!.kind).toBe("string"); + expect(tools[0]!.definition.inputSchema).toMatchObject({ + required: ["input"], + }); + }); + + test("add forwards write_file with Codex trailing newline", async () => { + const { calls, files, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Add File: hello.txt ++Hello world ++second line +*** End Patch +`, + ); + expect(result.isError).toBeFalsy(); + expect(calls).toEqual([ + { + name: "write_file", + args: { path: "hello.txt", content: "Hello world\nsecond line\n" }, + }, + ]); + expect(files.get("hello.txt")).toBe("Hello world\nsecond line\n"); + expect(result.content).toContain("Wrote file: hello.txt"); + }); + + test("delete forwards delete_file", async () => { + const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Delete File: obsolete.txt +*** End Patch +`, + ); + expect(result.isError).toBeFalsy(); + expect(calls).toEqual([{ name: "delete_file", args: { path: "obsolete.txt" } }]); + expect(files.has("obsolete.txt")).toBe(false); + expect(result.content).toContain("Deleted file: obsolete.txt"); + }); + + test("allowDelete false refuses Delete without calling delete_file", async () => { + const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Delete File: obsolete.txt +*** End Patch +`, + ); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/Delete File is not allowed/); + expect(result.content).toMatch(/delete_file capability missing/); + expect(calls).toEqual([]); + expect(files.get("obsolete.txt")).toBe("gone"); + }); + + test("allowDelete false refuses Update+Move without calling delete_file", async () => { + const original = `def greet(): +print("Hi") +`; + const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Update File: src/app.py +*** Move to: src/main.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`, + ); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/Move to is not allowed/); + expect(calls).toEqual([]); + expect(files.get("src/app.py")).toBe(original); + expect(files.has("src/main.py")).toBe(false); + }); + + test("allowDelete false still allows Update without move", async () => { + const original = `def greet(): +print("Hi") +`; + const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Update File: src/app.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`, + ); + expect(result.isError).toBeFalsy(); + expect(calls.map((c) => c.name)).toEqual(["read_file", "write_file"]); + expect(files.get("src/app.py")).toBe(`def greet(): +print("Hello, world!") +`); + }); + + test("update reads, applies hunks, and writes", async () => { + const original = `def greet(): +print("Hi") +print("bye") +`; + const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Update File: src/app.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`, + ); + 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(files.get("src/app.py")).toBe(`def greet(): +print("Hello, world!") +print("bye") +`); + }); + + test("update with Move to writes new path then deletes old", async () => { + const original = `def greet(): +print("Hi") +`; + const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Update File: src/app.py +*** Move to: src/main.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** End Patch +`, + ); + 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(): +print("Hello, world!") +`); + expect(calls[2]!.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!") +`); + }); + + test("multi-op patch runs each op in order", async () => { + const { calls, files, runTool } = makeRecorder({ + "src/app.py": "old\n", + "obsolete.txt": "x", + }); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Add File: hello.txt ++Hello world +*** Update File: src/app.py +@@ +-old ++new +*** Delete File: obsolete.txt +*** End Patch +`, + ); + expect(result.isError).toBeFalsy(); + expect(calls.map((c) => c.name)).toEqual([ + "write_file", + "read_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 tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Add File: a.txt ++hi +*** End Patch +`, + ); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/Begin Patch/); + expect(calls).toEqual([]); + }); + + test("missing input surfaces as tool error", async () => { + const tools = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "unused" }), + }); + const runner = createToolRunner(tools); + const result = await runner.run( + { id: "call-1", name: "apply_patch", arguments: {} }, + new AbortController().signal, + ); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/input/); + }); + + test("runTool isError aborts the patch with isError", async () => { + const { runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeApplyPatch( + tools, + `*** Begin Patch +*** Update File: missing.py +@@ +-a ++b +*** End Patch +`, + ); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/missing\.py/); + }); +}); + +describe("allowDeleteFromCapabilities", () => { + test("docs allowlist (no delete_file) → false; build → true", () => { + expect( + allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS }), + ).toBe(false); + expect( + allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), + ).toBe(true); + expect(allowDeleteFromCapabilities(undefined)).toBe(true); + expect( + allowDeleteFromCapabilities({ mode: "exclude", tools: ["run_shell"] }), + ).toBe(true); + expect( + allowDeleteFromCapabilities({ mode: "exclude", tools: ["delete_file"] }), + ).toBe(false); + }); +}); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts new file mode 100644 index 000000000..478a40b13 --- /dev/null +++ b/src/agent/codex-tool-proxies.ts @@ -0,0 +1,243 @@ +/** + * Codex-only tool proxies. Factory only — mounting into createAgentToolset / + * runSubAgent is intentionally out of scope for this module. + */ + +import { type } from "arktype"; +import { stringTool } from "@intx/agent"; +import type { AgentTool } from "@intx/agent"; +import type { ToolDefinition } from "@intx/types/runtime"; + +import { + CodexApplyPatchError, + applyUpdateHunks, + parseCodexApplyPatch, + type PatchOp, +} from "./codex-apply-patch.js"; + +export type CodexRunTool = ( + name: string, + args: Record, +) => Promise<{ content: string; isError?: boolean }>; + +export type CreateCodexToolProxiesOpts = { + isCodex: boolean; + runTool: CodexRunTool; + /** + * When false, Delete File and Update+Move refuse without calling `delete_file`. + * Defaults to true (implement / unconstrained). Docs leaves pass false because + * DOCS_TOOLS includes apply_patch but not delete_file. + */ + allowDelete?: boolean; +}; + +const ApplyPatchArgs = type({ + input: "string>0", +}); + +/** Mirrors APPLY_PATCH_JSON_TOOL_DESCRIPTION from openai/codex apply_patch_tool.rs. */ +export const APPLY_PATCH_DESCRIPTION = `Use the \`apply_patch\` tool to edit files. +Your patch language is a stripped-down, file-oriented diff format designed to be easy to parse and safe to apply. You can think of it as a high-level envelope: + +*** Begin Patch +[ one or more file sections ] +*** End Patch + +Within that envelope, you get a sequence of file operations. +You MUST include a header to specify the action you are taking. +Each operation starts with one of three headers: + +*** Add File: - create a new file. Every following line is a + line (the initial contents). +*** Delete File: - remove an existing file. Nothing follows. +*** Update File: - patch an existing file in place (optionally with a rename). + +May be immediately followed by *** Move to: if you want to rename the file. +Then one or more “hunks”, each introduced by @@ (optionally followed by a hunk header). +Within a hunk each line starts with: + +For instructions on [context_before] and [context_after]: +- By default, show 3 lines of code immediately above and 3 lines immediately below each change. If a change is within 3 lines of a previous change, do NOT duplicate the first change’s [context_after] lines in the second change’s [context_before] lines. +- If 3 lines of context is insufficient to uniquely identify the snippet of code within the file, use the @@ operator to indicate the class or function to which the snippet belongs. For instance, we might have: +@@ class BaseClass +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] + +- If a code block is repeated so many times in a class or function such that even a single \`@@\` statement and 3 lines of context cannot uniquely identify the snippet of code, you can use multiple \`@@\` statements to jump to the right context. For instance: + +@@ class BaseClass +@@ \tdef method(): +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] + +The full grammar definition is below: +Patch := Begin { FileOp } End +Begin := "*** Begin Patch" NEWLINE +End := "*** End Patch" NEWLINE +FileOp := AddFile | DeleteFile | UpdateFile +AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE } +DeleteFile := "*** Delete File: " path NEWLINE +UpdateFile := "*** Update File: " path NEWLINE [ MoveTo ] { Hunk } +MoveTo := "*** Move to: " newPath NEWLINE +Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ] +HunkLine := (" " | "-" | "+") text NEWLINE + +A full patch can combine several operations: + +*** Begin Patch +*** Add File: hello.txt ++Hello world +*** Update File: src/app.py +*** Move to: src/main.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** Delete File: obsolete.txt +*** End Patch + +It is important to remember: + +- You must include a header with your intended action (Add/Delete/Update) +- You must prefix new lines with \`+\` even when creating a new file +- File references can only be relative, NEVER ABSOLUTE. +`; + +export const applyPatchDefinition: ToolDefinition = { + name: "apply_patch", + description: APPLY_PATCH_DESCRIPTION, + inputSchema: { + type: "object", + properties: { + input: { + type: "string", + description: "The entire contents of the apply_patch command", + }, + }, + required: ["input"], + }, +}; + +/** + * When `isCodex` is false, returns []. Otherwise returns a single `apply_patch` + * stringTool that parses the Codex envelope and forwards each op through + * `runTool` (write_file / delete_file / read_file). + */ +export function createCodexToolProxies(opts: CreateCodexToolProxiesOpts): AgentTool[] { + if (!opts.isCodex) return []; + const allowDelete = opts.allowDelete !== false; + return [createApplyPatchProxy(opts.runTool, allowDelete)]; +} + +/** + * Resolve whether apply_patch may forward Delete / Move-delete given a leaf + * capability filter. Allow-mode lists that omit `delete_file` (docs) refuse; + * unconstrained / exclude-without-delete keep delete enabled. + */ +export function allowDeleteFromCapabilities( + capabilities: { mode: "allow" | "exclude"; tools: readonly string[] } | undefined, +): boolean { + if (capabilities === undefined) return true; + if (capabilities.mode === "allow") { + return capabilities.tools.includes("delete_file"); + } + return !capabilities.tools.includes("delete_file"); +} + +function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): AgentTool { + return stringTool({ + definition: applyPatchDefinition, + handler: async (rawArgs: Record): Promise => { + const parsed = ApplyPatchArgs(rawArgs); + if (parsed instanceof type.errors) { + // stringTool surfaces thrown errors as ToolResult.isError via createToolRunner. + throw new Error("Error: apply_patch requires a non-empty input (string)."); + } + + let patch; + try { + patch = parseCodexApplyPatch(parsed.input); + } catch (err) { + if (err instanceof CodexApplyPatchError) throw err; + throw err; + } + + const lines: string[] = []; + for (const op of patch.ops) { + const result = await applyOp(op, runTool, allowDelete); + lines.push(result); + } + if (lines.length === 0) return "apply_patch: no file operations in envelope."; + return lines.join("\n"); + }, + }); +} + +async function applyOp( + op: PatchOp, + runTool: CodexRunTool, + allowDelete: boolean, +): Promise { + if (op.type === "add") { + return requireOk( + await runTool("write_file", { path: op.path, content: op.content }), + `add ${op.path}`, + ); + } + + if (op.type === "delete") { + if (!allowDelete) { + throw new Error( + `apply_patch: Delete File is not allowed for this agent (delete_file capability missing): ${op.path}`, + ); + } + return requireOk(await runTool("delete_file", { path: op.path }), `delete ${op.path}`); + } + + // update (+ optional move): read → applyUpdateHunks → write (to moveTo or path) + // → delete old path when moving. Refuse Move before any I/O when delete is disallowed. + if (op.moveTo !== undefined && !allowDelete) { + throw new Error( + `apply_patch: Update File with Move to is not allowed for this agent (delete_file capability missing): ${op.path} → ${op.moveTo}`, + ); + } + + const read = await runTool("read_file", { path: op.path }); + const original = requireOk(read, `read ${op.path}`); + + let updated: string; + try { + updated = applyUpdateHunks(original, op.hunks); + } catch (err) { + if (err instanceof CodexApplyPatchError) throw err; + throw err; + } + + const writePath = op.moveTo ?? op.path; + const writeMsg = requireOk( + await runTool("write_file", { path: writePath, content: updated }), + `write ${writePath}`, + ); + + if (op.moveTo !== undefined) { + const deleteMsg = requireOk( + await runTool("delete_file", { path: op.path }), + `delete ${op.path} (after move to ${op.moveTo})`, + ); + return `${writeMsg}\n${deleteMsg}`; + } + + return writeMsg; +} + +function requireOk( + result: { content: string; isError?: boolean }, + label: string, +): string { + if (result.isError === true) { + throw new Error(`apply_patch failed (${label}): ${result.content}`); + } + return result.content; +} diff --git a/src/agent/directors/build/package.test.ts b/src/agent/directors/build/package.test.ts index f941fd4ba..6b754227b 100644 --- a/src/agent/directors/build/package.test.ts +++ b/src/agent/directors/build/package.test.ts @@ -24,6 +24,7 @@ describe("buildDirectorPackage", () => { expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); expect(allow).toContain("delete_file"); + expect(allow).toContain("apply_patch"); }); test("report.requiredSections includes Summary, Findings, Blockers, Paths", () => { diff --git a/src/agent/directors/registry.test.ts b/src/agent/directors/registry.test.ts index 826853434..0d1869bb1 100644 --- a/src/agent/directors/registry.test.ts +++ b/src/agent/directors/registry.test.ts @@ -156,7 +156,7 @@ describe("director registry", () => { test("build mounts product writes; intern is shell-only; other leaves do not spawn", () => { expect(DIRECTOR_REGISTRY.build.tools?.allow).toEqual( - expect.arrayContaining(["write_file", "edit_file", "delete_file"]), + expect.arrayContaining(["write_file", "edit_file", "delete_file", "apply_patch"]), ); const internAllow = DIRECTOR_REGISTRY.intern.tools?.allow ?? []; expect(internAllow).toContain("run_shell"); diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index 74e443814..be8bc0a52 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -13,7 +13,7 @@ describe("DOCS_TOOLS", () => { expect(DOCS_TOOLS).not.toContain("delete_file"); }); - test("keeps read/search/lsp/web + file writes", () => { + test("keeps read/search/lsp/web + file writes + apply_patch", () => { const expected: readonly string[] = [ "read_file", "grep", @@ -24,6 +24,7 @@ describe("DOCS_TOOLS", () => { "web_search", "write_file", "edit_file", + "apply_patch", ]; for (const tool of expected) { expect(DOCS_TOOLS as readonly string[]).toContain(tool); @@ -47,3 +48,12 @@ describe("SKYWALKER_TOOLS / ORCHESTRATOR_TOOLS", () => { expect(ORCHESTRATOR_TOOLS).toContain("task"); }); }); + +describe("BUILD_TOOLS", () => { + test("includes apply_patch alongside path mutation tools", () => { + expect(BUILD_TOOLS).toContain("write_file"); + expect(BUILD_TOOLS).toContain("edit_file"); + expect(BUILD_TOOLS).toContain("delete_file"); + expect(BUILD_TOOLS).toContain("apply_patch"); + }); +}); \ No newline at end of file diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index 42cc41f4a..bc7ec0d0f 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -22,6 +22,7 @@ export const BUILD_TOOLS = [ "write_file", "edit_file", "delete_file", + "apply_patch", ] as const; /** @@ -31,12 +32,14 @@ export const BUILD_TOOLS = [ * is still enforced by the permission gate on path-keyed write tools. * * Composed from READ_TOOLS minus run_shell so it tracks the read surface - * automatically; only the write tools are added explicitly. + * automatically; only the write tools are added explicitly. `apply_patch` is + * included so Codex docs leaves keep the proxy after the capability filter. */ export const DOCS_TOOLS = [ ...READ_TOOLS.filter((t) => t !== "run_shell"), "write_file", "edit_file", + "apply_patch", ] as const; /** Review / counsel: read surface, no writes. */ diff --git a/src/agent/product-mutation-tools.test.ts b/src/agent/product-mutation-tools.test.ts new file mode 100644 index 000000000..4a4cf6f7f --- /dev/null +++ b/src/agent/product-mutation-tools.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, test } from "bun:test"; + +import { + PRODUCT_MUTATION_TOOLS, + isProductMutationTool, + productMutationPaths, +} from "./product-mutation-tools.js"; +import { buildRequests } from "../permission/classify.js"; + +describe("PRODUCT_MUTATION_TOOLS", () => { + test("includes apply_patch alongside path-arg mutation tools", () => { + expect([...PRODUCT_MUTATION_TOOLS]).toEqual([ + "write_file", + "edit_file", + "delete_file", + "apply_patch", + ]); + }); + + test("isProductMutationTool recognizes apply_patch and rejects reads", () => { + expect(isProductMutationTool("apply_patch")).toBe(true); + expect(isProductMutationTool("read_file")).toBe(false); + }); + + test("productMutationPaths extracts apply_patch envelope subjects", () => { + const input = `*** Begin Patch +*** Add File: hello.txt ++Hello +*** Update File: src/app.py +@@ +-old ++new +*** End Patch +`; + expect(productMutationPaths("apply_patch", { input })).toEqual([ + "hello.txt", + "src/app.py", + ]); + }); + + test("productMutationPaths returns [] for malformed apply_patch input", () => { + expect(productMutationPaths("apply_patch", { input: "not a patch" })).toEqual([]); + expect(productMutationPaths("apply_patch", {})).toEqual([]); + }); + + test("classify buildRequests recognizes apply_patch as a mutation tool", () => { + const input = `*** Begin Patch +*** Add File: src/a.ts ++x +*** End Patch +`; + const reqs = buildRequests({ id: "c", name: "apply_patch", arguments: { input } }); + expect(reqs).toHaveLength(1); + expect(reqs[0]?.tool).toBe("apply_patch"); + expect(reqs[0]?.subject).toBe("src/a.ts"); + expect(reqs[0]?.action).toBe("Apply patch"); + }); + + test("isProductMutationTool names are auto-allow candidates by shared membership", () => { + for (const name of PRODUCT_MUTATION_TOOLS) { + expect(isProductMutationTool(name)).toBe(true); + } + }); +}); diff --git a/src/agent/product-mutation-tools.ts b/src/agent/product-mutation-tools.ts new file mode 100644 index 000000000..8271d2a5a --- /dev/null +++ b/src/agent/product-mutation-tools.ts @@ -0,0 +1,55 @@ +/** + * Single ownership set for product file-mutation tools. + * + * Primary deny, auto-allow, classify, thrash, and tool-preview all consume this + * list so write_file / edit_file / delete_file / apply_patch cannot drift apart. + * Proxy mounting is out of scope for this module. + */ + +import { + CodexApplyPatchError, + extractAffectedPaths, + parseCodexApplyPatch, +} from "./codex-apply-patch.js"; + +export const PRODUCT_MUTATION_TOOLS = [ + "write_file", + "edit_file", + "delete_file", + "apply_patch", +] as const; + +export type ProductMutationToolName = (typeof PRODUCT_MUTATION_TOOLS)[number]; + +const PRODUCT_MUTATION_TOOL_SET: ReadonlySet = new Set(PRODUCT_MUTATION_TOOLS); + +export function isProductMutationTool(name: string): boolean { + return PRODUCT_MUTATION_TOOL_SET.has(name); +} + +/** + * Paths a product-mutation tool call would touch. + * Path-arg tools use `path`; apply_patch parses envelope `input` when present. + * Malformed / missing apply_patch input yields [] (subjects refine when a proxy mounts). + */ +export function productMutationPaths(name: string, args: unknown): string[] { + if (!isProductMutationTool(name)) return []; + const record = + args !== null && typeof args === "object" && !Array.isArray(args) + ? (args as Record) + : {}; + + if (name === "apply_patch") { + const input = record.input; + if (typeof input !== "string" || input.length === 0) return []; + try { + return extractAffectedPaths(parseCodexApplyPatch(input)); + } catch (err) { + if (err instanceof CodexApplyPatchError) return []; + throw err; + } + } + + const path = record.path; + return typeof path === "string" && path.length > 0 ? [path] : []; +} diff --git a/src/agent/tool-search.test.ts b/src/agent/tool-search.test.ts index b516eb50c..b1b92b2be 100644 --- a/src/agent/tool-search.test.ts +++ b/src/agent/tool-search.test.ts @@ -87,6 +87,8 @@ describe("createToolIndex", () => { expect(CORE_TOOL_NAMES).toContain(name); expect(CATALOG_TOOL_NAMES).not.toContain(name); } + expect(CORE_TOOL_NAMES).not.toContain("apply_patch"); + expect(CATALOG_TOOL_NAMES).not.toContain("apply_patch"); }); test("catalog advertises web_fetch and web_search so URL work needs no tool_search", () => { diff --git a/src/agent/tool-search.ts b/src/agent/tool-search.ts index 736d2a5ca..727ca3a0f 100644 --- a/src/agent/tool-search.ts +++ b/src/agent/tool-search.ts @@ -20,6 +20,8 @@ import { sessionModeEnablesSubAgents } from "../config/session-mode.js"; // the primary Skywalker session can DIY tiny/bounded edits without a // tool_search round-trip. Substantial work still spawns build / docs // directors — that is a prompt judgment call, not a toolset strip. +// Codex `apply_patch` is mounted only when isCodex and kept on build/docs +// leaves — it is intentionally absent from CORE/CATALOG. export const CORE_TOOL_NAMES: readonly string[] = [ "read_file", "write_file", diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 4661e05c8..b5b89fc02 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -48,6 +48,10 @@ import { createWebSearchTool, disposeWebSearchClients } from "../tools/web-searc import { createUseSkillTool } from "./use-skill.js"; import { createToolIndex, createToolSearchTool } from "./tool-search.js"; import { createSearchAgentsTool } from "./agent-search.js"; +import { + createCodexToolProxies, + type CodexRunTool, +} from "./codex-tool-proxies.js"; import type { ReactorEmittedEvent } from "@intx/inference"; const AskOperatorArgs = type({ @@ -135,6 +139,12 @@ export type AgentToolsetArgs = { // sharing this session's cwd. See src/subagent/worktree.ts. useWorktree?: boolean; }; + /** + * When true, mount Codex-only tool proxies (apply_patch) into baseTools. + * Primary then strips apply_patch so DIY stays on write_file/edit_file/delete_file; + * leaves keep apply_patch when their allowlist includes it. + */ + isCodex?: boolean; }; // Per-server connection state surfaced to the TUI. @@ -208,6 +218,23 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise { + const result = await posixTools.run( + { id: "codex-proxy", name, arguments: args }, + new AbortController().signal, + ); + return { + content: + typeof result.content === "string" + ? result.content + : JSON.stringify(result.content), + ...(result.isError === true ? { isError: true } : {}), + }; + }; + // Align the advertised run_shell timeout with shell-guard's resolved default. const baseTools: AgentTool[] = [ ...fromToolRunner(posixTools).map((tool) => ({ @@ -341,7 +368,15 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise tool.definition.name !== "apply_patch"); + + const dynamicRunner = createDynamicToolRunner(primaryTools, toolWatchdog); runnerRef = dynamicRunner; const connectedClients: MCPClient[] = []; diff --git a/src/exec/runner.ts b/src/exec/runner.ts index 692bd6b97..cbf0f69a6 100644 --- a/src/exec/runner.ts +++ b/src/exec/runner.ts @@ -25,7 +25,7 @@ import { shellTimeoutFromSettings, toolWatchdogFromSettings, } from "../config/settings.js"; -import { codexProfileFromProviderName } from "../config/codex-providers.js"; +import { codexProfileFromProviderName, isCodexProviderName } from "../config/codex-providers.js"; import { xaiProfileFromProviderName } from "../config/xai-providers.js"; import { formatDirectorSystemPrompt } from "../agent/directors/identity.js"; import { DIRECTOR_REGISTRY } from "../agent/directors/registry.js"; @@ -399,6 +399,7 @@ export async function runExec(config: Config): Promise { permissionGate, skillDirs, telemetry: liveTelemetry, + isCodex: isCodexProviderName(config.providerName), ...(shellTimeout !== undefined ? { shellTimeout } : {}), ...(toolWatchdog !== undefined ? { toolWatchdog } : {}), ...(localSettingsForMode?.env !== undefined ? { shellEnv: localSettingsForMode.env } : {}), diff --git a/src/permission/classify.ts b/src/permission/classify.ts index efbf1ca4b..122f2fdee 100644 --- a/src/permission/classify.ts +++ b/src/permission/classify.ts @@ -10,6 +10,7 @@ import { import { runShellAuthzBlockReason, runShellAuthzSegmentBlockReason } from "../shell/run-shell-authz.js"; import { resolveWorkspacePath } from "./path-restriction.js"; import type { RootsProvider } from "./worktree-roots.js"; +import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; // Read-only tools never need approval as long as they don't touch a restricted // path; they cannot change the workspace. `lsp` is included here even though @@ -29,7 +30,8 @@ const READ_ONLY_TOOLS = new Set(["read_file", "search_files", "grep", "list_dir" // restriction (outside the workspace boundary, or writes under the session state root). // Covers both read-only tools (dropped from allow to ask) and the mutating -// file tools (dropped from auto-allow to ask in auto mode). +// file tools (dropped from auto-allow to ask in auto mode). apply_patch is +// omitted here — its subjects come from the envelope (see productMutationPaths). const PATH_ARG_TOOLS = new Set(["read_file", "search_files", "grep", "list_dir", "lsp", "write_file", "edit_file", "delete_file"]); // `lsp` names its target `filePath`; every other path-arg tool uses `path`. @@ -37,13 +39,13 @@ function pathArgKey(toolName: string): string { return toolName === "lsp" ? "filePath" : "path"; } -// write_file/edit_file/delete_file mutate the target; every other path-arg tool only +// Product mutation tools mutate the target; every other path-arg tool only // reads it. Restriction policy (see path-restriction.ts) treats reads and // writes of a session-state path differently, so callers need to tell the // gate which mode a given tool call is in. function isWriteTool(toolName: string): boolean { - return toolName === "write_file" || toolName === "edit_file" || toolName === "delete_file"; + return isProductMutationTool(toolName); } export type Tier = "allow" | "ask"; @@ -207,6 +209,9 @@ export function callTargetsRestricted( isRestricted: (path: string, isWrite: boolean) => boolean, ): boolean { if (call.name === "run_shell") return commandTargetsRestricted(stringArg(call, "command"), isRestricted); + if (call.name === "apply_patch") { + return productMutationPaths(call.name, call.arguments).some((path) => isRestricted(path, true)); + } return restrictedPathArg(call, isRestricted) !== undefined; } @@ -443,7 +448,28 @@ export function buildRequests(call: ToolCall): PermissionRequest[] { }, ]; } - if (call.name === "write_file" || call.name === "edit_file" || call.name === "delete_file") { + if (isProductMutationTool(call.name)) { + if (call.name === "apply_patch") { + const paths = productMutationPaths(call.name, call.arguments); + if (paths.length === 0) { + return [ + { + tool: "apply_patch", + action: "Apply patch", + subject: "", + arguments: call.arguments, + scopes: [], + }, + ]; + } + return paths.map((path) => ({ + tool: "apply_patch", + action: "Apply patch", + subject: path, + arguments: call.arguments, + scopes: fileScopes(path), + })); + } const path = stringArg(call, "path"); const action = call.name === "write_file" ? "Write file" : call.name === "edit_file" ? "Edit file" : "Delete file"; return [{ tool: call.name, action, subject: path, arguments: call.arguments, scopes: fileScopes(path) }]; diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 4460600d3..111456119 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -24,6 +24,11 @@ import { matchesWritePathAllowlist, writePathDeniedReason, } from "./write-path-policy.js"; +import { + isProductMutationTool, + productMutationPaths, + PRODUCT_MUTATION_TOOLS, +} from "../agent/product-mutation-tools.js"; import { createMcpToolPermissionRegistry, @@ -197,9 +202,7 @@ export function isRequestCoveredByGrant( // classifyTool. Everything else — ask_operator, unknown built-ins, mutating MCP — // falls through to prompt. const AUTO_ALLOWED_TOOLS = new Set([ - "write_file", - "edit_file", - "delete_file", + ...PRODUCT_MUTATION_TOOLS, "manage_tasks", "present", "tool_search", @@ -374,20 +377,24 @@ export function createPermissionGate(options: PermissionGateOptions): Permission if ( subAgentIdentity?.writePaths !== undefined && subAgentIdentity.writePaths.length > 0 && - (call.name === "write_file" || call.name === "edit_file" || call.name === "delete_file") + isProductMutationTool(call.name) ) { - const path = - typeof call.arguments === "object" && - call.arguments !== null && - typeof (call.arguments as { path?: unknown }).path === "string" - ? (call.arguments as { path: string }).path - : ""; - if (!matchesWritePathAllowlist(path, subAgentIdentity.writePaths, effectiveCwd)) { + const paths = productMutationPaths(call.name, call.arguments); + // Fail-closed: no extractable subject is the same as an empty path deny. + if (paths.length === 0) { return { allowed: false, - reason: writePathDeniedReason(path, subAgentIdentity.writePaths), + reason: writePathDeniedReason("", subAgentIdentity.writePaths), }; } + for (const path of paths) { + if (!matchesWritePathAllowlist(path, subAgentIdentity.writePaths, effectiveCwd)) { + return { + allowed: false, + reason: writePathDeniedReason(path, subAgentIdentity.writePaths), + }; + } + } } const isRestrictedHere = bindRestrictedToProcessCwd(isRestricted, effectiveCwd); diff --git a/src/permission/write-path-policy.ts b/src/permission/write-path-policy.ts index 17eb150d8..1e69e780f 100644 --- a/src/permission/write-path-policy.ts +++ b/src/permission/write-path-policy.ts @@ -4,7 +4,8 @@ import { realpathNearestOr, UNRESOLVABLE } from "./path-restriction.js"; /** * Director write-path allowlist (authz, not prompt policy). - * When set on a sub-agent identity, write_file / edit_file / delete_file must + * When set on a sub-agent identity, write_file / edit_file / delete_file / + * apply_patch must * target a path matching one of these patterns. Enforced in the permission * gate; skipPermissions (yolo) bypasses the whole gate before this runs. * diff --git a/src/subagent/run.ts b/src/subagent/run.ts index c0fc6c31e..9d3e15362 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -35,6 +35,13 @@ import { advertiseEditFileLineRange } from "../plugins/edit-file-line-range.js"; import { createWebFetchTool } from "../tools/web-fetch.js"; import { createWebSearchTool } from "../tools/web-search.js"; import { buildCorePosixToolPlugins } from "../agent/posix-tool-plugins.js"; +import { + allowDeleteFromCapabilities, + createCodexToolProxies, + type CodexRunTool, +} from "../agent/codex-tool-proxies.js"; + +import { isCodexProviderName } from "../config/codex-providers.js"; import { createCompositeBlobReader } from "../agent/lazy-blob-reader.js"; import { buildSubAgentSystemPrompt } from "../agent/prompts.js"; @@ -309,6 +316,33 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { tools = [...tools, ...inherited]; } + // Codex apply_patch proxy: mount after posix+web(+mcp), before capability + // filter, so implement/docs allowlists can keep it when Codex. allowDelete + // follows whether delete_file is in the leaf capability include list (docs + // omits it; implement includes it). + const runTool: CodexRunTool = async (name, args) => { + const result = await posixTools.run( + { id: "codex-proxy", name, arguments: args }, + new AbortController().signal, + ); + return { + content: + typeof result.content === "string" + ? result.content + : JSON.stringify(result.content), + ...(result.isError === true ? { isError: true } : {}), + }; + }; + tools = [ + ...tools, + ...createCodexToolProxies({ + isCodex: isCodexProviderName(params.provider.providerName), + runTool, + allowDelete: allowDeleteFromCapabilities(params.capabilities), + }), + ]; + + if (params.capabilities !== undefined) { tools = applyCapabilityFilter(tools, params.capabilities); } diff --git a/src/subagent/thrash.test.ts b/src/subagent/thrash.test.ts index 3cda21106..72d4e801b 100644 --- a/src/subagent/thrash.test.ts +++ b/src/subagent/thrash.test.ts @@ -112,6 +112,31 @@ describe("thrash pure module", () => { expect(thrashFromReRead(deleted)).toBe(true); }); + test("apply_patch marks envelope paths as edited for re-read thrash", () => { + const patch = `*** Begin Patch +*** Update File: a.ts +@@ +-old ++new +*** End Patch +`; + const patched = applyAll([ + { + type: "tool_call", + name: "apply_patch", + arguments: { input: patch }, + }, + read("a.ts"), + read("a.ts"), + read("a.ts"), + read("a.ts"), + grep("p1"), + grep("p2"), + grep("p3"), + ]); + expect(thrashFromReRead(patched)).toBe(true); + }); + test("multi-file unique reads do NOT thrash", () => { const calls: ThrashToolCallBlock[] = []; for (let i = 0; i < 20; i++) { diff --git a/src/subagent/thrash.ts b/src/subagent/thrash.ts index b8847b06b..5c98ed69e 100644 --- a/src/subagent/thrash.ts +++ b/src/subagent/thrash.ts @@ -9,6 +9,8 @@ * report-forced are one-shot wrap-up / redirect nudges, not stops. */ +import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; + /** Tunable thresholds for thrash / force-report detection. */ export type ThrashConfig = { /** Same path read this many times triggers hard re-read thrash stop. */ @@ -73,7 +75,6 @@ export type ThrashToolCallBlock = { const READ_TOOLS = new Set(["read_file"]); const SEARCH_TOOLS = new Set(["grep", "search_files"]); -const EDIT_TOOLS = new Set(["edit_file", "write_file", "delete_file"]); function parseArgs(raw: unknown): Record { let args: unknown = raw ?? {}; @@ -158,11 +159,16 @@ export function nextThrashState( if (readCounts === null) readCounts = new Map(prev.readCounts); const key = searchKey(name, args); readCounts.set(key, (readCounts.get(key) ?? 0) + 1); - } else if (EDIT_TOOLS.has(name) && path !== null) { - if (editedPaths === null) editedPaths = new Set(prev.editedPaths); - editedPaths.add(path); - if (readCounts === null) readCounts = new Map(prev.readCounts); - decayReadsForPath(readCounts, path); + } else if (isProductMutationTool(name)) { + const paths = productMutationPaths(name, args); + if (paths.length > 0) { + if (editedPaths === null) editedPaths = new Set(prev.editedPaths); + if (readCounts === null) readCounts = new Map(prev.readCounts); + for (const edited of paths) { + editedPaths.add(edited); + decayReadsForPath(readCounts, edited); + } + } } } diff --git a/src/subagent/tool-preview.test.ts b/src/subagent/tool-preview.test.ts index 77c764525..21076aee9 100644 --- a/src/subagent/tool-preview.test.ts +++ b/src/subagent/tool-preview.test.ts @@ -14,6 +14,15 @@ describe("toolCallPreview", () => { ).toBe("src/subagent/session-store.ts"); }); + test("apply_patch preview uses the first envelope path", () => { + const input = `*** Begin Patch +*** Add File: hello.txt ++Hello +*** End Patch +`; + expect(toolCallPreview("apply_patch", JSON.stringify({ input }))).toBe("hello.txt"); + }); + test("grep shows the pattern", () => { expect( toolCallPreview("grep", JSON.stringify({ pattern: "currentToolPreview", path: "src" })), diff --git a/src/subagent/tool-preview.ts b/src/subagent/tool-preview.ts index ab3ff1a74..a0d7d4fb3 100644 --- a/src/subagent/tool-preview.ts +++ b/src/subagent/tool-preview.ts @@ -9,6 +9,7 @@ */ import { scrubSecrets } from "../web/secret-scrub.js"; +import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; /** Hard cap so a long command cannot shove the row's other columns off-screen. */ export const TOOL_PREVIEW_MAX = 48; @@ -56,13 +57,15 @@ function extractSubject(name: string, rawArgs: string): string | null { if ( tool === "read_file" || - tool === "write_file" || - tool === "edit_file" || - tool === "delete_file" || + isProductMutationTool(tool) || tool.endsWith("__read_file") || tool.endsWith("__write_file") || tool.endsWith("__edit_file") ) { + if (tool === "apply_patch") { + const paths = productMutationPaths(tool, args); + return paths[0] ?? null; + } return stringField(args, "path") ?? stringField(args, "file_path"); } diff --git a/src/tui/runner.ts b/src/tui/runner.ts index f994d39da..2d0ecb266 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -1172,6 +1172,7 @@ export async function runTUI(initialConfig: Config): Promise { permissionGate, skillDirs, telemetry: liveTelemetry, + isCodex: isCodexProviderName(config.providerName), ...(shellTimeout !== undefined ? { shellTimeout } : {}), ...(localSettingsForEnv?.env !== undefined ? { shellEnv: localSettingsForEnv.env } : {}), toolWatchdog: liveToolWatchdog, diff --git a/tests/unit/tui/agent-tools.test.ts b/tests/unit/tui/agent-tools.test.ts index a8c94bde2..23a116df6 100644 --- a/tests/unit/tui/agent-tools.test.ts +++ b/tests/unit/tui/agent-tools.test.ts @@ -197,6 +197,8 @@ test("dynamicRunner contains posix tool names plus ask_operator", async () => { expect(names).toContain("write_file"); expect(names).toContain("edit_file"); expect(names).toContain("delete_file"); + // apply_patch is Codex-only and stripped on primary even when mounted. + expect(names).not.toContain("apply_patch"); }); test("onOperatorGate callback is invoked when the operator tool handler is called", async () => { From 18e31b6b4f2a18619ddef59ec1564da249effb86 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 15:35:46 -0700 Subject: [PATCH 2/6] Proxy Codex exec_command and update_plan onto Corbits tools --- docs/IMPLEMENTATION.md | 2 +- src/agent/codex-tool-mount.test.ts | 75 ++++++++-- src/agent/codex-tool-proxies.test.ts | 144 ++++++++++++++++++- src/agent/codex-tool-proxies.ts | 192 +++++++++++++++++++++++++- src/agent/directors/tool-sets.test.ts | 12 +- src/agent/directors/tool-sets.ts | 12 +- src/agent/tools.ts | 7 +- src/subagent/run.ts | 2 + 8 files changed, 420 insertions(+), 26 deletions(-) diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index 0d270a65e..a50976777 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -160,7 +160,7 @@ Sixteen packages under `src/agent/directors//` register in `DIRECTOR_REGISTR 4. `directorProfiles()` is the spawn catalog (`default-agents.ts`) — closed set minus skywalker; plugin agent profiles still load and can override by id. 5. Primary chat role is Skywalker: `buildChatRole()` → `createSkywalkerSystemPrompt()`. Product mutation tools (`write_file` / `edit_file` / `delete_file`) live in CORE (and `SKYWALKER_TOOLS`) so they are advertised on the primary without a `tool_search` round-trip. DIY tiny/bounded edits on the parent; spawn build/docs directors for substantial work — a prompt judgment call, not a toolset strip. `PRIMARY_DENIED_PRODUCT_TOOLS` is gone. Shell file-writes stay denied; MCP tools are not re-filtered by a product-write deny list. Optional `writePaths` (when a profile sets it) only gate path-keyed product tools. - **Codex `apply_patch` proxy.** When the active provider is Codex (`isCodexProviderName`), `createAgentToolset` and `runSubAgent` mount an `apply_patch` stringTool from `createCodexToolProxies` that parses the Codex envelope and forwards each op through the posix `ToolRunner` (`write_file` / `delete_file` / `read_file`) so permission plugins still apply. Primary strips `apply_patch` after mount (Corbits DIY stays on `write_file` / `edit_file` / `delete_file`); build and docs leaf allowlists (`BUILD_TOOLS` / `DOCS_TOOLS`) include it so Codex workers keep the proxy after the capability filter. `CORE_TOOL_NAMES` does not list it. + **Codex tool proxies.** When the active provider is Codex (`isCodexProviderName`), `createAgentToolset` and `runSubAgent` mount `apply_patch`, `shell`, and `update_plan` stringTools from `createCodexToolProxies`, all forwarding through the same posix `ToolRunner` seam (`runTool`) so permission plugins still apply. `apply_patch` parses the Codex envelope and forwards each op (`write_file` / `delete_file` / `read_file`). `shell` — the native Codex name is `shell`, not `exec_command`, per the pinned base-instructions text quoted in `codex-responses-adapter.ts`'s bridge message — normalizes Codex's `command` (string or `["bash","-lc",script]`-style argv array), `workdir`, and `timeout_ms` onto `run_shell`'s `{command, cwd?, timeout?}` and is gated by `allowShellFromCapabilities` (mirrors `allowDeleteFromCapabilities` against `run_shell`). `update_plan` maps Codex's `plan: [{step, status}]` onto `manage_tasks(action: "create")`; `pending`/`in_progress`/`completed` map to `todo`/`doing`/`done` — `manage_tasks`'s `cancelled` status has no Codex equivalent and is never produced by this proxy. Primary strips `apply_patch` after mount (Corbits DIY stays on `write_file` / `edit_file` / `delete_file`); `shell` and `update_plan` stay on primary (same classification as `run_shell` / `manage_tasks`). Build and docs leaf allowlists (`BUILD_TOOLS` / `DOCS_TOOLS`) include `apply_patch` so Codex workers keep the proxy after the capability filter. `CORE_TOOL_NAMES` does not list it. 6. Shipped directors omit `writePaths`. The optional field is still enforced in the permission gate via ALS identity (`identity-context.ts` + `write-path-policy.ts`) when a plugin/custom profile sets it. 7. Spawn effort: pin > package `modelRole` default (`defaultEffortForDirector`; intern=low; plan/review/orchestrator=high; implement/explore/docs/test=medium) > orchestrator/worker binary > parent inheritance. Optional skills are listed in the identity header for awareness; workers do not mount `use_skill` (guidance is baked into package system prompts). Primary mounts `use_skill` for its own skill list. diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index 884ce73f8..5dbaa28a4 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -1,6 +1,13 @@ /** - * Mount coverage for Codex apply_patch proxies: primary strip, allowlists, - * and build-shaped capability filter retention. + * Mount coverage for Codex tool proxies (apply_patch, shell, update_plan): + * primary strip of apply_patch, allowlists, and build-shaped capability filter + * retention. + * + * runSubAgent has no standalone toolset-factory export to import directly (the + * mount is inline in runSubAgent's tool-assembly), so the subagent mount path + * is covered here via the same allowDeleteFromCapabilities / + * allowShellFromCapabilities calls runSubAgent makes against a leaf + * capability filter, feeding createCodexToolProxies exactly as run.ts does. */ import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -8,7 +15,11 @@ import { join } from "node:path"; import { afterEach, describe, expect, test, spyOn } from "bun:test"; import * as posixModule from "@intx/tools-posix"; -import { createCodexToolProxies } from "./codex-tool-proxies.js"; +import { + allowDeleteFromCapabilities, + allowShellFromCapabilities, + createCodexToolProxies, +} from "./codex-tool-proxies.js"; import { BUILD_TOOLS, DOCS_TOOLS } from "./directors/tool-sets.js"; import { CORE_TOOL_NAMES } from "./tool-search.js"; @@ -16,8 +27,8 @@ afterEach(() => { spyOn(posixModule, "createPosixTools").mockRestore(); }); -describe("Codex apply_patch mount", () => { - test("non-Codex createAgentToolset does not advertise apply_patch on primary", async () => { +describe("Codex tool proxy mount", () => { + test("non-Codex createAgentToolset does not advertise proxies on primary", async () => { const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); spyOn(posixModule, "createPosixTools").mockReturnValue({ definitions: [], @@ -39,10 +50,12 @@ describe("Codex apply_patch mount", () => { }); const names = toolset.dynamicRunner.currentDefinitions().map((d) => d.name); expect(names).not.toContain("apply_patch"); + expect(names).not.toContain("shell"); + expect(names).not.toContain("update_plan"); await toolset.dispose(); }); - test("Codex createAgentToolset strips apply_patch on primary after mount", async () => { + test("Codex createAgentToolset strips apply_patch on primary; keeps shell/update_plan", async () => { const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); spyOn(posixModule, "createPosixTools").mockReturnValue({ definitions: [], @@ -68,6 +81,11 @@ describe("Codex apply_patch mount", () => { expect(names).toContain("write_file"); expect(names).toContain("edit_file"); expect(names).toContain("delete_file"); + // shell / update_plan are not product-mutation tools (same classification + // as run_shell / manage_tasks), so the primary apply_patch strip does not + // remove them — they stay mounted on primary, mirroring run_shell. + expect(names).toContain("shell"); + expect(names).toContain("update_plan"); await toolset.dispose(); }); @@ -77,19 +95,56 @@ describe("Codex apply_patch mount", () => { expect(CORE_TOOL_NAMES).not.toContain("apply_patch"); }); - test("capability include-filter keeps apply_patch for build-shaped allowlists", () => { + test("capability include-filter keeps proxies for build-shaped allowlists", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), }); - expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch"]); + expect(proxies.map((t) => t.definition.name)).toEqual([ + "apply_patch", + "shell", + "update_plan", + ]); const allow = new Set(BUILD_TOOLS); const kept = proxies.filter((t) => allow.has(t.definition.name)); - expect(kept.map((t) => t.definition.name)).toContain("apply_patch"); + expect(kept.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); const docsAllow = new Set(DOCS_TOOLS); const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); - expect(docsKept.map((t) => t.definition.name)).toContain("apply_patch"); + expect(docsKept.map((t) => t.definition.name)).toEqual(["apply_patch", "update_plan"]); + }); + + test("runSubAgent-shaped mount: docs capability filter denies shell, keeps update_plan", () => { + // Mirrors run.ts: allowDelete / allowShell are derived from the leaf + // capability filter before createCodexToolProxies runs. update_plan is + // never gated by it (manage_tasks is unconditionally mounted for every + // sub-agent), so it stays regardless of the allowlist shape. + const docsCapabilities = { mode: "allow" as const, tools: DOCS_TOOLS }; + const proxies = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "ok" }), + allowDelete: allowDeleteFromCapabilities(docsCapabilities), + allowShell: allowShellFromCapabilities(docsCapabilities), + }); + expect(proxies.map((t) => t.definition.name)).toEqual([ + "apply_patch", + "shell", + "update_plan", + ]); + + const docsAllow = new Set(DOCS_TOOLS); + const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); + expect(docsKept.map((t) => t.definition.name)).toEqual(["apply_patch", "update_plan"]); + }); + + test("non-Codex runSubAgent-shaped mount produces no proxies at all", () => { + const proxies = createCodexToolProxies({ + isCodex: false, + runTool: async () => ({ content: "ok" }), + allowDelete: allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), + allowShell: allowShellFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), + }); + expect(proxies).toEqual([]); }); }); diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 9881483eb..a9d1a6882 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -4,6 +4,7 @@ import type { AgentTool } from "@intx/agent"; import { allowDeleteFromCapabilities, + allowShellFromCapabilities, createCodexToolProxies, type CodexRunTool, } from "./codex-tool-proxies.js"; @@ -38,6 +39,12 @@ function makeRecorder(initial: Record = {}): { files.delete(path); return { content: `Deleted file: ${path}` }; } + if (name === "run_shell") { + return { content: `ran: ${JSON.stringify(args)}` }; + } + if (name === "manage_tasks") { + return { content: "Tasks updated." }; + } return { content: `unknown tool: ${name}`, isError: true }; }; return { calls, files, runTool }; @@ -51,6 +58,11 @@ async function invokeApplyPatch(tools: AgentTool[], input: string) { ); } +async function invokeTool(tools: AgentTool[], name: string, args: Record) { + const runner = createToolRunner(tools); + return runner.run({ id: "call-1", name, arguments: args }, new AbortController().signal); +} + describe("createCodexToolProxies", () => { test("returns [] when not Codex", () => { const tools = createCodexToolProxies({ @@ -60,14 +72,13 @@ describe("createCodexToolProxies", () => { expect(tools).toEqual([]); }); - test("returns apply_patch stringTool when Codex", () => { + test("returns apply_patch, shell, update_plan stringTools when Codex", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), }); - expect(tools).toHaveLength(1); - expect(tools[0]!.definition.name).toBe("apply_patch"); - expect(tools[0]!.kind).toBe("string"); + expect(tools.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); + expect(tools.every((t) => t.kind === "string")).toBe(true); expect(tools[0]!.definition.inputSchema).toMatchObject({ required: ["input"], }); @@ -312,6 +323,117 @@ print("Hello, world!") }); }); +describe("shell proxy", () => { + test("string command forwards to run_shell", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "shell", { command: "ls -la" }); + expect(result.isError).toBeFalsy(); + expect(calls).toEqual([{ name: "run_shell", args: { command: "ls -la" } }]); + }); + + test("bash -lc argv triple unwraps to the script", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + await invokeTool(tools, "shell", { command: ["bash", "-lc", "echo 'hi there'"] }); + expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hi there'" } }]); + }); + + test("other argv arrays are shell-quoted and joined", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + await invokeTool(tools, "shell", { command: ["echo", "hello world"] }); + expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hello world'" } }]); + }); + + test("workdir and timeout_ms translate to cwd and timeout", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + await invokeTool(tools, "shell", { + command: "pwd", + workdir: "/tmp/work", + timeout_ms: 5000, + }); + expect(calls).toEqual([ + { name: "run_shell", args: { command: "pwd", cwd: "/tmp/work", timeout: 5000 } }, + ]); + }); + + test("missing command surfaces as tool error", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "shell", {}); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/command/); + expect(calls).toEqual([]); + }); + + test("allowShell false refuses without calling run_shell", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false }); + const result = await invokeTool(tools, "shell", { command: "ls" }); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/not allowed/); + expect(calls).toEqual([]); + }); + + test("run_shell isError propagates as tool error", async () => { + const runTool: CodexRunTool = async () => ({ content: "boom", isError: true }); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "shell", { command: "ls" }); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/boom/); + }); +}); + +describe("update_plan proxy", () => { + test("maps plan steps onto manage_tasks(action=create)", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "update_plan", { + explanation: "getting started", + plan: [ + { step: "Read the file", status: "completed" }, + { step: "Write the fix", status: "in_progress" }, + { step: "Run tests", status: "pending" }, + ], + }); + expect(result.isError).toBeFalsy(); + expect(calls).toEqual([ + { + name: "manage_tasks", + args: { + action: "create", + tasks: [ + { id: "p1", title: "Read the file", status: "done" }, + { id: "p2", title: "Write the fix", status: "doing" }, + { id: "p3", title: "Run tests", status: "todo" }, + ], + }, + }, + ]); + }); + + test("malformed plan surfaces as tool error", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "update_plan", { + plan: [{ step: "no status here" }], + }); + expect(result.isError).toBe(true); + expect(result.content).toMatch(/plan/); + expect(calls).toEqual([]); + }); + + test("missing plan surfaces as tool error", async () => { + const { calls, runTool } = makeRecorder(); + const tools = createCodexToolProxies({ isCodex: true, runTool }); + const result = await invokeTool(tools, "update_plan", {}); + expect(result.isError).toBe(true); + expect(calls).toEqual([]); + }); +}); + describe("allowDeleteFromCapabilities", () => { test("docs allowlist (no delete_file) → false; build → true", () => { expect( @@ -329,3 +451,17 @@ describe("allowDeleteFromCapabilities", () => { ).toBe(false); }); }); + +describe("allowShellFromCapabilities", () => { + test("docs allowlist (no run_shell) → false; build → true", () => { + expect(allowShellFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(false); + expect(allowShellFromCapabilities({ mode: "allow", tools: BUILD_TOOLS })).toBe(true); + expect(allowShellFromCapabilities(undefined)).toBe(true); + expect( + allowShellFromCapabilities({ mode: "exclude", tools: ["delete_file"] }), + ).toBe(true); + expect( + allowShellFromCapabilities({ mode: "exclude", tools: ["run_shell"] }), + ).toBe(false); + }); +}); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index 478a40b13..c19778293 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -1,6 +1,7 @@ /** - * Codex-only tool proxies. Factory only — mounting into createAgentToolset / - * runSubAgent is intentionally out of scope for this module. + * Codex-only tool proxies: `apply_patch`, `shell`, `update_plan`. Factory + * only — mounting into createAgentToolset / runSubAgent is intentionally out + * of scope for this module. */ import { type } from "arktype"; @@ -14,6 +15,7 @@ import { parseCodexApplyPatch, type PatchOp, } from "./codex-apply-patch.js"; +import type { TaskStatus } from "./tasks.js"; export type CodexRunTool = ( name: string, @@ -29,6 +31,11 @@ export type CreateCodexToolProxiesOpts = { * DOCS_TOOLS includes apply_patch but not delete_file. */ allowDelete?: boolean; + /** + * When false, `shell` refuses without calling `run_shell`. Defaults to true. + * Docs leaves pass false because DOCS_TOOLS omits run_shell. + */ + allowShell?: boolean; }; const ApplyPatchArgs = type({ @@ -121,14 +128,21 @@ export const applyPatchDefinition: ToolDefinition = { }; /** - * When `isCodex` is false, returns []. Otherwise returns a single `apply_patch` - * stringTool that parses the Codex envelope and forwards each op through - * `runTool` (write_file / delete_file / read_file). + * When `isCodex` is false, returns []. Otherwise returns the `apply_patch`, + * `shell`, and `update_plan` stringTools: `apply_patch` parses the Codex + * envelope and forwards each op through `runTool` (write_file / delete_file / + * read_file); `shell` forwards onto `run_shell`; `update_plan` forwards onto + * `manage_tasks`. */ export function createCodexToolProxies(opts: CreateCodexToolProxiesOpts): AgentTool[] { if (!opts.isCodex) return []; const allowDelete = opts.allowDelete !== false; - return [createApplyPatchProxy(opts.runTool, allowDelete)]; + const allowShell = opts.allowShell !== false; + return [ + createApplyPatchProxy(opts.runTool, allowDelete), + createShellProxy(opts.runTool, allowShell), + createUpdatePlanProxy(opts.runTool), + ]; } /** @@ -146,6 +160,21 @@ export function allowDeleteFromCapabilities( return !capabilities.tools.includes("delete_file"); } +/** + * Resolve whether `shell` may forward onto `run_shell` given a leaf + * capability filter. Mirrors allowDeleteFromCapabilities against `run_shell` + * instead of `delete_file` — docs leaves (DOCS_TOOLS omits run_shell) refuse. + */ +export function allowShellFromCapabilities( + capabilities: { mode: "allow" | "exclude"; tools: readonly string[] } | undefined, +): boolean { + if (capabilities === undefined) return true; + if (capabilities.mode === "allow") { + return capabilities.tools.includes("run_shell"); + } + return !capabilities.tools.includes("run_shell"); +} + function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): AgentTool { return stringTool({ definition: applyPatchDefinition, @@ -241,3 +270,154 @@ function requireOk( } return result.content; } + +// --- shell (Codex's native command-execution tool) --- +// +// The pinned Codex base instructions (bridgeMessage in +// codex-responses-adapter.ts) name this tool `shell`, not `exec_command` — that +// is the only native name this codebase's own reference material documents, so +// it is the name proxied here. + +const ShellArgs = type({ + command: "string | string[]", + "workdir?": "string", + "timeout_ms?": "number", +}); + +export const shellDefinition: ToolDefinition = { + name: "shell", + description: "Runs a shell command and returns its output.", + inputSchema: { + type: "object", + properties: { + command: { + description: + "The command to run, as a shell string or an argv array (e.g. [\"bash\",\"-lc\",\"ls\"]).", + }, + workdir: { type: "string", description: "Working directory for the command." }, + timeout_ms: { type: "number", description: "Timeout in milliseconds." }, + }, + required: ["command"], + }, +}; + +const SHELL_WRAPPERS = new Set(["bash", "sh", "zsh"]); + +function shellQuote(arg: string): string { + if (/^[A-Za-z0-9_\-./:=@%]+$/.test(arg)) return arg; + return `'${arg.replace(/'/g, `'\\''`)}'`; +} + +/** + * Codex's `shell` tool sends `command` as either a plain string or an argv + * array. `run_shell` takes a single shell string. The common argv shape is a + * `[shell, "-lc"|"-c", script]` triple — unwrap that to the script verbatim so + * embedded spaces/quoting survive. Any other array is shell-quoted element by + * element and joined, which is lossy for exotic argv (e.g. a NUL byte in an + * arg) but matches ordinary command arrays. + */ +function normalizeShellCommand(command: string | string[]): string { + if (typeof command === "string") return command; + if ( + command.length === 3 && + SHELL_WRAPPERS.has(command[0]!.replace(/^.*\//, "")) && + (command[1] === "-lc" || command[1] === "-c") + ) { + return command[2]!; + } + return command.map(shellQuote).join(" "); +} + +function createShellProxy(runTool: CodexRunTool, allowShell: boolean): AgentTool { + return stringTool({ + definition: shellDefinition, + handler: async (rawArgs: Record): Promise => { + const parsed = ShellArgs(rawArgs); + if (parsed instanceof type.errors) { + throw new Error("Error: shell requires a command (string or string[])."); + } + if (!allowShell) { + throw new Error( + "shell: not allowed for this agent (run_shell capability missing).", + ); + } + const args: Record = { command: normalizeShellCommand(parsed.command) }; + if (parsed.workdir !== undefined) args.cwd = parsed.workdir; + if (parsed.timeout_ms !== undefined) args.timeout = parsed.timeout_ms; + return requireOk(await runTool("run_shell", args), "shell"); + }, + }); +} + +// --- update_plan (Codex's native plan/checklist tool) --- + +const CodexPlanStatus = type("'pending' | 'in_progress' | 'completed'"); + +const UpdatePlanArgs = type({ + "explanation?": "string", + plan: type({ + step: "string>0", + status: CodexPlanStatus, + }).array(), +}); + +export const updatePlanDefinition: ToolDefinition = { + name: "update_plan", + description: + "Updates your task plan. Provide the full ordered list of plan steps, each with a status.", + inputSchema: { + type: "object", + properties: { + explanation: { type: "string" }, + plan: { + type: "array", + items: { + type: "object", + properties: { + step: { type: "string" }, + status: { + type: "string", + enum: ["pending", "in_progress", "completed"], + }, + }, + required: ["step", "status"], + }, + }, + }, + required: ["plan"], + }, +}; + +function codexPlanStatusToTaskStatus(status: typeof CodexPlanStatus.infer): TaskStatus { + if (status === "pending") return "todo"; + if (status === "in_progress") return "doing"; + return "done"; +} + +function createUpdatePlanProxy(runTool: CodexRunTool): AgentTool { + return stringTool({ + definition: updatePlanDefinition, + handler: async (rawArgs: Record): Promise => { + const parsed = UpdatePlanArgs(rawArgs); + if (parsed instanceof type.errors) { + throw new Error( + "Error: update_plan requires a plan array of { step, status }.", + ); + } + // manage_tasks has no "cancelled" equivalent in Codex's plan shape + // (pending/in_progress/completed) — this proxy never produces it, so a + // Codex model cannot cancel a step through update_plan. That is a + // lossy-but-safe narrowing (dropped, not misrepresented), not a bug fix + // for the underlying task tool, which stays out of scope here. + const tasks = parsed.plan.map((item, i) => ({ + id: `p${i + 1}`, + title: item.step, + status: codexPlanStatusToTaskStatus(item.status), + })); + return requireOk( + await runTool("manage_tasks", { action: "create", tasks }), + "update_plan", + ); + }, + }); +} diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index be8bc0a52..4deae1f3f 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -36,6 +36,11 @@ describe("DOCS_TOOLS", () => { expect(surface).toContain("run_shell"); } }); + + test("excludes the shell proxy (no run_shell) but keeps update_plan", () => { + expect(DOCS_TOOLS).not.toContain("shell"); + expect(DOCS_TOOLS).toContain("update_plan"); + }); }); describe("SKYWALKER_TOOLS / ORCHESTRATOR_TOOLS", () => { @@ -56,4 +61,9 @@ describe("BUILD_TOOLS", () => { expect(BUILD_TOOLS).toContain("delete_file"); expect(BUILD_TOOLS).toContain("apply_patch"); }); -}); \ No newline at end of file + + test("includes the Codex shell and update_plan proxy names", () => { + expect(BUILD_TOOLS).toContain("shell"); + expect(BUILD_TOOLS).toContain("update_plan"); + }); +}); diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index bc7ec0d0f..2d14231c0 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -16,13 +16,20 @@ export const READ_TOOLS = [ "web_search", ] as const; -/** Build: read + full file mutation. */ +/** + * Build: read + full file mutation. `shell` and `update_plan` are Codex + * proxy names (createCodexToolProxies) for `run_shell` / the plan tool; both + * are listed here so Codex build leaves keep the proxies after the + * capability filter, same rationale as `apply_patch` below. + */ export const BUILD_TOOLS = [ ...READ_TOOLS, "write_file", "edit_file", "delete_file", "apply_patch", + "shell", + "update_plan", ] as const; /** @@ -34,12 +41,15 @@ export const BUILD_TOOLS = [ * Composed from READ_TOOLS minus run_shell so it tracks the read surface * automatically; only the write tools are added explicitly. `apply_patch` is * included so Codex docs leaves keep the proxy after the capability filter. + * `update_plan` is included for the same reason (its proxy has no `run_shell` + * dependency, so it is not excluded alongside `shell`). */ export const DOCS_TOOLS = [ ...READ_TOOLS.filter((t) => t !== "run_shell"), "write_file", "edit_file", "apply_patch", + "update_plan", ] as const; /** Review / counsel: read surface, no writes. */ diff --git a/src/agent/tools.ts b/src/agent/tools.ts index b5b89fc02..7a0e1f1aa 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -140,9 +140,10 @@ export type AgentToolsetArgs = { useWorktree?: boolean; }; /** - * When true, mount Codex-only tool proxies (apply_patch) into baseTools. - * Primary then strips apply_patch so DIY stays on write_file/edit_file/delete_file; - * leaves keep apply_patch when their allowlist includes it. + * When true, mount Codex-only tool proxies (apply_patch, shell, update_plan) + * into baseTools. Primary then strips apply_patch so DIY stays on + * write_file/edit_file/delete_file; shell and update_plan stay mounted. + * Leaves keep apply_patch when their allowlist includes it. */ isCodex?: boolean; }; diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 9d3e15362..300091cc6 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -37,6 +37,7 @@ import { createWebSearchTool } from "../tools/web-search.js"; import { buildCorePosixToolPlugins } from "../agent/posix-tool-plugins.js"; import { allowDeleteFromCapabilities, + allowShellFromCapabilities, createCodexToolProxies, type CodexRunTool, } from "../agent/codex-tool-proxies.js"; @@ -339,6 +340,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { isCodex: isCodexProviderName(params.provider.providerName), runTool, allowDelete: allowDeleteFromCapabilities(params.capabilities), + allowShell: allowShellFromCapabilities(params.capabilities), }), ]; From 7174271b836b57c5d5a12b97898302f6b5c79f15 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 15:51:52 -0700 Subject: [PATCH 3/6] Route update_plan onto the real manage_tasks handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createUpdatePlanProxy called runTool("manage_tasks", ...), but runTool forwards only to posixTools, which has no manage_tasks handler — every update_plan call errored with "unknown tool: manage_tasks". Give createCodexToolProxies its own runManageTasks callback and wire each mount site (tools.ts, subagent/run.ts) to the same manage_tasks logic their stringTool handler already uses. Also fix requireOk's hardcoded "apply_patch failed" label to use the actual label argument. Tests: dropped the manage_tasks special case from the apply_patch mock recorder (posixTools has no such handler, so it now falls through to the accurate "unknown tool" branch), and added coverage that dispatches through the real parseManageTasksArgs/applyManageTasks pair and through the real createAgentToolset mount with unstubbed posixTools — both would have caught the dead dispatch. --- src/agent/codex-tool-mount.test.ts | 37 ++++++++ src/agent/codex-tool-proxies.test.ts | 133 +++++++++++++++++++-------- src/agent/codex-tool-proxies.ts | 21 ++++- src/agent/tools.ts | 27 ++++-- src/subagent/run.ts | 25 ++++- 5 files changed, 192 insertions(+), 51 deletions(-) diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index 5dbaa28a4..3d83b3e11 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -89,6 +89,40 @@ describe("Codex tool proxy mount", () => { await toolset.dispose(); }); + test("update_plan dispatches through the real mount without hitting posixTools", async () => { + // Unstubbed createPosixTools (real temp dir): update_plan used to call + // runTool("manage_tasks", ...), which forwards onto posixTools.run and + // fails with "unknown tool: manage_tasks" — posixTools has no + // manage_tasks handler. This exercises the real createAgentToolset mount + // (src/agent/tools.ts) end to end, not a mock recorder, so it would have + // caught that dead dispatch. + const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); + const { createAgentToolset } = await import("./tools.js"); + const permissionGate = { + check: async () => ({ allowed: true }), + getSkipPermissions: () => false, + } as never; + + const toolset = await createAgentToolset({ + cwd, + permissionGate, + onOperatorGate: async () => ({ kind: "option", index: 0 }), + isCodex: true, + }); + const result = await toolset.dynamicRunner.run( + { + id: "call-1", + name: "update_plan", + arguments: { + plan: [{ step: "Do the thing", status: "in_progress" }], + }, + }, + new AbortController().signal, + ); + expect(result.isError).toBeFalsy(); + await toolset.dispose(); + }); + test("BUILD_TOOLS and DOCS_TOOLS include apply_patch; CORE_TOOL_NAMES does not", () => { expect(BUILD_TOOLS).toContain("apply_patch"); expect(DOCS_TOOLS).toContain("apply_patch"); @@ -99,6 +133,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), + runManageTasks: async () => ({ content: "ok" }), }); expect(proxies.map((t) => t.definition.name)).toEqual([ "apply_patch", @@ -124,6 +159,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), + runManageTasks: async () => ({ content: "ok" }), allowDelete: allowDeleteFromCapabilities(docsCapabilities), allowShell: allowShellFromCapabilities(docsCapabilities), }); @@ -142,6 +178,7 @@ describe("Codex tool proxy mount", () => { const proxies = createCodexToolProxies({ isCodex: false, runTool: 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 a9d1a6882..7908c8b79 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -6,12 +6,19 @@ import { allowDeleteFromCapabilities, allowShellFromCapabilities, createCodexToolProxies, + type CodexRunManageTasks, type CodexRunTool, } from "./codex-tool-proxies.js"; import { DOCS_TOOLS, BUILD_TOOLS } from "./directors/tool-sets.js"; +import { applyManageTasks, parseManageTasksArgs, type Task } from "./tasks.js"; type Call = { name: string; args: Record }; +// `manage_tasks` is deliberately NOT a branch here: the real posixTools +// registry runTool forwards to has no manage_tasks handler (only +// read_file/write_file/run_shell/edit_file/search_files/grep + the +// delete_file plugin), so an unrecognized name falling through to the +// `unknown tool` branch is the accurate stand-in for that registry. function makeRecorder(initial: Record = {}): { calls: Call[]; files: Map; @@ -42,14 +49,39 @@ function makeRecorder(initial: Record = {}): { if (name === "run_shell") { return { content: `ran: ${JSON.stringify(args)}` }; } - if (name === "manage_tasks") { - return { content: "Tasks updated." }; - } return { content: `unknown tool: ${name}`, isError: true }; }; return { calls, files, runTool }; } +const unusedManageTasks: CodexRunManageTasks = 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 +// tasks.ts — the same two functions the manage_tasks stringTool handlers in +// src/agent/tools.ts and src/subagent/run.ts call. No mock recorder involved. +function makeRealManageTasks(): { + calls: Record[]; + getTasks: () => Task[]; + runManageTasks: CodexRunManageTasks; +} { + let tasks: Task[] = []; + const calls: Record[] = []; + const runManageTasks: CodexRunManageTasks = async (rawArgs) => { + calls.push(rawArgs); + const parsed = parseManageTasksArgs(rawArgs); + if (parsed === null) { + return { + content: "Error: manage_tasks requires action ('create' or 'update').", + isError: true, + }; + } + tasks = applyManageTasks(tasks, parsed); + return { content: "Tasks updated." }; + }; + return { calls, getTasks: () => tasks, runManageTasks }; +} + async function invokeApplyPatch(tools: AgentTool[], input: string) { const runner = createToolRunner(tools); return runner.run( @@ -68,6 +100,7 @@ describe("createCodexToolProxies", () => { const tools = createCodexToolProxies({ isCodex: false, runTool: async () => ({ content: "unused" }), + runManageTasks: unusedManageTasks, }); expect(tools).toEqual([]); }); @@ -76,6 +109,7 @@ describe("createCodexToolProxies", () => { const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + runManageTasks: unusedManageTasks, }); expect(tools.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); expect(tools.every((t) => t.kind === "string")).toBe(true); @@ -86,7 +120,7 @@ describe("createCodexToolProxies", () => { test("add forwards write_file with Codex trailing newline", async () => { const { calls, files, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -109,7 +143,7 @@ describe("createCodexToolProxies", () => { test("delete forwards delete_file", async () => { const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -125,7 +159,7 @@ describe("createCodexToolProxies", () => { test("allowDelete false refuses Delete without calling delete_file", async () => { const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -145,7 +179,7 @@ describe("createCodexToolProxies", () => { print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -169,7 +203,7 @@ print("Hi") print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -193,7 +227,7 @@ print("Hi") print("bye") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -219,7 +253,7 @@ print("bye") print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -250,7 +284,7 @@ print("Hello, world!") "src/app.py": "old\n", "obsolete.txt": "x", }); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -278,7 +312,7 @@ print("Hello, world!") test("parse failure surfaces as tool error (isError)", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Add File: a.txt @@ -295,6 +329,7 @@ print("Hello, world!") const tools = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "unused" }), + runManageTasks: unusedManageTasks, }); const runner = createToolRunner(tools); const result = await runner.run( @@ -307,7 +342,7 @@ print("Hello, world!") test("runTool isError aborts the patch with isError", async () => { const { runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -326,7 +361,7 @@ print("Hello, world!") describe("shell proxy", () => { test("string command forwards to run_shell", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeTool(tools, "shell", { command: "ls -la" }); expect(result.isError).toBeFalsy(); expect(calls).toEqual([{ name: "run_shell", args: { command: "ls -la" } }]); @@ -334,21 +369,21 @@ describe("shell proxy", () => { test("bash -lc argv triple unwraps to the script", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); await invokeTool(tools, "shell", { command: ["bash", "-lc", "echo 'hi there'"] }); expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hi there'" } }]); }); test("other argv arrays are shell-quoted and joined", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); await invokeTool(tools, "shell", { command: ["echo", "hello world"] }); expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hello world'" } }]); }); test("workdir and timeout_ms translate to cwd and timeout", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); await invokeTool(tools, "shell", { command: "pwd", workdir: "/tmp/work", @@ -361,7 +396,7 @@ describe("shell proxy", () => { test("missing command surfaces as tool error", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeTool(tools, "shell", {}); expect(result.isError).toBe(true); expect(result.content).toMatch(/command/); @@ -370,7 +405,7 @@ describe("shell proxy", () => { test("allowShell false refuses without calling run_shell", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false }); + const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false, runManageTasks: unusedManageTasks }); const result = await invokeTool(tools, "shell", { command: "ls" }); expect(result.isError).toBe(true); expect(result.content).toMatch(/not allowed/); @@ -379,7 +414,7 @@ describe("shell proxy", () => { test("run_shell isError propagates as tool error", async () => { const runTool: CodexRunTool = async () => ({ content: "boom", isError: true }); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); const result = await invokeTool(tools, "shell", { command: "ls" }); expect(result.isError).toBe(true); expect(result.content).toMatch(/boom/); @@ -387,9 +422,23 @@ describe("shell proxy", () => { }); describe("update_plan proxy", () => { - test("maps plan steps onto manage_tasks(action=create)", async () => { - const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + // Real dispatch, not a mock recorder: runManageTasks here is + // makeRealManageTasks, which parses with the real parseManageTasksArgs and + // mutates a real Task[] with the real applyManageTasks reducer from + // tasks.ts — the same two functions the manage_tasks stringTool handlers + // wire up in src/agent/tools.ts and src/subagent/run.ts. This is what would + // have caught the dead-dispatch bug: routing update_plan through `runTool` + // (which only reaches posixTools, with no manage_tasks handler) fails with + // "unknown tool: manage_tasks" the instant this real dispatch is invoked, + // even though the old mock recorder's `if (name === "manage_tasks")` + // special case made every existing test pass. + test("maps plan steps onto manage_tasks(action=create) and actually mutates the task list", async () => { + const { calls, getTasks, runManageTasks } = makeRealManageTasks(); + const tools = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "unused" }), + runManageTasks, + }); const result = await invokeTool(tools, "update_plan", { explanation: "getting started", plan: [ @@ -401,22 +450,30 @@ describe("update_plan proxy", () => { expect(result.isError).toBeFalsy(); expect(calls).toEqual([ { - name: "manage_tasks", - args: { - action: "create", - tasks: [ - { id: "p1", title: "Read the file", status: "done" }, - { id: "p2", title: "Write the fix", status: "doing" }, - { id: "p3", title: "Run tests", status: "todo" }, - ], - }, + action: "create", + tasks: [ + { id: "p1", title: "Read the file", status: "done" }, + { id: "p2", title: "Write the fix", status: "doing" }, + { id: "p3", title: "Run tests", status: "todo" }, + ], }, ]); + // The real Task[] state, produced by the real applyManageTasks reducer — + // proof the dispatch reaches an actual task store, not just a recorded call. + expect(getTasks()).toEqual([ + { id: "p1", title: "Read the file", status: "done" }, + { id: "p2", title: "Write the fix", status: "doing" }, + { id: "p3", title: "Run tests", status: "todo" }, + ]); }); test("malformed plan surfaces as tool error", async () => { - const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const { calls, runManageTasks } = makeRealManageTasks(); + const tools = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "unused" }), + runManageTasks, + }); const result = await invokeTool(tools, "update_plan", { plan: [{ step: "no status here" }], }); @@ -426,8 +483,12 @@ describe("update_plan proxy", () => { }); test("missing plan surfaces as tool error", async () => { - const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool }); + const { calls, runManageTasks } = makeRealManageTasks(); + const tools = createCodexToolProxies({ + isCodex: true, + runTool: async () => ({ content: "unused" }), + runManageTasks, + }); const result = await invokeTool(tools, "update_plan", {}); expect(result.isError).toBe(true); expect(calls).toEqual([]); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index c19778293..dc9a5b589 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -22,9 +22,22 @@ export type CodexRunTool = ( args: Record, ) => 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 + * posixTools registry `runTool` forwards to — so this is its own callback, + * wired at each mount site (src/agent/tools.ts, src/subagent/run.ts) to the + * exact same manage_tasks stringTool handler that site installs. + */ +export type CodexRunManageTasks = ( + args: Record, +) => Promise<{ content: string; isError?: boolean }>; + export type CreateCodexToolProxiesOpts = { isCodex: boolean; runTool: CodexRunTool; + /** Dispatches update_plan's translated manage_tasks(action="create") call. */ + runManageTasks: CodexRunManageTasks; /** * When false, Delete File and Update+Move refuse without calling `delete_file`. * Defaults to true (implement / unconstrained). Docs leaves pass false because @@ -141,7 +154,7 @@ export function createCodexToolProxies(opts: CreateCodexToolProxiesOpts): AgentT return [ createApplyPatchProxy(opts.runTool, allowDelete), createShellProxy(opts.runTool, allowShell), - createUpdatePlanProxy(opts.runTool), + createUpdatePlanProxy(opts.runManageTasks), ]; } @@ -266,7 +279,7 @@ function requireOk( label: string, ): string { if (result.isError === true) { - throw new Error(`apply_patch failed (${label}): ${result.content}`); + throw new Error(`${label} failed: ${result.content}`); } return result.content; } @@ -394,7 +407,7 @@ function codexPlanStatusToTaskStatus(status: typeof CodexPlanStatus.infer): Task return "done"; } -function createUpdatePlanProxy(runTool: CodexRunTool): AgentTool { +function createUpdatePlanProxy(runManageTasks: CodexRunManageTasks): AgentTool { return stringTool({ definition: updatePlanDefinition, handler: async (rawArgs: Record): Promise => { @@ -415,7 +428,7 @@ function createUpdatePlanProxy(runTool: CodexRunTool): AgentTool { status: codexPlanStatusToTaskStatus(item.status), })); return requireOk( - await runTool("manage_tasks", { action: "create", tasks }), + await runManageTasks({ action: "create", tasks }), "update_plan", ); }, diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 7a0e1f1aa..75728f226 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -50,6 +50,7 @@ import { createToolIndex, createToolSearchTool } from "./tool-search.js"; import { createSearchAgentsTool } from "./agent-search.js"; import { createCodexToolProxies, + type CodexRunManageTasks, type CodexRunTool, } from "./codex-tool-proxies.js"; import type { ReactorEmittedEvent } from "@intx/inference"; @@ -236,6 +237,23 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise { + const parsed = parseManageTasksArgs(rawArgs); + if (parsed === null) { + return { + content: "Error: manage_tasks requires action ('create' or 'update').", + isError: true, + }; + } + return { content: "Tasks updated." }; + }; + // Align the advertised run_shell timeout with shell-guard's resolved default. const baseTools: AgentTool[] = [ ...fromToolRunner(posixTools).map((tool) => ({ @@ -289,11 +307,8 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise): Promise => { - const parsed = parseManageTasksArgs(rawArgs); - if (parsed === null) { - return "Error: manage_tasks requires action ('create' or 'update')."; - } - return "Tasks updated."; + const result = await runManageTasks(rawArgs); + return result.content; }, }), stringTool({ @@ -372,7 +387,7 @@ 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 300091cc6..1ed97472b 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -39,6 +39,7 @@ import { allowDeleteFromCapabilities, allowShellFromCapabilities, createCodexToolProxies, + type CodexRunManageTasks, type CodexRunTool, } from "../agent/codex-tool-proxies.js"; @@ -334,11 +335,28 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { ...(result.isError === true ? { isError: true } : {}), }; }; + // manage_tasks is not a posix tool — task state here is owned by the + // director observing manage_tasks tool_calls in the model's own output, + // not by this handler's return value (see applyManageTasksToolCall in + // director.ts). This handler only validates, so update_plan's proxy shares + // it rather than forwarding through posixTools (which has no manage_tasks + // handler to forward to). + const runManageTasks: CodexRunManageTasks = async (rawArgs) => { + const parsed = parseManageTasksArgs(rawArgs); + if (parsed === null) { + return { + content: "Error: manage_tasks requires action ('create' or 'update').", + isError: true, + }; + } + return { content: "Tasks updated." }; + }; tools = [ ...tools, ...createCodexToolProxies({ isCodex: isCodexProviderName(params.provider.providerName), runTool, + runManageTasks, allowDelete: allowDeleteFromCapabilities(params.capabilities), allowShell: allowShellFromCapabilities(params.capabilities), }), @@ -357,11 +375,8 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { stringTool({ definition: manageTasksDefinition, handler: async (rawArgs: Record): Promise => { - const parsed = parseManageTasksArgs(rawArgs); - if (parsed === null) { - return "Error: manage_tasks requires action ('create' or 'update')."; - } - return "Tasks updated."; + const result = await runManageTasks(rawArgs); + return result.content; }, }), ]; From 00ab2d922606c1bc4fabc40600f6d4f56ca5a99c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 17:36:08 -0700 Subject: [PATCH 4/6] Fix Codex mount test DIY assertions under real posix tools Empty createPosixTools stub hid write_file/edit_file/delete_file, so the primary DIY-remains expects could never pass. Use the real mount like the update_plan e2e case. --- src/agent/codex-tool-mount.test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index 3d83b3e11..ed4c4a58b 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -56,13 +56,10 @@ describe("Codex tool proxy mount", () => { }); test("Codex createAgentToolset strips apply_patch on primary; keeps shell/update_plan", async () => { + // Unstubbed createPosixTools: write_file / edit_file / delete_file come from + // the real posix + delete-file plugin mount. An empty stub would hide them + // and make the DIY-remains assertion meaningless. const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-")); - spyOn(posixModule, "createPosixTools").mockReturnValue({ - definitions: [], - run: async () => ({ id: "x", content: "" }), - dispose: async () => {}, - } as unknown as ReturnType); - const { createAgentToolset } = await import("./tools.js"); const permissionGate = { check: async () => ({ allowed: true }), From ad9206f62b5a1314e5e22ba90694324bee36c9f9 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 18:39:02 -0700 Subject: [PATCH 5/6] Format codex proxy files with prettier --- src/agent/codex-apply-patch.test.ts | 6 +- src/agent/codex-apply-patch.ts | 36 +++---- src/agent/codex-tool-mount.test.ts | 12 +-- src/agent/codex-tool-proxies.test.ts | 130 +++++++++++++++++------ src/agent/codex-tool-proxies.ts | 26 ++--- src/agent/product-mutation-tools.test.ts | 5 +- 6 files changed, 118 insertions(+), 97 deletions(-) diff --git a/src/agent/codex-apply-patch.test.ts b/src/agent/codex-apply-patch.test.ts index fdf64bde9..8ff0c5da6 100644 --- a/src/agent/codex-apply-patch.test.ts +++ b/src/agent/codex-apply-patch.test.ts @@ -157,11 +157,7 @@ describe("extractAffectedPaths", () => { *** Delete File: obsolete.txt *** End Patch `); - expect(extractAffectedPaths(patch)).toEqual([ - "hello.txt", - "src/app.py", - "obsolete.txt", - ]); + expect(extractAffectedPaths(patch)).toEqual(["hello.txt", "src/app.py", "obsolete.txt"]); }); test("move path extraction includes source and destination", () => { diff --git a/src/agent/codex-apply-patch.ts b/src/agent/codex-apply-patch.ts index 1aa98ca55..e93401319 100644 --- a/src/agent/codex-apply-patch.ts +++ b/src/agent/codex-apply-patch.ts @@ -115,8 +115,7 @@ export function parseCodexApplyPatch(input: string): ParsedPatch { ); } // Codex-rs: each '+' line contributes text + "\n". - const content = - contentLines.length === 0 ? "" : contentLines.map((l) => `${l}\n`).join(""); + const content = contentLines.length === 0 ? "" : contentLines.map((l) => `${l}\n`).join(""); ops.push({ type: "add", path, content }); continue; } @@ -147,7 +146,11 @@ export function parseCodexApplyPatch(input: string): ParsedPatch { `malformed Update File '${path}': expected hunk ('@@') or next file op, got: ${body[i]}`, ); } - ops.push(moveTo === undefined ? { type: "update", path, hunks } : { type: "update", path, moveTo, hunks }); + ops.push( + moveTo === undefined + ? { type: "update", path, hunks } + : { type: "update", path, moveTo, hunks }, + ); continue; } @@ -198,9 +201,7 @@ export function applyUpdateHunks(original: string, hunks: PatchHunk[]): string { if (hunk.header !== undefined && hunk.header.length > 0) { const idx = findLineFrom(lines, hunk.header, cursor); if (idx === -1) { - throw new CodexApplyPatchError( - `failed to find hunk context header '${hunk.header}'`, - ); + throw new CodexApplyPatchError(`failed to find hunk context header '${hunk.header}'`); } cursor = idx + 1; } @@ -245,10 +246,7 @@ export function contentFromAddOp(op: PatchAddOp): string { return op.content; } -function parseHunk( - body: string[], - start: number, -): { hunk: PatchHunk; next: number } { +function parseHunk(body: string[], start: number): { hunk: PatchHunk; next: number } { const headerLine = body[start]!; let header: string | undefined; if (headerLine === "@@") { @@ -268,9 +266,8 @@ function parseHunk( if (raw === END_OF_FILE) { i += 1; return { - hunk: header === undefined - ? { lines, endOfFile: true } - : { header, lines, endOfFile: true }, + hunk: + header === undefined ? { lines, endOfFile: true } : { header, lines, endOfFile: true }, next: i, }; } @@ -305,11 +302,7 @@ function parseHunk( } function isFileOpHeader(line: string): boolean { - return ( - line.startsWith(ADD_FILE) || - line.startsWith(DELETE_FILE) || - line.startsWith(UPDATE_FILE) - ); + return line.startsWith(ADD_FILE) || line.startsWith(DELETE_FILE) || line.startsWith(UPDATE_FILE); } function isHunkStart(line: string): boolean { @@ -366,9 +359,7 @@ function findSequence( if (pattern.length > lines.length) return -1; const searchStart = - endOfFile && lines.length >= pattern.length - ? lines.length - pattern.length - : from; + endOfFile && lines.length >= pattern.length ? lines.length - pattern.length : from; const tryFrom = (start: number, eq: (a: string, b: string) => boolean): number => { for (let i = start; i <= lines.length - pattern.length; i++) { @@ -385,8 +376,7 @@ function findSequence( }; // When eof, try the eof-aligned window first, then fall through from `from`. - const starts = - endOfFile && searchStart !== from ? [searchStart, from] : [searchStart]; + const starts = endOfFile && searchStart !== from ? [searchStart, from] : [searchStart]; for (const start of starts) { const exact = tryFrom(start, (a, b) => a === b); diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index ed4c4a58b..5c3cea10b 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -132,11 +132,7 @@ describe("Codex tool proxy mount", () => { runTool: async () => ({ content: "ok" }), runManageTasks: async () => ({ content: "ok" }), }); - expect(proxies.map((t) => t.definition.name)).toEqual([ - "apply_patch", - "shell", - "update_plan", - ]); + expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); const allow = new Set(BUILD_TOOLS); const kept = proxies.filter((t) => allow.has(t.definition.name)); @@ -160,11 +156,7 @@ describe("Codex tool proxy mount", () => { allowDelete: allowDeleteFromCapabilities(docsCapabilities), allowShell: allowShellFromCapabilities(docsCapabilities), }); - expect(proxies.map((t) => t.definition.name)).toEqual([ - "apply_patch", - "shell", - "update_plan", - ]); + expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]); const docsAllow = new Set(DOCS_TOOLS); const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 7908c8b79..8f66ca0fc 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -120,7 +120,11 @@ describe("createCodexToolProxies", () => { test("add forwards write_file with Codex trailing newline", async () => { const { calls, files, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -143,7 +147,11 @@ describe("createCodexToolProxies", () => { test("delete forwards delete_file", async () => { const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -159,7 +167,12 @@ describe("createCodexToolProxies", () => { test("allowDelete false refuses Delete without calling delete_file", async () => { const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + allowDelete: false, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -179,7 +192,12 @@ describe("createCodexToolProxies", () => { print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + allowDelete: false, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -203,7 +221,12 @@ print("Hi") print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + allowDelete: false, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -227,7 +250,11 @@ print("Hi") print("bye") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -253,7 +280,11 @@ print("bye") print("Hi") `; const { calls, files, runTool } = makeRecorder({ "src/app.py": original }); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -284,7 +315,11 @@ print("Hello, world!") "src/app.py": "old\n", "obsolete.txt": "x", }); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -312,7 +347,11 @@ print("Hello, world!") test("parse failure surfaces as tool error (isError)", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Add File: a.txt @@ -342,7 +381,11 @@ print("Hello, world!") test("runTool isError aborts the patch with isError", async () => { const { runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeApplyPatch( tools, `*** Begin Patch @@ -361,7 +404,11 @@ print("Hello, world!") describe("shell proxy", () => { test("string command forwards to run_shell", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeTool(tools, "shell", { command: "ls -la" }); expect(result.isError).toBeFalsy(); expect(calls).toEqual([{ name: "run_shell", args: { command: "ls -la" } }]); @@ -369,21 +416,33 @@ describe("shell proxy", () => { test("bash -lc argv triple unwraps to the script", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); await invokeTool(tools, "shell", { command: ["bash", "-lc", "echo 'hi there'"] }); expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hi there'" } }]); }); test("other argv arrays are shell-quoted and joined", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); await invokeTool(tools, "shell", { command: ["echo", "hello world"] }); expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hello world'" } }]); }); test("workdir and timeout_ms translate to cwd and timeout", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); await invokeTool(tools, "shell", { command: "pwd", workdir: "/tmp/work", @@ -396,7 +455,11 @@ describe("shell proxy", () => { test("missing command surfaces as tool error", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeTool(tools, "shell", {}); expect(result.isError).toBe(true); expect(result.content).toMatch(/command/); @@ -405,7 +468,12 @@ describe("shell proxy", () => { test("allowShell false refuses without calling run_shell", async () => { const { calls, runTool } = makeRecorder(); - const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + allowShell: false, + runManageTasks: unusedManageTasks, + }); const result = await invokeTool(tools, "shell", { command: "ls" }); expect(result.isError).toBe(true); expect(result.content).toMatch(/not allowed/); @@ -414,7 +482,11 @@ describe("shell proxy", () => { test("run_shell isError propagates as tool error", async () => { const runTool: CodexRunTool = async () => ({ content: "boom", isError: true }); - const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks }); + const tools = createCodexToolProxies({ + isCodex: true, + runTool, + runManageTasks: unusedManageTasks, + }); const result = await invokeTool(tools, "shell", { command: "ls" }); expect(result.isError).toBe(true); expect(result.content).toMatch(/boom/); @@ -497,19 +569,11 @@ describe("update_plan proxy", () => { describe("allowDeleteFromCapabilities", () => { test("docs allowlist (no delete_file) → false; build → true", () => { - expect( - allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS }), - ).toBe(false); - expect( - allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }), - ).toBe(true); + expect(allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(false); + expect(allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS })).toBe(true); expect(allowDeleteFromCapabilities(undefined)).toBe(true); - expect( - allowDeleteFromCapabilities({ mode: "exclude", tools: ["run_shell"] }), - ).toBe(true); - expect( - allowDeleteFromCapabilities({ mode: "exclude", tools: ["delete_file"] }), - ).toBe(false); + expect(allowDeleteFromCapabilities({ mode: "exclude", tools: ["run_shell"] })).toBe(true); + expect(allowDeleteFromCapabilities({ mode: "exclude", tools: ["delete_file"] })).toBe(false); }); }); @@ -518,11 +582,7 @@ describe("allowShellFromCapabilities", () => { expect(allowShellFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(false); expect(allowShellFromCapabilities({ mode: "allow", tools: BUILD_TOOLS })).toBe(true); expect(allowShellFromCapabilities(undefined)).toBe(true); - expect( - allowShellFromCapabilities({ mode: "exclude", tools: ["delete_file"] }), - ).toBe(true); - expect( - allowShellFromCapabilities({ mode: "exclude", tools: ["run_shell"] }), - ).toBe(false); + expect(allowShellFromCapabilities({ mode: "exclude", tools: ["delete_file"] })).toBe(true); + expect(allowShellFromCapabilities({ mode: "exclude", tools: ["run_shell"] })).toBe(false); }); }); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index dc9a5b589..25fc5e21c 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -217,11 +217,7 @@ function createApplyPatchProxy(runTool: CodexRunTool, allowDelete: boolean): Age }); } -async function applyOp( - op: PatchOp, - runTool: CodexRunTool, - allowDelete: boolean, -): Promise { +async function applyOp(op: PatchOp, runTool: CodexRunTool, allowDelete: boolean): Promise { if (op.type === "add") { return requireOk( await runTool("write_file", { path: op.path, content: op.content }), @@ -274,10 +270,7 @@ async function applyOp( return writeMsg; } -function requireOk( - result: { content: string; isError?: boolean }, - label: string, -): string { +function requireOk(result: { content: string; isError?: boolean }, label: string): string { if (result.isError === true) { throw new Error(`${label} failed: ${result.content}`); } @@ -305,7 +298,7 @@ export const shellDefinition: ToolDefinition = { properties: { command: { description: - "The command to run, as a shell string or an argv array (e.g. [\"bash\",\"-lc\",\"ls\"]).", + 'The command to run, as a shell string or an argv array (e.g. ["bash","-lc","ls"]).', }, workdir: { type: "string", description: "Working directory for the command." }, timeout_ms: { type: "number", description: "Timeout in milliseconds." }, @@ -350,9 +343,7 @@ function createShellProxy(runTool: CodexRunTool, allowShell: boolean): AgentTool throw new Error("Error: shell requires a command (string or string[])."); } if (!allowShell) { - throw new Error( - "shell: not allowed for this agent (run_shell capability missing).", - ); + throw new Error("shell: not allowed for this agent (run_shell capability missing)."); } const args: Record = { command: normalizeShellCommand(parsed.command) }; if (parsed.workdir !== undefined) args.cwd = parsed.workdir; @@ -413,9 +404,7 @@ function createUpdatePlanProxy(runManageTasks: CodexRunManageTasks): AgentTool { handler: async (rawArgs: Record): Promise => { const parsed = UpdatePlanArgs(rawArgs); if (parsed instanceof type.errors) { - throw new Error( - "Error: update_plan requires a plan array of { step, status }.", - ); + throw new Error("Error: update_plan requires a plan array of { step, status }."); } // manage_tasks has no "cancelled" equivalent in Codex's plan shape // (pending/in_progress/completed) — this proxy never produces it, so a @@ -427,10 +416,7 @@ function createUpdatePlanProxy(runManageTasks: CodexRunManageTasks): AgentTool { title: item.step, status: codexPlanStatusToTaskStatus(item.status), })); - return requireOk( - await runManageTasks({ action: "create", tasks }), - "update_plan", - ); + return requireOk(await runManageTasks({ action: "create", tasks }), "update_plan"); }, }); } diff --git a/src/agent/product-mutation-tools.test.ts b/src/agent/product-mutation-tools.test.ts index 4a4cf6f7f..66cacd9ba 100644 --- a/src/agent/product-mutation-tools.test.ts +++ b/src/agent/product-mutation-tools.test.ts @@ -32,10 +32,7 @@ describe("PRODUCT_MUTATION_TOOLS", () => { +new *** End Patch `; - expect(productMutationPaths("apply_patch", { input })).toEqual([ - "hello.txt", - "src/app.py", - ]); + expect(productMutationPaths("apply_patch", { input })).toEqual(["hello.txt", "src/app.py"]); }); test("productMutationPaths returns [] for malformed apply_patch input", () => { From 5dba707bc28d9dc22ed116d44ddaadaba5c24b1e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 21:11:58 -0700 Subject: [PATCH 6/6] Fix lint on Codex apply-patch proxy types --- docs/IMPLEMENTATION.md | 1 + src/agent/codex-apply-patch.ts | 24 ++++++++++++------------ src/agent/codex-tool-proxies.test.ts | 5 ++++- src/agent/codex-tool-proxies.ts | 4 ++-- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index c22bfc641..5d6700d78 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -161,6 +161,7 @@ Sixteen packages under `src/agent/directors//` register in `DIRECTOR_REGISTR 5. Primary chat role is Skywalker: `buildChatRole()` → `createSkywalkerSystemPrompt()`. Product mutation tools (`write_file` / `edit_file` / `delete_file`) live in CORE (and `SKYWALKER_TOOLS`) so they are advertised on the primary without a `tool_search` round-trip. DIY tiny/bounded edits on the parent; spawn build/docs directors for substantial work — a prompt judgment call, not a toolset strip. `PRIMARY_DENIED_PRODUCT_TOOLS` is gone. Shell file-writes stay denied; MCP tools are not re-filtered by a product-write deny list. Optional `writePaths` (when a profile sets it) only gate path-keyed product tools. **Codex tool proxies.** When the active provider is Codex (`isCodexProviderName`), `createAgentToolset` and `runSubAgent` mount `apply_patch`, `shell`, and `update_plan` stringTools from `createCodexToolProxies`, all forwarding through the same posix `ToolRunner` seam (`runTool`) so permission plugins still apply. `apply_patch` parses the Codex envelope and forwards each op (`write_file` / `delete_file` / `read_file`). `shell` — the native Codex name is `shell`, not `exec_command`, per the pinned base-instructions text quoted in `codex-responses-adapter.ts`'s bridge message — normalizes Codex's `command` (string or `["bash","-lc",script]`-style argv array), `workdir`, and `timeout_ms` onto `run_shell`'s `{command, cwd?, timeout?}` and is gated by `allowShellFromCapabilities` (mirrors `allowDeleteFromCapabilities` against `run_shell`). `update_plan` maps Codex's `plan: [{step, status}]` onto `manage_tasks(action: "create")`; `pending`/`in_progress`/`completed` map to `todo`/`doing`/`done` — `manage_tasks`'s `cancelled` status has no Codex equivalent and is never produced by this proxy. Primary strips `apply_patch` after mount (Corbits DIY stays on `write_file` / `edit_file` / `delete_file`); `shell` and `update_plan` stay on primary (same classification as `run_shell` / `manage_tasks`). Build and docs leaf allowlists (`BUILD_TOOLS` / `DOCS_TOOLS`) include `apply_patch` so Codex workers keep the proxy after the capability filter. `CORE_TOOL_NAMES` does not list it. + 6. Shipped directors omit `writePaths`. The optional field is still enforced in the permission gate via ALS identity (`identity-context.ts` + `write-path-policy.ts`) when a plugin/custom profile sets it. 7. Spawn effort: pin > package `modelRole` default (`defaultEffortForDirector`; intern=low; plan/review/orchestrator=high; implement/explore/docs/test=medium) > orchestrator/worker binary > parent inheritance. Optional skills are listed in the identity header for awareness; workers do not mount `use_skill` (guidance is baked into package system prompts). Primary mounts `use_skill` for its own skill list. diff --git a/src/agent/codex-apply-patch.ts b/src/agent/codex-apply-patch.ts index e93401319..817be6396 100644 --- a/src/agent/codex-apply-patch.ts +++ b/src/agent/codex-apply-patch.ts @@ -27,19 +27,19 @@ const END_OF_FILE = "*** End of File"; export type HunkLineKind = " " | "-" | "+"; -export type PatchHunkLine = { +export interface PatchHunkLine { kind: HunkLineKind; text: string; -}; +} -export type PatchHunk = { +export interface PatchHunk { /** Optional text after `@@` (class/method anchor). */ header?: string; lines: PatchHunkLine[]; endOfFile?: boolean; -}; +} -export type PatchAddOp = { +export interface PatchAddOp { type: "add"; path: string; /** @@ -48,25 +48,25 @@ export type PatchAddOp = { * An Add File with no `+` lines yields `""`. */ content: string; -}; +} -export type PatchDeleteOp = { +export interface PatchDeleteOp { type: "delete"; path: string; -}; +} -export type PatchUpdateOp = { +export interface PatchUpdateOp { type: "update"; path: string; moveTo?: string; hunks: PatchHunk[]; -}; +} export type PatchOp = PatchAddOp | PatchDeleteOp | PatchUpdateOp; -export type ParsedPatch = { +export interface ParsedPatch { ops: PatchOp[]; -}; +} export class CodexApplyPatchError extends Error { constructor(message: string) { diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 8f66ca0fc..71bc6ded9 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -12,7 +12,10 @@ import { import { DOCS_TOOLS, BUILD_TOOLS } from "./directors/tool-sets.js"; import { applyManageTasks, parseManageTasksArgs, type Task } from "./tasks.js"; -type Call = { name: string; args: Record }; +interface Call { + name: string; + args: Record; +} // `manage_tasks` is deliberately NOT a branch here: the real posixTools // registry runTool forwards to has no manage_tasks handler (only diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index 25fc5e21c..f2313846b 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -33,7 +33,7 @@ export type CodexRunManageTasks = ( args: Record, ) => Promise<{ content: string; isError?: boolean }>; -export type CreateCodexToolProxiesOpts = { +export interface CreateCodexToolProxiesOpts { isCodex: boolean; runTool: CodexRunTool; /** Dispatches update_plan's translated manage_tasks(action="create") call. */ @@ -49,7 +49,7 @@ export type CreateCodexToolProxiesOpts = { * Docs leaves pass false because DOCS_TOOLS omits run_shell. */ allowShell?: boolean; -}; +} const ApplyPatchArgs = type({ input: "string>0",