From 1e2649e1695438b4e497878b3db6f62408f389d3 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 24 Aug 2026 11:54:45 -0700 Subject: [PATCH 1/2] Mount write tools on every closed director Compose PRODUCT_WRITE_TOOLS onto review, orchestrator, intern, and docs surfaces so lane discipline stays in prompts instead of tool stripping. Closes CL-7014 --- src/agent/codex-tool-proxies.test.ts | 4 +- src/agent/codex-tool-proxies.ts | 4 +- .../directors/brand-reviewer/package.test.ts | 1 + .../directors/bruckheimer/package.test.ts | 1 + src/agent/directors/critique/package.test.ts | 8 +-- src/agent/directors/draper/package.test.ts | 8 +-- src/agent/directors/emil/package.test.ts | 8 +-- src/agent/directors/explore/package.test.ts | 8 +-- src/agent/directors/explore/package.ts | 4 +- src/agent/directors/gaasbot/package.test.ts | 8 +-- src/agent/directors/greybeard/package.test.ts | 8 +-- src/agent/directors/intern/package.test.ts | 7 ++- src/agent/directors/intern/package.ts | 2 +- src/agent/directors/neckbeard/package.test.ts | 8 +-- src/agent/directors/plan/package.test.ts | 8 +-- src/agent/directors/registry.test.ts | 26 +++++++--- .../directors/shakespeare/package.test.ts | 1 + src/agent/directors/tester/package.test.ts | 8 +-- src/agent/directors/tester/package.ts | 4 +- src/agent/directors/testsmith/package.test.ts | 8 +-- src/agent/directors/testsmith/package.ts | 6 +-- src/agent/directors/tool-sets.test.ts | 52 +++++++++++++++++-- src/agent/directors/tool-sets.ts | 49 ++++++++++------- tests/unit/subagent.test.ts | 6 +-- 24 files changed, 157 insertions(+), 90 deletions(-) diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 002c63222..525b9393e 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -597,8 +597,8 @@ describe("update_plan proxy", () => { }); describe("allowDeleteFromCapabilities", () => { - test("docs allowlist (no delete_file) → false; build → true", () => { - expect(allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(false); + 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); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index 14716d2a4..56451072f 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -55,8 +55,8 @@ export interface CreateCodexToolProxiesOpts { runManageTasks: CodexRunManageTasks; /** * When false, Delete File and Update+Move refuse without calling `delete_file`. - * Defaults to true (implement / unconstrained). Docs leaves pass false because - * DOCS_TOOLS includes apply_patch but not delete_file. + * Defaults to true (implement / unconstrained). Pass false when the + * director allowlist omits delete_file (docs leaves mount it today). */ allowDelete?: boolean; /** diff --git a/src/agent/directors/brand-reviewer/package.test.ts b/src/agent/directors/brand-reviewer/package.test.ts index 8c6dae28d..3a8d1200c 100644 --- a/src/agent/directors/brand-reviewer/package.test.ts +++ b/src/agent/directors/brand-reviewer/package.test.ts @@ -73,6 +73,7 @@ describe("brandReviewerPackage", () => { const allow = brandReviewerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("systemPrompt mentions DESIGN.md", () => { diff --git a/src/agent/directors/bruckheimer/package.test.ts b/src/agent/directors/bruckheimer/package.test.ts index 7e9d6f4e1..1056d5b85 100644 --- a/src/agent/directors/bruckheimer/package.test.ts +++ b/src/agent/directors/bruckheimer/package.test.ts @@ -26,6 +26,7 @@ describe("bruckheimerPackage", () => { const allow = bruckheimerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is docs", () => { diff --git a/src/agent/directors/critique/package.test.ts b/src/agent/directors/critique/package.test.ts index cfe4e6dc2..c57864f86 100644 --- a/src/agent/directors/critique/package.test.ts +++ b/src/agent/directors/critique/package.test.ts @@ -84,14 +84,14 @@ describe("critiquePackage", () => { expect(critiquePackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = critiquePackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).toContain("read_file"); expect(allow).not.toContain("use_skill"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/draper/package.test.ts b/src/agent/directors/draper/package.test.ts index 528244e85..0dabcd4b4 100644 --- a/src/agent/directors/draper/package.test.ts +++ b/src/agent/directors/draper/package.test.ts @@ -76,13 +76,13 @@ describe("draperPackage", () => { expect(draperPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = draperPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).not.toContain("use_skill"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/emil/package.test.ts b/src/agent/directors/emil/package.test.ts index 665983797..8d6d2523b 100644 --- a/src/agent/directors/emil/package.test.ts +++ b/src/agent/directors/emil/package.test.ts @@ -73,13 +73,13 @@ describe("emilPackage", () => { expect(emilPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = emilPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).not.toContain("use_skill"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/explore/package.test.ts b/src/agent/directors/explore/package.test.ts index 3139e29ea..dd43a433b 100644 --- a/src/agent/directors/explore/package.test.ts +++ b/src/agent/directors/explore/package.test.ts @@ -82,13 +82,13 @@ describe("explorePackage", () => { expect(explorePackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: no product edits)", () => { const allow = explorePackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).toContain("grep"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is explore", () => { diff --git a/src/agent/directors/explore/package.ts b/src/agent/directors/explore/package.ts index 18eebf6fa..be9c0e09d 100644 --- a/src/agent/directors/explore/package.ts +++ b/src/agent/directors/explore/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; /** * Explorer leaf (CL-7020). @@ -33,7 +33,7 @@ FINDINGS SHAPE: Findings must be a scannable map — key paths, symbols, call fl FINISH BIAS: Prefer one thorough pass then report. Expand Findings, change approach, or write the final report — do not keep re-reading the same paths. OUT OF LANE: product writes, drive-by fixes, shipping features, review severity theater, orchestration, spawning specialists, fleet discovery, becoming Builder/Critic/orchestrator as primary.`, - tools: { allow: READ_TOOLS }, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "explore", diff --git a/src/agent/directors/gaasbot/package.test.ts b/src/agent/directors/gaasbot/package.test.ts index d65fffee0..128f844b6 100644 --- a/src/agent/directors/gaasbot/package.test.ts +++ b/src/agent/directors/gaasbot/package.test.ts @@ -67,12 +67,12 @@ describe("gaasbotPackage", () => { expect(gaasbotPackage.spawn.maySpawn).toBe(false); }); - test("denies product write tools (advice only)", () => { + test("mounts product write tools (lane discipline in prompts)", () => { const allow = gaasbotPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is plan", () => { diff --git a/src/agent/directors/greybeard/package.test.ts b/src/agent/directors/greybeard/package.test.ts index 8c6e3de6a..25e7e4b4d 100644 --- a/src/agent/directors/greybeard/package.test.ts +++ b/src/agent/directors/greybeard/package.test.ts @@ -88,13 +88,13 @@ describe("greybeardPackage", () => { expect(allow).not.toContain("plan"); }); - test("tools.allow is orchestrator surface without product writes", () => { + test("tools.allow is orchestrator surface with product writes", () => { const allow = greybeardPackage.tools?.allow ?? []; expect(allow).toContain("task"); expect(allow).toContain("search_agents"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/intern/package.test.ts b/src/agent/directors/intern/package.test.ts index c2921de4d..db899154c 100644 --- a/src/agent/directors/intern/package.test.ts +++ b/src/agent/directors/intern/package.test.ts @@ -17,12 +17,15 @@ describe("internPackage", () => { expect(internPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is shell-first minimal surface", () => { + test("tools.allow is shell-first with path writes", () => { const allow = internPackage.tools?.allow ?? []; expect(allow).toContain("run_shell"); expect(allow).toContain("read_file"); expect(allow).toContain("list_dir"); - for (const name of ["write_file", "edit_file", "delete_file", "grep", "search_files", "task"]) { + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); + for (const name of ["grep", "search_files", "task", "apply_patch"]) { expect(allow).not.toContain(name); } }); diff --git a/src/agent/directors/intern/package.ts b/src/agent/directors/intern/package.ts index c247909c2..fa9e7b1ff 100644 --- a/src/agent/directors/intern/package.ts +++ b/src/agent/directors/intern/package.ts @@ -3,7 +3,7 @@ import { INTERN_TOOLS } from "../tool-sets.js"; /** * Mechanical intern leaf (CL-5822). - * Shell/commands only — no judgment, no exploration, no product writes. + * Shell/commands first — no judgment, no exploration; path writes only when the brief requires them. */ export const internPackage: DirectorPackage = { id: "intern", diff --git a/src/agent/directors/neckbeard/package.test.ts b/src/agent/directors/neckbeard/package.test.ts index 20bc1cdce..9a9bf5889 100644 --- a/src/agent/directors/neckbeard/package.test.ts +++ b/src/agent/directors/neckbeard/package.test.ts @@ -25,12 +25,12 @@ describe("neckbeardPackage", () => { expect(neckbeardPackage.spawn.maySpawn).toBe(false); }); - test("denies product write tools", () => { + test("mounts product write tools", () => { const allow = neckbeardPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/plan/package.test.ts b/src/agent/directors/plan/package.test.ts index b78fed582..7402f6a1b 100644 --- a/src/agent/directors/plan/package.test.ts +++ b/src/agent/directors/plan/package.test.ts @@ -66,12 +66,12 @@ describe("planPackage", () => { expect(planPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = planPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is plan", () => { diff --git a/src/agent/directors/registry.test.ts b/src/agent/directors/registry.test.ts index be552635a..c3d2081d3 100644 --- a/src/agent/directors/registry.test.ts +++ b/src/agent/directors/registry.test.ts @@ -97,7 +97,9 @@ describe("director registry", () => { expect(explore.description).toContain("agent id: explore"); expect(explore.capabilities?.mode).toBe("allow"); expect(explore.capabilities?.tools).toContain("read_file"); - expect(explore.capabilities?.tools).not.toContain("write_file"); + expect(explore.capabilities?.tools).toContain("write_file"); + expect(explore.capabilities?.tools).toContain("edit_file"); + expect(explore.capabilities?.tools).toContain("delete_file"); expect(explore.orchestrator).toBe(false); const grey = packageToProfile(DIRECTOR_REGISTRY.greybeard); @@ -123,7 +125,7 @@ describe("director registry", () => { expect(packageToProfile(g).orchestrator).toBe(true); }); - test("review and design leaves deny product write tools", () => { + test("closed directors mount product write tools", () => { for (const id of [ "critique", "greybeard", @@ -135,22 +137,30 @@ describe("director registry", () => { "testsmith", "tester", "gaasbot", + "intern", + "build", + "shakespeare", + "bruckheimer", + "brand-reviewer", + "skywalker", ] as const) { const allow = DIRECTOR_REGISTRY[id].tools?.allow ?? []; - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); } }); - test("build mounts product writes; intern is shell-only; other leaves do not spawn", () => { + test("build mounts product writes + apply_patch; intern mounts writes without apply_patch; other leaves do not spawn", () => { expect(DIRECTOR_REGISTRY.build.tools?.allow).toEqual( expect.arrayContaining(["write_file", "edit_file", "delete_file", "apply_patch"]), ); const internAllow = DIRECTOR_REGISTRY.intern.tools?.allow ?? []; expect(internAllow).toContain("run_shell"); - expect(internAllow).not.toContain("write_file"); - expect(internAllow).not.toContain("edit_file"); + expect(internAllow).toContain("write_file"); + expect(internAllow).toContain("edit_file"); + expect(internAllow).toContain("delete_file"); + expect(internAllow).not.toContain("apply_patch"); for (const id of DIRECTOR_IDS) { if (id === "skywalker" || id === "greybeard") continue; expect(DIRECTOR_REGISTRY[id].spawn.maySpawn).toBe(false); diff --git a/src/agent/directors/shakespeare/package.test.ts b/src/agent/directors/shakespeare/package.test.ts index 6855b0b31..df2482a72 100644 --- a/src/agent/directors/shakespeare/package.test.ts +++ b/src/agent/directors/shakespeare/package.test.ts @@ -76,6 +76,7 @@ describe("shakespearePackage", () => { const allow = shakespearePackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is docs", () => { diff --git a/src/agent/directors/tester/package.test.ts b/src/agent/directors/tester/package.test.ts index 42cc41d38..e92fcabb6 100644 --- a/src/agent/directors/tester/package.test.ts +++ b/src/agent/directors/tester/package.test.ts @@ -60,13 +60,13 @@ describe("testerPackage", () => { expect(testerPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: never fix)", () => { const allow = testerPackage.tools?.allow ?? []; expect(allow).toContain("run_shell"); expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is test", () => { diff --git a/src/agent/directors/tester/package.ts b/src/agent/directors/tester/package.ts index 8cc8c07af..4cd431ceb 100644 --- a/src/agent/directors/tester/package.ts +++ b/src/agent/directors/tester/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; /** * Tester leaf (CL-7026). @@ -34,7 +34,7 @@ DONE GATE: Stop when the brief's verify ask is answered with evidence OR explici REPORT MAP: Findings must map each requested check → pass | fail | blocked, with commands run and key failure excerpts. Paths list suites/files exercised. OUT OF LANE: fixing product code, "just quickly" fixing, redesigning the suite as Testsmith's primary job, fleet orchestration, architecture essays, exploration maps as primary.`, - tools: { allow: READ_TOOLS }, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "test", diff --git a/src/agent/directors/testsmith/package.test.ts b/src/agent/directors/testsmith/package.test.ts index eedb84ba0..ab37e84c9 100644 --- a/src/agent/directors/testsmith/package.test.ts +++ b/src/agent/directors/testsmith/package.test.ts @@ -21,12 +21,12 @@ describe("testsmithPackage", () => { expect(testsmithPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: design only)", () => { const allow = testsmithPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is test", () => { diff --git a/src/agent/directors/testsmith/package.ts b/src/agent/directors/testsmith/package.ts index 3f13c63be..6783f7554 100644 --- a/src/agent/directors/testsmith/package.ts +++ b/src/agent/directors/testsmith/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; /** * Testsmith: test design specialist — strategy and cases only; never implements product @@ -29,8 +29,8 @@ Design in the report. Prefer: OUT OF LANE: fixing production code, becoming the implementer, running the full verify-and-fix loop, fleet orchestration. -Read and search the codebase to ground the design; you have no product-mutation tools.`, - tools: { allow: READ_TOOLS }, +Read and search the codebase to ground the design; do not mutate product code.`, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "test", diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index 4deae1f3f..2e73f12ae 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -3,14 +3,31 @@ import { DOCS_TOOLS, BUILD_TOOLS, ORCHESTRATOR_TOOLS, + PRODUCT_WRITE_TOOLS, READ_TOOLS, + REVIEW_TOOLS, + INTERN_TOOLS, SKYWALKER_TOOLS, } from "./tool-sets.js"; +describe("PRODUCT_WRITE_TOOLS", () => { + test("is write_file / edit_file / delete_file", () => { + expect([...PRODUCT_WRITE_TOOLS]).toEqual(["write_file", "edit_file", "delete_file"]); + }); +}); + +describe("READ_TOOLS", () => { + test("stays read-only (no path mutation)", () => { + for (const name of PRODUCT_WRITE_TOOLS) { + expect(READ_TOOLS as readonly string[]).not.toContain(name); + } + }); +}); + describe("DOCS_TOOLS", () => { - test("excludes run_shell and delete_file as envelope policy", () => { + test("excludes run_shell as envelope policy; includes delete_file", () => { expect(DOCS_TOOLS).not.toContain("run_shell"); - expect(DOCS_TOOLS).not.toContain("delete_file"); + expect(DOCS_TOOLS).toContain("delete_file"); }); test("keeps read/search/lsp/web + file writes + apply_patch", () => { @@ -24,6 +41,7 @@ describe("DOCS_TOOLS", () => { "web_search", "write_file", "edit_file", + "delete_file", "apply_patch", ]; for (const tool of expected) { @@ -44,16 +62,34 @@ describe("DOCS_TOOLS", () => { }); describe("SKYWALKER_TOOLS / ORCHESTRATOR_TOOLS", () => { - test("Skywalker mounts product writes; greybeard orchestrator surface does not", () => { - for (const name of ["write_file", "edit_file", "delete_file"] as const) { + test("both mount product writes and task", () => { + for (const name of PRODUCT_WRITE_TOOLS) { expect(SKYWALKER_TOOLS as readonly string[]).toContain(name); - expect(ORCHESTRATOR_TOOLS as readonly string[]).not.toContain(name); + expect(ORCHESTRATOR_TOOLS as readonly string[]).toContain(name); } expect(SKYWALKER_TOOLS).toContain("task"); expect(ORCHESTRATOR_TOOLS).toContain("task"); }); }); +describe("REVIEW_TOOLS / INTERN_TOOLS", () => { + test("compose PRODUCT_WRITE_TOOLS", () => { + for (const name of PRODUCT_WRITE_TOOLS) { + expect(REVIEW_TOOLS as readonly string[]).toContain(name); + expect(INTERN_TOOLS as readonly string[]).toContain(name); + } + }); + + test("intern stays shell-first without grep/search/task", () => { + expect(INTERN_TOOLS).toContain("run_shell"); + expect(INTERN_TOOLS).toContain("read_file"); + expect(INTERN_TOOLS).toContain("list_dir"); + for (const name of ["grep", "search_files", "task"] as const) { + expect(INTERN_TOOLS as readonly string[]).not.toContain(name); + } + }); +}); + describe("BUILD_TOOLS", () => { test("includes apply_patch alongside path mutation tools", () => { expect(BUILD_TOOLS).toContain("write_file"); @@ -66,4 +102,10 @@ describe("BUILD_TOOLS", () => { expect(BUILD_TOOLS).toContain("shell"); expect(BUILD_TOOLS).toContain("update_plan"); }); + + test("review/orchestrator/intern do not mount 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 9bf82dfbe..213d8d7f7 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -16,6 +16,13 @@ export const READ_TOOLS = [ "web_search", ] 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). + */ +export const PRODUCT_WRITE_TOOLS = ["write_file", "edit_file", "delete_file"] as const; + /** * Build: read + full file mutation. `shell` and `update_plan` are Codex * proxy names (createCodexToolProxies) for `run_shell` / the plan tool; both @@ -24,46 +31,48 @@ export const READ_TOOLS = [ */ export const BUILD_TOOLS = [ ...READ_TOOLS, - "write_file", - "edit_file", - "delete_file", + ...PRODUCT_WRITE_TOOLS, "apply_patch", "shell", "update_plan", ] as const; /** - * Docs leaves: read/search/lsp/web + file writes — no run_shell, no delete_file. + * Docs leaves: read/search/lsp/web + file writes — no run_shell. * Envelope policy only: docs leaves omit shell so they cannot mutate via the * 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; only the write tools are added explicitly. `apply_patch` is + * automatically; path writes come from PRODUCT_WRITE_TOOLS. `apply_patch` is * included so Codex docs leaves keep the proxy after the capability filter. * `update_plan` is included for the same reason (its proxy has no `run_shell` * dependency, so it is not excluded alongside `shell`). */ export const DOCS_TOOLS = [ ...READ_TOOLS.filter((t) => t !== "run_shell"), - "write_file", - "edit_file", + ...PRODUCT_WRITE_TOOLS, "apply_patch", "update_plan", ] as const; -/** Review / counsel: read surface, no writes. */ -export const REVIEW_TOOLS = [...READ_TOOLS] as const; +/** Review / counsel: read surface + path writes (lane discipline in prompts). */ +export const REVIEW_TOOLS = [...READ_TOOLS, ...PRODUCT_WRITE_TOOLS] as const; -/** Mechanical intern: shell-first, minimal surface. */ -export const INTERN_TOOLS = ["run_shell", "read_file", "list_dir"] as const; - -/** Nested orchestrator surface (greybeard / package filter): dispatch only. */ -export const ORCHESTRATOR_TOOLS = [...READ_TOOLS, "search_agents", "task"] as const; +/** Mechanical intern: shell-first + path writes when the brief requires them. */ +export const INTERN_TOOLS = [ + "run_shell", + "read_file", + "list_dir", + ...PRODUCT_WRITE_TOOLS, +] as const; -/** Skywalker primary: orchestrator surface plus product writes for DIY tiny work. */ -export const SKYWALKER_TOOLS = [ - ...ORCHESTRATOR_TOOLS, - "write_file", - "edit_file", - "delete_file", +/** Nested orchestrator surface (greybeard / package filter): dispatch + path writes. */ +export const ORCHESTRATOR_TOOLS = [ + ...READ_TOOLS, + ...PRODUCT_WRITE_TOOLS, + "search_agents", + "task", ] as const; + +/** Skywalker primary: orchestrator surface (writes already composed). */ +export const SKYWALKER_TOOLS = [...ORCHESTRATOR_TOOLS] as const; diff --git a/tests/unit/subagent.test.ts b/tests/unit/subagent.test.ts index a78df4283..e1d624efa 100644 --- a/tests/unit/subagent.test.ts +++ b/tests/unit/subagent.test.ts @@ -283,9 +283,9 @@ test("intent maps to closed director without profiles", async () => { expect(received?.systemPromptRole).toContain("PRIMARY INTENT"); expect(received?.capabilities?.mode).toBe("allow"); expect(received?.capabilities?.tools).toContain("read_file"); - expect(received?.capabilities?.tools).not.toContain("write_file"); - expect(received?.capabilities?.tools).not.toContain("edit_file"); - expect(received?.capabilities?.tools).not.toContain("delete_file"); + expect(received?.capabilities?.tools).toContain("write_file"); + expect(received?.capabilities?.tools).toContain("edit_file"); + expect(received?.capabilities?.tools).toContain("delete_file"); }); test("intent general is refused (no general director)", async () => { From d8c6200c64b91bf38d5a6a55c149c3b04e13fafe Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 24 Aug 2026 15:31:10 -0700 Subject: [PATCH 2/2] Fix prettier formatting in director tool-sets --- src/agent/directors/tool-sets.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index 213d8d7f7..7cd5b1a4d 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -59,12 +59,7 @@ export const DOCS_TOOLS = [ export const REVIEW_TOOLS = [...READ_TOOLS, ...PRODUCT_WRITE_TOOLS] as const; /** Mechanical intern: shell-first + path writes when the brief requires them. */ -export const INTERN_TOOLS = [ - "run_shell", - "read_file", - "list_dir", - ...PRODUCT_WRITE_TOOLS, -] as const; +export const INTERN_TOOLS = ["run_shell", "read_file", "list_dir", ...PRODUCT_WRITE_TOOLS] as const; /** Nested orchestrator surface (greybeard / package filter): dispatch + path writes. */ export const ORCHESTRATOR_TOOLS = [