From 8764920123935ee4d2eca43ed91b5a82a1451593 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 08:09:33 -0700 Subject: [PATCH 1/4] feat(tools): advertise one posix wire set; hide engine aliases Advertise read/write/edit/delete/bash/grep/glob plus the control-plane. Registry engines stay posix-named. Hidden aliases dispatch; grants canonicalize both sides. Codex does not advertise apply_patch, shell, or update_plan. Fixes CL-8400 --- docs/IMPLEMENTATION.md | 2 +- packages/prompt-variance/src/rows.test.ts | 2 +- packages/prompt-variance/src/rows.ts | 2 +- .../skills/native-integration/SKILL.md | 12 +- .../skills/native-runtime/SKILL.md | 6 +- scripts/eval-capability.test.ts | 6 +- src/agent/canonical-tool-name.ts | 7 +- src/agent/codex-tool-mount.test.ts | 83 +++---- .../directors/bruckheimer/package.test.ts | 8 +- src/agent/directors/bruckheimer/package.ts | 4 +- src/agent/directors/gauntlet/package.ts | 8 +- src/agent/directors/greybeard/package.test.ts | 2 +- src/agent/directors/greybeard/package.ts | 4 +- src/agent/directors/migrator/package.ts | 2 +- src/agent/directors/neckbeard/package.ts | 4 +- src/agent/directors/registry.test.ts | 14 +- src/agent/directors/skywalker/package.test.ts | 6 +- src/agent/directors/skywalker/package.ts | 2 +- src/agent/directors/tool-sets.test.ts | 23 +- src/agent/directors/tool-sets.ts | 29 +-- src/agent/product-mutation-tools.ts | 8 +- src/agent/prompt-contract.ts | 2 +- src/agent/prompt-sizes.ts | 52 ++-- src/agent/prompts.test.ts | 18 +- src/agent/prompts.ts | 57 ++--- src/agent/tool-aliases.test.ts | 228 ++++++++++++++++++ src/agent/tool-aliases.ts | 209 ++++++++++++++++ src/agent/tool-search.test.ts | 13 +- src/agent/tool-search.ts | 75 ++++-- src/agent/tools.ts | 51 +--- src/exec/runner.test.ts | 8 +- src/exec/runner.ts | 17 +- src/plugins/data-only-agent.ts | 15 +- src/plugins/permission-plugin.test.ts | 6 +- src/prompts.test.ts | 32 +-- src/session/assemble-runtime.test.ts | 4 +- src/session/assemble-runtime.ts | 12 +- src/shell/run-shell-authz.test.ts | 2 +- src/shell/run-shell-authz.ts | 4 +- src/subagent/run.ts | 55 ++--- src/tui/dynamic-tool-runner.ts | 13 +- src/tui/runner/session.ts | 14 +- tests/unit/exec/runner.test.ts | 32 +-- 43 files changed, 744 insertions(+), 409 deletions(-) create mode 100644 src/agent/tool-aliases.test.ts create mode 100644 src/agent/tool-aliases.ts diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index c89685079..7b488e170 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -165,7 +165,7 @@ Twenty packages under `src/agent/directors//` register in `DIRECTOR_REGISTRY **Grok infer envelope.** `loadSessionChatPrompt` always appends `loadAgentContextExtensions` (`AGENTS.md`, capped at `MAX_AGENTS_MD_BYTES`) and advertises CORE+CATALOG full schemas via `advertisedToolNamesForSessionMode`. That assembly is family-agnostic — Grok does not substitute the trimmed director prompt (`buildSubAgentSystemPrompt` + `formatDirectorSystemPrompt`). Workers already use that trimmed path (no `AGENTS.md`, mounted-tool schemas only). Keep the infer envelope on Grok; do not strip `AGENTS.md` or core schemas. Measure in `src/agent/prompt-sizes.ts` (`assembleSkywalkerInferEnvelope` vs `assembleDirectorPrompt("skywalker", "grok")`). - **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` — 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 worker 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. + **One advertised posix set (CL-8400).** Registry engines stay posix-named (`read_file`, `write_file`, `edit_file`, `delete_file`, `run_shell`, `search_files`, `grep`). Advertise is a 1:1 projection onto wire names (`read`, `write`, `edit`, `delete`, `bash`, `glob`, `grep`) plus the unchanged control-plane. Incoming aliases (wire names, old posix ids, Codex `shell` → `run_shell`, `update_plan` → `manage_tasks`) canonicalize onto the engine id for dispatch and grants. `apply_patch` is neither advertised nor dispatched (not an alias of `edit`). `list_dir` stays mounted and unadvertised. Director `tools.allow` stays engine names. Codex does not dual-publish `shell`+`run_shell`. Hidden `shell` coerces Codex `command` (string or `["bash","-lc",script]` argv), `workdir`, and `timeout_ms` onto `run_shell`. Hidden `update_plan` maps `plan: [{step, status}]` onto `manage_tasks(action: "create")` (`pending`/`in_progress`/`completed` → `todo`/`doing`/`done`). 6. There is no static write-path declaration on packages or profiles (CL-6952 removed it — no shipped director ever set one). Instead, `agent-fleet.ts` tracks each running dispatch by cwd; a new mutating dispatch that lands on the same cwd as a live mutating peer (`pending_init`/`running`, and not a declared read-only `modelRole` of `explore`/`plan`/`review`/`test`) records at most one `concurrent-lane-overlap` entry per cwd wave in `intervention-log.ts` (class `conflict`). The wave flag clears when no live mutating writer remains for that cwd. Terminal-but-unsettled lanes (for example cancelled with `finishedAt` set while the run promise has not reached `finally`) are pruned from the map and do not warn. This is advisory only — it never blocks the spawn, since cwd overlap does not prove the two lanes touch the same files. 7. Spawn effort: pin > package `modelRole` default (`defaultEffortForDirector`; intern=low; plan/review/orchestrator=high; implement/explore/docs/test=medium) > orchestrator/worker binary > parent inheritance. Attached skills (style + philosophy on directors that listed both; never intern or Skywalker primary) are injected into the worker system prompt at spawn from plugin skill dirs only (no project-local `.agents`/`.claude`/`.codex` fallback). Optional skills are listed in the identity header for awareness; workers mount `skill_search` + `use_skill` on every family, scoped to the union of `attachedSkills` and `optionalSkills`. `use_skill` refuses names already attached or already loaded this session and does not return the body again. Primary mounts `use_skill` for its own skill list (same in-session refuse; no attached set). diff --git a/packages/prompt-variance/src/rows.test.ts b/packages/prompt-variance/src/rows.test.ts index 5376b4e2d..9d11bbd30 100644 --- a/packages/prompt-variance/src/rows.test.ts +++ b/packages/prompt-variance/src/rows.test.ts @@ -74,7 +74,7 @@ describe("prompt-variance family rows", () => { expect(grokRow.residual).toContain("prefer the structured report"); expect(grokRow.residual).toContain("re-open paths you already read"); expect(grokRow.residual).toContain("done-definition is met"); - expect(grokRow.residual).toContain("never run_shell"); + expect(grokRow.residual).toContain("never bash"); }); test("each ceremony line appears exactly once in the grok row (P2 invariant)", () => { diff --git a/packages/prompt-variance/src/rows.ts b/packages/prompt-variance/src/rows.ts index 45add5ee0..21c1067f3 100644 --- a/packages/prompt-variance/src/rows.ts +++ b/packages/prompt-variance/src/rows.ts @@ -64,7 +64,7 @@ export const grokRow: PromptVarianceRow = { "- Once you can answer the dispatch brief, prefer the structured report over another speculative tool call.", "- If the next call would only re-open paths you already read, write the report instead.", "- When the dispatch brief's done-definition is met, write the report envelope instead of making one more search or micro-edit.", - "- Route file and web work through the dedicated tools, never run_shell — mining showed grok reaching for shell first when a typed tool already covered the job.", + "- Route file and web work through the dedicated tools, never bash — mining showed grok reaching for shell first when a typed tool already covered the job.", "- Never run git add, git commit, git stash, or any other state-changing git command unless the user asks.", "- Do not narrate a plan before acting on a small task; act, then report.", "- Verify with the test command once at the end, not after every edit.", diff --git a/plugins/corbits-skills/skills/native-integration/SKILL.md b/plugins/corbits-skills/skills/native-integration/SKILL.md index a1b327ffa..dcba4ca90 100644 --- a/plugins/corbits-skills/skills/native-integration/SKILL.md +++ b/plugins/corbits-skills/skills/native-integration/SKILL.md @@ -16,7 +16,7 @@ Do not delete Corbits-only skills (`plan`, `git-worktrees`, `idiot-proof`). They Corbits tests use `bun:test` (`bun test`, `bun run test`), not GaaS `tap` (`import t from "tap"`). When the typescript skill shows tap examples, map them to bun:test (`import { expect, test } from "bun:test"`). Do not fork the typescript skill body. -GaaS opsh scripts are bash (`#!/usr/bin/env opsh`, `lib::import`) and use TAP via `prove` (`test-harness`). That harness is not Corbits `bun:test`. Write scripts with `write_file`/`edit_file`; agent commands use `run_shell`. Do not fork the GaaS opsh body. +GaaS opsh scripts are bash (`#!/usr/bin/env opsh`, `lib::import`) and use TAP via `prove` (`test-harness`). That harness is not Corbits `bun:test`. Write scripts with `write`/`edit`; agent commands use `bash`. Do not fork the GaaS opsh body. ## Tool mapping @@ -32,14 +32,14 @@ When a GaaS skill names a Claude/GaaS tool, use the Corbits equivalent. Do not c | `@critic` / `@critique` | `spawn_agent(agent="critic")` | | `@intern` | `spawn_agent(agent="intern")` | | `@explorer` | `spawn_agent(agent="explorer")` | -| Read / Write / Edit | `read_file` / `write_file` / `edit_file` | -| Glob / Grep | `search_files` / `grep` | -| Bash | `run_shell` | +| Read / Write / Edit | `read` / `write` / `edit` | +| Glob / Grep | `glob` / `grep` | +| Bash | `bash` | | WebFetch / WebSearch | `web_fetch` / `web_search` | `intent="general"` is not a Corbits spawn. Use a closed director id. -GaaS ast-grep invokes `sg` as a CLI. Corbits extras: run `sg` via `run_shell`. Do not fork the GaaS ast-grep body. +GaaS ast-grep invokes `sg` as a CLI. Corbits extras: run `sg` via `bash`. Do not fork the GaaS ast-grep body. Slash names that differ from GaaS skill ids: `/review` is GaaS `code-review`; `/create-issue` is GaaS `linear-create`. Keep those Corbits names. @@ -67,7 +67,7 @@ GaaS linear-issue-workflow inlines `git worktree add` and marks In Progress afte GaaS `style` refuses to operate outside a git repo. Corbits does not: a folder without `.git` is a valid working directory (scratch, unpacked tarball, new project). Git-using skills (`implement`, `review`, `git-rebase`, `pull-request-review`) still no-op or ask when they need a repo. Do not invent a git repo to satisfy those skills. -When GaaS git-rebase writes `/tmp` editor scripts, Corbits still plans on the primary and intern executes sequenced git via `run_shell`; intern may use inline `GIT_SEQUENCE_EDITOR` instead of write_file editor scripts. Do not fork the GaaS git-rebase body. +When GaaS git-rebase writes `/tmp` editor scripts, Corbits still plans on the primary and intern executes sequenced git via `bash`; intern may use inline `GIT_SEQUENCE_EDITOR` instead of write editor scripts. Do not fork the GaaS git-rebase body. ## Tracker-agnostic issues diff --git a/plugins/corbits-skills/skills/native-runtime/SKILL.md b/plugins/corbits-skills/skills/native-runtime/SKILL.md index 3da17c9b0..ef0d23894 100644 --- a/plugins/corbits-skills/skills/native-runtime/SKILL.md +++ b/plugins/corbits-skills/skills/native-runtime/SKILL.md @@ -5,13 +5,13 @@ disable-model-invocation: true description: Compact Corbits worker runtime invariants for baked prompts. --- -Use Corbits tool names: `read_file`, `write_file`, `edit_file`, `delete_file`, -`grep`, `search_files`, `run_shell`, `web_search`, `web_fetch`, +Use Corbits tool names: `read`, `write`, `edit`, `delete`, +`grep`, `glob`, `bash`, `web_search`, `web_fetch`, `manage_tasks`, and `ask_director` for worker questions. Use file tools for file reads, edits, writes, and deletions. Never use shell redirects, heredocs, `echo`, `cat`, stream editors, or remove commands as -substitutes for file tools. Use bounded `grep` and `search_files` instead of +substitutes for file tools. Use bounded `grep` and `glob` instead of unbounded recursive shell searches. Use web tools for URLs; never use curl or wget. diff --git a/scripts/eval-capability.test.ts b/scripts/eval-capability.test.ts index 6d6ca258b..10ebba4be 100644 --- a/scripts/eval-capability.test.ts +++ b/scripts/eval-capability.test.ts @@ -381,8 +381,8 @@ describe("buildEvalDiagnostics", () => { const diagnostics = await buildEvalDiagnostics( sampleConfig({ providerName: "openai" }), ); - expect(diagnostics.advertisedTools).toContain("read_file"); - expect(diagnostics.advertisedTools).toContain("run_shell"); + expect(diagnostics.advertisedTools).toContain("read"); + expect(diagnostics.advertisedTools).toContain("bash"); expect(diagnostics.advertisedTools).not.toContain("ask_operator"); expect(diagnostics.reasoningEffort).toBeNull(); }); @@ -394,7 +394,7 @@ describe("buildEvalDiagnostics", () => { sampleConfig({ providerName }), ); expect(diagnostics).not.toHaveProperty("codexInstructionsHash"); - expect(diagnostics.advertisedTools).toContain("read_file"); + expect(diagnostics.advertisedTools).toContain("read"); }, ); diff --git a/src/agent/canonical-tool-name.ts b/src/agent/canonical-tool-name.ts index 78398ab6c..2ed49d5d7 100644 --- a/src/agent/canonical-tool-name.ts +++ b/src/agent/canonical-tool-name.ts @@ -1,15 +1,20 @@ +import { engineToolName } from "./tool-aliases.js"; + const DEFAULT_PREFIX = "default."; // Muse Spark emits `default.` and duplicated `.`. Dispatch // already strips those onto catalog keys; classify, grants, and the execution // cache must use the same name so an alias cannot force a second ask/deny. +// Wire names (read/bash/…) and hidden aliases (shell/update_plan) collapse onto +// the registry engine id so a grant stored as run_shell covers bash. export function canonicalToolName(requested: string): string { let name = requested; if (name.startsWith(DEFAULT_PREFIX)) { const stripped = name.slice(DEFAULT_PREFIX.length); if (stripped.length > 0) name = stripped; } - return undoubledName(name) ?? name; + name = undoubledName(name) ?? name; + return engineToolName(name); } function undoubledName(requested: string): string | undefined { diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index 525fc0d7f..c99b34e43 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -1,13 +1,6 @@ /** - * 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. + * Mount coverage for Codex: no advertised apply_patch/shell/update_plan, + * engines stay posix-named, hidden aliases dispatch without dual-publish. */ import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -21,7 +14,7 @@ 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"; +import { advertisedTools, CORE_TOOL_NAMES } from "./tool-search.js"; afterEach(() => { spyOn(posixModule, "createPosixTools").mockRestore(); @@ -55,7 +48,7 @@ describe("Codex tool proxy mount", () => { await toolset.dispose(); }); - test("Codex createAgentToolset strips apply_patch on primary; keeps shell/update_plan", async () => { + test("Codex createAgentToolset does not advertise apply_patch/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. @@ -74,19 +67,24 @@ describe("Codex tool proxy mount", () => { }); 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"); - // 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"); + expect(names).toContain("run_shell"); + expect(names).not.toContain("shell"); + expect(names).not.toContain("update_plan"); + const advertised = advertisedTools( + toolset.dynamicRunner.currentDefinitions(), + ).map((d) => d.name); + expect(advertised).toContain("bash"); + expect(advertised).not.toContain("run_shell"); + expect(advertised).not.toContain("shell"); + expect(advertised).not.toContain("apply_patch"); + expect(advertised).not.toContain("update_plan"); await toolset.dispose(); }); - test("update_plan dispatches through the real mount without hitting posixTools", async () => { + test("update_plan hidden-dispatches through manage_tasks without a proxy mount", 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 @@ -120,13 +118,13 @@ describe("Codex tool proxy mount", () => { 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"); + test("BUILD_TOOLS and DOCS_TOOLS omit apply_patch; CORE_TOOL_NAMES does not list it", () => { + expect(BUILD_TOOLS).not.toContain("apply_patch"); + expect(DOCS_TOOLS).not.toContain("apply_patch"); expect(CORE_TOOL_NAMES).not.toContain("apply_patch"); }); - test("capability include-filter keeps proxies for build-shaped allowlists", () => { + test("capability include-filter no longer keeps Codex proxy names", () => { const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), @@ -141,46 +139,31 @@ describe("Codex tool proxy mount", () => { const allow = new Set(BUILD_TOOLS); const kept = proxies.filter((t) => allow.has(t.definition.name)); - expect(kept.map((t) => t.definition.name)).toEqual([ - "apply_patch", - "shell", - "update_plan", - ]); + expect(kept).toEqual([]); 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", - ]); + expect(docsKept).toEqual([]); }); - 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 }; + test("runSubAgent-shaped allowlists do not keep Codex proxy names", () => { + const docsAllow = new Set(DOCS_TOOLS); const proxies = createCodexToolProxies({ isCodex: true, runTool: async () => ({ content: "ok" }), readRawFile: async () => ({ content: "ok" }), runManageTasks: async () => ({ content: "ok" }), - allowDelete: allowDeleteFromCapabilities(docsCapabilities), - allowShell: allowShellFromCapabilities(docsCapabilities), + allowDelete: allowDeleteFromCapabilities({ + mode: "allow", + tools: DOCS_TOOLS, + }), + allowShell: allowShellFromCapabilities({ + mode: "allow", + tools: DOCS_TOOLS, + }), }); - 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", - ]); + expect(docsKept).toEqual([]); }); test("non-Codex runSubAgent-shaped mount produces no proxies at all", () => { diff --git a/src/agent/directors/bruckheimer/package.test.ts b/src/agent/directors/bruckheimer/package.test.ts index bed10faed..363088913 100644 --- a/src/agent/directors/bruckheimer/package.test.ts +++ b/src/agent/directors/bruckheimer/package.test.ts @@ -42,10 +42,10 @@ describe("bruckheimerPackage", () => { test("systemPrompt uses Corbits file tools (not Read/Write/Bash)", () => { const p = bruckheimerPackage.systemPrompt; - expect(p).toMatch(/read_file/); - expect(p).toMatch(/write_file/); - expect(p).toMatch(/edit_file/); - expect(p).toMatch(/search_files/); + expect(p).toMatch(/`read`/); + expect(p).toMatch(/`write`/); + expect(p).toMatch(/`edit`/); + expect(p).toMatch(/`glob`/); expect(p).not.toMatch(/\bAskUserQuestion\b/); expect(p).not.toMatch(/Use Read and Write/); expect(p).not.toMatch(/Use Bash sparingly/); diff --git a/src/agent/directors/bruckheimer/package.ts b/src/agent/directors/bruckheimer/package.ts index 9312b84f5..36e58d189 100644 --- a/src/agent/directors/bruckheimer/package.ts +++ b/src/agent/directors/bruckheimer/package.ts @@ -83,7 +83,7 @@ You never tell someone their idea is bad. You tell them what about the idea, as When the conversation has nailed the three things — audience, hook, win — plus enough scope and constraint to make it buildable, offer to write it up. Do not ambush the person with a doc. Say you think you have enough and ask if they are ready to see it on paper. -Write the brief as a markdown file. If a \`briefs/\` folder exists in the working directory, put it there. Otherwise put it in the working directory with a filename derived from the one-liner. Use \`search_files\` / \`read_file\` to locate an existing \`briefs/\` tree; use \`write_file\` / \`edit_file\` to create or revise the brief (create under \`briefs/\` by writing the path directly — you do not need shell mkdir). +Write the brief as a markdown file. If a \`briefs/\` folder exists in the working directory, put it there. Otherwise put it in the working directory with a filename derived from the one-liner. Use \`glob\` / \`read\` to locate an existing \`briefs/\` tree; use \`write\` / \`edit\` to create or revise the brief (create under \`briefs/\` by writing the path directly — you do not need shell mkdir). The brief contains: @@ -125,7 +125,7 @@ Never use \`ask_director\` as a naked question with no setup. The parent should Reserve open-ended prose questions for moments when the answer space is genuinely wide — early riffing, surfacing the original dream, asking the person to walk you through a scene. The moment you can see two to four real shapes the answer might take, switch to \`ask_director\`. -Use \`read_file\`, \`write_file\`, and \`edit_file\` to manage the brief. Use \`search_files\` to find or confirm a \`briefs/\` folder. Do not use shell for brief I/O. +Use \`read\`, \`write\`, and \`edit\` to manage the brief. Use \`glob\` to find or confirm a \`briefs/\` folder. Do not use shell for brief I/O. # Report (when dispatched as a worker) diff --git a/src/agent/directors/gauntlet/package.ts b/src/agent/directors/gauntlet/package.ts index f5907cb69..d4f6c5e7f 100644 --- a/src/agent/directors/gauntlet/package.ts +++ b/src/agent/directors/gauntlet/package.ts @@ -33,13 +33,13 @@ BLINDERS ON: check what the brief's success_criteria name, nothing else. One nam 1. Read the named test and the code it covers. Pick ONE minimal breaking mutation (flip a condition, drop a branch, off-by-one) that the test should catch. -2. Apply the mutation with edit_file. Record the exact file, symbol, and +2. Apply the mutation with edit. Record the exact file, symbol, and mutation so the restore is exact. -3. Run the named test with run_shell (foreground, with a timeout — never +3. Run the named test with bash (foreground, with a timeout — never background). It must FAIL. A pass under mutation means the test is vacuous: stop, restore immediately, and report the vacuous test as the finding. -4. Restore the mutation exactly (edit_file back, or git checkout the file +4. Restore the mutation exactly (edit back, or git checkout the file when the mutation is the only change). Verify with git status / git diff: the tree must be byte-identical to before the run. 5. Re-run the named test. It must PASS on the clean tree. @@ -50,7 +50,7 @@ BLINDERS ON: check what the brief's success_criteria name, nothing else. One nam # Rules -- run_shell is for the named suite command only, foreground with timeouts. +- bash is for the named suite command only, foreground with timeouts. - Never leave a breaking edit in the tree, not even briefly past the run. - Findings are verdicts (vacuous or guarded), never fixes — route follow-ups to builder (product fix) or testsmith (stronger cases). diff --git a/src/agent/directors/greybeard/package.test.ts b/src/agent/directors/greybeard/package.test.ts index fb799dd81..902655720 100644 --- a/src/agent/directors/greybeard/package.test.ts +++ b/src/agent/directors/greybeard/package.test.ts @@ -13,7 +13,7 @@ describe("greybeardPackage", () => { test("systemPrompt frames value as analysis via Corbits read tools", () => { const p = greybeardPackage.systemPrompt; expect(p).toMatch(/value is analysis/i); - expect(p).toContain("read_file"); + expect(p).toContain("targeted reads (read, grep)"); expect(p).toContain("grep"); expect(p).toContain("ask_director"); }); diff --git a/src/agent/directors/greybeard/package.ts b/src/agent/directors/greybeard/package.ts index c3fa43aa3..162c6987d 100644 --- a/src/agent/directors/greybeard/package.ts +++ b/src/agent/directors/greybeard/package.ts @@ -6,7 +6,7 @@ import { REVIEW_TOOLS } from "../tool-sets.js"; * Review checklist ported from the GaaS greybeard original (CL-7662) — the * GaaS source was unavailable locally, so this is a Corbits-idiom restoration * rather than a 1:1 copy. Self-read deviation: the GaaS delegate-for-review - * shape becomes read_file/grep/ask_director first, concluding with a verdict + * shape becomes read/grep/ask_director first, concluding with a verdict * rather than a spawn. Architecture judgment as a leaf — never ships product code. */ export const greybeardPackage: DirectorPackage = { @@ -32,7 +32,7 @@ You are Greybeard — not a second Skywalker, not Critic (code defects with evid Follow style and philosophy conventions (attached above) when reviewing plans or approaches — skills are active constraints, not background docs. Your value is analysis, not delegation: reach the judgment yourself with -targeted reads (read_file, grep) and pointed questions (ask_director) +targeted reads (read, grep) and pointed questions (ask_director) before concluding. When reviewing plans and approaches, verify against the loaded skills and AGENTS.md: diff --git a/src/agent/directors/migrator/package.ts b/src/agent/directors/migrator/package.ts index 4990297e9..b58d4fc4f 100644 --- a/src/agent/directors/migrator/package.ts +++ b/src/agent/directors/migrator/package.ts @@ -26,5 +26,5 @@ export const migratorPackage: DirectorPackage = { modelRole: "plan", systemPrompt: `PRIMARY INTENT: Ship reversible data migrations with dry-run evidence and a tested rollback path. -You are MigratorDirector (Migrator), the reversible-migration leaf. You own settings-schema, config-key, run.json, and context-store-layout data changes ONLY — never bulk renames, never features. Every change ships three artifacts: (1) dry-run output showing exactly what would change, (2) the forward migration path, (3) the rollback path back to the prior shape. State what happens to in-flight sessions on both paths. Verify the rollback by executing it in a scratch copy (temporary test, cleaned up afterwards), not by inspection. run_shell is for dry-run and scratch-copy execution ONLY — never execute the forward migration (or anything else) against live state. Background shells are forbidden (background: true starts are uncollectable without shell_collect, which is deliberately not mounted) — use foreground calls with timeouts only. Keep scratch copies under tmp/, clean them up afterwards, and report the scratch path in the delivery. If a change cannot be rolled back, say so plainly and stop — do not ship it. Report: dry-run output, forward path, rollback path, in-flight impact.`, +You are MigratorDirector (Migrator), the reversible-migration leaf. You own settings-schema, config-key, run.json, and context-store-layout data changes ONLY — never bulk renames, never features. Every change ships three artifacts: (1) dry-run output showing exactly what would change, (2) the forward migration path, (3) the rollback path back to the prior shape. State what happens to in-flight sessions on both paths. Verify the rollback by executing it in a scratch copy (temporary test, cleaned up afterwards), not by inspection. bash is for dry-run and scratch-copy execution ONLY — never execute the forward migration (or anything else) against live state. Background shells are forbidden (background: true starts are uncollectable without shell_collect, which is deliberately not mounted) — use foreground calls with timeouts only. Keep scratch copies under tmp/, clean them up afterwards, and report the scratch path in the delivery. If a change cannot be rolled back, say so plainly and stop — do not ship it. Report: dry-run output, forward path, rollback path, in-flight impact.`, }; diff --git a/src/agent/directors/neckbeard/package.ts b/src/agent/directors/neckbeard/package.ts index 2b5eb3a47..44858c210 100644 --- a/src/agent/directors/neckbeard/package.ts +++ b/src/agent/directors/neckbeard/package.ts @@ -42,8 +42,8 @@ Your purpose is to review documentation (\`docs/PRODUCT.md\`, \`docs/ARCHITECTUR You review by reading and searching. Prefer: -- \`read_file\` -- \`search_files\` / \`list_dir\` +- \`read\` +- \`glob\` / \`list_dir\` - \`grep\` - \`lsp\` when symbol context helps the nit diff --git a/src/agent/directors/registry.test.ts b/src/agent/directors/registry.test.ts index 251567309..fede4ba92 100644 --- a/src/agent/directors/registry.test.ts +++ b/src/agent/directors/registry.test.ts @@ -192,15 +192,13 @@ describe("director registry", () => { } }); - test("builder mounts product writes + apply_patch; intern mounts writes without apply_patch; other leaves do not spawn", () => { + test("builder mounts product writes; intern mounts writes without apply_patch; other leaves do not spawn", () => { expect(DIRECTOR_REGISTRY.builder.tools?.allow).toEqual( - expect.arrayContaining([ - "write_file", - "edit_file", - "delete_file", - "apply_patch", - ]), + expect.arrayContaining(["write_file", "edit_file", "delete_file"]), ); + expect( + DIRECTOR_REGISTRY.builder.tools?.allow as readonly string[], + ).not.toContain("apply_patch"); const internAllow = DIRECTOR_REGISTRY.intern.tools?.allow ?? []; expect(internAllow).toContain("run_shell"); expect(internAllow).toContain("write_file"); @@ -215,7 +213,7 @@ describe("director registry", () => { test("skywalker primary stance: DIY tiny writes, spawn for substantial work", () => { const s = DIRECTOR_REGISTRY.skywalker; - expect(s.systemPrompt).toContain("write_file/edit_file/delete_file"); + expect(s.systemPrompt).toContain("write/edit/delete"); expect(s.systemPrompt).toContain("DIY tiny/single-file/one-route"); expect(s.systemPrompt).toContain("You are Skywalker"); expect(s.systemPrompt).toMatch(/No catch-all worker/i); diff --git a/src/agent/directors/skywalker/package.test.ts b/src/agent/directors/skywalker/package.test.ts index e2d9f4543..0513e3857 100644 --- a/src/agent/directors/skywalker/package.test.ts +++ b/src/agent/directors/skywalker/package.test.ts @@ -7,9 +7,7 @@ describe("skywalkerPackage", () => { expect(skywalkerPackage.systemPrompt).toContain( "When asked your name, answer: Skywalker", ); - expect(skywalkerPackage.systemPrompt).toContain( - "write_file/edit_file/delete_file", - ); + expect(skywalkerPackage.systemPrompt).toContain("write/edit/delete"); expect(skywalkerPackage.systemPrompt).toContain( "DIY tiny/single-file/one-route", ); @@ -160,7 +158,7 @@ describe("skywalkerPackage", () => { test("systemPrompt simple path skips explorer+critic for tiny work", () => { const p = skywalkerPackage.systemPrompt; expect(p).toContain("Skip spawn, skip explorer, skip plan, skip critic"); - expect(p).toContain("write_file/edit_file"); + expect(p).toContain("write/edit"); }); test("systemPrompt routes URL reads through web_fetch on primary", () => { diff --git a/src/agent/directors/skywalker/package.ts b/src/agent/directors/skywalker/package.ts index bb6aab34b..e896d20a5 100644 --- a/src/agent/directors/skywalker/package.ts +++ b/src/agent/directors/skywalker/package.ts @@ -23,7 +23,7 @@ You are the only surface that talks to the operator. Give frequent short status # Tiny DIY -Tiny/single-file/one-route product edits: write_file/edit_file/delete_file yourself — same neighborhood as Builder tiny work. Skip spawn, skip explorer, skip plan, skip critic. Path tools are the DIY surface; shell file-writes stay denied. Do not run long-blocking jobs on the parent (evals, full suites, long installs, long implementation) — dispatch intern, tester, or builder. URLs: web_fetch is already mounted; do not curl/wget. +Tiny/single-file/one-route product edits: write/edit/delete yourself — same neighborhood as Builder tiny work. Skip spawn, skip explorer, skip plan, skip critic. Path tools are the DIY surface; shell file-writes stay denied. Do not run long-blocking jobs on the parent (evals, full suites, long installs, long implementation) — dispatch intern, tester, or builder. URLs: web_fetch is already mounted; do not curl/wget. # Spawn diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index 1f3eeb4fc..328b5daf3 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -35,7 +35,7 @@ describe("DOCS_TOOLS", () => { expect(DOCS_TOOLS).toContain("delete_file"); }); - test("keeps read/search/lsp/web + file writes + apply_patch", () => { + test("keeps read/search/lsp/web + file writes", () => { const expected: readonly string[] = [ "read_file", "grep", @@ -47,7 +47,6 @@ describe("DOCS_TOOLS", () => { "write_file", "edit_file", "delete_file", - "apply_patch", ]; for (const tool of expected) { expect(DOCS_TOOLS as readonly string[]).toContain(tool); @@ -60,9 +59,10 @@ describe("DOCS_TOOLS", () => { } }); - test("excludes the shell proxy (no run_shell) but keeps update_plan", () => { - expect(DOCS_TOOLS).not.toContain("shell"); - expect(DOCS_TOOLS).toContain("update_plan"); + test("omits Codex native names", () => { + expect(DOCS_TOOLS as readonly string[]).not.toContain("shell"); + expect(DOCS_TOOLS as readonly string[]).not.toContain("update_plan"); + expect(DOCS_TOOLS as readonly string[]).not.toContain("apply_patch"); }); }); @@ -151,19 +151,16 @@ describe("REVIEW_TOOLS / INTERN_TOOLS", () => { }); describe("BUILD_TOOLS", () => { - test("includes apply_patch alongside path mutation tools", () => { + test("includes path mutation tools and omits Codex natives", () => { expect(BUILD_TOOLS).toContain("write_file"); expect(BUILD_TOOLS).toContain("edit_file"); expect(BUILD_TOOLS).toContain("delete_file"); - expect(BUILD_TOOLS).toContain("apply_patch"); + expect(BUILD_TOOLS as readonly string[]).not.toContain("apply_patch"); + expect(BUILD_TOOLS as readonly string[]).not.toContain("shell"); + expect(BUILD_TOOLS as readonly string[]).not.toContain("update_plan"); }); - test("includes the Codex shell and update_plan proxy names", () => { - expect(BUILD_TOOLS).toContain("shell"); - expect(BUILD_TOOLS).toContain("update_plan"); - }); - - test("review/orchestrator/intern do not mount apply_patch", () => { + test("review/orchestrator/intern do not list apply_patch", () => { for (const surface of [REVIEW_TOOLS, ORCHESTRATOR_TOOLS, INTERN_TOOLS]) { expect(surface as readonly string[]).not.toContain("apply_patch"); } diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index aaf224f6f..233fa9225 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -23,9 +23,10 @@ export const READ_TOOLS = [ ] as const; /** - * Path mutation tools shared by closed directors. Codex `apply_patch` stays on - * build/docs only — review/explore/orchestrator/intern mount these path tools - * alone (lane discipline lives in prompts, not the capability filter). + * Path mutation tools shared by closed directors. Review/explore/orchestrator + * /intern mount these path tools (lane discipline lives in prompts, not the + * capability filter). delete is advertised on write surfaces, omitted from + * READ_TOOLS only. */ export const PRODUCT_WRITE_TOOLS = [ "write_file", @@ -34,18 +35,11 @@ export const PRODUCT_WRITE_TOOLS = [ ] as const; /** - * 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 workers keep the proxies after the - * capability filter, same rationale as `apply_patch` below. + * Build: read + full file mutation. Codex natives are not advertised and are + * not mounted as extra AgentTools — hidden aliases dispatch onto run_shell / + * manage_tasks when those engines are mounted. */ -export const BUILD_TOOLS = [ - ...READ_TOOLS, - ...PRODUCT_WRITE_TOOLS, - "apply_patch", - "shell", - "update_plan", -] as const; +export const BUILD_TOOLS = [...READ_TOOLS, ...PRODUCT_WRITE_TOOLS] as const; /** * Docs workers: read/search/lsp/web + file writes — no run_shell. @@ -53,16 +47,11 @@ export const BUILD_TOOLS = [ * terminal. There is no separate path-level lock on top of the tool envelope. * * Composed from READ_TOOLS minus run_shell so it tracks the read surface - * automatically; path writes come from PRODUCT_WRITE_TOOLS. `apply_patch` is - * included so Codex docs workers 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`). + * automatically; path writes come from PRODUCT_WRITE_TOOLS. */ export const DOCS_TOOLS = [ ...READ_TOOLS.filter((t) => t !== "run_shell" && t !== "shell_collect"), ...PRODUCT_WRITE_TOOLS, - "apply_patch", - "update_plan", ] as const; /** Review / counsel: read surface + path writes (skill tools arrive via READ_TOOLS; lane discipline in prompts). */ diff --git a/src/agent/product-mutation-tools.ts b/src/agent/product-mutation-tools.ts index fe56a54c7..5891c703e 100644 --- a/src/agent/product-mutation-tools.ts +++ b/src/agent/product-mutation-tools.ts @@ -11,6 +11,7 @@ import { extractAffectedPaths, parseCodexApplyPatch, } from "./codex-apply-patch.js"; +import { canonicalToolName } from "./canonical-tool-name.js"; export const PRODUCT_MUTATION_TOOLS = [ "write_file", @@ -24,7 +25,7 @@ const PRODUCT_MUTATION_TOOL_SET: ReadonlySet = new Set( ); export function isProductMutationTool(name: string): boolean { - return PRODUCT_MUTATION_TOOL_SET.has(name); + return PRODUCT_MUTATION_TOOL_SET.has(canonicalToolName(name)); } /** @@ -33,13 +34,14 @@ export function isProductMutationTool(name: string): boolean { * 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 engine = canonicalToolName(name); + if (!isProductMutationTool(engine)) return []; const record = args !== null && typeof args === "object" && !Array.isArray(args) ? (args as Record) : {}; - if (name === "apply_patch") { + if (engine === "apply_patch") { const input = record.input; if (typeof input !== "string" || input.length === 0) return []; try { diff --git a/src/agent/prompt-contract.ts b/src/agent/prompt-contract.ts index 2454475fa..36d35f364 100644 --- a/src/agent/prompt-contract.ts +++ b/src/agent/prompt-contract.ts @@ -16,7 +16,7 @@ export const CHAT_PROMPT_QUALITY_MARKERS = [ "skill_search when choosing", "use_skill style and philosophy when starting repo work", "advertised catalog (including skill_search) are resident", - "grep or search_files", + "grep or glob", "never shell-write (echo/heredoc/sed/rm)", "ask_director", "send_input", diff --git a/src/agent/prompt-sizes.ts b/src/agent/prompt-sizes.ts index e81b3b249..1c11af1a5 100644 --- a/src/agent/prompt-sizes.ts +++ b/src/agent/prompt-sizes.ts @@ -18,13 +18,9 @@ import { MAX_AGENTS_MD_BYTES, } from "./context-extensions.js"; import { shouldApplyGrokAntiThrash } from "../subagent/provider-family.js"; -import { isCodexProviderName } from "../config/codex-providers.js"; import { shellCollectDefinition } from "./background-shell-tool.js"; -import { - applyPatchDefinition, - shellDefinition, - updatePlanDefinition, -} from "./codex-tool-proxies.js"; +import { advertisedToolName } from "./tool-aliases.js"; +import { canonicalToolName } from "./canonical-tool-name.js"; import { manageTasksDefinition } from "./tasks.js"; import { DELETE_FILE_DEFINITION } from "../plugins/delete-file-plugin.js"; import { webFetchDefinition } from "../tools/web-fetch.js"; @@ -90,11 +86,9 @@ export const CANONICAL_AGENTS_MD = "Follow the repository conventions.\n"; * Pre-filter mount names in run.ts install order: posix base (TOOL_NAMES, * shared with createPosixTools) + delete_file / lsp plugin tools * (buildCorePosixToolPlugins) + core web tools (coreSubAgentWebTools) + - * shell_collect (run.ts:678-694). Codex proxies (apply_patch, shell, - * update_plan) join only when isCodex — createCodexToolProxies returns [] - * otherwise (run.ts:708-718, codex-tool-proxies.ts:163-166). + * shell_collect (run.ts). Codex natives are not mounted. */ -function preFilterMountNames(isCodex: boolean): readonly string[] { +function preFilterMountNames(): readonly string[] { return [ ...Object.values(TOOL_NAMES), DELETE_FILE_DEFINITION.name, @@ -102,13 +96,6 @@ function preFilterMountNames(isCodex: boolean): readonly string[] { webFetchDefinition.name, webSearchDefinition.name, shellCollectDefinition.name, - ...(isCodex - ? [ - applyPatchDefinition.name, - shellDefinition.name, - updatePlanDefinition.name, - ] - : []), ]; } @@ -125,26 +112,27 @@ function preFilterMountNames(isCodex: boolean): readonly string[] { */ export function canonicalToolNamesForDirector( pkg: DirectorPackage, - family: PromptSizeFamily, + _family: PromptSizeFamily, ): readonly string[] { - const providerName = - family === "grok" - ? GROK_PROVIDER.providerName - : family === "muse" - ? MUSE_PROVIDER.providerName - : family === "claude" - ? CLAUDE_PROVIDER.providerName - : family === "gpt" - ? GPT_PROVIDER.providerName - : DEFAULT_PROVIDER.providerName; - const filtered = [...preFilterMountNames(isCodexProviderName(providerName))]; + const filtered = [...preFilterMountNames()]; const capabilities = packageToCapabilities(pkg); const names = capabilities === undefined ? filtered : capabilities.mode === "allow" - ? filtered.filter((name) => capabilities.tools.includes(name)) - : filtered.filter((name) => !capabilities.tools.includes(name)); + ? filtered.filter((name) => + capabilities.tools.some( + (allowed) => + canonicalToolName(allowed) === canonicalToolName(name), + ), + ) + : filtered.filter( + (name) => + !capabilities.tools.some( + (denied) => + canonicalToolName(denied) === canonicalToolName(name), + ), + ); names.push(manageTasksDefinition.name); if (pkg.tier === "leaf") { names.push("submit_result", "ask_director"); @@ -168,7 +156,7 @@ export function canonicalToolNamesForDirector( `canonicalToolNamesForDirector(${pkg.id}): "${dupe}" mounted twice — the assembly drifted from src/subagent/run.ts`, ); } - return names; + return names.map(advertisedToolName); } /** Assemble one director prompt exactly as run.ts does. */ diff --git a/src/agent/prompts.test.ts b/src/agent/prompts.test.ts index c8c6cb6d7..a1f3d551d 100644 --- a/src/agent/prompts.test.ts +++ b/src/agent/prompts.test.ts @@ -20,10 +20,10 @@ const REGISTERED_TOOL_NAMES = new Set([ ]); const REFERENCED_TOOL_NAMES = [ - "read_file", - "edit_file", - "write_file", - "run_shell", + "read", + "edit", + "write", + "bash", "web_fetch", "web_search", ]; @@ -72,7 +72,7 @@ describe("buildPromptDisciplineBlock", () => { it("contains the load-bearing prohibitions", () => { const block = PROMPT_DISCIPLINE_BLOCK; // Dedicated tools over shell. - expect(block).toContain("run_shell"); + expect(block).toContain("bash"); expect(block).toContain("cat/head/tail"); expect(block).toContain("heredoc/echo"); // Environment. @@ -199,9 +199,9 @@ Response style: Tool choice: - Prefer spawn_agent(agent=…) then idle for substantial product implementation, exploration, review, and docs — mailbox mail arrives as inbound; do not poll. Spawn remains default for substantial work, not a tool ban. -- read_file for file contents; grep or search_files to locate code; lsp for symbols, types, references, or call flow before opening large files. -- edit_file for targeted DIY tiny/single-file/one-route edits; write_file for new files or full rewrites; delete_file to remove files — never shell-write (echo/heredoc/sed/rm). Spawn builder (or a docs director) for substantial/multi-file/parallel/specialist work. -- run_shell for builds, tests, git, and one-off commands — not for shell find, head-position rg, or recursive grep -r (OOM risk), cat, or messaging the user. +- read for file contents; grep or glob to locate code; lsp for symbols, types, references, or call flow before opening large files. +- edit for targeted DIY tiny/single-file/one-route edits; write for new files or full rewrites; delete to remove files — never shell-write (echo/heredoc/sed/rm). Spawn builder (or a docs director) for substantial/multi-file/parallel/specialist work. +- bash for builds, tests, git, and one-off commands — not for shell find, head-position rg, or recursive grep -r (OOM risk), cat, or messaging the user. - tool_search before assuming a plugin or MCP tool exists; skill_search when choosing among listed skills, use_skill to load a body. Ask vs proceed: @@ -361,7 +361,7 @@ describe("grok finish-bias residual gating (extends existing provider-family tes it("reinforces tool routing (dedicated tools over shell) for grok, not just finish bias", () => { const note = buildGrokLeafAntiThrashNote(); - expect(note).toMatch(/run_shell/); + expect(note).toMatch(/bash/); }); it("has no kimi residual — the seam is intentionally left unfilled", () => { diff --git a/src/agent/prompts.ts b/src/agent/prompts.ts index 2b5637708..085b921ab 100644 --- a/src/agent/prompts.ts +++ b/src/agent/prompts.ts @@ -29,11 +29,12 @@ import { SETTINGS_DIR_NAME } from "../branding.js"; // Fallback tool list for worker prompts when the caller does not pass the // installed set. Matches the worker install (posix + manage_tasks + ask_director). const defaultChatTools = [ - "read_file", - "write_file", - "edit_file", - "run_shell", - "search_files", + "read", + "write", + "edit", + "delete", + "bash", + "glob", "grep", "list_dir", "lsp", @@ -81,16 +82,16 @@ export function buildHarnessFacts( "Harness facts:", ...(subAgent ? [ - "- Change files with write_file/edit_file and remove files with delete_file; shell file-writes and deletions are blocked.", + "- Change files with write/edit and remove files with delete; shell file-writes and deletions are blocked.", ] : [ - "- Change files with write_file/edit_file and remove files with delete_file for tiny/single-file/one-route bounded edits. Spawn builder for substantial/multi-file/parallel/specialist work. Docs/design still spawn shakespeare/bruckheimer/rand except one-line fixes.", + "- Change files with write/edit and remove files with delete for tiny/single-file/one-route bounded edits. Spawn builder for substantial/multi-file/parallel/specialist work. Docs/design still spawn shakespeare/bruckheimer/rand except one-line fixes.", "- Shell file-writes and deletions are blocked; never use echo/heredoc/sed/rm as a substitute for product tools. Path tools are the DIY surface.", ]), "- Use the provided tools for file reads/searches instead of shelling out as a substitute.", - "- read_file accepts a filesystem path or a tool-output:///{callId} URI from a prior tool result when the harness exposes one. Only read_file a tool-output:// URI if the truncation notice on that result named one; do not re-read a complete inline result.", - "- run_shell defaults to a 120s foreground timeout; pass timeout to override with no ceiling. Prefer background:true for builds, test suites, and dev servers: it returns a shell_id at once, the result is delivered when the process finishes (foreground runs hold steers; background runs do not), and shell_collect collects or cancels later. background does not change the retained shell cwd and has no default timeout.", - "- Shell find, rg, and grep -r are blocked — they can walk huge trees and OOM the host. Prefer the bounded grep/search_files tools, and do not substitute another unbounded walk (fd, ls -R, scripted os.walk).", + "- read accepts a filesystem path or a tool-output:///{callId} URI from a prior tool result when the harness exposes one. Only read a tool-output:// URI if the truncation notice on that result named one; do not re-read a complete inline result.", + "- bash defaults to a 120s foreground timeout; pass timeout to override with no ceiling. Prefer background:true for builds, test suites, and dev servers: it returns a shell_id at once, the result is delivered when the process finishes (foreground runs hold steers; background runs do not), and shell_collect collects or cancels later. background does not change the retained shell cwd and has no default timeout.", + "- Shell find, rg, and grep -r are blocked — they can walk huge trees and OOM the host. Prefer the bounded grep/glob tools, and do not substitute another unbounded walk (fd, ls -R, scripted os.walk).", ...(subAgent ? [ "- You share the parent session's permission gate: matching persisted grants and auto mode proceed without a new prompt; other consequential actions may require operator approval (interactive) or are denied (headless).", @@ -162,11 +163,11 @@ const GUIDELINE_SUB_BLOCKS: Record< : "mailbox mail arrives as inbound; do not poll.") + " Spawn remains default for substantial work, not a tool ban.", ]), - "- read_file for file contents; grep or search_files to locate code; lsp for symbols, types, references, or call flow before opening large files.", + "- read for file contents; grep or glob to locate code; lsp for symbols, types, references, or call flow before opening large files.", ctx.subAgent - ? "- edit_file for targeted changes; write_file for new files or full rewrites; delete_file to remove files — never echo, heredoc, sed, or rm in the shell for those jobs." - : "- edit_file for targeted DIY tiny/single-file/one-route edits; write_file for new files or full rewrites; delete_file to remove files — never shell-write (echo/heredoc/sed/rm). Spawn builder (or a docs director) for substantial/multi-file/parallel/specialist work.", - "- run_shell for builds, tests, git, and one-off commands — not for shell find, head-position rg, or recursive grep -r (OOM risk), cat, or messaging the user.", + ? "- edit for targeted changes; write for new files or full rewrites; delete to remove files — never echo, heredoc, sed, or rm in the shell for those jobs." + : "- edit for targeted DIY tiny/single-file/one-route edits; write for new files or full rewrites; delete to remove files — never shell-write (echo/heredoc/sed/rm). Spawn builder (or a docs director) for substantial/multi-file/parallel/specialist work.", + "- bash for builds, tests, git, and one-off commands — not for shell find, head-position rg, or recursive grep -r (OOM risk), cat, or messaging the user.", ...(ctx.subAgent ? [] : [ @@ -262,8 +263,8 @@ export function buildPromptDisciplineBlock( ): string { const subAgent = opts.subAgent ?? false; const toolsOverShell = subAgent - ? "- Never use run_shell to read, edit, or write files — use read_file, edit_file, write_file; cat/head/tail, sed/awk/perl -i, and heredoc/echo redirection are prohibited substitutes." - : "- Never use run_shell to read, edit, or write files — use read_file, edit_file, write_file for tiny/bounded DIY; spawn builder/docs directors for substantial work; cat/head/tail, sed/awk/perl -i, and heredoc/echo redirection are prohibited substitutes."; + ? "- Never use bash to read, edit, or write files — use read, edit, write; cat/head/tail, sed/awk/perl -i, and heredoc/echo redirection are prohibited substitutes." + : "- Never use bash to read, edit, or write files — use read, edit, write for tiny/bounded DIY; spawn builder/docs directors for substantial work; cat/head/tail, sed/awk/perl -i, and heredoc/echo redirection are prohibited substitutes."; return [ "Prompt discipline:", "", @@ -279,7 +280,7 @@ export function buildPromptDisciplineBlock( "- Never hand-roll a web query — use web_search.", "", "Command shape:", - "- Never chain unrelated operations into one run_shell call — one logical operation per call, no multi-line scripts; a pipeline that performs one job is one operation.", + "- Never chain unrelated operations into one bash call — one logical operation per call, no multi-line scripts; a pipeline that performs one job is one operation.", "- Every command must be legible to the operator reviewing it before it runs.", "", "Turn semantics:", @@ -295,16 +296,12 @@ export function buildPromptDisciplineBlock( } const TOOL_SUMMARIES: Record = { - read_file: - "read a file or tool-output:///{callId} from a prior tool result (prefer over cat/head/tail in the shell). Only read_file a tool-output:// URI if the truncation notice named one", - write_file: "create or overwrite a file (never shell redirects or heredocs)", - edit_file: - "make a surgical edit (exact old_string match, or start_line/end_line line-range mode; never include read_file's NNNNNN\\t line prefix; substring failures include nearby file text; prefer over sed/awk in the shell)", - delete_file: "delete one file with an explicit outcome (never shell rm)", - run_shell: - "run a shell command (builds, tests, git; pass timeout ms to bound long commands; never to read/write/delete files, search trees, or talk to the user)", - search_files: - "find files by name or pattern (bounded; timeout + output caps — safer than open-ended shell find)", + read: "read a file or tool-output:///{callId} from a prior tool result (prefer over cat/head/tail in the shell). Only read a tool-output:// URI if the truncation notice named one", + write: "create or overwrite a file (never shell redirects or heredocs)", + edit: "make a surgical edit (exact old_string match, or start_line/end_line line-range mode; never include read's NNNNNN\\t line prefix; substring failures include nearby file text; prefer over sed/awk in the shell)", + delete: "delete one file with an explicit outcome (never shell rm)", + bash: "run a shell command (builds, tests, git; pass timeout ms to bound long commands; never to read/write/delete files, search trees, or talk to the user)", + glob: "find files by name or pattern (bounded; timeout + output caps — safer than open-ended shell find)", grep: "search file contents (bounded; timeout + output caps — safer than open-ended shell grep -r/rg)", list_dir: "list a directory's entries (bounded listing)", lsp: "resolve symbols — goToDefinition, findReferences, hover (prefer before reading huge files)", @@ -336,10 +333,8 @@ const TOOL_SUMMARIES: Record = { }; const ARCHIVE_TOOL_SUMMARIES: Partial> = { - read_file: - "read a file, tool-output:///{callId} from a prior tool result, or archive:///{occurrenceId} (prefer over cat/head/tail in the shell). Only read_file a tool-output:// URI if the truncation notice named one", - search_files: - "find files by name or pattern (bounded; timeout + output caps — safer than open-ended shell find); path archive:/// lists evidence-archive refs", + read: "read a file, tool-output:///{callId} from a prior tool result, or archive:///{occurrenceId} (prefer over cat/head/tail in the shell). Only read a tool-output:// URI if the truncation notice named one", + glob: "find files by name or pattern (bounded; timeout + output caps — safer than open-ended shell find); path archive:/// lists evidence-archive refs", grep: "search file contents (bounded; timeout + output caps — safer than open-ended shell grep -r/rg); path archive:/// searches this session's evidence archive", }; diff --git a/src/agent/tool-aliases.test.ts b/src/agent/tool-aliases.test.ts new file mode 100644 index 000000000..0cb1d53b4 --- /dev/null +++ b/src/agent/tool-aliases.test.ts @@ -0,0 +1,228 @@ +import { describe, expect, test } from "bun:test"; +import type { ToolDefinition } from "@intx/types/runtime"; +import { createDynamicToolRunner } from "../tui/dynamic-tool-runner.js"; +import { + advertisedTools, + CORE_TOOL_NAMES, + CATALOG_TOOL_NAMES, +} from "./tool-search.js"; +import { canonicalToolName } from "./canonical-tool-name.js"; +import { advertisedToolName, WIRE_TO_ENGINE } from "./tool-aliases.js"; +import { evaluateApprovals } from "../permission/authz-grants.js"; + +const noWorkspace = { resolvedCwd: "/repo", roots: ["/repo"] }; + +const posixDef = (name: string): ToolDefinition => ({ + name, + description: name, + inputSchema: { type: "object", properties: {}, required: [] }, +}); + +describe("one advertised posix set", () => { + test("CORE+CATALOG is the 1:1 wire set without engine or Codex names", () => { + const advertised = [...CORE_TOOL_NAMES, ...CATALOG_TOOL_NAMES]; + expect(CORE_TOOL_NAMES.slice(0, 6)).toEqual([ + "read", + "write", + "edit", + "delete", + "lsp", + "bash", + ]); + expect(CATALOG_TOOL_NAMES[0]).toBe("glob"); + expect(advertised).toContain("grep"); + expect(advertised).not.toContain("read_file"); + expect(advertised).not.toContain("write_file"); + expect(advertised).not.toContain("edit_file"); + expect(advertised).not.toContain("delete_file"); + expect(advertised).not.toContain("run_shell"); + expect(advertised).not.toContain("search_files"); + expect(advertised).not.toContain("list_dir"); + expect(advertised).not.toContain("apply_patch"); + expect(advertised).not.toContain("shell"); + expect(advertised).not.toContain("update_plan"); + expect(new Set(advertised).size).toBe(advertised.length); + }); + + test("advertisedTools projects engine defs onto one wire name each", () => { + const registry = [ + posixDef("read_file"), + posixDef("write_file"), + posixDef("edit_file"), + posixDef("delete_file"), + posixDef("run_shell"), + posixDef("search_files"), + posixDef("grep"), + posixDef("list_dir"), + posixDef("lsp"), + ]; + const names = advertisedTools(registry).map((d) => d.name); + expect(names).toContain("read"); + expect(names).toContain("write"); + expect(names).toContain("edit"); + expect(names).toContain("delete"); + expect(names).toContain("bash"); + expect(names).toContain("glob"); + expect(names).toContain("grep"); + expect(names).not.toContain("read_file"); + expect(names).not.toContain("run_shell"); + expect(names).not.toContain("search_files"); + expect(names).not.toContain("list_dir"); + expect(names).not.toContain("delete_file"); + }); + + test("delete is advertised; list_dir is not", () => { + expect(CORE_TOOL_NAMES).toContain("delete"); + expect(CATALOG_TOOL_NAMES).not.toContain("list_dir"); + expect(CORE_TOOL_NAMES).not.toContain("list_dir"); + }); +}); + +describe("grant aliases", () => { + test("a grant stored as run_shell covers bash", async () => { + expect( + await evaluateApprovals({ + tool: "bash", + subject: "npm test", + approvals: [{ tool: "run_shell", pattern: "npm *" }], + workspace: noWorkspace, + }), + ).toBe(true); + }); + + test("a grant stored as bash matches a run_shell request after canonicalize", async () => { + expect( + await evaluateApprovals({ + tool: "run_shell", + subject: "npm test", + approvals: [{ tool: "bash", pattern: "npm *" }], + workspace: noWorkspace, + }), + ).toBe(true); + }); + + test("canonicalToolName maps wire and hidden aliases onto engines", () => { + expect(canonicalToolName("bash")).toBe("run_shell"); + expect(canonicalToolName("shell")).toBe("run_shell"); + expect(canonicalToolName("read")).toBe("read_file"); + expect(canonicalToolName("update_plan")).toBe("manage_tasks"); + expect(canonicalToolName("default.bash")).toBe("run_shell"); + expect(canonicalToolName("apply_patch")).toBe("apply_patch"); + }); +}); + +describe("hidden alias dispatch", () => { + test("bash and run_shell both reach the engine without dual-registering", async () => { + let seen = ""; + const runner = createDynamicToolRunner([ + { + kind: "string", + definition: posixDef("run_shell"), + handler: async (args) => { + seen = String(args.command ?? ""); + return "ok"; + }, + }, + ]); + runner.setCallGate((name) => name === "run_shell" || name === "bash"); + const viaBash = await runner.run( + { id: "1", name: "bash", arguments: { command: "echo hi" } }, + new AbortController().signal, + ); + expect(viaBash.content).toBe("ok"); + expect(seen).toBe("echo hi"); + const viaEngine = await runner.run( + { id: "2", name: "run_shell", arguments: { command: "echo ho" } }, + new AbortController().signal, + ); + expect(viaEngine.content).toBe("ok"); + expect(seen).toBe("echo ho"); + }); + + test("shell coerces argv/workdir/timeout_ms onto run_shell", async () => { + let seen: Record = {}; + const runner = createDynamicToolRunner([ + { + kind: "string", + definition: posixDef("run_shell"), + handler: async (args) => { + seen = args; + return "ok"; + }, + }, + ]); + runner.setCallGate(() => true); + const result = await runner.run( + { + id: "1", + name: "shell", + arguments: { + command: ["bash", "-lc", "ls"], + workdir: "/tmp", + timeout_ms: 5000, + }, + }, + new AbortController().signal, + ); + expect(result.content).toBe("ok"); + expect(seen.command).toBe("ls"); + expect(seen.cwd).toBe("/tmp"); + expect(seen.timeout).toBe(5000); + }); + + test("update_plan hidden-dispatches onto manage_tasks; apply_patch does not", async () => { + const runner = createDynamicToolRunner([ + { + kind: "string", + definition: posixDef("manage_tasks"), + handler: async (args) => JSON.stringify(args), + }, + ]); + runner.setCallGate(() => true); + const plan = await runner.run( + { + id: "1", + name: "update_plan", + arguments: { + plan: [{ step: "Do the thing", status: "in_progress" }], + }, + }, + new AbortController().signal, + ); + expect(plan.isError).toBeFalsy(); + expect(plan.content).toContain('"action":"create"'); + const patch = await runner.run( + { id: "2", name: "apply_patch", arguments: { input: "x" } }, + new AbortController().signal, + ); + expect(patch.isError).toBe(true); + expect(patch.content).toContain("unknown tool"); + }); + + test("WIRE_TO_ENGINE is 1:1", () => { + const engines = Object.values(WIRE_TO_ENGINE); + expect(new Set(engines).size).toBe(engines.length); + expect(advertisedToolName("run_shell")).toBe("bash"); + }); + + test("Codex does not advertise shell+run_shell or apply_patch", () => { + const names = advertisedTools([ + posixDef("read_file"), + posixDef("write_file"), + posixDef("edit_file"), + posixDef("delete_file"), + posixDef("run_shell"), + posixDef("search_files"), + posixDef("grep"), + posixDef("manage_tasks"), + posixDef("list_dir"), + ]).map((d) => d.name); + expect( + names.filter((n) => n === "bash" || n === "run_shell" || n === "shell"), + ).toEqual(["bash"]); + expect(names).not.toContain("apply_patch"); + expect(names).not.toContain("update_plan"); + expect(names).not.toContain("list_dir"); + expect(new Set(names).size).toBe(names.length); + }); +}); diff --git a/src/agent/tool-aliases.ts b/src/agent/tool-aliases.ts new file mode 100644 index 000000000..20f4e7f27 --- /dev/null +++ b/src/agent/tool-aliases.ts @@ -0,0 +1,209 @@ +/** + * One advertised posix set (CL-8400). Registry engines stay posix-named; + * advertise is a projection onto wire names. Incoming aliases resolve onto + * the same engine id for dispatch and grants. + * + * Wire: read write edit delete bash grep glob + * Engine: read_file write_file edit_file delete_file run_shell grep search_files + * Hidden dispatch: shell → run_shell (Codex argv/workdir/timeout_ms coerce), + * update_plan → manage_tasks. apply_patch is neither advertised nor dispatched. + */ + +import { type } from "arktype"; +import type { ToolCall, ToolDefinition } from "@intx/types/runtime"; + +/** Advertised posix names → registry engine ids. 1:1, never dual-publish. */ +export const WIRE_TO_ENGINE = { + read: "read_file", + write: "write_file", + edit: "edit_file", + delete: "delete_file", + bash: "run_shell", + glob: "search_files", +} as const; + +/** Hidden incoming names that dispatch onto a mounted engine (not advertised). */ +export const HIDDEN_TO_ENGINE = { + shell: "run_shell", + update_plan: "manage_tasks", +} as const; + +const ENGINE_TO_WIRE: Record = Object.fromEntries( + Object.entries(WIRE_TO_ENGINE).map(([wire, engine]) => [engine, wire]), +); + +const ALIAS_TO_ENGINE: Record = { + ...WIRE_TO_ENGINE, + ...HIDDEN_TO_ENGINE, +}; + +/** Map an incoming alias (wire or hidden) onto the registry engine id. */ +export function engineToolName(requested: string): string { + return ( + ALIAS_TO_ENGINE[requested] ?? + ALIAS_TO_ENGINE[requested.toLowerCase()] ?? + requested + ); +} + +/** Project a registry engine id onto the advertised wire name. */ +export function advertisedToolName(engine: string): string { + return ENGINE_TO_WIRE[engine] ?? engine; +} + +/** + * True when `name` (wire, engine, or hidden alias) is covered by an advertised + * or activated listing that may itself be stored as either wire or engine ids. + */ +export function nameMatchesAdvertisedListing( + name: string, + isListed: (candidate: string) => boolean, +): boolean { + if (isListed(name)) return true; + const engine = engineToolName(name); + if (engine !== name && isListed(engine)) return true; + const wire = advertisedToolName(engine); + return wire !== name && isListed(wire); +} + +export function projectToolDefinition(def: ToolDefinition): ToolDefinition { + const wire = advertisedToolName(def.name); + return wire === def.name ? def : { ...def, name: wire }; +} + +export function projectToolDefinitions( + defs: readonly ToolDefinition[], +): ToolDefinition[] { + return defs.map(projectToolDefinition); +} + +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 `shell` sends `command` as a string or argv array. `run_shell` takes a + * single shell string. Unwrap `[shell, "-lc"|"-c", script]` to the script. + */ +export function normalizeShellCommand(command: string | string[]): string { + if (typeof command === "string") return command; + const wrapper = command[0]; + const flag = command[1]; + const script = command[2]; + if ( + command.length === 3 && + wrapper !== undefined && + script !== undefined && + SHELL_WRAPPERS.has(wrapper.replace(/^.*\//, "")) && + (flag === "-lc" || flag === "-c") + ) { + return script; + } + return command.map(shellQuote).join(" "); +} + +const CodexShellArgs = type({ + command: "string | string[]", + "workdir?": "string", + "timeout_ms?": "number", +}); + +export function looksLikeCodexShellArgs( + args: Record, +): boolean { + return ( + Array.isArray(args.command) || + args.workdir !== undefined || + args.timeout_ms !== undefined + ); +} + +export function coerceShellArgs( + args: Record, +): Record { + const parsed = CodexShellArgs(args); + if (parsed instanceof type.errors) { + throw new Error("Error: shell requires a command (string or string[])."); + } + const coerced: Record = { + command: normalizeShellCommand(parsed.command), + }; + if (parsed.workdir !== undefined) coerced.cwd = parsed.workdir; + if (parsed.timeout_ms !== undefined) coerced.timeout = parsed.timeout_ms; + return coerced; +} + +const CodexPlanStatus = type("'pending' | 'in_progress' | 'completed'"); +const UpdatePlanArgs = type({ + "explanation?": "string", + plan: type({ + step: "string>0", + status: CodexPlanStatus, + }).array(), +}); + +function codexPlanStatusToTaskStatus( + status: typeof CodexPlanStatus.infer, +): "todo" | "doing" | "done" { + if (status === "pending") return "todo"; + if (status === "in_progress") return "doing"; + return "done"; +} + +export function translateUpdatePlanArgs( + args: Record, +): Record { + const parsed = UpdatePlanArgs(args); + if (parsed instanceof type.errors) { + throw new Error( + "Error: update_plan requires a plan array of { step, status }.", + ); + } + return { + action: "create", + tasks: parsed.plan.map((item, i) => ({ + id: `p${i + 1}`, + title: item.step, + status: codexPlanStatusToTaskStatus(item.status), + })), + }; +} + +function incomingAlias(requested: string): string { + let name = requested; + if (name.startsWith("default.")) { + const stripped = name.slice("default.".length); + if (stripped.length > 0) name = stripped; + } + return name; +} + +/** + * Coerce hidden Codex-shaped arguments onto the engine tool, and rewrite the + * dispatched name to the engine id. Callers pass the already-resolved engine. + */ +export function prepareDispatchedToolCall( + call: ToolCall, + engine: string, +): ToolCall { + const incoming = incomingAlias(call.name); + let args = call.arguments; + if ( + incoming === "shell" || + incoming.toLowerCase() === "shell" || + (engine === "run_shell" && looksLikeCodexShellArgs(args)) + ) { + args = coerceShellArgs(args); + } + if ( + (incoming === "update_plan" || incoming.toLowerCase() === "update_plan") && + engine === "manage_tasks" + ) { + args = translateUpdatePlanArgs(args); + } + if (args === call.arguments && engine === call.name) return call; + return { ...call, name: engine, arguments: args }; +} diff --git a/src/agent/tool-search.test.ts b/src/agent/tool-search.test.ts index e07938604..82db5c2b2 100644 --- a/src/agent/tool-search.test.ts +++ b/src/agent/tool-search.test.ts @@ -116,8 +116,9 @@ describe("createToolIndex", () => { }); test("never returns a core tool (those are always loaded)", () => { - expect(CORE_TOOL_NAMES).toContain("read_file"); + expect(CORE_TOOL_NAMES).toContain("read"); expect(index.search("read a file")).not.toContain("read_file"); + expect(index.search("read a file")).not.toContain("read"); }); test("orchestrator mode advertises split fleet tools and search_agents", () => { @@ -188,7 +189,7 @@ describe("createToolIndex", () => { }); test("primary CORE includes product mutation tools; CATALOG does not duplicate them", () => { - for (const name of ["write_file", "edit_file", "delete_file"] as const) { + for (const name of ["write", "edit", "delete"] as const) { expect(CORE_TOOL_NAMES).toContain(name); expect(CATALOG_TOOL_NAMES).not.toContain(name); } @@ -623,16 +624,16 @@ describe("advertisedTools", () => { // advertisedTools only emits tools present in the registry; multi-agent // tools appear on the wire when createAgentToolset registers them. const names = advertisedTools(registry, [], prefix).map((d) => d.name); - expect(names).toContain("read_file"); + expect(names).toContain("read"); expect(names).not.toContain("mcp__linear__create_issue"); }); test("with no activation, advertises only the fixed built-in set, never MCP tools", () => { const names = advertisedTools(registry).map((d) => d.name); - expect(names).toContain("read_file"); + expect(names).toContain("read"); expect(names).toContain("grep"); - // write_file is in CORE so the primary can DIY tiny/bounded edits. - expect(names).toContain("write_file"); + // write is in CORE so the primary can DIY tiny/bounded edits. + expect(names).toContain("write"); expect(names).not.toContain("mcp__linear__create_issue"); }); diff --git a/src/agent/tool-search.ts b/src/agent/tool-search.ts index 2ed8ed3bf..2125ffd54 100644 --- a/src/agent/tool-search.ts +++ b/src/agent/tool-search.ts @@ -11,6 +11,8 @@ import { } from "./lexical-rank.js"; import type { SessionMode } from "../config/session-mode.js"; import { sessionModeEnablesSubAgents } from "../config/session-mode.js"; +import { advertisedToolName, projectToolDefinition } from "./tool-aliases.js"; +import { canonicalToolName } from "./canonical-tool-name.js"; // Tools whose full schema is always advertised to the model. Everything else is // registered but discovered on demand via tool_search, which promotes matches @@ -22,19 +24,18 @@ import { sessionModeEnablesSubAgents } from "../config/session-mode.js"; // advertised prefix — the model finds it via tool_search when a session // actually needs it. // -// Product mutation tools (write_file / edit_file / delete_file) sit in CORE so +// Product mutation tools (write / edit / delete) sit in CORE so // 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. +// Codex natives (apply_patch / shell / update_plan) are not advertised. export const CORE_TOOL_NAMES: readonly string[] = [ - "read_file", - "write_file", - "edit_file", - "delete_file", + "read", + "write", + "edit", + "delete", "lsp", - "run_shell", + "bash", "shell_collect", "ask_operator", "manage_tasks", @@ -117,15 +118,15 @@ export function advertisedToolNamesForSessionMode( // Built-in file/search/web tools advertised alongside the core set. They carry full // schemas on the wire so the model can call them directly; MCP tools are not // listed at all — they are discovered blind via tool_search. -// write_file / edit_file / delete_file live in CORE (not here) so they are -// advertised without a tool_search round-trip. +// write / edit / delete live in CORE (not here) so they are +// advertised without a tool_search round-trip. list_dir stays mounted +// but unadvertised and is excluded from tool_search (use glob). // web_fetch / web_search are catalog (not deferred): URL reads and search are // first-class primary work; requiring tool_search before web_fetch caused // thrash on web-bait and contradicted the skywalker "already mounted" rule. export const CATALOG_TOOL_NAMES: readonly string[] = [ - "search_files", + "glob", "grep", - "list_dir", "web_fetch", "web_search", "skill_search", @@ -146,6 +147,22 @@ export const ADVERTISED_TOOL_NAMES: readonly string[] = [ ...CATALOG_TOOL_NAMES, ]; +function isAlreadyAdvertised( + defName: string, + advertisedNames: readonly string[], +): boolean { + const engine = canonicalToolName(defName); + const wire = advertisedToolName(engine); + return ( + advertisedNames.includes(defName) || + advertisedNames.includes(engine) || + advertisedNames.includes(wire) + ); +} + +// Mounted built-ins that stay off the advertised prefix and off tool_search. +const UNADVERTISED_MOUNTED_BUILTINS = new Set(["list_dir"]); + // Project the live tool registry onto the advertised set: the fixed built-in // prefix (its order never changes — this is what keeps the provider cache // prefix stable across no-discovery turns) followed by wire-committed tools @@ -161,17 +178,26 @@ export function advertisedTools( activated: readonly string[] = [], builtInPrefix: readonly string[] = ADVERTISED_TOOL_NAMES, ): ToolDefinition[] { - const byName = new Map(all.map((def) => [def.name, def])); + const byName = new Map(); + for (const def of all) { + byName.set(def.name, def); + const engine = canonicalToolName(def.name); + if (!byName.has(engine)) byName.set(engine, def); + const wire = advertisedToolName(engine); + if (!byName.has(wire)) byName.set(wire, def); + } const seen = new Set(); const orderedNames = [ ...builtInPrefix, ...activated.filter((name) => !builtInPrefix.includes(name)), ]; return orderedNames.flatMap((name) => { - if (seen.has(name)) return []; - seen.add(name); - const def = byName.get(name); - return def !== undefined ? [def] : []; + const def = byName.get(name) ?? byName.get(canonicalToolName(name)); + if (def === undefined) return []; + const projected = projectToolDefinition(def); + if (seen.has(projected.name)) return []; + seen.add(projected.name); + return [projected]; }); } @@ -217,7 +243,7 @@ export function createActivatedToolTracker(): ActivatedToolTracker { export const toolSearchDefinition: ToolDefinition = { name: "tool_search", description: - "Discover callable tools by capability. Most tools — MCP servers, present, and other integrations — are not on the wire until this search promotes them onto the next inference. Core tools (read_file, run_shell, web_fetch, web_search, spawn_agent, …) are already on the wire — do not tool_search for them. wait_agents is mounted on exec-primary runs only, so it is not on the wire elsewhere and this search cannot promote it there. Call this with a short description of what you need (e.g. 'issue tracker', 'render layout', 'granola notes') to get a ranked handful of matching names and short descriptions. Matched tools join the next inference tool list — call them on the next turn, not from this result.", + "Discover callable tools by capability. Most tools — MCP servers, present, and other integrations — are not on the wire until this search promotes them onto the next inference. Core tools (read, bash, web_fetch, web_search, spawn_agent, …) are already on the wire — do not tool_search for them. wait_agents is mounted on exec-primary runs only, so it is not on the wire elsewhere and this search cannot promote it there. Call this with a short description of what you need (e.g. 'issue tracker', 'render layout', 'granola notes') to get a ranked handful of matching names and short descriptions. Matched tools join the next inference tool list — call them on the next turn, not from this result.", inputSchema: { type: "object", properties: { @@ -266,8 +292,17 @@ export function createToolIndex( const queryTokens = tokenizeLexical(query); if (queryTokens.length === 0) return []; const candidates = getDefs() - .filter((def) => !advertisedNames.includes(def.name)) - .filter((def) => allow === undefined || allow.includes(def.name)); + .filter((def) => !isAlreadyAdvertised(def.name, advertisedNames)) + .filter((def) => !UNADVERTISED_MOUNTED_BUILTINS.has(def.name)) + .filter( + (def) => + allow === undefined || + allow.includes(def.name) || + allow.some( + (allowed) => + canonicalToolName(allowed) === canonicalToolName(def.name), + ), + ); return rankAndCut( candidates, (def) => score(def, queryTokens, rawQuery), diff --git a/src/agent/tools.ts b/src/agent/tools.ts index ce624a9c3..2a82a1d9e 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -117,11 +117,6 @@ import { } from "./lexical-rank.js"; import { createSearchAgentsTool } from "./agent-search.js"; import { createReadAgentTraceTool } from "../subagent/trace-tool.js"; -import { - createCodexToolProxies, - type CodexRunTool, -} from "./codex-tool-proxies.js"; -import { createCodexReadRawFile } from "./codex-read-raw-file.js"; import { errorMessage } from "./error-message.js"; import type { ReactorEmittedEvent } from "@intx/inference"; @@ -268,10 +263,8 @@ export interface AgentToolsetArgs { useWorktree?: boolean; }; /** - * 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. + * Retained so callers that still pass the Codex family flag do not break. + * Proxies are no longer mounted; hidden aliases dispatch onto engine tools. */ isCodex?: boolean; /** @@ -542,31 +535,6 @@ export async function createAgentToolset( }), }); - // Codex apply_patch proxy forwards ops through posixTools.run so permission - // plugins (gate, path policy, etc.) still apply — same call shape as - // posix-tool-plugins.test.ts. - 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 } : {}), - }; - }; - - // manage_tasks is not a posix tool — task state is owned by the director, - // which derives it from the manage_tasks tool_call it observes in the - // model's own output (see applyManageTasksToolCall in director.ts), not - // from this handler's return value. 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 = createManageTasksRunner(); - // Align the advertised run_shell timeout with shell-guard (120s foreground // default; advertise settings.shell.timeoutMs when set). // Orchestrator tools (search / trace / fleet) are assembled once so the fleet @@ -648,6 +616,8 @@ export async function createAgentToolset( } } + const runManageTasks = createManageTasksRunner(); + const baseTools: AgentTool[] = [ ...fromToolRunner(posixTools).map((tool) => { let definition = advertiseEditFileLineRange( @@ -845,19 +815,8 @@ export async function createAgentToolset( ); } - // Codex apply_patch mounts when isCodex; primary strips it so Corbits DIY - // stays on write_file/edit_file/delete_file. Leaves keep it via BUILD/DOCS allowlists. - baseTools.push( - ...createCodexToolProxies({ - isCodex: args.isCodex === true, - runTool, - readRawFile: createCodexReadRawFile(cwd, permissionGate), - runManageTasks, - }), - ); - const primaryTools = wrapAgentToolsWithResultTruncation( - baseTools.filter((tool) => tool.definition.name !== "apply_patch"), + baseTools, truncationOptions, ); diff --git a/src/exec/runner.test.ts b/src/exec/runner.test.ts index 8d1554026..d6ce4596c 100644 --- a/src/exec/runner.test.ts +++ b/src/exec/runner.test.ts @@ -92,9 +92,7 @@ describe("exec director allowlist", () => { promote([OUTSIDE_ALLOW]); expect(activated.has(OUTSIDE_ALLOW)).toBe(false); expect(isAdvertised(OUTSIDE_ALLOW)).toBe(false); - expect( - createExecToolCallGate(isAdvertised, { isCodex: false })(OUTSIDE_ALLOW), - ).toBe(false); + expect(createExecToolCallGate(isAdvertised)(OUTSIDE_ALLOW)).toBe(false); }); test("the promoter commits allowed names onto the next infer wire", () => { @@ -125,9 +123,7 @@ describe("exec director allowlist", () => { expect(activated.has(OUTSIDE_ALLOW)).toBe(false); expect(activated.has("read_file")).toBe(true); expect(committed).toBe(1); - expect(computeAdvertised(registry).map((d) => d.name)).toContain( - "read_file", - ); + expect(computeAdvertised(registry).map((d) => d.name)).toContain("read"); }); test("the promoter does not commit a name outside the overlay allow list", () => { diff --git a/src/exec/runner.ts b/src/exec/runner.ts index bbb3f2a39..5e2e742ce 100644 --- a/src/exec/runner.ts +++ b/src/exec/runner.ts @@ -24,10 +24,6 @@ import { DIRECTOR_REGISTRY } from "../agent/directors/registry.js"; import type { DirectorId, DirectorPackage } from "../agent/directors/types.js"; import { submitOutputDefinition } from "../agent/director.js"; import { handleChatDirectorEvent } from "../agent/chat-event-subscribers.js"; -import { - shellDefinition, - updatePlanDefinition, -} from "../agent/codex-tool-proxies.js"; import { CodexRefreshLockError, codexAuthFailureDiagnostic, @@ -385,15 +381,8 @@ export interface ExecResult { export function createExecToolCallGate( isAdvertised: (name: string) => boolean, - options: { isCodex: boolean }, ): (name: string) => boolean { - const unadvertisedCallable = new Set([ - submitOutputDefinition.name, - ...(options.isCodex - ? [shellDefinition.name, updatePlanDefinition.name] - : []), - ]); - return (name) => unadvertisedCallable.has(name) || isAdvertised(name); + return (name) => name === submitOutputDefinition.name || isAdvertised(name); } export function createExecToolPromoter(args: { @@ -838,9 +827,7 @@ export async function runExec(config: Config): Promise { // Same wire contract as the TUI: a registered tool the model was never // shown errors toward tool_search instead of dispatching blind. agentToolset.dynamicRunner.setCallGate( - createExecToolCallGate(isAdvertised, { - isCodex: isCodexProviderName(config.providerName), - }), + createExecToolCallGate(isAdvertised), // Same promoted-but-unmounted contract as the TUI gate: an activated // name missing from the registry errors toward retry (see run() in // DynamicToolRunner). diff --git a/src/plugins/data-only-agent.ts b/src/plugins/data-only-agent.ts index 937392066..c332ebdf9 100644 --- a/src/plugins/data-only-agent.ts +++ b/src/plugins/data-only-agent.ts @@ -11,6 +11,7 @@ import { AgentProfileSchema } from "../agent/profiles.js"; import { REASONING_EFFORTS } from "../agent/profile-types.js"; import { splitFrontmatter } from "./frontmatter.js"; import { type } from "arktype"; +import { WIRE_TO_ENGINE, HIDDEN_TO_ENGINE } from "../agent/tool-aliases.js"; // Reasoning-effort schema derived from the canonical array, mirroring the // pattern in ../agent/profiles.ts (arktype's `type()` needs a literal union @@ -59,14 +60,14 @@ const NativeCapabilitiesModeSchema = type("'allow' | 'exclude'"); // Native Corbits Code keys also work and win ties: inference, capabilities, // skills (frontmatter list, in addition to body `Load the X skill` lines). -// Upstream tool-name aliases mapped to Corbits Code tool ids. Case-insensitive. +// Upstream tool-name aliases mapped to Corbits Code engine ids. Case-insensitive. +// Posix wire/hidden names come from the shared CL-8400 table. const TOOL_ALIASES: Record = { - read: ["read_file"], - write: ["write_file"], - edit: ["edit_file"], - bash: ["run_shell"], - shell: ["run_shell"], - glob: ["search_files"], + ...Object.fromEntries( + Object.entries({ ...WIRE_TO_ENGINE, ...HIDDEN_TO_ENGINE }).map( + ([alias, engine]) => [alias, [engine]], + ), + ), find: ["search_files"], grep: ["grep"], ls: ["list_dir"], diff --git a/src/plugins/permission-plugin.test.ts b/src/plugins/permission-plugin.test.ts index e0ed635d6..f3851fe47 100644 --- a/src/plugins/permission-plugin.test.ts +++ b/src/plugins/permission-plugin.test.ts @@ -341,13 +341,13 @@ describe("gateToolCall", () => { const dir = mkdtempSync(join(tmpdir(), "approval-log-reactor-")); const cwd = mkdtempSync(join(tmpdir(), "gate-cwd-")); const { gate, log } = approvalGate(dir, cwd, { - approvals: [{ tool: "shell", pattern: "shell" }], + approvals: [{ tool: "mcp__acme__do", pattern: "mcp__acme__do" }], requestApproval: refuseApproval, }); const outer: ToolCall = { id: "codex-proxy", - name: "shell", - arguments: { command: "echo x | tee src/a.ts" }, + name: "mcp__acme__do", + arguments: {}, }; const inner: ToolCall = { id: "codex-proxy", diff --git a/src/prompts.test.ts b/src/prompts.test.ts index 16d32fe8b..b1790c639 100644 --- a/src/prompts.test.ts +++ b/src/prompts.test.ts @@ -70,7 +70,7 @@ test("agent identity is Skywalker orchestrator", () => { }); test("harness facts state only the non-derivable tool and safety rules", () => { - expect(HARNESS_FACTS).toContain("write_file/edit_file"); + expect(HARNESS_FACTS).toContain("write/edit"); expect(HARNESS_FACTS).toContain("tiny/single-file/one-route"); expect(HARNESS_FACTS).toContain("Spawn builder"); expect(HARNESS_FACTS).not.toContain( @@ -81,7 +81,7 @@ test("harness facts state only the non-derivable tool and safety rules", () => { expect(HARNESS_FACTS).toContain("no default timeout"); expect(HARNESS_FACTS).toContain("find, rg, and grep -r"); expect(HARNESS_FACTS).toMatch(/OOM the host/); - expect(HARNESS_FACTS).toMatch(/Prefer the bounded grep\/search_files tools/); + expect(HARNESS_FACTS).toMatch(/Prefer the bounded grep\/glob tools/); expect(HARNESS_FACTS).toMatch( /not substitute another unbounded walk \(fd, ls -R, scripted os\.walk\)/, ); @@ -100,7 +100,7 @@ test("harness facts state only the non-derivable tool and safety rules", () => { }); test("harness facts gate tool-output URI reads on a named truncation notice", () => { - expect(HARNESS_FACTS).toContain("read_file"); + expect(HARNESS_FACTS).toContain("Only read a tool-output:// URI"); expect(HARNESS_FACTS).toMatch(/filesystem path/i); expect(HARNESS_FACTS).toMatch(/tool-output:\/\//); expect(HARNESS_FACTS).toContain("truncation notice on that result named one"); @@ -109,9 +109,9 @@ test("harness facts gate tool-output URI reads on a named truncation notice", () expect(HARNESS_FACTS).not.toMatch(/re-reading huge blobs/i); }); -test("read_file catalog summary gates tool-output URI reads on truncation", () => { - const listed = buildAvailableTools(["read_file"]); - expect(listed).toContain("read_file"); +test("read catalog summary gates tool-output URI reads on truncation", () => { + const listed = buildAvailableTools(["read"]); + expect(listed).toContain("read"); expect(listed).toMatch(/tool-output:\/\//); expect(listed).toContain("truncation notice named one"); expect(listed).toContain("cat/head/tail"); @@ -129,7 +129,7 @@ test("harness facts name skill_search as a resident catalog tool", () => { test("leaf harness facts advertise product write tools", () => { const facts = buildHarnessFacts({ subAgent: true, dynamicTools: false }); - expect(facts).toContain("write_file/edit_file"); + expect(facts).toContain("write/edit"); expect(facts).not.toContain("not mounted on the primary Skywalker session"); }); @@ -151,7 +151,7 @@ test("guidelines cover response style, tool choice, ask vs proceed, and scope", expect(GUIDELINES).toContain("Tool choice:"); expect(GUIDELINES).toContain("Ask vs proceed:"); expect(GUIDELINES).toContain("Scope and conventions:"); - expect(GUIDELINES).toContain("grep or search_files"); + expect(GUIDELINES).toContain("grep or glob"); expect(GUIDELINES).toContain("ask_operator only when permission blocks you"); expect(GUIDELINES).toContain("skill_search when choosing"); expect(GUIDELINES).toContain( @@ -247,7 +247,7 @@ test("default session lists split fleet tools and search_agents", () => { }); test("chat prompt advertises core tools but never enumerates MCP integrations", () => { - expect(CHAT_SYSTEM_PROMPT).toContain("read_file"); + expect(CHAT_SYSTEM_PROMPT).toContain("- read:"); expect(CHAT_SYSTEM_PROMPT).toContain("tool_search"); expect(CHAT_SYSTEM_PROMPT).not.toContain("mcp__"); // No static catalog dump — discovery is via tool_search, not a listed catalog. @@ -390,10 +390,10 @@ test("buildEnvironmentContext reports a clean tree and a non-git directory", () }); test("buildAvailableTools lists exactly the tools it is given", () => { - const custom = ["read_file", "write_file"]; + const custom = ["read", "write"]; const listed = buildAvailableTools(custom); - expect(listed).toContain("read_file"); - expect(listed).toContain("write_file"); + expect(listed).toContain("read"); + expect(listed).toContain("write"); expect(listed).not.toContain("tool_search"); }); @@ -487,11 +487,11 @@ test("sub-agent prompt does not advertise tool_search (it gets names only)", () test("worker prompt does not advertise archive:///; primary chat prompt does", () => { expect(SUBAGENT_SYSTEM_PROMPT).not.toContain("archive:///"); expect(CHAT_SYSTEM_PROMPT).toContain("archive:///"); + expect(buildAvailableTools(["read", "grep", "glob"])).not.toContain( + "archive:///", + ); expect( - buildAvailableTools(["read_file", "grep", "search_files"]), - ).not.toContain("archive:///"); - expect( - buildAvailableTools(["read_file", "grep", "search_files"], { + buildAvailableTools(["read", "grep", "glob"], { advertiseArchive: true, }), ).toContain("archive:///"); diff --git a/src/session/assemble-runtime.test.ts b/src/session/assemble-runtime.test.ts index b44d4cca3..9f695400a 100644 --- a/src/session/assemble-runtime.test.ts +++ b/src/session/assemble-runtime.test.ts @@ -44,7 +44,7 @@ describe("createAdvertisedToolset", () => { def("write_file"), def("mystery_tool"), ]).map((d) => d.name); - expect(names).toContain("write_file"); + expect(names).toContain("write"); expect(names).not.toContain("mystery_tool"); }); @@ -98,7 +98,7 @@ describe("createAdvertisedToolset", () => { computeAdvertised([def("read_file"), def("write_file")]).map( (d) => d.name, ), - ).toEqual(["read_file"]); + ).toEqual(["read"]); }); test("advertises nothing from an empty registry", () => { diff --git a/src/session/assemble-runtime.ts b/src/session/assemble-runtime.ts index a40296a48..1e1cf773e 100644 --- a/src/session/assemble-runtime.ts +++ b/src/session/assemble-runtime.ts @@ -49,6 +49,8 @@ import { type ActivatedToolTracker, type ToolAvailability, } from "../agent/tool-search.js"; +import { nameMatchesAdvertisedListing } from "../agent/tool-aliases.js"; +import { canonicalToolName } from "../agent/canonical-tool-name.js"; import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js"; import { resolveModelFamilyPolicy } from "../agent/model-family-policy.js"; import { @@ -447,8 +449,14 @@ export function createAdvertisedToolset(args: { ); }; const isAdvertised = (name: string): boolean => { - if (deniedFor(args.getProvider()).includes(name)) return false; - return prefix.includes(name) || activated.has(name); + const denied = deniedFor(args.getProvider()); + if (denied.includes(name) || denied.includes(canonicalToolName(name))) { + return false; + } + return nameMatchesAdvertisedListing( + name, + (n) => prefix.includes(n) || activated.has(n), + ); }; const flushPromotions = (): boolean => { let grew = false; diff --git a/src/shell/run-shell-authz.test.ts b/src/shell/run-shell-authz.test.ts index 70d4ebb49..75acb13a7 100644 --- a/src/shell/run-shell-authz.test.ts +++ b/src/shell/run-shell-authz.test.ts @@ -457,7 +457,7 @@ describe("authz hard-deny peels glued and trailing env -S forms", () => { expect(reason).toMatch(openEnded); expect(reason).toMatch(/OOM the host/); expect(reason).toMatch(/walk huge trees/); - expect(reason).toMatch(/Prefer the bounded grep\/search_files tools/); + expect(reason).toMatch(/Prefer the bounded grep\/glob tools/); expect(reason).toMatch( /not substitute another unbounded walk \(fd, ls -R, scripted os\.walk\)/, ); diff --git a/src/shell/run-shell-authz.ts b/src/shell/run-shell-authz.ts index d2e358f0a..21233ea48 100644 --- a/src/shell/run-shell-authz.ts +++ b/src/shell/run-shell-authz.ts @@ -105,7 +105,7 @@ const BLOCKED_QUOTED_PAYLOAD_PATTERNS: RegExp[] = [ // Open-ended tree walks via the shell OOM the host: `find | tail` still forces // the full stream through the collector, and recursive grep/rg walks huge trees // before any pipe limit applies. Hard-deny those shapes for host safety; the -// bounded grep/search_files tools remain practical alternatives (timeout + +// bounded grep/glob tools remain practical alternatives (timeout + // output caps). (`git log | tail` and similar non-walk pipes are fine — the // 512KB shell output cap is the backstop for those.) const OPEN_ENDED_SEARCH_PATTERNS: RegExp[] = [ @@ -1196,7 +1196,7 @@ function openEndedSearchReason(command: string): string | undefined { // and would otherwise look like sanctioned ways to do the same thing. return ( `Open-ended shell search blocked — shell find, head-position rg, and recursive ` + - `grep -r can walk huge trees and OOM the host. Prefer the bounded grep/search_files ` + + `grep -r can walk huge trees and OOM the host. Prefer the bounded grep/glob ` + `tools (timeout + output caps). Do not substitute another unbounded walk ` + `(fd, ls -R, scripted os.walk). Command: ${command}` ); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 1fb518a27..af13a923a 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -56,15 +56,8 @@ import { wrapAgentToolsWithResultTruncation, type SpillBlobWriter, } from "../plugins/result-truncation-plugin.js"; -import { - allowDeleteFromCapabilities, - allowShellFromCapabilities, - createCodexToolProxies, - type CodexRunTool, -} from "../agent/codex-tool-proxies.js"; -import { createCodexReadRawFile } from "../agent/codex-read-raw-file.js"; -import { isCodexProviderName } from "../config/codex-providers.js"; +import { type CodexRunTool } from "../agent/codex-tool-proxies.js"; import { isOpenCodeGoProvider } from "../../packages/opencode-go/src/index.js"; import { createCompositeBlobReader } from "../agent/lazy-blob-reader.js"; @@ -78,6 +71,8 @@ import { type InterventionSink, } from "./intervention-log.js"; import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js"; +import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { projectToolDefinitions } from "../agent/tool-aliases.js"; import { buildCompactionContinuationMessage, @@ -377,11 +372,15 @@ function applyCapabilityFilter( tools: AgentTool[], capabilities: CapabilityFilter, ): AgentTool[] { - const nameSet = new Set(capabilities.tools); + const engines = new Set( + capabilities.tools.map((name) => canonicalToolName(name)), + ); if (capabilities.mode === "exclude") { - return tools.filter((t) => !nameSet.has(t.definition.name)); + return tools.filter( + (t) => !engines.has(canonicalToolName(t.definition.name)), + ); } - return tools.filter((t) => nameSet.has(t.definition.name)); + return tools.filter((t) => engines.has(canonicalToolName(t.definition.name))); } export interface SubAgentRunController { @@ -754,30 +753,7 @@ async function runSubAgentInner( 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 = createCodexProxyRunTool(posixTools); - // 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 = createManageTasksRunner(); - tools = [ - ...tools, - ...createCodexToolProxies({ - isCodex: isCodexProviderName(params.provider.providerName), - runTool, - readRawFile: createCodexReadRawFile(params.cwd, permissionGate), - runManageTasks, - allowDelete: allowDeleteFromCapabilities(params.capabilities), - allowShell: allowShellFromCapabilities(params.capabilities), - }), - ]; - // Worker skill mounts: every worker, including grok/kimi leaves, mounts // skill_search + use_skill. Scoped to the dispatch's allowedSkillNames // (union of pkg.attachedSkills and optionalSkills). Mounted before the @@ -1109,10 +1085,13 @@ async function runSubAgentInner( factory: (_config, _env, agentCtx) => { const director = new SubAgentDirector( agentCtx.systemPrompt, - normalizeToolDefinitionsForProvider([...agentCtx.toolDefinitions], { - providerName: params.provider.providerName, - model: params.provider.model, - }), + normalizeToolDefinitionsForProvider( + projectToolDefinitions([...agentCtx.toolDefinitions]), + { + providerName: params.provider.providerName, + model: params.provider.model, + }, + ), requestContinuation, modelFamilyPolicy.subAgentStallTimeoutMs, Date.now, diff --git a/src/tui/dynamic-tool-runner.ts b/src/tui/dynamic-tool-runner.ts index 192f52ecc..d3b168ea0 100644 --- a/src/tui/dynamic-tool-runner.ts +++ b/src/tui/dynamic-tool-runner.ts @@ -12,6 +12,7 @@ import { } from "./tool-execution-watchdog.js"; import { resolveRegisteredToolName } from "./resolve-registered-tool-name.js"; import { stripTerminalControlSequences } from "../util/control-char-strip.js"; +import { prepareDispatchedToolCall } from "../agent/tool-aliases.js"; // A tool runner whose set of tools can grow after construction. The static // createToolRunner freezes its name map at build time, which cannot accommodate @@ -150,8 +151,16 @@ export function createDynamicToolRunner( isError: true, }; } - const dispatchCall = - resolved === call.name ? call : { ...call, name: resolved }; + let dispatchCall: ToolCall; + try { + dispatchCall = prepareDispatchedToolCall(call, resolved); + } catch (err) { + return { + callId: call.id, + content: err instanceof Error ? err.message : String(err), + isError: true, + }; + } const executionTimeoutMs = resolveToolExecutionTimeoutMs( watchdogConfig, dispatchCall, diff --git a/src/tui/runner/session.ts b/src/tui/runner/session.ts index 542ee3818..d5400d60e 100644 --- a/src/tui/runner/session.ts +++ b/src/tui/runner/session.ts @@ -107,10 +107,6 @@ import { createChatDirector, submitOutputDefinition, } from "../../agent/director.js"; -import { - shellDefinition, - updatePlanDefinition, -} from "../../agent/codex-tool-proxies.js"; import { attachApprovalBudget } from "../request-approval.js"; import { createGateRequestApproval } from "../request-approval.js"; import { getActivePricingCache } from "../../cost/cost-visibility.js"; @@ -501,14 +497,8 @@ export async function assembleTUISession( // A registered tool the wire never advertised must error toward tool_search // instead of dispatching blind — the transcript would otherwise claim a call // the next infer does not declare. submit_output rides every infer via the - // director, and Codex's native proxies answer calls Codex models emit - // unaided; neither flows through the advertised set. - const unadvertisedCallable = new Set([ - submitOutputDefinition.name, - ...(isCodexProviderName(config.providerName) - ? [shellDefinition.name, updatePlanDefinition.name] - : []), - ]); + // director; hidden posix aliases dispatch when their engine is advertised. + const unadvertisedCallable = new Set([submitOutputDefinition.name]); toolset.dynamicRunner.setCallGate( (name) => unadvertisedCallable.has(name) || isAdvertised(name), // A promoted-but-unmounted name (server dropped between search and call) diff --git a/tests/unit/exec/runner.test.ts b/tests/unit/exec/runner.test.ts index 264f9382f..dec3ae865 100644 --- a/tests/unit/exec/runner.test.ts +++ b/tests/unit/exec/runner.test.ts @@ -23,10 +23,6 @@ import { EXEC_MCP_HANDSHAKE_TIMEOUT_MS, } from "../../../src/exec/mcp-handshake.js"; import { submitOutputDefinition } from "../../../src/agent/director.js"; -import { - shellDefinition, - updatePlanDefinition, -} from "../../../src/agent/codex-tool-proxies.js"; import { advertisedToolNamesForSessionMode, createToolIndex, @@ -1029,7 +1025,7 @@ describe("exec tool call gate and promoter", () => { handler: async () => reply, }); - function wireExecDiscovery(isCodex: boolean) { + function wireExecDiscovery() { const runner = createDynamicToolRunner([ stringTool("read_file", "core", "read a file"), stringTool( @@ -1044,8 +1040,6 @@ describe("exec tool call gate and promoter", () => { ), stringTool("plugin__notes__save", "noted", "Save granola notes"), stringTool(submitOutputDefinition.name, "submitted", "submit output"), - stringTool(shellDefinition.name, "sh", "run a shell command"), - stringTool(updatePlanDefinition.name, "planned", "update the plan"), ]); const { activated, isAdvertised, computeAdvertised, flushPromotions } = createAdvertisedToolset({ @@ -1053,7 +1047,7 @@ describe("exec tool call gate and promoter", () => { toolAvailability: { languageServerAvailable: false }, getProvider: () => ({ providerName: "test", model: "test" }), }); - runner.setCallGate(createExecToolCallGate(isAdvertised, { isCodex })); + runner.setCallGate(createExecToolCallGate(isAdvertised)); let persistCount = 0; const promote = createExecToolPromoter({ activate: (names) => activated.activate(names), @@ -1093,7 +1087,7 @@ describe("exec tool call gate and promoter", () => { test("tool_search then MCP dispatch with the gate on", async () => { const { runner, search, persistCount, computeAdvertised } = - wireExecDiscovery(false); + wireExecDiscovery(); const blocked = await dispatch(runner, "mcp__linear__save_issue"); expect(blocked.isError).toBe(true); expect(blocked.content).toContain("tool_search"); @@ -1111,7 +1105,7 @@ describe("exec tool call gate and promoter", () => { }); test("present and plugin names pass the gate after tool_search promote", async () => { - const { runner, search } = wireExecDiscovery(false); + const { runner, search } = wireExecDiscovery(); expect((await dispatch(runner, "present")).isError).toBe(true); expect((await dispatch(runner, "plugin__notes__save")).isError).toBe(true); @@ -1132,25 +1126,9 @@ describe("exec tool call gate and promoter", () => { }); test("gate admits submit_output without activation", async () => { - const { runner } = wireExecDiscovery(false); + const { runner } = wireExecDiscovery(); const result = await dispatch(runner, submitOutputDefinition.name); expect(result.content).toBe("submitted"); expect(result.isError).toBeUndefined(); }); - - test("Codex gate admits shell and update_plan without activation", async () => { - const { runner } = wireExecDiscovery(true); - const shell = await dispatch(runner, shellDefinition.name); - expect(shell.content).toBe("sh"); - expect(shell.isError).toBeUndefined(); - const plan = await dispatch(runner, updatePlanDefinition.name); - expect(plan.content).toBe("planned"); - expect(plan.isError).toBeUndefined(); - }); - - test("non-Codex gate refuses shell until it is advertised", async () => { - const { runner } = wireExecDiscovery(false); - const blocked = await dispatch(runner, shellDefinition.name); - expect(blocked.isError).toBe(true); - }); }); From 4b473a4aee21b6c78e292e1aa3fce2d0cc239e8b Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 08:32:06 -0700 Subject: [PATCH 2/4] fix(permissions): coerce hidden shell argv before authorize Authorize classified Codex command arrays as empty strings, so reactor-gated hidden shell auto-allowed and the unwrapped script ran. --- src/agent/apply-patch-diff.test.ts | 272 -------- src/agent/codex-read-raw-file.ts | 105 --- src/agent/codex-tool-mount.test.ts | 65 -- src/agent/codex-tool-proxies.test.ts | 681 -------------------- src/agent/codex-tool-proxies.ts | 484 -------------- src/agent/prompt-sizes.test.ts | 3 +- src/agent/worker-contract.test.ts | 8 +- src/agent/worker-contract.ts | 3 +- src/permission/gate.ts | 35 +- src/permission/shell-argv-authorize.test.ts | 62 ++ src/plugins/secret-guard-symlink.test.ts | 29 - src/subagent/run-codex-proxy.test.ts | 99 --- src/subagent/run.ts | 39 +- 13 files changed, 108 insertions(+), 1777 deletions(-) delete mode 100644 src/agent/apply-patch-diff.test.ts delete mode 100644 src/agent/codex-read-raw-file.ts delete mode 100644 src/agent/codex-tool-proxies.test.ts delete mode 100644 src/agent/codex-tool-proxies.ts create mode 100644 src/permission/shell-argv-authorize.test.ts delete mode 100644 src/subagent/run-codex-proxy.test.ts diff --git a/src/agent/apply-patch-diff.test.ts b/src/agent/apply-patch-diff.test.ts deleted file mode 100644 index 162b93304..000000000 --- a/src/agent/apply-patch-diff.test.ts +++ /dev/null @@ -1,272 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { createPosixTools } from "@intx/tools-posix"; -import { createToolRunner } from "@intx/agent"; -import type { AgentTool } from "@intx/agent"; - -import { - createCodexToolProxies, - type CodexRunTool, -} from "./codex-tool-proxies.js"; -import { createCodexReadRawFile } from "./codex-read-raw-file.js"; -import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js"; -import { createPermissionGate } from "../permission/gate.js"; - -/** - * apply_patch forwards each op through the same posixTools.run chain the rest - * of the agent uses (see tools.ts), so verify-plugin's and delete-file-plugin's - * diffs surface here too without any apply_patch-specific plumbing. - */ -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, - ); -} - -async function makeApplyPatch( - cwd: string, - options: { skipPermissions?: boolean } = {}, -): Promise { - const gate = createPermissionGate({ - approvals: [], - interactive: false, - skipPermissions: options.skipPermissions ?? true, - reactorGated: false, - auto: false, - cwd, - }); - const posixTools = createPosixTools({ - cwd, - plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }), - }); - 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 } : {}), - }; - }; - // Real production path (CL-6966): Update File matches patch context against - // raw file content, not read_file's cat -n formatted output. - return createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile: createCodexReadRawFile(cwd, gate), - runManageTasks: async () => ({ content: "ok" }), - }); -} - -describe("apply_patch Update File matches raw content, not read_file's numbered output (CL-6966)", () => { - test("multi-hunk Update File succeeds through the real production path", async () => { - const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-")); - try { - await writeFile( - join(cwd, "app.py"), - "def greet():\n print('hi')\n\n\ndef farewell():\n print('bye')\n", - ); - const tools = await makeApplyPatch(cwd); - const input = [ - "*** Begin Patch", - "*** Update File: app.py", - "@@ def greet():", - "- print('hi')", - "+ print('hello')", - "@@ def farewell():", - "- print('bye')", - "+ print('goodbye')", - "*** End Patch", - ].join("\n"); - - const result = await invokeApplyPatch(tools, input); - - expect(result.isError).not.toBe(true); - const written = await Bun.file(join(cwd, "app.py")).text(); - expect(written).toBe( - "def greet():\n print('hello')\n\n\ndef farewell():\n print('goodbye')\n", - ); - } finally { - await rm(cwd, { recursive: true, force: true }); - } - }); - - test("Update File shows the diff", async () => { - const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-")); - try { - await writeFile(join(cwd, "a.txt"), "line1\nworld\nline3\n"); - const tools = await makeApplyPatch(cwd); - const input = [ - "*** Begin Patch", - "*** Update File: a.txt", - "@@", - " line1", - "-world", - "+universe", - " line3", - "*** End Patch", - ].join("\n"); - - const result = await invokeApplyPatch(tools, input); - - expect(result.isError).not.toBe(true); - expect(String(result.content)).toContain("-world"); - expect(String(result.content)).toContain("+universe"); - } finally { - await rm(cwd, { recursive: true, force: true }); - } - }); - - test("Delete File shows the removed content", async () => { - const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-")); - try { - await writeFile(join(cwd, "gone.txt"), "bye\n"); - const tools = await makeApplyPatch(cwd); - const input = [ - "*** Begin Patch", - "*** Delete File: gone.txt", - "*** End Patch", - ].join("\n"); - - const result = await invokeApplyPatch(tools, input); - - expect(result.isError).not.toBe(true); - expect(String(result.content)).toContain("-bye"); - } finally { - await rm(cwd, { recursive: true, force: true }); - } - }); - - test("Add File shows the added content", async () => { - const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-")); - try { - const tools = await makeApplyPatch(cwd); - const input = [ - "*** Begin Patch", - "*** Add File: new.txt", - "+hello", - "*** End Patch", - ].join("\n"); - - const result = await invokeApplyPatch(tools, input); - - expect(result.isError).not.toBe(true); - expect(String(result.content)).toContain("+hello"); - } finally { - await rm(cwd, { recursive: true, force: true }); - } - }); -}); - -describe("apply_patch Update File refuses reads outside the sanctioned workspace (CL-6966 follow-up)", () => { - // Insertion-only hunk (no context to match): the shape that makes an - // unauthorized raw read exploitable rather than merely wrong, since it - // requires no content match to "succeed" and hands the read content - // straight to write_file via Move to. - const insertionOnlyMoveInput = (path: string, moveTo: string) => - [ - "*** Begin Patch", - `*** Update File: ${path}`, - `*** Move to: ${moveTo}`, - "@@", - "+", - "*** End Patch", - ].join("\n"); - - test("../ traversal out of the workspace is refused", async () => { - const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-parent-")); - const cwd = join(parent, "workspace"); - await mkdir(cwd); - try { - await writeFile(join(parent, "victim.txt"), "outside secret\n"); - const tools = await makeApplyPatch(cwd, { skipPermissions: false }); - - const result = await invokeApplyPatch( - tools, - insertionOnlyMoveInput("../victim.txt", "leaked.txt"), - ); - - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch(/escapes working directory/i); - } finally { - await rm(parent, { recursive: true, force: true }); - } - }); - - test("a symlinked directory leading outside the workspace is refused", async () => { - const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-symlink-")); - const cwd = join(parent, "workspace"); - const outside = join(parent, "outside"); - await mkdir(cwd); - await mkdir(outside); - try { - await writeFile(join(outside, "victim.txt"), "outside secret\n"); - await symlink(outside, join(cwd, "escape-link")); - const tools = await makeApplyPatch(cwd, { skipPermissions: false }); - - const result = await invokeApplyPatch( - tools, - insertionOnlyMoveInput("escape-link/victim.txt", "leaked.txt"), - ); - - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch(/escapes working directory/i); - } finally { - await rm(parent, { recursive: true, force: true }); - } - }); - - test("a secret-guard path (.env) is refused even with skipPermissions", async () => { - const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-secret-")); - try { - await writeFile(join(cwd, ".env"), "API_KEY=super-secret\n"); - // skipPermissions: true (yolo) — secret-guard has no bypass, unlike containment. - const tools = await makeApplyPatch(cwd, { skipPermissions: true }); - - const result = await invokeApplyPatch( - tools, - insertionOnlyMoveInput(".env", "leaked.txt"), - ); - - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch(/sensitive file/i); - // The secret must never have reached the workspace under a new name. - expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false); - } finally { - await rm(cwd, { recursive: true, force: true }); - } - }); - - test("../ traversal to a secret file is refused (secret-guard applies to relative paths too)", async () => { - const parent = await mkdtemp( - join(tmpdir(), "apply-patch-cl6966-secret-parent-"), - ); - const cwd = join(parent, "workspace"); - await mkdir(cwd); - try { - await writeFile(join(parent, ".env"), "API_KEY=super-secret\n"); - const tools = await makeApplyPatch(cwd, { skipPermissions: false }); - - const result = await invokeApplyPatch( - tools, - insertionOnlyMoveInput("../.env", "leaked.txt"), - ); - - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch( - /sensitive file|escapes working directory/i, - ); - expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false); - } finally { - await rm(parent, { recursive: true, force: true }); - } - }); -}); diff --git a/src/agent/codex-read-raw-file.ts b/src/agent/codex-read-raw-file.ts deleted file mode 100644 index 85623ab0b..000000000 --- a/src/agent/codex-read-raw-file.ts +++ /dev/null @@ -1,105 +0,0 @@ -/** - * Raw (non-`cat -n`) file reads for apply_patch's Update File leg (CL-6966). - * - * `applyOp` calls this directly from outside the posixTools middleware chain - * — no ToolPlugin ever sees `op.path` here, unlike the write leg which still - * goes through the full pathEscapePlugin / secretGuardPlugin / - * permissionPlugin stack (see buildCorePosixToolPlugins in - * posix-tool-plugins.ts). `requireRelativePath` in codex-apply-patch.ts only - * rejects absolute paths — it does nothing about `../` traversal — so this - * reader must apply the same containment and secret-file checks itself, or - * an Update File op naming e.g. `../../.env` (with a no-op insertion hunk, - * which requires no context match) can read a secret and hand it straight to - * write_file as an exfiltration primitive. - * - * Reuses the existing containment and secret-file authorities rather than - * reimplementing them: `resolveWorkspacePath` (symlink-aware realpath - * containment, the same function pathEscapePlugin calls) and - * `isSensitivePathResolved` (the secretGuardPlugin denylist, including the - * CL-6971 realpath floor). Both are hard denials — the latter has no - * yolo/allowOutside bypass, matching secretGuardPlugin's own unconditional - * behavior. - */ - -import { readFile } from "node:fs/promises"; -import { resolve } from "node:path"; -import { hasCode } from "@intx/types"; -import { resolveWorkspacePath } from "../permission/path-restriction.js"; -import { createWorktreeRootsProvider } from "../permission/worktree-roots.js"; -import { - isSensitivePath, - isSensitivePathResolved, -} from "../plugins/secret-guard-plugin.js"; -import type { PermissionGate } from "../permission/gate.js"; -import type { CodexReadRawFile } from "./codex-tool-proxies.js"; -import { errorMessage } from "./error-message.js"; - -/** `path` is workspace-relative (apply_patch's parser rejects absolute paths). */ -export function createCodexReadRawFile( - cwd: string, - permissionGate: PermissionGate, -): CodexReadRawFile { - const rootsProvider = createWorktreeRootsProvider(cwd); - const allowOutside = (): boolean => permissionGate.getSkipPermissions(); - - return async (path) => { - // Secret-file denylist first, on the raw path — this must hold regardless - // of containment or yolo, exactly like secretGuardPlugin's hard deny. - if (isSensitivePath(path)) { - return { - content: `Access to sensitive file blocked by policy: ${path}`, - isError: true, - }; - } - - const resolved = resolveWorkspacePath(cwd, path, rootsProvider); - let absolutePath: string; - if (resolved !== undefined) { - absolutePath = resolved; - } else if (allowOutside()) { - // Mirrors pathEscapePlugin's own allowOutside fallback (yolo mode): - // lexical absolutize. The realpath floor below still catches symlinks - // whose innocuous names would otherwise defeat the denylist (CL-6971). - absolutePath = resolve(cwd, path); - } else { - return { - content: `Path escapes working directory: ${path}`, - isError: true, - }; - } - - // Re-check after resolve — and realpath when the allowOutside branch left - // a symlink unresolved — so a link can name something innocuous while - // pointing at a sensitive real path. - if (isSensitivePathResolved(absolutePath)) { - return { - content: `Access to sensitive file blocked by policy: ${path}`, - isError: true, - }; - } - - try { - const buf = await readFile(absolutePath); - if (buf.includes(0)) { - return { - content: `refusing to read binary file: ${path}`, - isError: true, - }; - } - return { content: buf.toString("utf8") }; - } catch (err) { - if (hasCode(err)) { - if (err.code === "ENOENT") - return { content: `file not found: ${path}`, isError: true }; - if (err.code === "EACCES") - return { content: `permission denied: ${path}`, isError: true }; - if (err.code === "EISDIR") - return { content: `path is a directory: ${path}`, isError: true }; - } - return { - content: errorMessage(err), - isError: true, - }; - } - }; -} diff --git a/src/agent/codex-tool-mount.test.ts b/src/agent/codex-tool-mount.test.ts index c99b34e43..8715fb359 100644 --- a/src/agent/codex-tool-mount.test.ts +++ b/src/agent/codex-tool-mount.test.ts @@ -8,11 +8,6 @@ import { join } from "node:path"; import { afterEach, describe, expect, test, spyOn } from "bun:test"; import * as posixModule from "@intx/tools-posix"; -import { - allowDeleteFromCapabilities, - allowShellFromCapabilities, - createCodexToolProxies, -} from "./codex-tool-proxies.js"; import { BUILD_TOOLS, DOCS_TOOLS } from "./directors/tool-sets.js"; import { advertisedTools, CORE_TOOL_NAMES } from "./tool-search.js"; @@ -123,64 +118,4 @@ describe("Codex tool proxy mount", () => { expect(DOCS_TOOLS).not.toContain("apply_patch"); expect(CORE_TOOL_NAMES).not.toContain("apply_patch"); }); - - test("capability include-filter no longer keeps Codex proxy names", () => { - const proxies = createCodexToolProxies({ - isCodex: true, - runTool: async () => ({ content: "ok" }), - readRawFile: async () => ({ content: "ok" }), - runManageTasks: async () => ({ content: "ok" }), - }); - expect(proxies.map((t) => t.definition.name)).toEqual([ - "apply_patch", - "shell", - "update_plan", - ]); - - const allow = new Set(BUILD_TOOLS); - const kept = proxies.filter((t) => allow.has(t.definition.name)); - expect(kept).toEqual([]); - - const docsAllow = new Set(DOCS_TOOLS); - const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); - expect(docsKept).toEqual([]); - }); - - test("runSubAgent-shaped allowlists do not keep Codex proxy names", () => { - const docsAllow = new Set(DOCS_TOOLS); - const proxies = createCodexToolProxies({ - isCodex: true, - runTool: async () => ({ content: "ok" }), - readRawFile: async () => ({ content: "ok" }), - runManageTasks: async () => ({ content: "ok" }), - allowDelete: allowDeleteFromCapabilities({ - mode: "allow", - tools: DOCS_TOOLS, - }), - allowShell: allowShellFromCapabilities({ - mode: "allow", - tools: DOCS_TOOLS, - }), - }); - const docsKept = proxies.filter((t) => docsAllow.has(t.definition.name)); - expect(docsKept).toEqual([]); - }); - - test("non-Codex runSubAgent-shaped mount produces no proxies at all", () => { - const proxies = createCodexToolProxies({ - isCodex: false, - runTool: async () => ({ content: "ok" }), - readRawFile: async () => ({ content: "ok" }), - runManageTasks: async () => ({ content: "ok" }), - allowDelete: allowDeleteFromCapabilities({ - mode: "allow", - tools: BUILD_TOOLS, - }), - allowShell: allowShellFromCapabilities({ - mode: "allow", - tools: BUILD_TOOLS, - }), - }); - expect(proxies).toEqual([]); - }); }); diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts deleted file mode 100644 index 306789643..000000000 --- a/src/agent/codex-tool-proxies.test.ts +++ /dev/null @@ -1,681 +0,0 @@ -import { defined } from "../../tests/helpers/defined.js"; -import { describe, expect, test } from "bun:test"; -import { createToolRunner } from "@intx/agent"; -import type { AgentTool } from "@intx/agent"; - -import { - allowDeleteFromCapabilities, - allowShellFromCapabilities, - createCodexToolProxies, - type CodexReadRawFile, - type CodexRunTool, -} from "./codex-tool-proxies.js"; -import { DOCS_TOOLS, BUILD_TOOLS } from "./directors/tool-sets.js"; -import { - applyManageTasks, - parseManageTasksArgs, - type ManageTasksRunner, - type Task, -} from "./tasks.js"; - -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 -// 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; - runTool: CodexRunTool; - readRawFile: CodexReadRawFile; -} { - const files = new Map(Object.entries(initial)); - const calls: Call[] = []; - const readRawFile: CodexReadRawFile = async (path) => { - const content = files.get(path); - if (content === undefined) { - return { content: `File not found: ${path}`, isError: true }; - } - return { content }; - }; - const runTool: CodexRunTool = async (name, args) => { - calls.push({ name, args }); - if (name === "read_file") { - 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}` }; - } - if (name === "run_shell") { - return { content: `ran: ${JSON.stringify(args)}` }; - } - return { content: `unknown tool: ${name}`, isError: true }; - }; - return { calls, files, runTool, readRawFile }; -} - -const unusedManageTasks: ManageTasksRunner = async () => ({ - content: "unused", -}); -const unusedReadRawFile: CodexReadRawFile = async () => ({ content: "unused" }); - -// A real manage_tasks dispatch: parses with the actual arktype schema and -// mutates a real Task[] with the actual applyManageTasks reducer from -// 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: ManageTasksRunner; -} { - let tasks: Task[] = []; - const calls: Record[] = []; - const runManageTasks: ManageTasksRunner = 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( - { id: "call-1", name: "apply_patch", arguments: { input } }, - new AbortController().signal, - ); -} - -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({ - isCodex: false, - runTool: async () => ({ content: "unused" }), - readRawFile: unusedReadRawFile, - runManageTasks: unusedManageTasks, - }); - expect(tools).toEqual([]); - }); - - test("returns apply_patch, shell, update_plan stringTools when Codex", () => { - const tools = createCodexToolProxies({ - isCodex: true, - runTool: async () => ({ content: "unused" }), - readRawFile: unusedReadRawFile, - runManageTasks: unusedManageTasks, - }); - expect(tools.map((t) => t.definition.name)).toEqual([ - "apply_patch", - "shell", - "update_plan", - ]); - expect(tools.every((t) => t.kind === "string")).toBe(true); - expect(defined(tools[0]).definition.inputSchema).toMatchObject({ - required: ["input"], - }); - }); - - test("add forwards write_file with Codex trailing newline", async () => { - const { calls, files, runTool, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder({ - "obsolete.txt": "gone", - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder({ - "obsolete.txt": "gone", - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - allowDelete: false, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder({ - "src/app.py": original, - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - allowDelete: false, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder({ - "src/app.py": original, - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - allowDelete: false, - runManageTasks: unusedManageTasks, - }); - 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(["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, readRawFile } = makeRecorder({ - "src/app.py": original, - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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(["write_file"]); - expect(defined(calls[0]).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, readRawFile } = makeRecorder({ - "src/app.py": original, - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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(["write_file", "delete_file"]); - expect(defined(calls[0]).args.path).toBe("src/main.py"); - expect(defined(calls[0]).args.content).toBe(`def greet(): -print("Hello, world!") -`); - expect(defined(calls[1]).args).toEqual({ path: "src/app.py" }); - expect(files.has("src/app.py")).toBe(false); - expect(files.get("src/main.py")).toBe(`def greet(): -print("Hello, world!") -`); - }); - - test("multi-op patch runs each op in order", async () => { - const { calls, files, runTool, readRawFile } = makeRecorder({ - "src/app.py": "old\n", - "obsolete.txt": "x", - }); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - const result = await invokeApplyPatch( - 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", - "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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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" }), - readRawFile: unusedReadRawFile, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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("shell proxy", () => { - test("string command forwards to run_shell", async () => { - const { calls, runTool, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - 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" } }]); - }); - - test("bash -lc argv triple unwraps to the script", async () => { - const { calls, runTool, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - 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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - 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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - runManageTasks: unusedManageTasks, - }); - 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, readRawFile } = makeRecorder(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool, - readRawFile, - allowShell: false, - runManageTasks: unusedManageTasks, - }); - 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, - readRawFile: unusedReadRawFile, - runManageTasks: unusedManageTasks, - }); - const result = await invokeTool(tools, "shell", { command: "ls" }); - expect(result.isError).toBe(true); - expect(result.content).toMatch(/boom/); - }); -}); - -describe("update_plan proxy", () => { - // 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" }), - readRawFile: unusedReadRawFile, - runManageTasks, - }); - 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([ - { - 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, runManageTasks } = makeRealManageTasks(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool: async () => ({ content: "unused" }), - readRawFile: unusedReadRawFile, - runManageTasks, - }); - 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, runManageTasks } = makeRealManageTasks(); - const tools = createCodexToolProxies({ - isCodex: true, - runTool: async () => ({ content: "unused" }), - readRawFile: unusedReadRawFile, - runManageTasks, - }); - const result = await invokeTool(tools, "update_plan", {}); - expect(result.isError).toBe(true); - expect(calls).toEqual([]); - }); -}); - -describe("allowDeleteFromCapabilities", () => { - test("docs allowlist (includes delete_file) → true; build → true", () => { - expect( - allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS }), - ).toBe(true); - 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); - }); -}); - -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 deleted file mode 100644 index 9f0852c8a..000000000 --- a/src/agent/codex-tool-proxies.ts +++ /dev/null @@ -1,484 +0,0 @@ -/** - * 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"; -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"; -import type { ManageTasksRunner, TaskStatus } from "./tasks.js"; - -export type CodexRunTool = ( - name: string, - args: Record, -) => Promise<{ content: string; isError?: boolean }>; - -/** - * Reads a file's raw content (no `cat -n` line-number prefixes) for the - * Update File leg of apply_patch. `read_file` — both the guard plugin and - * the underlying @intx/tools-posix implementation — always numbers its - * output for model display, so it cannot supply the raw text - * `applyUpdateHunks` needs to match a patch's context lines against (CL-6966). - */ -export type CodexReadRawFile = ( - path: string, -) => Promise<{ content: string; isError?: boolean }>; - -/** - * Dispatches update_plan's translated call onto the real manage_tasks - * handler. `manage_tasks` is not a posix tool — it has no handler in the - * 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 interface CreateCodexToolProxiesOpts { - isCodex: boolean; - runTool: CodexRunTool; - /** - * Reads raw file content for apply_patch's Update File leg (CL-6966). Kept - * separate from `runTool` because there is no tool name that returns raw - * content — `read_file` always numbers its output. - */ - readRawFile: CodexReadRawFile; - /** Dispatches update_plan's translated manage_tasks(action="create") call. */ - runManageTasks: ManageTasksRunner; - /** - * When false, Delete File and Update+Move refuse without calling `delete_file`. - * Defaults to true (implement / unconstrained). Pass false when the - * director allowlist omits delete_file (docs leaves mount it today). - */ - 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({ - 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 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; - const allowShell = opts.allowShell !== false; - return [ - createApplyPatchProxy(opts.runTool, opts.readRawFile, allowDelete), - createShellProxy(opts.runTool, allowShell), - createUpdatePlanProxy(opts.runManageTasks), - ]; -} - -/** - * 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"); -} - -/** - * 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, - readRawFile: CodexReadRawFile, - 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, readRawFile, 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, - readRawFile: CodexReadRawFile, - 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}`, - ); - } - - // read_file (both the guard plugin and the underlying tools-posix impl) - // numbers its output for model display, so it cannot supply the raw text - // applyUpdateHunks needs to match context lines against (CL-6966). - // readRawFile reads the file directly instead. - const read = await readRawFile(op.path); - const original = requireOk(read, `read ${op.path}`); - - let updated: string; - 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(`${label} failed: ${result.content}`); - } - return result.content; -} - -// --- shell (Codex's native command-execution tool) --- -// -// Codex's native command-execution tool is named `shell`, not `exec_command`. - -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; - const wrapper = command[0]; - const flag = command[1]; - const script = command[2]; - if ( - command.length === 3 && - wrapper !== undefined && - script !== undefined && - SHELL_WRAPPERS.has(wrapper.replace(/^.*\//, "")) && - (flag === "-lc" || flag === "-c") - ) { - return script; - } - 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(runManageTasks: ManageTasksRunner): 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 manage_tasks 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 runManageTasks({ action: "create", tasks }), - "update_plan", - ); - }, - }); -} diff --git a/src/agent/prompt-sizes.test.ts b/src/agent/prompt-sizes.test.ts index f79f6bdac..30e743687 100644 --- a/src/agent/prompt-sizes.test.ts +++ b/src/agent/prompt-sizes.test.ts @@ -255,8 +255,7 @@ describe("director prompt size budget", () => { expect(new Set(names).size, `${directorId} [${family}]`).toBe( names.length, ); - // No fixture family is Codex, so the Codex proxies - // (createCodexToolProxies returns [] when !isCodex) must be absent, + // Hidden Codex aliases (apply_patch/shell/update_plan) are not advertised, // as must list_dir, which no subagent mount installs. for (const phantom of [ "list_dir", diff --git a/src/agent/worker-contract.test.ts b/src/agent/worker-contract.test.ts index 727b8bc8c..272cbb2b1 100644 --- a/src/agent/worker-contract.test.ts +++ b/src/agent/worker-contract.test.ts @@ -100,7 +100,13 @@ describe("buildWorkerContract", () => { describe("buildWorkerToolNames", () => { test("lists names only, no catalog summaries", () => { const listed = buildWorkerToolNames(["read_file", "ask_director"]); - expect(listed).toBe("Tools (names only): read_file, ask_director"); + expect(listed).toBe("Tools (names only): read, ask_director"); expect(listed).not.toContain("cat/head/tail"); }); + + test("projects mounted engine names onto advertised wire names", () => { + expect( + buildWorkerToolNames(["read_file", "run_shell", "search_files", "grep"]), + ).toBe("Tools (names only): read, bash, glob, grep"); + }); }); diff --git a/src/agent/worker-contract.ts b/src/agent/worker-contract.ts index b2dee5f05..947f64a21 100644 --- a/src/agent/worker-contract.ts +++ b/src/agent/worker-contract.ts @@ -1,4 +1,5 @@ import { PRODUCT_NAME } from "../branding.js"; +import { advertisedToolName } from "./tool-aliases.js"; import { buildSubAgentReportContract } from "./prompts.js"; export interface WorkerContractOptions { @@ -38,5 +39,5 @@ export function buildWorkerContract(opts: WorkerContractOptions = {}): string { * primary chat prompt. */ export function buildWorkerToolNames(toolNames: readonly string[]): string { - return `Tools (names only): ${toolNames.join(", ")}`; + return `Tools (names only): ${toolNames.map(advertisedToolName).join(", ")}`; } diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 13016d3c0..9ab1b3214 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -48,6 +48,7 @@ import { DenialMemory, stableRequestId } from "./denial-memory.js"; import { getSubAgentIdentity } from "../subagent/identity-context.js"; import { PRODUCT_MUTATION_TOOLS } from "../agent/product-mutation-tools.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { prepareDispatchedToolCall } from "../agent/tool-aliases.js"; import { createMcpToolPermissionRegistry, @@ -517,6 +518,20 @@ function withCanonicalToolName(call: ToolCall): ToolCall { return name === call.name ? call : { ...call, name }; } +/** Coerce hidden Codex argv/workdir onto run_shell before policy, not after. */ +function coercePolicyCall(rawCall: ToolCall): ToolCall { + const named = withCanonicalToolName(rawCall); + return prepareDispatchedToolCall(named, named.name); +} + +function callForIdentity(rawCall: ToolCall): ToolCall { + try { + return coercePolicyCall(rawCall); + } catch { + return withCanonicalToolName(rawCall); + } +} + export function createPermissionGate( options: PermissionGateOptions, ): PermissionGate { @@ -693,7 +708,15 @@ export function createPermissionGate( }; const decide = async (rawCall: ToolCall): Promise => { - const call = withCanonicalToolName(rawCall); + let call: ToolCall; + try { + call = coercePolicyCall(rawCall); + } catch (err) { + return { + kind: "deny", + reason: err instanceof Error ? err.message : String(err), + }; + } // Catastrophic shell commands are hard-denied here, at the top of the // single verdict path every entry (evaluate, authorizeCall, // executionVerdict) flows through — this is the owning enforcement point @@ -1067,10 +1090,11 @@ export function createPermissionGate( const authorizeCall = async (call: ToolCall): Promise => { const verdict = mapAuthorizeVerdict(await decide(call)); const identityCwd = getSubAgentIdentity()?.cwd ?? resolvedCwd; + const identityCall = callForIdentity(call); authorizedByCallId.set(call.id, { - name: canonicalToolName(call.name), + name: canonicalToolName(identityCall.name), arguments: identityArguments( - call.arguments, + identityCall.arguments, identityCwd, rootsProvider, trustedPluginRoots, @@ -1085,12 +1109,13 @@ export function createPermissionGate( ): Promise => { const cached = authorizedByCallId.get(call.id); const identityCwd = getSubAgentIdentity()?.cwd ?? resolvedCwd; + const identityCall = callForIdentity(call); if ( cached !== undefined && - cached.name === canonicalToolName(call.name) && + cached.name === canonicalToolName(identityCall.name) && cached.arguments === identityArguments( - call.arguments, + identityCall.arguments, identityCwd, rootsProvider, trustedPluginRoots, diff --git a/src/permission/shell-argv-authorize.test.ts b/src/permission/shell-argv-authorize.test.ts new file mode 100644 index 000000000..7935e94f2 --- /dev/null +++ b/src/permission/shell-argv-authorize.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, test } from "bun:test"; +import type { ToolCall } from "@intx/types/runtime"; + +import { prepareDispatchedToolCall } from "../agent/tool-aliases.js"; +import { gateToolCall } from "../plugins/permission-plugin.js"; +import { BLOCKED_BY_POLICY_PREFIX } from "./decline-markers.js"; +import { createPermissionGate } from "./gate.js"; +import { workerPermissionGate } from "./reactor-authorize.js"; + +const DESTRUCTIVE = "rm -rf node_modules"; + +function hiddenShellArgv(id: string): ToolCall { + return { + id, + name: "shell", + arguments: { command: ["bash", "-lc", DESTRUCTIVE] }, + }; +} + +describe("reactor-gated hidden shell argv coerce-before-authorize", () => { + test("command string[] does not auto-allow; unwrapped script is the ask subject; ask does not execute", async () => { + const gate = createPermissionGate({ + approvals: [], + cwd: process.cwd(), + requestApproval: async () => ({ allow: false }), + interactive: true, + skipPermissions: false, + reactorGated: true, + }); + const call = hiddenShellArgv("shell-argv-1"); + const authorized = await gate.authorizeCall(call); + expect(authorized.effect).not.toBe("allow"); + expect(authorized.effect).toBe("ask"); + if (authorized.effect === "ask") { + expect(authorized.request.subject).toBe(DESTRUCTIVE); + expect(authorized.request.tool).toBe("run_shell"); + } + + const coerced = prepareDispatchedToolCall(call, "run_shell"); + expect(coerced.arguments.command).toBe(DESTRUCTIVE); + const executed = await gate.executionVerdict(coerced); + expect(executed.effect).toBe("ask"); + + const worker = workerPermissionGate(gate); + const workerCall = hiddenShellArgv("shell-argv-worker"); + expect((await worker.authorizeCall(workerCall)).effect).toBe("deny"); + const workerCoerced = prepareDispatchedToolCall(workerCall, "run_shell"); + let nextCalled = false; + const result = await gateToolCall( + worker, + workerCoerced, + new AbortController().signal, + async () => { + nextCalled = true; + return { callId: workerCall.id, content: "ran" }; + }, + ); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain(BLOCKED_BY_POLICY_PREFIX); + expect(nextCalled).toBe(false); + }); +}); diff --git a/src/plugins/secret-guard-symlink.test.ts b/src/plugins/secret-guard-symlink.test.ts index 72f0c3a68..3d0a3e0ca 100644 --- a/src/plugins/secret-guard-symlink.test.ts +++ b/src/plugins/secret-guard-symlink.test.ts @@ -5,7 +5,6 @@ import { join } from "node:path"; import { createPosixTools } from "@intx/tools-posix"; import { createPermissionGate } from "../permission/gate.js"; import { buildCorePosixToolPlugins } from "../agent/posix-tool-plugins.js"; -import { createCodexReadRawFile } from "../agent/codex-read-raw-file.js"; /** * CL-6971: under skip-permissions (yolo), pathEscape absolutizes outside paths @@ -62,7 +61,6 @@ function runner(cwd: string, skipPermissions: boolean) { cwd, }); return { - gate, tools: createPosixTools({ cwd, plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }), @@ -148,33 +146,6 @@ describe("CL-6971 secret-guard realpaths before denylist (symlink floor)", () => expect(await Bun.file(target).text()).toBe(before); }); }); - - test(`${mode}: file symlink → outside .env is blocked for apply_patch raw read`, async () => { - await withFixture(async ({ cwd }) => { - const { gate } = runner(cwd, skipPermissions); - const result = await createCodexReadRawFile(cwd, gate)("config.txt"); - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch( - /sensitive file|escapes working directory/i, - ); - expect(String(result.content)).not.toContain("SECRET=outside-env"); - }); - }); - - test(`${mode}: dir symlink → outside .aws/credentials is blocked for apply_patch raw read`, async () => { - await withFixture(async ({ cwd }) => { - const { gate } = runner(cwd, skipPermissions); - const result = await createCodexReadRawFile( - cwd, - gate, - )("cache/credentials"); - expect(result.isError).toBe(true); - expect(String(result.content)).toMatch( - /sensitive file|escapes working directory/i, - ); - expect(String(result.content)).not.toContain("LEAKED"); - }); - }); } // In-workspace file symlink → .env: pathEscape already realpaths in-bounds, but diff --git a/src/subagent/run-codex-proxy.test.ts b/src/subagent/run-codex-proxy.test.ts deleted file mode 100644 index 158d606f3..000000000 --- a/src/subagent/run-codex-proxy.test.ts +++ /dev/null @@ -1,99 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { createToolRunner } from "@intx/agent"; -import { - createBlobReader, - type ToolCall, - type ToolResult, -} from "@intx/types/runtime"; - -import { createCodexToolProxies } from "../agent/codex-tool-proxies.js"; -import type { ManageTasksRunner } from "../agent/tasks.js"; -import { - MAX_RESULT_CHARS, - truncateToolResultContent, -} from "../plugins/result-truncation-plugin.js"; -import { createCodexProxyRunTool } from "./run.js"; - -function fakeBlobStore() { - const blobs = new Map(); - return { - blobs, - writeBlob: async (key: string, bytes: Uint8Array, contentType: string) => { - blobs.set(key, { bytes, contentType }); - }, - readBlob: async (key: string) => { - const entry = blobs.get(key); - if (entry === undefined) throw new Error(`Blob not found: ${key}`); - return entry.bytes; - }, - }; -} - -const unusedManageTasks: ManageTasksRunner = async () => ({ - content: "unused", -}); - -function extractToolOutputURI(content: unknown): string { - const match = /tool-output:\/\/\/\S+/.exec(String(content)); - if (match === null) - throw new Error(`missing tool-output URI in ${String(content)}`); - return match[0].replace(/[.\]]+$/, ""); -} - -describe("createCodexProxyRunTool", () => { - test("oversized proxied shell calls get distinct recoverable spill URIs", async () => { - const store = fakeBlobStore(); - const outputs: [string, string] = [ - `${"a".repeat(MAX_RESULT_CHARS)}FIRST-TAIL`, - `${"b".repeat(MAX_RESULT_CHARS)}SECOND-TAIL`, - ]; - const seenCallIds: string[] = []; - const posixTools = { - run: async (call: ToolCall): Promise => { - seenCallIds.push(call.id); - const index = seenCallIds.length - 1; - return { - callId: call.id, - content: await truncateToolResultContent( - outputs[index] ?? "", - MAX_RESULT_CHARS, - { - callId: call.id, - writeBlob: store.writeBlob, - }, - ), - }; - }, - }; - - const tools = createCodexToolProxies({ - isCodex: true, - runTool: createCodexProxyRunTool(posixTools), - readRawFile: async () => ({ content: "unused" }), - runManageTasks: unusedManageTasks, - }); - const runner = createToolRunner(tools); - - const first = await runner.run( - { id: "outer-1", name: "shell", arguments: { command: "first" } }, - new AbortController().signal, - ); - const second = await runner.run( - { id: "outer-2", name: "shell", arguments: { command: "second" } }, - new AbortController().signal, - ); - - const firstURI = extractToolOutputURI(first.content); - const secondURI = extractToolOutputURI(second.content); - expect(firstURI).not.toBe(secondURI); - expect(seenCallIds).toEqual(["codex-proxy-1", "codex-proxy-2"]); - - const reader = createBlobReader(store); - expect(new TextDecoder().decode(await reader.read(firstURI))).toBe( - outputs[0], - ); - expect(new TextDecoder().decode(await reader.read(secondURI))).toBe( - outputs[1], - ); - }); -}); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index af13a923a..d87b64aaf 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -28,12 +28,7 @@ import { type } from "arktype"; import { createPosixTools } from "@intx/tools-posix"; import { createDynamicToolRunner } from "../tui/dynamic-tool-runner.js"; import type { ReactorEmittedEvent } from "@intx/inference"; -import type { - BlobReader, - ToolCall, - ToolDefinition, - ToolResult, -} from "@intx/types/runtime"; +import type { BlobReader, ToolDefinition } from "@intx/types/runtime"; import { buildBifrostSource, @@ -57,7 +52,6 @@ import { type SpillBlobWriter, } from "../plugins/result-truncation-plugin.js"; -import { type CodexRunTool } from "../agent/codex-tool-proxies.js"; import { isOpenCodeGoProvider } from "../../packages/opencode-go/src/index.js"; import { createCompositeBlobReader } from "../agent/lazy-blob-reader.js"; @@ -72,7 +66,10 @@ import { } from "./intervention-log.js"; import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; -import { projectToolDefinitions } from "../agent/tool-aliases.js"; +import { + advertisedToolName, + projectToolDefinitions, +} from "../agent/tool-aliases.js"; import { buildCompactionContinuationMessage, @@ -539,30 +536,6 @@ const askDirectorDefinition: ToolDefinition = { }, }; -interface CodexProxyToolRunner { - run(call: ToolCall, signal: AbortSignal): Promise; -} - -export function createCodexProxyRunTool( - posixTools: CodexProxyToolRunner, -): CodexRunTool { - let invocation = 0; - return async (name, args) => { - invocation += 1; - const result = await posixTools.run( - { id: `codex-proxy-${invocation}`, name, arguments: args }, - new AbortController().signal, - ); - return { - content: - typeof result.content === "string" - ? result.content - : JSON.stringify(result.content), - ...(result.isError === true ? { isError: true } : {}), - }; - }; -} - // Spin up an isolated, autonomous agent loop, hand it one task, and return // its final report. `params.cwd` is either the dispatcher's own cwd (shared // mode) or a worktree snapshotted from the dispatcher's last commit @@ -1035,7 +1008,7 @@ async function runSubAgentInner( : []), ...(attachedSection !== undefined ? [attachedSection] : []), ]; - const toolNames = tools.map((t) => t.definition.name); + const toolNames = tools.map((t) => advertisedToolName(t.definition.name)); const systemPrompt = buildSubAgentSystemPrompt( extensions.length > 0 ? extensions : undefined, environment, From 9d6c6b478b142067e02c0adeaa31b8285e13786e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 19:07:38 -0700 Subject: [PATCH 3/4] fix(permissions): stop update_plan grants covering manage_tasks Stored update_plan grants canonicalized onto manage_tasks, so a create-only plan approval auto-allowed full task lifecycle calls. Coverage is now one-directional and seeders drop the narrow key. --- src/agent/canonical-tool-name.ts | 51 ++++++++++++++++-- src/agent/tool-aliases.test.ts | 65 +++++++++++++++++++++++ src/permission/authz-grants.ts | 31 +++++++++-- src/permission/gate.ts | 11 ++-- src/permission/grant-scope.test.ts | 85 ++++++++++++++++++++++++++++++ 5 files changed, 234 insertions(+), 9 deletions(-) diff --git a/src/agent/canonical-tool-name.ts b/src/agent/canonical-tool-name.ts index 2ed49d5d7..643b7b07f 100644 --- a/src/agent/canonical-tool-name.ts +++ b/src/agent/canonical-tool-name.ts @@ -7,14 +7,59 @@ const DEFAULT_PREFIX = "default."; // cache must use the same name so an alias cannot force a second ask/deny. // Wire names (read/bash/…) and hidden aliases (shell/update_plan) collapse onto // the registry engine id so a grant stored as run_shell covers bash. -export function canonicalToolName(requested: string): string { +function baseToolName(requested: string): string { let name = requested; if (name.startsWith(DEFAULT_PREFIX)) { const stripped = name.slice(DEFAULT_PREFIX.length); if (stripped.length > 0) name = stripped; } - name = undoubledName(name) ?? name; - return engineToolName(name); + return undoubledName(name) ?? name; +} + +export function canonicalToolName(requested: string): string { + return engineToolName(baseToolName(requested)); +} + +// Grant coverage across the alias→engine collapse. Comparisons run in native +// key space (both sides resolve onto the engine id first); a raw alias name is +// never compared against a native id. Pure renames +// (read/write/edit/delete/bash/glob, shell) are capability-identical, so a +// grant stored under either name covers the other. update_plan is the +// exception: it hidden-dispatches onto manage_tasks but only ever translates +// to action:"create" with todo/doing/done statuses (see +// translateUpdatePlanArgs), while manage_tasks spans the full lifecycle +// (create/update, including cancelled). Coverage is therefore one-directional: +// a stored manage_tasks (engine) grant covers update_plan use, but a stored +// update_plan grant covers only update_plan-presenting requests — never a +// manage_tasks request, which may carry update/cancel payloads the operator +// never approved. +export function grantToolCovers( + storedTool: string, + requestTool: string, +): boolean { + const storedBase = baseToolName(storedTool); + const requestBase = baseToolName(requestTool); + if (engineToolName(storedBase) !== engineToolName(requestBase)) return false; + if ( + storedBase.toLowerCase() === "update_plan" && + requestBase.toLowerCase() !== "update_plan" + ) { + return false; + } + return true; +} + +// Native key for a stored grant. Pure renames collapse onto the engine id +// (capability-identical, so the collapse is behavior-preserving). update_plan +// has no native key that preserves its narrow create-only capability: +// collapsing it onto manage_tasks would read a stored plan approval as full +// task lifecycle, so it maps to null and the seeder drops it fail-closed. +// Live update_plan use auto-allows and never mints, so no reachable flow +// needs the dropped key. +export function canonicalGrantTool(storedTool: string): string | null { + const base = baseToolName(storedTool); + if (base.toLowerCase() === "update_plan") return null; + return engineToolName(base); } function undoubledName(requested: string): string | undefined { diff --git a/src/agent/tool-aliases.test.ts b/src/agent/tool-aliases.test.ts index 0cb1d53b4..e34e5fc26 100644 --- a/src/agent/tool-aliases.test.ts +++ b/src/agent/tool-aliases.test.ts @@ -101,6 +101,71 @@ describe("grant aliases", () => { ).toBe(true); }); + test("pure renames stay bidirectional: read covers read_file and vice versa", async () => { + expect( + await evaluateApprovals({ + tool: "read_file", + subject: "src/a.ts", + approvals: [{ tool: "read", pattern: "src/*" }], + workspace: noWorkspace, + }), + ).toBe(true); + expect( + await evaluateApprovals({ + tool: "read", + subject: "src/a.ts", + approvals: [{ tool: "read_file", pattern: "src/*" }], + workspace: noWorkspace, + }), + ).toBe(true); + }); + + // update_plan hidden-dispatches onto manage_tasks but is NOT + // capability-identical: it only ever translates to action:"create" with + // todo/doing/done statuses, while manage_tasks spans the full lifecycle + // (create/update, including cancelled). Grant coverage is one-directional: + // a stored update_plan grant must never cover a manage_tasks request. + test("a stored update_plan grant does not cover manage_tasks", async () => { + expect( + await evaluateApprovals({ + tool: "manage_tasks", + subject: "manage_tasks", + approvals: [{ tool: "update_plan", pattern: "*" }], + workspace: noWorkspace, + }), + ).toBe(false); + }); + + test("a stored update_plan grant still covers update_plan (create-equivalent) use", async () => { + expect( + await evaluateApprovals({ + tool: "update_plan", + subject: "update_plan", + approvals: [{ tool: "update_plan", pattern: "*" }], + workspace: noWorkspace, + }), + ).toBe(true); + }); + + test("a stored manage_tasks grant covers update_plan (engine covers alias)", async () => { + expect( + await evaluateApprovals({ + tool: "update_plan", + subject: "update_plan", + approvals: [{ tool: "manage_tasks", pattern: "*" }], + workspace: noWorkspace, + }), + ).toBe(true); + expect( + await evaluateApprovals({ + tool: "manage_tasks", + subject: "manage_tasks", + approvals: [{ tool: "manage_tasks", pattern: "*" }], + workspace: noWorkspace, + }), + ).toBe(true); + }); + test("canonicalToolName maps wire and hidden aliases onto engines", () => { expect(canonicalToolName("bash")).toBe("run_shell"); expect(canonicalToolName("shell")).toBe("run_shell"); diff --git a/src/permission/authz-grants.ts b/src/permission/authz-grants.ts index 1fc9e69a3..6c8bcef51 100644 --- a/src/permission/authz-grants.ts +++ b/src/permission/authz-grants.ts @@ -1,6 +1,10 @@ import { evaluateGrants, type GrantRule } from "@intx/authz"; -import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { + canonicalGrantTool, + canonicalToolName, + grantToolCovers, +} from "../agent/canonical-tool-name.js"; import type { Approval } from "./types.js"; import { matchesPattern } from "./matcher.js"; import { realpathOr } from "./worktree-roots.js"; @@ -68,6 +72,22 @@ export function cwdMatchesGrant( return workspace.roots.includes(realpathOr(requestCwd)); } +// Seeded grants enter the gate in native key space: pure renames collapse +// onto the engine id, and narrow update_plan keys (which no native key can +// represent without overclaiming capability) are dropped fail-closed. Fresh +// array; the gate owns it. +export function normalizeSeededApprovals( + seeded: readonly Approval[], +): Approval[] { + const out: Approval[] = []; + for (const approval of seeded) { + const tool = canonicalGrantTool(approval.tool); + if (tool === null) continue; + out.push(tool === approval.tool ? approval : { ...approval, tool }); + } + return out; +} + // The single place that decides whether a grant's tool/providerModel/cwd // scope covers a request, independent of whether the grant's pattern matches // the request's subject. Every live call site that needs to know "does this @@ -82,7 +102,7 @@ export function grantScopeMatches( workspace: GrantWorkspace, ): boolean { return ( - canonicalToolName(approval.tool) === canonicalToolName(tool) && + grantToolCovers(approval.tool, tool) && (approval.providerModel === undefined || approval.providerModel === activeProviderModel) && cwdMatchesGrant(approval.cwd, requestCwd, workspace) @@ -117,8 +137,13 @@ export async function approvalCoversSubject( workspace, } = input; const action = canonicalToolName(tool); + // Scope-matching sees the raw request name: grantToolCovers is directional + // for the update_plan/manage_tasks pair (a stored update_plan grant covers + // only update_plan-presenting requests), so pre-canonicalizing here would + // erase the alias and wrongly deny same-alias replay. The @intx/authz call + // below still uses the canonical action on both sides. const scoped = approvals.filter((a) => - grantScopeMatches(a, action, activeProviderModel, requestCwd, workspace), + grantScopeMatches(a, tool, activeProviderModel, requestCwd, workspace), ); if (scoped.length === 0) return false; diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 9ab1b3214..f44a28ccc 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -30,6 +30,7 @@ import { matchesPattern, escapeGlobLiteral } from "./matcher.js"; import { approvalCoversSubject, grantScopeMatches, + normalizeSeededApprovals, type GrantWorkspace, } from "./authz-grants.js"; import { @@ -559,7 +560,9 @@ export function createPermissionGate( let auto = options.auto; let skipPermissions = options.skipPermissions; // Own a private copy so evaluating a grant never mutates the caller's array. - const approvals: Approval[] = [...options.approvals]; + // Seeded grants enter in native key space (pure renames collapsed, narrow + // update_plan keys dropped fail-closed). + const approvals: Approval[] = normalizeSeededApprovals(options.approvals); let activeProviderModel = providerName !== undefined && model !== undefined ? `${providerName}:${model}` @@ -572,7 +575,9 @@ export function createPermissionGate( // scope-appropriate home: session grants stay in memory, everything else is // persisted. Both approval branches must mint identically — this is the // single place a grant comes into existence. - const mintGrant = (tool: string, outcome: ApprovalOutcome): void => { + const mintGrant = (requestedTool: string, outcome: ApprovalOutcome): void => { + // Mint in native key space; live requests are already post-coercion. + const tool = canonicalToolName(requestedTool); if (!outcome.persist || outcome.persist.pattern === null) return; const grant: GrantScope = outcome.persist.grant ?? "session"; // A run_shell pattern may still carry a model-authored comment line (the @@ -1205,7 +1210,7 @@ export function createPermissionGate( const setSeededApprovals = (seeded: readonly Approval[]): void => { approvals.length = 0; - approvals.push(...seeded, ...sessionGrants); + approvals.push(...normalizeSeededApprovals(seeded), ...sessionGrants); // Re-seeded approvals can cover previously-denied requests — cached // denies must re-evaluate instead of serving stale reasons. denialMemory.clear(); diff --git a/src/permission/grant-scope.test.ts b/src/permission/grant-scope.test.ts index d1c941357..40b20e866 100644 --- a/src/permission/grant-scope.test.ts +++ b/src/permission/grant-scope.test.ts @@ -5,6 +5,7 @@ import { approvalCoversSubject, evaluateApprovals, grantScopeMatches, + normalizeSeededApprovals, cwdMatchesGrant, type GrantWorkspace, } from "./authz-grants.js"; @@ -27,6 +28,8 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => { tool: "run_shell", pattern: "npm test", providerModel: "openai:gpt-5" }, { tool: "run_shell", pattern: "npm test", cwd: "/proj" }, { tool: "write_file", pattern: "npm test" }, + { tool: "update_plan", pattern: "npm test" }, + { tool: "manage_tasks", pattern: "npm test" }, ]; const requests: { @@ -39,6 +42,8 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => { tool: "run_shell", cwd: "/other", activeProviderModel: "openai:gpt-5" }, { tool: "run_shell", cwd: undefined, activeProviderModel: undefined }, { tool: "write_file", cwd: "/proj", activeProviderModel: undefined }, + { tool: "manage_tasks", cwd: "/proj", activeProviderModel: undefined }, + { tool: "update_plan", cwd: "/proj", activeProviderModel: undefined }, ]; for (const grant of grants) { @@ -85,6 +90,86 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => }); } } + + test("stored update_plan grant never covers a manage_tasks request", async () => { + const grant: Approval = { tool: "update_plan", pattern: "*" }; + expect( + grantScopeMatches(grant, "manage_tasks", undefined, "/proj", workspace), + ).toBe(false); + expect( + await approvalCoversSubject({ + tool: "manage_tasks", + subject: "manage_tasks", + approvals: [grant], + activeProviderModel: undefined, + requestCwd: "/proj", + workspace, + }), + ).toBe(false); + }); + + test("stored manage_tasks grant covers an update_plan-presenting request", async () => { + const grant: Approval = { tool: "manage_tasks", pattern: "*" }; + expect( + grantScopeMatches(grant, "update_plan", undefined, "/proj", workspace), + ).toBe(true); + expect( + await approvalCoversSubject({ + tool: "update_plan", + subject: "update_plan", + approvals: [grant], + activeProviderModel: undefined, + requestCwd: "/proj", + workspace, + }), + ).toBe(true); + }); +}); + +// Seeded grants enter the gate in native key space. Pure renames collapse onto +// the engine id (capability-identical, behavior-preserving); a stored +// update_plan key has no native representation that preserves its narrow +// create-only capability, so it is dropped fail-closed instead of widening +// onto manage_tasks. +describe("normalizeSeededApprovals", () => { + test("collapses pure renames onto the engine id, preserving other fields", () => { + expect( + normalizeSeededApprovals([ + { + tool: "bash", + pattern: "npm test", + providerModel: "openai:gpt-5", + cwd: "/proj", + }, + { tool: "read", pattern: "README.md" }, + ]), + ).toEqual([ + { + tool: "run_shell", + pattern: "npm test", + providerModel: "openai:gpt-5", + cwd: "/proj", + }, + { tool: "read_file", pattern: "README.md" }, + ]); + }); + + test("keeps native keys as-is", () => { + const seeded: Approval[] = [ + { tool: "run_shell", pattern: "*" }, + { tool: "manage_tasks", pattern: "*" }, + ]; + expect(normalizeSeededApprovals(seeded)).toEqual(seeded); + }); + + test("drops update_plan keys fail-closed", () => { + expect( + normalizeSeededApprovals([ + { tool: "update_plan", pattern: "*" }, + { tool: "manage_tasks", pattern: "*" }, + ]), + ).toEqual([{ tool: "manage_tasks", pattern: "*" }]); + }); }); // Grant minting decomposes a multi-segment chain scope into one grant per From 93ed58a9f3125e203d5a99f019b7a245340deb84 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 19:12:48 -0700 Subject: [PATCH 4/4] test(permissions): drop alias-presenting grant tests Live requests never present as aliases (coerced before matching), so tests presenting update_plan requests exercise an unreachable path. Fail-closed pins and seeder tests remain. --- src/agent/canonical-tool-name.ts | 4 +++- src/agent/tool-aliases.test.ts | 24 ++++-------------------- src/permission/grant-scope.test.ts | 18 ------------------ 3 files changed, 7 insertions(+), 39 deletions(-) diff --git a/src/agent/canonical-tool-name.ts b/src/agent/canonical-tool-name.ts index 643b7b07f..143591ae5 100644 --- a/src/agent/canonical-tool-name.ts +++ b/src/agent/canonical-tool-name.ts @@ -32,7 +32,9 @@ export function canonicalToolName(requested: string): string { // a stored manage_tasks (engine) grant covers update_plan use, but a stored // update_plan grant covers only update_plan-presenting requests — never a // manage_tasks request, which may carry update/cancel payloads the operator -// never approved. +// never approved. The update_plan-presenting allowance is unreachable live +// (requests are post-coercion and seeders drop stored update_plan keys); it +// exists only so direct match-API callers keep narrow-narrow coverage. export function grantToolCovers( storedTool: string, requestTool: string, diff --git a/src/agent/tool-aliases.test.ts b/src/agent/tool-aliases.test.ts index e34e5fc26..edef87ff0 100644 --- a/src/agent/tool-aliases.test.ts +++ b/src/agent/tool-aliases.test.ts @@ -125,6 +125,9 @@ describe("grant aliases", () => { // todo/doing/done statuses, while manage_tasks spans the full lifecycle // (create/update, including cancelled). Grant coverage is one-directional: // a stored update_plan grant must never cover a manage_tasks request. + // Live requests never present as aliases (coerced before matching) and + // seeders drop stored update_plan keys, so no same-alias replay test exists + // here: that path is unreachable in production. test("a stored update_plan grant does not cover manage_tasks", async () => { expect( await evaluateApprovals({ @@ -136,26 +139,7 @@ describe("grant aliases", () => { ).toBe(false); }); - test("a stored update_plan grant still covers update_plan (create-equivalent) use", async () => { - expect( - await evaluateApprovals({ - tool: "update_plan", - subject: "update_plan", - approvals: [{ tool: "update_plan", pattern: "*" }], - workspace: noWorkspace, - }), - ).toBe(true); - }); - - test("a stored manage_tasks grant covers update_plan (engine covers alias)", async () => { - expect( - await evaluateApprovals({ - tool: "update_plan", - subject: "update_plan", - approvals: [{ tool: "manage_tasks", pattern: "*" }], - workspace: noWorkspace, - }), - ).toBe(true); + test("a stored manage_tasks grant covers manage_tasks", async () => { expect( await evaluateApprovals({ tool: "manage_tasks", diff --git a/src/permission/grant-scope.test.ts b/src/permission/grant-scope.test.ts index 40b20e866..69b1edfce 100644 --- a/src/permission/grant-scope.test.ts +++ b/src/permission/grant-scope.test.ts @@ -43,7 +43,6 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => { tool: "run_shell", cwd: undefined, activeProviderModel: undefined }, { tool: "write_file", cwd: "/proj", activeProviderModel: undefined }, { tool: "manage_tasks", cwd: "/proj", activeProviderModel: undefined }, - { tool: "update_plan", cwd: "/proj", activeProviderModel: undefined }, ]; for (const grant of grants) { @@ -107,23 +106,6 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => }), ).toBe(false); }); - - test("stored manage_tasks grant covers an update_plan-presenting request", async () => { - const grant: Approval = { tool: "manage_tasks", pattern: "*" }; - expect( - grantScopeMatches(grant, "update_plan", undefined, "/proj", workspace), - ).toBe(true); - expect( - await approvalCoversSubject({ - tool: "update_plan", - subject: "update_plan", - approvals: [grant], - activeProviderModel: undefined, - requestCwd: "/proj", - workspace, - }), - ).toBe(true); - }); }); // Seeded grants enter the gate in native key space. Pure renames collapse onto