diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 943306a98..48ec68e92 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -229,7 +229,7 @@ Profiles with `orchestrator: true` may themselves call `task` (one hop only): ne #### Closed director fleet (`src/agent/directors/`) -Every shipped specialist is a **director package** — a prompt-first `DirectorPackage` (system prompt, tool envelope, spawn rights, nudge budget, report contract, optional `writePaths`, `modelRole`) registered in a **closed** set of 16 ids. There is no catch-all worker: `task` without `agent` or non-general `intent`, and `task(intent="general")`, fail closed so the primary reclassifies. Nested directors with a spawn allowlist reject off-list children at `createTaskTool` (not prompt-only). Skywalker is the primary session identity: `task(agent="skywalker")` is refused, and `directorProfiles()` omits it from the spawn catalog. +Every shipped specialist is a **director package** — a prompt-first `DirectorPackage` (system prompt, tool envelope, spawn rights, nudge budget, report contract, `modelRole`) registered in a **closed** set of 16 ids. There is no catch-all worker: `task` without `agent` or non-general `intent`, and `task(intent="general")`, fail closed so the primary reclassifies. Nested directors with a spawn allowlist reject off-list children at `createTaskTool` (not prompt-only). Skywalker is the primary session identity: `task(agent="skywalker")` is refused, and `directorProfiles()` omits it from the spawn catalog. **Primary** @@ -285,7 +285,7 @@ Every shipped specialist is a **director package** — a prompt-first `DirectorP | greybeard | intern, explore, critique only | | All other directors | no `task` | -**Tool envelopes** prefer small `tools.allow` mounts over deny-everything. Shipped docs/design directors (shakespeare, brand-reviewer, bruckheimer) mount write tools with **no** package `writePaths`. Lane routing is spawn policy (shakespeare = P/A/I docs, brand-reviewer = DESIGN.md, bruckheimer = product discovery), not a file lock. Optional `writePaths` still exists; the permission gate enforces it when a profile sets it. +**Tool envelopes** prefer small `tools.allow` mounts over deny-everything. Shipped docs/design directors (shakespeare, brand-reviewer, bruckheimer) mount write tools with no path-level lock. Lane routing is spawn policy (shakespeare = P/A/I docs, brand-reviewer = DESIGN.md, bruckheimer = product discovery), not a file lock. There is no static per-package write-path declaration (CL-6952 removed it — no shipped director ever set one); instead the task tool records, without blocking, when two concurrently running dispatches land on the same cwd (see `intervention-log.ts`'s `conflict` class). **Typical chain:** bruckheimer → plan → greybeard → build (+ intern) → critique (+ optional neckbeard), with skywalker coordinating throughout. @@ -299,7 +299,7 @@ Data-only agent plugins (`src/plugins/data-only-agent.ts`) synthesize `agentPlug ### System Prompt (`src/agent/prompts.ts`) -The primary session identity is **Skywalker** (`buildChatRole` → `createSkywalkerSystemPrompt`). Product name remains Corbits Code; when asked its name, the primary answers Skywalker. Role: orchestrate — classify, DIY tiny/single-file/one-route product edits, dispatch closed directors via `task` for substantial work, track the fleet, synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary session (CORE and `SKYWALKER_TOOLS`) so Skywalker can DIY bounded edits; spawn remains the default for substantial, multi-file, parallel, or specialist work (hard cap 4 workers). Shell file-writes stay denied by auto-shell policy. MCP tools are not re-filtered by a product-write deny list (that list is gone). Leaf `writePaths` only apply to path-keyed product tools when a profile sets them. A frontier model already knows how to code; the static prompt carries harness-specific facts and the closed-fleet orchestration policy. The base is three individually-exported sections: +The primary session identity is **Skywalker** (`buildChatRole` → `createSkywalkerSystemPrompt`). Product name remains Corbits Code; when asked its name, the primary answers Skywalker. Role: orchestrate — classify, DIY tiny/single-file/one-route product edits, dispatch closed directors via `task` for substantial work, track the fleet, synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary session (CORE and `SKYWALKER_TOOLS`) so Skywalker can DIY bounded edits; spawn remains the default for substantial, multi-file, parallel, or specialist work (hard cap 4 workers). Shell file-writes stay denied by auto-shell policy. MCP tools are not re-filtered by a product-write deny list (that list is gone). There is no static per-leaf write-path lock; concurrent lanes sharing a cwd are instead flagged (not blocked) as a `conflict` intervention. A frontier model already knows how to code; the static prompt carries harness-specific facts and the closed-fleet orchestration policy. The base is three individually-exported sections: - `buildChatRole` — Skywalker primary identity (orchestrate; DIY tiny/bounded product edits; spawn for substantial work). - `buildHarnessFacts` — the non-derivable rules: shell file-writes are blocked, path tools are the DIY surface on primary (spawn build/docs directors for substantial work), dependency installs and off-limits paths need approval, images are native multimodal input, only core tools are resident (load the rest via `tool_search`; use `search_agents` before dispatching specialists), workflows run only from slash-command steps, and session memory lives at `.corbits/MEMORY.md`. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index e847f0a36..43d9b0f78 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -88,9 +88,9 @@ src/ hooks.ts Lifecycle hooks: discovery, turn collector, run summary subagent/ index.ts Sub-agent spawn + SubAgentDirector - task-tool.ts task() — resolveDirector first; writePaths on child + task-tool.ts task() — resolveDirector first; concurrent-lane-overlap check on spawn session-store.ts Retained child session transcripts for observe UI - identity-context.ts ALS: worker cwd + optional writePaths for gate + identity-context.ts ALS: worker description + cwd for gate attribution config/ index.ts Config resolution (settings files + flags) (was config.ts) settings.ts Settings schema, validators, loaders, resolveProvider @@ -106,8 +106,7 @@ src/ classify.ts Tool tier + approval-request construction command.ts Chained-command split + command scopes auto-shell-policy.ts Auto-mode run_shell deny/ask rule table - gate.ts Permission gate evaluation (+ director writePaths) - write-path-policy.ts Basename/glob match for worker write allowlists + gate.ts Permission gate evaluation matcher.ts Approval glob matching store.ts Per-directory approval persistence types.ts Approval / scope / request / outcome types @@ -155,14 +154,14 @@ docs/ Sixteen packages under `src/agent/directors//` register in `DIRECTOR_REGISTRY` (`registry.ts`). Wire path: 1. `task(agent=…)` / `task(intent=…)` → `resolveDirector` in `task-tool.ts` before tools and system prompt are built. Bare `task` (neither field) and `intent=general` fail closed. -2. `packageToProfile` maps envelope (`tools.allow`/`deny`) to `AgentProfile.capabilities`, `spawn.maySpawn` → `orchestrator`, and optional `writePaths`. System prompts are prefixed with a stable identity block (`formatDirectorSystemPrompt`: agent id, model role, optional skills). +2. `packageToProfile` maps envelope (`tools.allow`/`deny`) to `AgentProfile.capabilities` and `spawn.maySpawn` → `orchestrator`. System prompts are prefixed with a stable identity block (`formatDirectorSystemPrompt`: agent id, model role, optional skills). 3. Nested spawn: packages with `spawn.allowlist` forward that list into nested `task` (`spawnAllowlist` on nestedDispatch). Off-list `agent` is refused. `task(agent=skywalker)` is refused (primary is not a spawned worker). Primary omits the list so plugin profiles stay reachable. 4. `directorProfiles()` is the spawn catalog (`default-agents.ts`) — closed set minus skywalker; plugin agent profiles still load and can override by id. -5. Primary chat role is Skywalker: `buildChatRole()` → `createSkywalkerSystemPrompt()`. Product mutation tools (`write_file` / `edit_file` / `delete_file`) live in CORE (and `SKYWALKER_TOOLS`) so they are advertised on the primary without a `tool_search` round-trip. DIY tiny/bounded edits on the parent; spawn build/docs directors for substantial work — a prompt judgment call, not a toolset strip. `PRIMARY_DENIED_PRODUCT_TOOLS` is gone. Shell file-writes stay denied; MCP tools are not re-filtered by a product-write deny list. Optional `writePaths` (when a profile sets it) only gate path-keyed product tools. +5. Primary chat role is Skywalker: `buildChatRole()` → `createSkywalkerSystemPrompt()`. Product mutation tools (`write_file` / `edit_file` / `delete_file`) live in CORE (and `SKYWALKER_TOOLS`) so they are advertised on the primary without a `tool_search` round-trip. DIY tiny/bounded edits on the parent; spawn build/docs directors for substantial work — a prompt judgment call, not a toolset strip. `PRIMARY_DENIED_PRODUCT_TOOLS` is gone. Shell file-writes stay denied; MCP tools are not re-filtered by a product-write deny list. There is no static per-profile write-path lock (CL-6952). **Codex tool proxies.** When the active provider is Codex (`isCodexProviderName`), `createAgentToolset` and `runSubAgent` mount `apply_patch`, `shell`, and `update_plan` stringTools from `createCodexToolProxies`, all forwarding through the same posix `ToolRunner` seam (`runTool`) so permission plugins still apply. `apply_patch` parses the Codex envelope and forwards each op (`write_file` / `delete_file` / `read_file`). `shell` — the native Codex name is `shell`, not `exec_command`, per the pinned base-instructions text quoted in `codex-responses-adapter.ts`'s bridge message — normalizes Codex's `command` (string or `["bash","-lc",script]`-style argv array), `workdir`, and `timeout_ms` onto `run_shell`'s `{command, cwd?, timeout?}` and is gated by `allowShellFromCapabilities` (mirrors `allowDeleteFromCapabilities` against `run_shell`). `update_plan` maps Codex's `plan: [{step, status}]` onto `manage_tasks(action: "create")`; `pending`/`in_progress`/`completed` map to `todo`/`doing`/`done` — `manage_tasks`'s `cancelled` status has no Codex equivalent and is never produced by this proxy. Primary strips `apply_patch` after mount (Corbits DIY stays on `write_file` / `edit_file` / `delete_file`); `shell` and `update_plan` stay on primary (same classification as `run_shell` / `manage_tasks`). Build and docs leaf allowlists (`BUILD_TOOLS` / `DOCS_TOOLS`) include `apply_patch` so Codex workers keep the proxy after the capability filter. `CORE_TOOL_NAMES` does not list it. -6. Shipped directors omit `writePaths`. The optional field is still enforced in the permission gate via ALS identity (`identity-context.ts` + `write-path-policy.ts`) when a plugin/custom profile sets it. +6. There is no static write-path declaration on packages or profiles (CL-6952 removed it — no shipped director ever set one). Instead, `task-tool.ts` tracks each running dispatch by cwd; a new dispatch that lands on the same cwd as a still-running lane records a `concurrent-lane-overlap` entry in `intervention-log.ts` (class `conflict`). 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. Optional skills are listed in the identity header for awareness; workers do not mount `use_skill` (guidance is baked into package system prompts). Primary mounts `use_skill` for its own skill list. Intent defaults: `intent=implement` → director `build`; explore/plan → same-named director; review → critique; general → error. Spawn: skywalker full fleet; greybeard intern/explore/critique only; all other directors no `task`. Live `` injects cwd, platform, arch, runtime, date, and git status on every chat and worker prompt. diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index f6e0f703c..412e24f62 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -142,7 +142,7 @@ Capabilities beyond the core toolset are opt-in plugins, enabled per workspace t ## Multi-agent (sub-agents) -The primary session is always **orchestrator** (single-agent mode is gone). Its identity is **Skywalker** (product name remains Corbits Code; when asked its name, answer Skywalker): classify work, DIY tiny/single-file/one-route product edits, dispatch a **closed fleet of 16 directors** for substantial work, track the fleet, and synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary (CORE / `SKYWALKER_TOOLS`) — path tools are the DIY surface; spawn remains the default for substantial, multi-file, parallel, or specialist work (hard cap 4). Shell file-writes stay denied. MCP tools are not re-filtered by a product-write deny list (that list is gone). Shipped directors have no package `writePaths`; the optional field still constrains path-keyed product tools (not shell) when a profile sets it. Yolo / skip-permissions still bypasses the write-path gate when enabled. Operator slash recipes (`/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`) tell Skywalker which directors to spawn for substantial work; tiny/bounded edits may run on the primary. +The primary session is always **orchestrator** (single-agent mode is gone). Its identity is **Skywalker** (product name remains Corbits Code; when asked its name, answer Skywalker): classify work, DIY tiny/single-file/one-route product edits, dispatch a **closed fleet of 16 directors** for substantial work, track the fleet, and synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary (CORE / `SKYWALKER_TOOLS`) — path tools are the DIY surface; spawn remains the default for substantial, multi-file, parallel, or specialist work (hard cap 4). Shell file-writes stay denied. MCP tools are not re-filtered by a product-write deny list (that list is gone). There is no static per-package write-path declaration (CL-6952 removed it — no shipped director ever set one). A concurrent dispatch landing on the same working directory as another still-running lane is recorded as a `conflict` intervention, not blocked. Operator slash recipes (`/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`) tell Skywalker which directors to spawn for substantial work; tiny/bounded edits may run on the primary. | Lane | Directors | | --------- | ---------------------------------------------------------------------------------- | diff --git a/src/agent/directors/brand-reviewer/package.test.ts b/src/agent/directors/brand-reviewer/package.test.ts index 86a0b59f0..ca7a68d19 100644 --- a/src/agent/directors/brand-reviewer/package.test.ts +++ b/src/agent/directors/brand-reviewer/package.test.ts @@ -21,11 +21,10 @@ describe("brandReviewerPackage", () => { expect(brandReviewerPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow includes write tools; writePaths is omitted", () => { + test("tools.allow includes write tools", () => { const allow = brandReviewerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); - expect(brandReviewerPackage.writePaths).toBeUndefined(); }); test("systemPrompt mentions DESIGN.md", () => { diff --git a/src/agent/directors/bruckheimer/package.test.ts b/src/agent/directors/bruckheimer/package.test.ts index d3ca05e4e..b1ed9f015 100644 --- a/src/agent/directors/bruckheimer/package.test.ts +++ b/src/agent/directors/bruckheimer/package.test.ts @@ -22,11 +22,10 @@ describe("bruckheimerPackage", () => { expect(bruckheimerPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow includes write tools; writePaths is omitted", () => { + test("tools.allow includes write tools", () => { const allow = bruckheimerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); - expect(bruckheimerPackage.writePaths).toBeUndefined(); }); test("report requires envelope sections", () => { diff --git a/src/agent/directors/registry.test.ts b/src/agent/directors/registry.test.ts index 528a033ed..504fe5802 100644 --- a/src/agent/directors/registry.test.ts +++ b/src/agent/directors/registry.test.ts @@ -107,7 +107,6 @@ describe("director registry", () => { expect(grey.maxTurns).toBe(DIRECTOR_REGISTRY.greybeard.nudge?.maxTurns); const shakespeare = packageToProfile(DIRECTOR_REGISTRY.shakespeare); - expect(shakespeare.writePaths).toBeUndefined(); expect(shakespeare.capabilities?.mode).toBe("allow"); expect(shakespeare.capabilities?.tools).toContain("write_file"); }); @@ -147,13 +146,6 @@ describe("director registry", () => { } }); - test("no shipped director in DIRECTOR_IDS has a non-empty writePaths", () => { - for (const id of DIRECTOR_IDS) { - const paths = DIRECTOR_REGISTRY[id].writePaths; - expect(paths === undefined || paths.length === 0).toBe(true); - } - }); - test("build mounts product writes; intern is shell-only; other leaves do not spawn", () => { expect(DIRECTOR_REGISTRY.build.tools?.allow).toEqual( expect.arrayContaining(["write_file", "edit_file", "delete_file", "apply_patch"]), @@ -181,15 +173,6 @@ describe("director registry", () => { expect(s.spawn.allowlist).toHaveLength(15); }); - test("writePaths guards: a director with non-empty writePaths never allows run_shell", () => { - for (const id of DIRECTOR_IDS) { - const pkg = DIRECTOR_REGISTRY[id]; - if (!pkg.writePaths || pkg.writePaths.length === 0) continue; - const allow = pkg.tools?.allow ?? []; - expect(allow).not.toContain("run_shell"); - } - }); - test("every director profile declares matching agent id in system prompt", () => { for (const id of DIRECTOR_IDS) { const profile = packageToProfile(DIRECTOR_REGISTRY[id]); diff --git a/src/agent/directors/registry.ts b/src/agent/directors/registry.ts index 13f6b62f3..480d1485b 100644 --- a/src/agent/directors/registry.ts +++ b/src/agent/directors/registry.ts @@ -127,9 +127,6 @@ export function packageToProfile(pkg: DirectorPackage): AgentProfile { orchestrator: pkg.spawn.maySpawn, ...(pkg.nudge?.maxTurns !== undefined ? { maxTurns: pkg.nudge.maxTurns } : {}), ...(capabilities !== undefined ? { capabilities } : {}), - ...(pkg.writePaths !== undefined && pkg.writePaths.length > 0 - ? { writePaths: [...pkg.writePaths] } - : {}), }; } diff --git a/src/agent/directors/shakespeare/package.test.ts b/src/agent/directors/shakespeare/package.test.ts index 41501eb47..f3a47c280 100644 --- a/src/agent/directors/shakespeare/package.test.ts +++ b/src/agent/directors/shakespeare/package.test.ts @@ -32,11 +32,10 @@ describe("shakespearePackage", () => { expect(shakespearePackage.spawn.maySpawn).toBe(false); }); - test("tools.allow includes write tools; writePaths is omitted", () => { + test("tools.allow includes write tools", () => { const allow = shakespearePackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); - expect(shakespearePackage.writePaths).toBeUndefined(); }); test("report.requiredSections includes Summary, Findings, Blockers, Paths", () => { diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index 140116f8b..9bf82dfbe 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -34,9 +34,8 @@ export const BUILD_TOOLS = [ /** * Docs leaves: read/search/lsp/web + file writes — no run_shell, no delete_file. - * Envelope policy, not a writePaths lock: docs leaves omit shell so they cannot - * mutate via the terminal. Optional package writePaths, when a profile sets it, - * is still enforced by the permission gate on path-keyed write tools. + * 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 diff --git a/src/agent/directors/types.ts b/src/agent/directors/types.ts index 259ea3054..98c430b16 100644 --- a/src/agent/directors/types.ts +++ b/src/agent/directors/types.ts @@ -69,15 +69,6 @@ export interface DirectorPackage { /** Optional skills the worker may load dynamically (ordered). */ readonly optionalSkills?: readonly string[]; readonly tools?: ToolEnvelope; - /** - * Authz write-path allowlist for write_file/edit_file/delete_file. - * Enforced by the permission gate (not prompt policy). A bare filename (no - * slash) matches only at the workspace root; a glob matches the resolved - * workspace-relative path; anything outside the worker cwd is denied. yolo - * mode bypasses this gate. Omitted = no path lock (tool allow/deny alone - * decides whether writes exist). - */ - readonly writePaths?: readonly string[]; readonly spawn: SpawnRights; readonly nudge?: NudgePolicy; readonly report: ReportContract; diff --git a/src/agent/profile-types.ts b/src/agent/profile-types.ts index 1e65de6f6..a2bd14ec5 100644 --- a/src/agent/profile-types.ts +++ b/src/agent/profile-types.ts @@ -55,11 +55,6 @@ export interface AgentProfile { inference?: InferenceSpec; // Optional tool restriction. Controls which tools the sub-agent can call. capabilities?: CapabilityFilter; - /** - * Authz write-path allowlist for write_file/edit_file/delete_file (director - * packages). Enforced by the permission gate, not prompt policy. - */ - writePaths?: readonly string[]; // Appended to the sub-agent's base system prompt to specialize its behavior. systemPromptRole?: string; // Relative path to a markdown file whose content is loaded as systemPromptRole diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index 9a3c41e14..09344b153 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -163,73 +163,6 @@ describe("grant coverage rebinds relative paths to the request process cwd", () }); }); -describe("director writePaths authz on evaluate", () => { - const cwd = mkdtempSync(join(tmpdir(), "gate-writepaths-")); - - test("denies write_file outside allowlist under ALS identity", async () => { - const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); - const gate = createPermissionGate({ - approvals: [{ tool: "write_file", pattern: "*" }], - interactive: false, - skipPermissions: false, - cwd, - }); - const verdict = await runWithSubAgentIdentity( - { description: "shakespeare", cwd, writePaths: ["PRODUCT.md"] }, - () => - gate.evaluate({ - id: "w1", - name: "write_file", - arguments: { path: "src/hack.ts", content: "nope" }, - }), - ); - expect(verdict.allowed).toBe(false); - if (!verdict.allowed) { - expect(verdict.reason).toMatch(/authz allowlist/i); - } - }); - - test("allows write_file matching bare basename allowlist", async () => { - const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); - const gate = createPermissionGate({ - approvals: [{ tool: "write_file", pattern: "*" }], - interactive: false, - skipPermissions: false, - cwd, - }); - const verdict = await runWithSubAgentIdentity( - { description: "shakespeare", cwd, writePaths: ["PRODUCT.md"] }, - () => - gate.evaluate({ - id: "w2", - name: "write_file", - arguments: { path: "PRODUCT.md", content: "ok" }, - }), - ); - expect(verdict.allowed).toBe(true); - }); - - test("yolo (skipPermissions) bypasses writePaths", async () => { - const { runWithSubAgentIdentity } = await import("../subagent/identity-context.js"); - const gate = createPermissionGate({ - approvals: [], - interactive: false, - skipPermissions: true, - cwd, - }); - const verdict = await runWithSubAgentIdentity( - { description: "shakespeare", cwd, writePaths: ["PRODUCT.md"] }, - () => - gate.evaluate({ - id: "w3", - name: "write_file", - arguments: { path: "src/hack.ts", content: "yolo" }, - }), - ); - expect(verdict.allowed).toBe(true); - }); -}); - // CL-5638: an Always-allow grant minted for `git worktree *` must cover a later // worktree command whose destination is a sibling directory the operator has // already implicitly approved under that pattern, without a second prompt. diff --git a/src/permission/gate.ts b/src/permission/gate.ts index f05aaa001..b14f05967 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -26,12 +26,7 @@ import { splitChainedCommand, isShellCommentOnly, stripCommentLines } from "./co import { createPathRestriction } from "./path-restriction.js"; import { createWorktreeRootsProvider, type RootsProvider } from "./worktree-roots.js"; import { getSubAgentIdentity } from "../subagent/identity-context.js"; -import { matchesWritePathAllowlist, writePathDeniedReason } from "./write-path-policy.js"; -import { - isProductMutationTool, - productMutationPaths, - PRODUCT_MUTATION_TOOLS, -} from "../agent/product-mutation-tools.js"; +import { PRODUCT_MUTATION_TOOLS } from "../agent/product-mutation-tools.js"; import { createMcpToolPermissionRegistry, @@ -432,31 +427,6 @@ export function createPermissionGate(options: PermissionGateOptions): Permission const subAgentIdentity = getSubAgentIdentity(); const effectiveCwd = subAgentIdentity?.cwd ?? resolvedCwd; - // Director write-path authz (not prompt policy). Leaves with writePaths only - // mutate matching subjects. auto mode still enforces; yolo already returned. - if ( - subAgentIdentity?.writePaths !== undefined && - subAgentIdentity.writePaths.length > 0 && - isProductMutationTool(call.name) - ) { - const paths = productMutationPaths(call.name, call.arguments); - // Fail-closed: no extractable subject is the same as an empty path deny. - if (paths.length === 0) { - return { - allowed: false, - reason: writePathDeniedReason("", subAgentIdentity.writePaths), - }; - } - for (const path of paths) { - if (!matchesWritePathAllowlist(path, subAgentIdentity.writePaths, effectiveCwd)) { - return { - allowed: false, - reason: writePathDeniedReason(path, subAgentIdentity.writePaths), - }; - } - } - } - const isRestrictedHere = bindRestrictedToProcessCwd(isRestricted, effectiveCwd); // A call targeting a restricted path (outside the workspace, or a write // under the session state root) drops from allow to ask, so it never auto-allows on diff --git a/src/permission/write-path-policy.test.ts b/src/permission/write-path-policy.test.ts deleted file mode 100644 index 91ffad184..000000000 --- a/src/permission/write-path-policy.test.ts +++ /dev/null @@ -1,117 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { mkdtempSync, realpathSync, rmSync, symlinkSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; -import { matchesWritePathAllowlist, writePathDeniedReason } from "./write-path-policy.js"; - -const cwd = resolve("/tmp/write-path-policy-fixture"); - -describe("matchesWritePathAllowlist", () => { - test("bare filename matches only the workspace-root file", () => { - expect(matchesWritePathAllowlist("PRODUCT.md", ["PRODUCT.md"], cwd)).toBe(true); - // Nested file of the same basename does NOT match a bare pattern. - expect(matchesWritePathAllowlist("docs/PRODUCT.md", ["PRODUCT.md"], cwd)).toBe(false); - expect(matchesWritePathAllowlist("vendor/x/PRODUCT.md", ["PRODUCT.md"], cwd)).toBe(false); - // An explicit glob reaches nested files of that name. - expect(matchesWritePathAllowlist("docs/PRODUCT.md", ["**/PRODUCT.md"], cwd)).toBe(true); - expect(matchesWritePathAllowlist("src/foo.ts", ["PRODUCT.md"], cwd)).toBe(false); - }); - - test("relative globs match workspace-relative paths", () => { - expect(matchesWritePathAllowlist("docs/a.md", ["docs/*"], cwd)).toBe(true); - expect(matchesWritePathAllowlist("src/a.md", ["docs/*"], cwd)).toBe(false); - }); - - test("empty allowlist or empty subject denies", () => { - expect(matchesWritePathAllowlist("PRODUCT.md", [], cwd)).toBe(false); - expect(matchesWritePathAllowlist("", ["PRODUCT.md"], cwd)).toBe(false); - }); - - test("absolute paths under cwd still match bare basename", () => { - const abs = resolve(cwd, "DESIGN.md"); - expect(matchesWritePathAllowlist(abs, ["DESIGN.md"], cwd)).toBe(true); - }); - - test("path traversal cannot escape a glob (subject never matched raw)", () => { - // `docs/../src/hack.ts` resolves under src/, not docs/ — must NOT match - // `docs/*` even though the raw subject string starts with `docs/`. - expect(matchesWritePathAllowlist("docs/../src/hack.ts", ["docs/*"], cwd)).toBe(false); - expect(matchesWritePathAllowlist("docs/../src/hack.ts", ["docs/**"], cwd)).toBe(false); - // Same escape via a bare-name allowlist. - expect(matchesWritePathAllowlist("PRODUCT.md/../src/hack.ts", ["PRODUCT.md"], cwd)).toBe(false); - // A legitimately nested docs file still matches. - expect(matchesWritePathAllowlist("docs/a/b.md", ["docs/**"], cwd)).toBe(true); - }); - - test("absolute paths outside cwd are hard-denied even if basename matches", () => { - // Outside root: /tmp/evil/PRODUCT.md is not under cwd, so PRODUCT.md must - // not match — no fallthrough to pattern match on the outside path. - const outside = resolve("/tmp/write-path-policy-elsewhere", "PRODUCT.md"); - expect(outside).not.toBe(resolve(cwd, "PRODUCT.md")); - expect(matchesWritePathAllowlist(outside, ["PRODUCT.md"], cwd)).toBe(false); - expect(matchesWritePathAllowlist(outside, ["**/PRODUCT.md"], cwd)).toBe(false); - // A sibling of cwd (shared /tmp parent) still denied. - expect(matchesWritePathAllowlist("../sibling/PRODUCT.md", ["PRODUCT.md"], cwd)).toBe(false); - }); -}); - -describe("matchesWritePathAllowlist with a symlinked cwd", () => { - test("allows a write under the canonical target of a symlinked cwd", () => { - // Mirrors macOS's /tmp -> /private/tmp: cwd is spelled via the symlink, - // but resolveWorkspacePath (and any tool arg it rewrites) hands the - // subject in already realpathed. Both sides of the compare must - // canonicalize the same way or a legitimate write is hard-denied. - const real = mkdtempSync(join(realpathSync(tmpdir()), "write-path-real-")); - const linkDir = join(realpathSync(tmpdir()), `write-path-link-${process.pid}`); - try { - symlinkSync(real, linkDir); - const symlinkedCwd = linkDir; // lexically distinct from `real` - const canonicalSubject = join(real, "docs", "a.md"); // already realpathed - - expect(matchesWritePathAllowlist(canonicalSubject, ["docs/*"], symlinkedCwd)).toBe(true); - - // A genuinely outside path is still denied. - const outsideReal = mkdtempSync(join(realpathSync(tmpdir()), "write-path-outside-")); - try { - expect( - matchesWritePathAllowlist(join(outsideReal, "docs", "a.md"), ["docs/*"], symlinkedCwd), - ).toBe(false); - } finally { - rmSync(outsideReal, { recursive: true, force: true }); - } - } finally { - rmSync(linkDir, { force: true }); - rmSync(real, { recursive: true, force: true }); - } - }); -}); - -describe("matchesWritePathAllowlist with an unresolvable cwd", () => { - test("a cwd whose final component is a dangling symlink is hard-denied, not spuriously allowed (CL-6715)", () => { - // If cwd itself is unresolvable, both absCwd and abs collapse to the same - // UNRESOLVABLE sentinel, `abs === absCwd` goes true, rel becomes ".", and - // a root-matching pattern (e.g. "**") would otherwise spuriously allow — - // turning a hard authz deny into an ask-prompt. - const parent = mkdtempSync(join(realpathSync(tmpdir()), "write-path-dangling-parent-")); - const danglingCwd = join(parent, "dangling-cwd"); - try { - symlinkSync(join(parent, "does-not-exist"), danglingCwd); - - expect(matchesWritePathAllowlist("anything.md", ["**"], danglingCwd)).toBe(false); - expect(matchesWritePathAllowlist("PRODUCT.md", ["PRODUCT.md"], danglingCwd)).toBe(false); - } finally { - rmSync(danglingCwd, { force: true }); - rmSync(parent, { recursive: true, force: true }); - } - }); -}); - -describe("writePathDeniedReason", () => { - test("names allowlist and subject", () => { - const reason = writePathDeniedReason("src/x.ts", ["PRODUCT.md", "docs/*"]); - expect(reason).toContain("PRODUCT.md"); - expect(reason).toContain("docs/*"); - expect(reason).toContain("src/x.ts"); - expect(reason).toMatch(/authz/i); - }); -}); diff --git a/src/permission/write-path-policy.ts b/src/permission/write-path-policy.ts deleted file mode 100644 index ba1d03207..000000000 --- a/src/permission/write-path-policy.ts +++ /dev/null @@ -1,72 +0,0 @@ -import { resolve, sep } from "node:path"; -import { matchesPattern } from "./matcher.js"; -import { realpathNearestOr, UNRESOLVABLE } from "./path-restriction.js"; - -/** - * Director write-path allowlist (authz, not prompt policy). - * When set on a sub-agent identity, write_file / edit_file / delete_file / - * apply_patch must - * target a path matching one of these patterns. Enforced in the permission - * gate; skipPermissions (yolo) bypasses the whole gate before this runs. - * - * Subject is resolved against cwd before any matching. Any path that resolves - * OUTSIDE cwd is hard-denied: the raw subject is never matched against a - * pattern, so traversal strings (e.g. `docs/../src/hack.ts`) cannot fool a - * `docs/*` glob and absolute paths under a different root never match. - * - * Patterns: - * - bare filename (`PRODUCT.md`, no `/ * ?`) matches ONLY the workspace-root - * file of that exact name — never a nested file sharing the basename. - * A bare name is compared to the workspace-relative path, so `docs/PRODUCT.md` - * or `vendor/x/PRODUCT.md` does NOT match `PRODUCT.md`. - * - relative globs (`docs/*`, a double-star-prefixed pattern) use matchesPattern - * against the workspace-relative path only; use a double-star prefix to - * match a basename at any depth. - */ -export function matchesWritePathAllowlist( - subject: string, - allowlist: readonly string[], - cwd: string, -): boolean { - if (allowlist.length === 0) return false; - if (subject.length === 0) return false; - - // Canonicalize both sides the same way path-restriction does: a symlinked - // cwd (e.g. macOS /tmp -> /private/tmp) must not desync from a subject - // already resolved to its realpath by resolveWorkspacePath, which would - // otherwise hard-deny a legitimate allowlisted write. - const absCwd = realpathNearestOr(resolve(cwd)); - const abs = realpathNearestOr(resolve(cwd, subject)); - // Either side unresolvable (dangling symlink/loop component) must hard-deny. - // Otherwise an unresolvable cwd and an unresolvable subject both collapse to - // the same sentinel, `abs === absCwd` goes true, rel becomes ".", and a - // root-matching allowlist pattern spuriously allows. - if (absCwd === UNRESOLVABLE || abs === UNRESOLVABLE) return false; - let rel: string; - if (abs === absCwd) { - rel = "."; - } else if (abs.startsWith(absCwd + sep)) { - rel = abs.slice(absCwd.length + 1); - } else { - // Outside cwd — hard-deny. Never fall through to pattern matching on the - // raw subject, which would let `docs/../src/hack.ts` match `docs/*`. - return false; - } - - for (const pattern of allowlist) { - // Bare filename (no path separators, no glob metacharacters): root-only. - // Matching on the basename would re-open any-depth matching for bare names, - // so a bare pattern is compared only against the workspace-relative path. - const isBareName = !pattern.includes("/") && !pattern.includes("*") && !pattern.includes("?"); - if (isBareName) { - if (rel === pattern) return true; - continue; - } - if (matchesPattern(rel, pattern)) return true; - } - return false; -} - -export function writePathDeniedReason(path: string, allowlist: readonly string[]): string { - return `Write path denied by director authz allowlist (not prompt policy). Allowed: ${allowlist.join(", ")}. Got: ${path || "(empty)"}. auto mode still enforces this; yolo (skipPermissions) bypasses.`; -} diff --git a/src/subagent/identity-context.ts b/src/subagent/identity-context.ts index 6e7e0912a..d47fd9de3 100644 --- a/src/subagent/identity-context.ts +++ b/src/subagent/identity-context.ts @@ -9,11 +9,6 @@ import { AsyncLocalStorage } from "node:async_hooks"; export interface SubAgentIdentity { description: string; cwd: string; - /** - * When set, write/edit/delete subjects must match (authz path lock). - * Omitted = no director path allowlist. - */ - writePaths?: readonly string[]; } const subAgentIdentityAls = new AsyncLocalStorage(); diff --git a/src/subagent/intervention-log.ts b/src/subagent/intervention-log.ts index 67f290247..5082962f2 100644 --- a/src/subagent/intervention-log.ts +++ b/src/subagent/intervention-log.ts @@ -35,9 +35,11 @@ export const INTERVENTION_FILE = "interventions.jsonl"; * independent of any stop/nudge/block — it is the log's real outcome signal: * a `block` record can be read alongside the `outcome` record(s) for later * dispatches of the same brief fingerprint to see what, if anything, the - * parent's re-dispatch after a mutated brief actually produced. + * parent's re-dispatch after a mutated brief actually produced. `conflict` + * records a detected overlap between two concurrently running lanes; it is + * advisory only — the dispatch that triggered it was never blocked. */ -export type InterventionClass = "stop" | "nudge" | "block" | "outcome"; +export type InterventionClass = "stop" | "nudge" | "block" | "outcome" | "conflict"; /** What a completed dispatch produced, for correlating against earlier stops. */ export interface InterventionOutcome { diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 091ec1012..5587a5e42 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -548,14 +548,11 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { if (typeof stallWatchdog.unref === "function") stallWatchdog.unref(); // Every tool call this sub-agent makes runs under its own identity in ALS - // (description + cwd + optional writePaths), so the permission gate can - // attribute approvals and enforce director path locks (see identity-context.ts). + // (description + cwd), so the permission gate can attribute approvals to + // the agent that raised them (see identity-context.ts). const subAgentIdentity = { description: params.description, cwd: params.cwd, - ...(params.writePaths !== undefined && params.writePaths.length > 0 - ? { writePaths: params.writePaths } - : {}), }; const toolsFactory = defineTool({ id: `${ID_PREFIX}/subagent-tools`, diff --git a/src/subagent/task-tool.ts b/src/subagent/task-tool.ts index f2cc67a65..861580847 100644 --- a/src/subagent/task-tool.ts +++ b/src/subagent/task-tool.ts @@ -262,6 +262,22 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { outcomeLog ??= createInterventionLog(deps.getWorkdirBase(), { role: "parent" }); outcomeLog({ id: "dispatch-outcome", class: "outcome", outcome: { kind, dispatchCount } }); }; + // Concurrent-lane overlap detection (CL-6952), replacing the static + // per-package writePaths lock. There is no field in the task() contract a + // caller uses to declare which files a dispatch will touch, so the only + // honestly knowable "intended scope" at spawn is the working directory the + // dispatch will run in — worktree-isolated lanes always get a fresh, + // disjoint path here, so this can only ever fire in the shared-cwd fallback, + // which is exactly where two lanes really can stomp each other's writes. + // Keyed by call.id so a completed lane (removed in the outer finally below) + // is never mistaken for one still running: sequential dispatches to the + // same cwd are always clean. + const activeLanes = new Map(); + let conflictLog: InterventionSink | null = null; + const recordConflict = (event: Parameters[0]): void => { + conflictLog ??= createInterventionLog(deps.getWorkdirBase(), { role: "parent" }); + conflictLog(event); + }; return tool({ definition: taskToolDefinition, handler: async (call, signal): Promise => { @@ -314,7 +330,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { let effortPin: ReasoningEffort | undefined; let capabilities: CapabilityFilter | undefined; let systemPromptRole: string | undefined; - let writePaths: readonly string[] | undefined; let orchestrator = false; let profileMaxTurns: number | undefined; let resolvedDirectorId: string | undefined; @@ -388,9 +403,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { systemPromptRole = formatDirectorSystemPrompt(pkg); const caps = packageToCapabilities(pkg); if (caps !== undefined) capabilities = caps; - if (pkg.writePaths !== undefined && pkg.writePaths.length > 0) { - writePaths = pkg.writePaths; - } if (pkg.nudge?.maxTurns !== undefined) profileMaxTurns = pkg.nudge.maxTurns; if (pkg.spawn.maySpawn && deps.allowOrchestrator !== false) { orchestrator = true; @@ -437,9 +449,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { if (profile.capabilities !== undefined) { capabilities = profile.capabilities; } - if (profile.writePaths !== undefined && profile.writePaths.length > 0) { - writePaths = profile.writePaths; - } if (profile.maxTurns !== undefined) { profileMaxTurns = profile.maxTurns; } @@ -481,9 +490,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { systemPromptRole = formatDirectorSystemPrompt(pkg); const caps = packageToCapabilities(pkg); if (caps !== undefined) capabilities = caps; - if (pkg.writePaths !== undefined && pkg.writePaths.length > 0) { - writePaths = pkg.writePaths; - } if (pkg.nudge?.maxTurns !== undefined) profileMaxTurns = pkg.nudge.maxTurns; if (pkg.spawn.maySpawn && deps.allowOrchestrator !== false) { orchestrator = true; @@ -700,6 +706,26 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { return taskToolResult(call.id, `Error: ${message}`); } } + // Detect, don't lock: warn when another lane still running right now + // is already working in this same cwd. Worktree-isolated lanes never + // collide here (each gets its own directory); this only fires in the + // shared-cwd fallback, where two lanes genuinely can overwrite each + // other's writes. Never blocks the spawn — the least destructive + // response that still tells the operator something true, since a + // shared cwd does not by itself prove the two lanes touch the same + // files, only that they could. + const laneCwd = worktreeCwd ?? deps.cwd; + for (const [otherId, other] of activeLanes) { + if (other.cwd !== laneCwd) continue; + recordConflict({ + id: "concurrent-lane-overlap", + class: "conflict", + detail: + `"${description}" (${call.id}) and "${other.description}" (${otherId}) ` + + `are both running against ${laneCwd} at once`, + }); + } + activeLanes.set(call.id, { description, cwd: laneCwd }); // Cleanup runs once the sub-agent's report is ready, regardless of // outcome, so a cancelled or failed run's worktree is still reclaimed // (or preserved with a notice) rather than leaked. @@ -736,7 +762,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { ...(deps.onProgress !== undefined ? { onProgress: deps.onProgress } : {}), ...(capabilities !== undefined ? { capabilities } : {}), ...(systemPromptRole !== undefined ? { systemPromptRole } : {}), - ...(writePaths !== undefined ? { writePaths } : {}), ...(orchestrator ? { orchestrator: true, nestedDispatch: nestedDispatch! } : {}), maxTurns: resolvedMaxTurns, ...(deps.deadlineMs !== undefined ? { deadlineMs: deps.deadlineMs } : {}), @@ -806,6 +831,7 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { signal.removeEventListener("abort", onParentAbort); } } finally { + activeLanes.delete(call.id); end(subagentSpanId); telemetry.capture("subagent_end", { agent_name: agentName, diff --git a/src/subagent/types.ts b/src/subagent/types.ts index 91f5dd057..1ef637676 100644 --- a/src/subagent/types.ts +++ b/src/subagent/types.ts @@ -98,11 +98,6 @@ export type RunSubAgentParams = { onProgress?: (info: { description: string; toolName: string }) => void; capabilities?: CapabilityFilter; systemPromptRole?: string; - /** - * Director authz write-path allowlist. Passed into sub-agent identity so the - * permission gate can deny out-of-lane writes (not prompt policy). - */ - writePaths?: readonly string[]; // When true, the assembled system prompt grants this sub-agent permission // to call `task` to spawn further agents (orchestrator exception to the // no-recursion rule). Set from AgentProfile.orchestrator at dispatch time.