From 99e4d7503da4e8a1f7eeb293e231a8d4b2dacbff Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 21:22:19 -0700 Subject: [PATCH 1/5] feat(subagent): verify requires_tools pre-spawn with fail-closed preflight --- src/agent/profiles-snapshot.test.ts | 93 +++++ src/agent/profiles.ts | 58 ++- .../agent-fleet-requires-tools.test.ts | 195 ++++++++++ src/subagent/agent-fleet.ts | 51 ++- src/subagent/capability-preflight.test.ts | 240 +++++++++++++ src/subagent/capability-preflight.ts | 337 ++++++++++++++++++ src/subagent/run-requires-tools.test.ts | 129 +++++++ src/subagent/run.ts | 27 ++ src/subagent/session-store.ts | 24 ++ src/subagent/types.ts | 7 + 10 files changed, 1156 insertions(+), 5 deletions(-) create mode 100644 src/agent/profiles-snapshot.test.ts create mode 100644 src/subagent/agent-fleet-requires-tools.test.ts create mode 100644 src/subagent/capability-preflight.test.ts create mode 100644 src/subagent/capability-preflight.ts create mode 100644 src/subagent/run-requires-tools.test.ts diff --git a/src/agent/profiles-snapshot.test.ts b/src/agent/profiles-snapshot.test.ts new file mode 100644 index 000000000..b6d275257 --- /dev/null +++ b/src/agent/profiles-snapshot.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, test } from "bun:test"; +import { mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + currentProfileSnapshotRevision, + loadAgentProfiles, + loadAgentProfilesWithDiagnostics, +} from "./profiles.js"; + +async function makeAgentsDir(files: Record): Promise { + const dir = await mkdtemp(join(tmpdir(), "cl9476-profiles-")); + for (const [name, content] of Object.entries(files)) { + await writeFile(join(dir, name), content); + } + return dir; +} + +const VALID_PROFILE = JSON.stringify({ + id: "local-reader", + systemPromptRole: "You read files.", + capabilities: { mode: "allow", tools: ["read_file"] }, +}); + +describe("loadAgentProfilesWithDiagnostics", () => { + test("malformed file is reported and never poisons the load", async () => { + const dir = await makeAgentsDir({ + "good.json": VALID_PROFILE, + "broken.json": "{ not valid json", + "notes.txt": "ignored, not a profile extension", + }); + + const { profiles, diagnostics } = + await loadAgentProfilesWithDiagnostics(dir); + + expect(profiles.some((p) => p.id === "local-reader")).toBe(true); + expect(diagnostics.malformed).toHaveLength(1); + expect(diagnostics.malformed[0]?.path).toBe(join(dir, "broken.json")); + expect(diagnostics.malformed[0]?.reason).toBe("invalid JSON"); + expect(typeof diagnostics.revision).toBe("number"); + }); + + test("schema-invalid file is reported with its reason", async () => { + const dir = await makeAgentsDir({ + "bad-shape.json": JSON.stringify({ id: 42 }), + }); + + const { profiles, diagnostics } = + await loadAgentProfilesWithDiagnostics(dir); + + expect(profiles.some((p) => p.id === "local-reader")).toBe(false); + expect(diagnostics.malformed).toHaveLength(1); + expect(diagnostics.malformed[0]?.reason).toBe("schema validation failed"); + }); + + test("loadAgentProfiles delegates and resolves identically", async () => { + const dir = await makeAgentsDir({ + "good.json": VALID_PROFILE, + "broken.json": "{ nope", + }); + + const viaDiagnostics = await loadAgentProfilesWithDiagnostics(dir); + const direct = await loadAgentProfiles(dir); + + expect(direct.map((p) => p.id).sort()).toEqual( + viaDiagnostics.profiles.map((p) => p.id).sort(), + ); + }); + + test("every load bumps the snapshot revision", async () => { + const dir = await makeAgentsDir({ "good.json": VALID_PROFILE }); + const before = currentProfileSnapshotRevision(); + + const first = await loadAgentProfilesWithDiagnostics(dir); + const second = await loadAgentProfilesWithDiagnostics(dir); + + expect(second.diagnostics.revision).toBeGreaterThan( + first.diagnostics.revision, + ); + expect(first.diagnostics.revision).toBeGreaterThan(before); + expect(currentProfileSnapshotRevision()).toBe(second.diagnostics.revision); + }); + + test("missing directory resolves defaults with no malformed entries", async () => { + const { profiles, diagnostics } = await loadAgentProfilesWithDiagnostics( + join(tmpdir(), "cl9476-does-not-exist"), + ); + + expect(profiles.length).toBeGreaterThan(0); + expect(diagnostics.malformed).toEqual([]); + }); +}); diff --git a/src/agent/profiles.ts b/src/agent/profiles.ts index 8f46d76ff..1c7d94d95 100644 --- a/src/agent/profiles.ts +++ b/src/agent/profiles.ts @@ -71,6 +71,28 @@ function isENOENT(err: unknown): boolean { // DIRECTOR_IDS, which are reserved and skipped at load (CL-7015). const registry: AgentProfile[] = [...defaultPlugin.agents]; +// Load diagnostics: additive over loadAgentProfiles. `revision` stamps the +// profile snapshot a dispatch was verified against (agent-fleet records it on +// the session); `malformed` names local files that failed to load and why. +// A malformed file never blocks the load — it fails closed only when a +// dispatch's requires_tools preflight must verify against it (CL-9476). +export interface MalformedAgentProfile { + path: string; + reason: string; +} + +export interface AgentProfileDiagnostics { + revision: number; + malformed: MalformedAgentProfile[]; +} + +let profileSnapshotRevision = 0; + +/** Revision of the most recent profile snapshot load. */ +export function currentProfileSnapshotRevision(): number { + return profileSnapshotRevision; +} + // Merge a profile into a list: replace a same-id entry or append. Used to layer // profiles by precedence (defaults < plugin < local). function mergeProfileInto(list: AgentProfile[], profile: AgentProfile): void { @@ -95,6 +117,28 @@ export async function loadAgentProfiles( dir: string, extraProfiles: AgentProfile[] = [], ): Promise { + return (await loadAgentProfilesWithDiagnostics(dir, extraProfiles)).profiles; +} + +/** + * loadAgentProfiles plus load diagnostics. Additive: profiles resolve + * exactly as before; unreadable/unparseable/invalid local files are named in + * `diagnostics.malformed` instead of skipped silently. Every call bumps the + * snapshot revision a dispatch stamps on its session record. + */ +export async function loadAgentProfilesWithDiagnostics( + dir: string, + extraProfiles: AgentProfile[] = [], +): Promise<{ profiles: AgentProfile[]; diagnostics: AgentProfileDiagnostics }> { + profileSnapshotRevision += 1; + const revision = profileSnapshotRevision; + const malformed: MalformedAgentProfile[] = []; + const done = ( + profiles: AgentProfile[], + ): { profiles: AgentProfile[]; diagnostics: AgentProfileDiagnostics } => ({ + profiles, + diagnostics: { revision, malformed }, + }); let entries: string[]; try { entries = await readdir(dir); @@ -105,7 +149,7 @@ export async function loadAgentProfiles( if (isReservedDirectorProfile(p)) continue; mergeProfileInto(merged, p); } - return merged; + return done(merged); } throw err; } @@ -121,16 +165,24 @@ export async function loadAgentProfiles( try { raw = await readFile(filePath, "utf8"); } catch { + malformed.push({ path: filePath, reason: "unreadable file" }); continue; } let parsed: unknown; try { parsed = isJSON ? JSON.parse(raw) : Bun.YAML.parse(raw); } catch { + malformed.push({ + path: filePath, + reason: isJSON ? "invalid JSON" : "invalid YAML", + }); continue; } const result = AgentProfileSchema(parsed); - if (result instanceof type.errors) continue; + if (result instanceof type.errors) { + malformed.push({ path: filePath, reason: "schema validation failed" }); + continue; + } const profile = result as AgentProfile; if (isReservedDirectorProfile(profile)) continue; // Resolve systemPromptPath relative to this directory. The file content @@ -158,5 +210,5 @@ export async function loadAgentProfiles( mergeProfileInto(merged, profile); } for (const profile of local) mergeProfileInto(merged, profile); - return merged; + return done(merged); } diff --git a/src/subagent/agent-fleet-requires-tools.test.ts b/src/subagent/agent-fleet-requires-tools.test.ts new file mode 100644 index 000000000..996819070 --- /dev/null +++ b/src/subagent/agent-fleet-requires-tools.test.ts @@ -0,0 +1,195 @@ +import { describe, expect, test } from "bun:test"; + +import { + createFleetMailbox, + createSpawnAgentTool, + type AgentFleetDeps, +} from "./agent-fleet.js"; +import { unlimitedAdmissionQueue } from "./admission.js"; +import { createPermissionGate } from "../permission/gate.js"; +import { NOOP_TELEMETRY, type TelemetryEvent } from "../telemetry/index.js"; +import type { AgentProfile } from "../agent/profiles.js"; +import { createSubAgentSessionStore } from "./session-store.js"; +import type { RunSubAgentParams, RunSubAgentResult } from "./types.js"; + +const testPermissionGate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: true, + reactorGated: false, +}); + +const provider = { + providerName: "test-provider", + baseURL: "http://localhost", + model: "test-model", +}; + +const READ_ONLY_PROFILE: AgentProfile = { + id: "read-only-worker", + systemPromptRole: "You read files.", + capabilities: { mode: "allow", tools: ["read_file"] }, +}; + +function makeDeps( + run: (params: RunSubAgentParams) => Promise, + capturedTelemetry: TelemetryEvent[], +): AgentFleetDeps { + const sessions = createSubAgentSessionStore(); + return { + permissionGate: testPermissionGate, + cwd: "/tmp", + getWorkdirBase: () => "/tmp/workdir", + provider, + run, + sessions, + fleetRecords: createFleetMailbox(sessions), + admission: unlimitedAdmissionQueue(), + profiles: [READ_ONLY_PROFILE], + telemetry: { + ...NOOP_TELEMETRY, + capture: (event: TelemetryEvent) => { + capturedTelemetry.push(event); + }, + }, + }; +} + +async function callSpawn( + tool: ReturnType, + args: Record, +): Promise<{ content: string; isError?: boolean }> { + if (tool.kind !== "full") throw new Error("expected full tool"); + const result = await tool.handler( + { + id: `spawn-${Math.random()}`, + name: "spawn_agent", + arguments: args, + }, + new AbortController().signal, + ); + const content = + typeof result.content === "string" + ? result.content + : JSON.stringify(result.content); + return { + content, + ...(result.isError !== undefined ? { isError: result.isError } : {}), + }; +} + +describe("spawn_agent requires_tools preflight", () => { + test("rejects a tool outside the allowlist with no session, telemetry, or run", async () => { + const telemetry: TelemetryEvent[] = []; + let runCalled = false; + const deps = makeDeps(async () => { + runCalled = true; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "shell job", + prompt: "run a command", + agent: "read-only-worker", + requires_tools: ["run_shell"], + }); + + expect(result.isError).toBe(true); + expect(result.content.startsWith("Error:")).toBe(true); + expect(result.content).toContain("run_shell"); + expect(result.content).toContain("Re-dispatch"); + expect(result.content).not.toContain("continuable"); + expect(runCalled).toBe(false); + expect(deps.sessions.list()).toEqual([]); + expect(telemetry).toEqual([]); + }); + + test("rejects an unknown tool with a did-you-mean hint and no session", async () => { + const telemetry: TelemetryEvent[] = []; + let runCalled = false; + const deps = makeDeps(async () => { + runCalled = true; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "typo job", + prompt: "do it", + agent: "read-only-worker", + requires_tools: ["run_shel"], + }); + + expect(result.isError).toBe(true); + expect(result.content).toContain('Did you mean "run_shell"?'); + expect(runCalled).toBe(false); + expect(deps.sessions.list()).toEqual([]); + expect(telemetry).toEqual([]); + }); + + test("accepts a mounted tool and stamps the session record", async () => { + const telemetry: TelemetryEvent[] = []; + let seenRequires: readonly string[] | undefined; + const deps = makeDeps(async (params) => { + seenRequires = params.requiresTools; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "read job", + prompt: "read a file", + agent: "read-only-worker", + requires_tools: ["read_file"], + }); + + expect(result.isError).not.toBe(true); + const body = JSON.parse(result.content) as { + agent_id: string; + status: string; + }; + const session = deps.sessions.get(body.agent_id); + expect(session?.requiresTools).toEqual(["read_file"]); + expect(typeof session?.snapshotRevision).toBe("number"); + expect(seenRequires).toEqual(["read_file"]); + expect(telemetry).toContain("subagent_start"); + }); + + test("aliases collapse at dispatch (shell stamps run_shell)", async () => { + const telemetry: TelemetryEvent[] = []; + const deps = makeDeps(async () => ({ report: "done" }), telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "alias read job", + prompt: "read a file", + agent: "read-only-worker", + requires_tools: ["read"], + }); + + expect(result.isError).not.toBe(true); + const body = JSON.parse(result.content) as { agent_id: string }; + expect(deps.sessions.get(body.agent_id)?.requiresTools).toEqual([ + "read_file", + ]); + }); + + test("omitted requires_tools leaves the session unstamped", async () => { + const telemetry: TelemetryEvent[] = []; + const deps = makeDeps(async () => ({ report: "done" }), telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "plain job", + prompt: "read a file", + agent: "read-only-worker", + }); + + expect(result.isError).not.toBe(true); + const body = JSON.parse(result.content) as { agent_id: string }; + const session = deps.sessions.get(body.agent_id); + expect(session?.requiresTools).toBeUndefined(); + expect(session?.snapshotRevision).toBeUndefined(); + }); +}); diff --git a/src/subagent/agent-fleet.ts b/src/subagent/agent-fleet.ts index 2da7c9b91..447f34afb 100644 --- a/src/subagent/agent-fleet.ts +++ b/src/subagent/agent-fleet.ts @@ -67,6 +67,12 @@ import { type ReasoningEffort, } from "../provider/reasoning-effort.js"; import type { AgentProfile, CapabilityFilter } from "../agent/profiles.js"; +import { currentProfileSnapshotRevision } from "../agent/profiles.js"; +import { + DEFAULT_KNOWN_ENGINES, + formatCapabilityUnavailable, + preflightCapabilities, +} from "./capability-preflight.js"; import { isCodexProviderName } from "../config/codex-providers.js"; import { buildDispatchBrief, type TaskIntent } from "./report.js"; import { @@ -543,12 +549,13 @@ const SpawnAgentArgs = type({ "success_criteria?": "string[]", "do_not?": "string[]", "report_focus?": "string", + "requires_tools?": "string[]", }); export const spawnAgentToolDefinition: ToolDefinition = { name: SPAWN_AGENT_TOOL_NAME, description: - "Start a worker agent and return IMMEDIATELY with its agent_id — this never blocks on the worker's completion. Pass agent= a director/profile id returned by search_agents, or intent= (one of explore|implement|review|plan|general). The child starts blank. One focused task per worker. success_criteria is required for implement/review (and their default directors). Fire several spawn_agent calls in one turn to start independent lanes in parallel, then reply and end the turn — workers keep running while you are idle. Reports arrive as mailbox mail where mailbox delivery is mounted; where wait_agents is mounted (exec primary), collect with it instead. Do not poll. Excess fan-out is queued rather than refused.", + "Start a worker agent and return IMMEDIATELY with its agent_id — this never blocks on the worker's completion. Pass agent= a director/profile id returned by search_agents, or intent= (one of explore|implement|review|plan|general). The child starts blank. One focused task per worker. success_criteria is required for implement/review (and their default directors). Fire several spawn_agent calls in one turn to start independent lanes in parallel, then reply and end the turn — workers keep running while you are idle. Reports arrive as mailbox mail where mailbox delivery is mounted; where wait_agents is mounted (exec primary), collect with it instead. Do not poll. Excess fan-out is queued rather than refused. requires_tools declares hard tool requirements verified pre-spawn against the worker's capability mount (fail-closed with a reroute hint); a preflight rejection or a mount-time stale_snapshot failure is non-continuable — re-dispatch deliberately, never auto-retry or spawn a speculative successor.", inputSchema: { type: "object", properties: { @@ -593,6 +600,12 @@ export const spawnAgentToolDefinition: ToolDefinition = { description: "Optional director id (e.g. from search_agents). Alternative to intent=.", }, + requires_tools: { + type: "array", + items: { type: "string" }, + description: + "Optional hard tool requirements (canonical names, e.g. run_shell). Verified pre-spawn against the worker's capability mount — the spawn fails closed with a reroute hint when a tool is missing, and the mount re-checks at run start. Rejections are non-continuable: re-dispatch deliberately, never auto-retry.", + }, }, required: ["description", "prompt"], }, @@ -624,7 +637,8 @@ export const waitAgentsToolDefinition: ToolDefinition = { `(interrupted, cancelled, incomplete-report, and similar). A "failed" entry with "continuable": true is a recoverable transient ` + `provider failure (retryable/timeout/overload) — terminal, not a timeout and not a stall: do not re-wait it, and you may spawn at most ` + `one successor with the same brief. "failed" without the marker (auth, quota, context-overflow, or other errors) is not continuable — ` + - `do not respawn it. awaiting_director is not terminal: re-wait while still pending re-delivers the same question. ` + + `do not respawn it. Capability preflight rejections and mount-time stale_snapshot failures report failed ` + + `without the marker for the same reason — re-dispatch deliberately instead of retrying. awaiting_director is not terminal: re-wait while still pending re-delivers the same question. ` + `Answer with send_input (soft). Do not call this in a tight zero-progress loop: a timeout means the targets are still ` + `queued, running, or awaiting a director answer, not "try again right away" — do other work, reply to the operator, or change the brief. Calling again with the ` + `same targets is a real timed wait, not a spin, but wastes turns if nothing has changed. ` + @@ -960,6 +974,7 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { success_criteria: rawSuccessCriteria, do_not: rawDoNot, report_focus: rawReportFocus, + requires_tools: rawRequiresTools, } = parsed; const description = rawDesc.trim(); const prompt = rawPrompt.trim(); @@ -979,6 +994,9 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { const doNot = rawDoNot?.map((d) => d.trim()).filter((d) => d.length > 0) ?? []; const reportFocus = rawReportFocus?.trim(); + const requiresToolsRaw = + rawRequiresTools?.map((t) => t.trim()).filter((t) => t.length > 0) ?? + []; let provider: SubAgentProvider = resolveDep(deps.provider); const parentEffort = provider.reasoningEffort; @@ -1065,6 +1083,32 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { ); } + // CL-9476: fail closed before any session, telemetry, or worktree exists. + // A rejection returns here — no auto re-dispatch, no successor, no retry. + let requiresTools: readonly string[] | undefined; + let snapshotRevision: number | undefined; + if (requiresToolsRaw.length > 0) { + const preflight = preflightCapabilities({ + required: requiresToolsRaw, + ...(resolved.capabilities !== undefined + ? { resolvedFilter: resolved.capabilities } + : {}), + knownEngines: DEFAULT_KNOWN_ENGINES, + agentLabel: resolved.agentLabel, + }); + if (!preflight.ok) { + return fleetResult( + call.id, + formatCapabilityUnavailable( + preflight.unavailable, + resolved.agentLabel, + ), + ); + } + requiresTools = preflight.canonical; + snapshotRevision = currentProfileSnapshotRevision(); + } + const orchestrator = resolved.orchestrator; const nestedSpawnAllowlist = resolved.nestedSpawnAllowlist; const effort = resolveEffortForRole({ @@ -1109,6 +1153,8 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { // governs it from here on. retained: true, provider: provider.providerName, + ...(requiresTools !== undefined ? { requiresTools } : {}), + ...(snapshotRevision !== undefined ? { snapshotRevision } : {}), ...(deps.parentSessionId !== undefined ? { parentSessionId: deps.parentSessionId } : {}), @@ -1415,6 +1461,7 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { ...(resolved.capabilities !== undefined ? { capabilities: resolved.capabilities } : {}), + ...(requiresTools !== undefined ? { requiresTools } : {}), ...(allowedSkillNames !== undefined ? { allowedSkillNames } : {}), ...(resolved.pkg?.attachedSkills !== undefined && resolved.pkg.attachedSkills.length > 0 diff --git a/src/subagent/capability-preflight.test.ts b/src/subagent/capability-preflight.test.ts new file mode 100644 index 000000000..717dc795e --- /dev/null +++ b/src/subagent/capability-preflight.test.ts @@ -0,0 +1,240 @@ +import { describe, expect, test } from "bun:test"; + +import { + checkMountedRequiresTools, + DEFAULT_KNOWN_ENGINES, + formatCapabilityUnavailable, + preflightCapabilities, + rerouteAlternatives, + type CapabilityUnavailable, +} from "./capability-preflight.js"; +import type { CapabilityFilter } from "../agent/profile-types.js"; + +const ALLOW_SHELL: CapabilityFilter = { + mode: "allow", + tools: ["run_shell", "read_file"], +}; +const ALLOW_READ_ONLY: CapabilityFilter = { + mode: "allow", + tools: ["read_file"], +}; +const DENY_SHELL: CapabilityFilter = { mode: "exclude", tools: ["run_shell"] }; + +function preflight( + required: readonly string[], + resolvedFilter?: CapabilityFilter, + knownEngines: readonly string[] = DEFAULT_KNOWN_ENGINES, +) { + return preflightCapabilities({ + required, + ...(resolvedFilter !== undefined ? { resolvedFilter } : {}), + knownEngines, + agentLabel: "test-worker", + }); +} + +describe("preflightCapabilities", () => { + test("allow filter mounting the tool passes and returns canonical names", () => { + const result = preflight(["run_shell"], ALLOW_SHELL); + expect(result).toEqual({ ok: true, canonical: ["run_shell"] }); + }); + + test("exclude filter omitting the tool passes", () => { + const result = preflight(["read_file"], DENY_SHELL); + expect(result).toEqual({ ok: true, canonical: ["read_file"] }); + }); + + test("undefined filter is a full mount — every known tool passes", () => { + const result = preflight(["run_shell", "web_fetch", "manage_tasks"]); + expect(result).toEqual({ + ok: true, + canonical: ["run_shell", "web_fetch", "manage_tasks"], + }); + }); + + test("aliases collapse on both sides (shell requirement vs run_shell filter)", () => { + const result = preflight(["shell"], ALLOW_SHELL); + expect(result).toEqual({ ok: true, canonical: ["run_shell"] }); + }); + + test("allowlist omission rejects missing_tool with reroute alternatives", () => { + const result = preflight(["run_shell"], ALLOW_READ_ONLY); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected rejection"); + expect(result.unavailable.code).toBe("missing_tool"); + expect(result.unavailable.tool).toBe("run_shell"); + expect(result.unavailable.alternatives?.length ?? 0).toBeGreaterThan(0); + }); + + test("denylist hit rejects permission_static", () => { + const result = preflight(["run_shell"], DENY_SHELL); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected rejection"); + expect(result.unavailable.code).toBe("permission_static"); + expect(result.unavailable.tool).toBe("run_shell"); + }); + + test("known tool with an absent binary rejects missing_binary", () => { + const result = preflight(["run_shell"], ALLOW_SHELL, []); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected rejection"); + expect(result.unavailable.code).toBe("missing_binary"); + }); + + test("post-filter mounted engines bypass a narrow allowlist", () => { + const result = preflight(["manage_tasks"], ALLOW_READ_ONLY); + expect(result).toEqual({ ok: true, canonical: ["manage_tasks"] }); + }); + + test("unknown tool rejects with a nearest-name suggestion", () => { + const result = preflight(["run_shel"], ALLOW_SHELL); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected rejection"); + expect(result.unavailable.code).toBe("unknown_tool"); + expect(result.unavailable.tool).toBe("run_shel"); + expect(result.unavailable.suggestion).toBe("run_shell"); + }); + + test("malformed profile source fails closed before per-tool checks", () => { + const result = preflightCapabilities({ + required: ["read_file"], + resolvedFilter: ALLOW_SHELL, + knownEngines: DEFAULT_KNOWN_ENGINES, + agentLabel: "test-worker", + profileSource: { + malformed: true, + path: "/agents/broken.json", + reason: "unexpected token at line 3", + }, + }); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected rejection"); + expect(result.unavailable.code).toBe("malformed_profile"); + }); + + test("reroute alternatives collapse aliases (shell and run_shell agree)", () => { + expect(rerouteAlternatives("shell")).toEqual( + rerouteAlternatives("run_shell"), + ); + expect(rerouteAlternatives("run_shell").length).toBeGreaterThan(0); + }); +}); + +describe("formatCapabilityUnavailable", () => { + const cases: { + code: CapabilityUnavailable["code"]; + unavailable: CapabilityUnavailable; + }[] = [ + { + code: "missing_tool", + unavailable: { + code: "missing_tool", + tool: "run_shell", + alternatives: ["builder"], + }, + }, + { + code: "permission_static", + unavailable: { + code: "permission_static", + tool: "run_shell", + alternatives: ["builder"], + }, + }, + { + code: "missing_binary", + unavailable: { code: "missing_binary", tool: "run_shell" }, + }, + { + code: "stale_snapshot", + unavailable: { code: "stale_snapshot", tool: "run_shell" }, + }, + { + code: "malformed_profile", + unavailable: { + code: "malformed_profile", + tool: "/agents/broken.json", + suggestion: "unexpected token at line 3", + }, + }, + { + code: "unknown_tool", + unavailable: { + code: "unknown_tool", + tool: "run_shel", + suggestion: "run_shell", + }, + }, + ]; + + for (const { code, unavailable } of cases) { + test(`${code} message starts with Error, names the tool, and ends in one action sentence`, () => { + const message = formatCapabilityUnavailable(unavailable, "test-worker"); + expect(message.startsWith("Error:")).toBe(true); + expect(message).toContain(unavailable.tool); + expect(message.endsWith(".")).toBe(true); + const sentences = message + .split(/(?<=\.) /) + .map((s) => s.trim()) + .filter((s) => s.length > 0); + // Fact sentence(s) plus exactly one trailing next-action sentence. + expect(sentences.length).toBeGreaterThanOrEqual(2); + const action = sentences[sentences.length - 1] ?? ""; + expect( + /re-dispatch|drop the requirement|install the backing runtime|fix the profile|correct requires_tools/i.test( + action, + ), + ).toBe(true); + }); + } + + test("stale_snapshot message carries setup_error and non-continuable, never a retry affordance", () => { + const message = formatCapabilityUnavailable( + { code: "stale_snapshot", tool: "run_shell" }, + "test-worker", + ); + expect(message).toContain("stale_snapshot"); + expect(message).toContain("setup_error"); + expect(message).toContain("non-continuable"); + expect(message.toLowerCase()).toContain("instead of retrying"); + expect(message.toLowerCase()).not.toContain("auto-retry"); + expect(message).not.toContain("successor"); + expect(message).not.toContain('continuable": true'); + }); + + test("missing_tool message names the reroute target", () => { + const alternatives = rerouteAlternatives("run_shell"); + const message = formatCapabilityUnavailable( + { code: "missing_tool", tool: "run_shell", alternatives }, + "test-worker", + ); + expect(message).toContain(alternatives[0] ?? ""); + }); + + test("unknown_tool message carries the did-you-mean hint", () => { + const message = formatCapabilityUnavailable( + { code: "unknown_tool", tool: "run_shel", suggestion: "run_shell" }, + "test-worker", + ); + expect(message).toContain('Did you mean "run_shell"?'); + }); +}); + +describe("checkMountedRequiresTools", () => { + test("stamped tool missing from the live mount reports stale", () => { + expect(checkMountedRequiresTools(["run_shell"], ["read_file"])).toEqual({ + ok: false, + missing: ["run_shell"], + }); + }); + + test("mounted tools pass, collapsing aliases", () => { + expect(checkMountedRequiresTools(["shell"], ["run_shell"])).toEqual({ + ok: true, + }); + }); + + test("empty requirements always pass", () => { + expect(checkMountedRequiresTools([], [])).toEqual({ ok: true }); + }); +}); diff --git a/src/subagent/capability-preflight.ts b/src/subagent/capability-preflight.ts new file mode 100644 index 000000000..d8fb0c9a0 --- /dev/null +++ b/src/subagent/capability-preflight.ts @@ -0,0 +1,337 @@ +/** + * Pre-spawn capability preflight for `spawn_agent(requires_tools=...)` (CL-9476). + * + * A `requires_tools` entry is a hard requirement: the named tool must be + * mounted on the worker or the dispatch is rejected before any session, + * telemetry, or worktree exists. Names are canonical engine ids on both sides + * (wire/hidden aliases collapse via canonicalToolName), so `shell` and + * `run_shell` are the same requirement. Fail-closed throughout: unknown names, + * unverifiable profiles, and absent binaries all reject. + * + * The `stale_snapshot` code is never emitted by preflightCapabilities — it is + * the mount-time echo in run.ts (a tool stamped at dispatch is missing from + * the live mount). It lives in this union so both paths share one formatter. + */ + +import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { + DIRECTOR_REGISTRY, + packageToCapabilities, +} from "../agent/directors/registry.js"; +import type { CapabilityFilter } from "../agent/profile-types.js"; + +export type CapabilityUnavailableCode = + | "missing_tool" + | "permission_static" + | "missing_binary" + | "stale_snapshot" + | "malformed_profile" + | "unknown_tool"; + +export interface CapabilityProfileSource { + /** Directory or file the dispatched profile was loaded from, for messages. */ + path?: string; + /** True when the profile file failed to parse/validate — unverifiable. */ + malformed?: boolean; + /** Loader's reason for the malformed flag, for messages. */ + reason?: string; +} + +export interface PreflightCapabilitiesInput { + /** Raw requires_tools entries (aliases welcome — canonicalized here). */ + required: readonly string[]; + /** Resolved dispatch filter; undefined means full mount (everything passes). */ + resolvedFilter?: CapabilityFilter | undefined; + /** Canonical engine ids available in this runtime (binary-present). */ + knownEngines: readonly string[]; + /** Worker label for messages (director id or profile id). */ + agentLabel: string; + /** Profile provenance; a malformed source fails closed. */ + profileSource?: CapabilityProfileSource; +} + +export interface CapabilityUnavailable { + code: CapabilityUnavailableCode; + /** Canonical tool id (or the raw entry for unknown/malformed). */ + tool: string; + /** Nearest known name, for the unknown_tool typo guard. */ + suggestion?: string; + /** Spawnable directors that mount the tool, for reroute hints. */ + alternatives?: readonly string[]; +} + +export type CapabilityPreflightResult = + | { ok: true; canonical: string[] } + | { ok: false; unavailable: CapabilityUnavailable }; + +/** + * Canonical engine ids the fleet knows how to mount: the director tool + * surfaces plus the worker/plumbing verbs mounted outside capability + * filters. Dispatch passes this as `knownEngines`; tests inject narrower + * sets to simulate an absent binary. + */ +export const KNOWN_CAPABILITY_ENGINES: readonly string[] = [ + "read_file", + "write_file", + "edit_file", + "delete_file", + "run_shell", + "grep", + "search_files", + "list_dir", + "lsp", + "shell_collect", + "web_fetch", + "web_search", + "skill_search", + "use_skill", + "manage_tasks", + "spawn_agent", + "list_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "send_input", + "read_agent_trace", + "search_agents", + "ask_director", + "submit_result", +]; + +/** Production `knownEngines`: every known engine is binary-present. */ +export const DEFAULT_KNOWN_ENGINES: readonly string[] = + KNOWN_CAPABILITY_ENGINES; + +/** + * Engines mounted outside the capability filter (run.ts mounts manage_tasks + * after filtering), so a requires_tools entry for them passes preflight even + * when the dispatch filter is a narrow allowlist. + */ +const POST_FILTER_MOUNTED_ENGINES: readonly string[] = ["manage_tasks"]; + +/** Alias spellings accepted in requires_tools, for typo suggestions. */ +const SUGGESTION_CANDIDATES: readonly string[] = [ + ...KNOWN_CAPABILITY_ENGINES, + "read", + "write", + "edit", + "delete", + "bash", + "glob", + "shell", + "update_plan", +]; + +function levenshtein(a: string, b: string): number { + const prev = Array.from({ length: b.length + 1 }, (_, j) => j); + for (let i = 1; i <= a.length; i++) { + let diag = prev[0] ?? 0; + prev[0] = i; + for (let j = 1; j <= b.length; j++) { + const cost = a[i - 1] === b[j - 1] ? 0 : 1; + const above = (prev[j] ?? 0) + 1; + const left = (prev[j - 1] ?? 0) + 1; + const next = Math.min(above, left, diag + cost); + diag = prev[j] ?? 0; + prev[j] = next; + } + } + return prev[b.length] ?? Number.MAX_SAFE_INTEGER; +} + +function nearestToolName(raw: string): string | undefined { + const lower = raw.toLowerCase(); + let best: string | undefined; + let bestDistance = Number.MAX_SAFE_INTEGER; + for (const candidate of SUGGESTION_CANDIDATES) { + const distance = levenshtein(lower, candidate.toLowerCase()); + if (distance < bestDistance) { + bestDistance = distance; + best = candidate; + } + } + return best !== undefined && bestDistance <= 2 ? best : undefined; +} + +/** + * Spawnable directors (closed set minus primary skywalker) whose mounted + * tool set includes `canonical` — derived from packageToCapabilities over + * DIRECTOR_REGISTRY, so the hint tracks the envelopes. Capped for messages. + */ +export function rerouteAlternatives(canonical: string): readonly string[] { + const want = canonicalToolName(canonical); + const out: string[] = []; + for (const pkg of Object.values(DIRECTOR_REGISTRY)) { + if (pkg.id === "skywalker") continue; + const capabilities = packageToCapabilities(pkg); + if (capabilities === undefined) { + out.push(pkg.id); + } else if (capabilities.mode === "allow") { + if (capabilities.tools.some((t) => canonicalToolName(t) === want)) { + out.push(pkg.id); + } + } else if (!capabilities.tools.some((t) => canonicalToolName(t) === want)) { + out.push(pkg.id); + } + if (out.length >= 3) break; + } + return out.sort(); +} + +export function preflightCapabilities( + input: PreflightCapabilitiesInput, +): CapabilityPreflightResult { + const { resolvedFilter, knownEngines, profileSource } = input; + if (profileSource?.malformed === true) { + return { + ok: false, + unavailable: { + code: "malformed_profile", + tool: profileSource.path ?? "(profile source)", + ...(profileSource.reason !== undefined + ? { suggestion: profileSource.reason } + : {}), + }, + }; + } + const known = new Set(knownEngines.map((name) => canonicalToolName(name))); + const allow = + resolvedFilter?.mode === "allow" + ? new Set(resolvedFilter.tools.map((name) => canonicalToolName(name))) + : undefined; + const deny = + resolvedFilter?.mode === "exclude" + ? new Set(resolvedFilter.tools.map((name) => canonicalToolName(name))) + : undefined; + const postFilter = new Set(POST_FILTER_MOUNTED_ENGINES); + const catalog = new Set(KNOWN_CAPABILITY_ENGINES); + const seen = new Set(); + const canonical: string[] = []; + for (const raw of input.required) { + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return { ok: false, unavailable: { code: "unknown_tool", tool: raw } }; + } + const engine = canonicalToolName(trimmed); + if (!catalog.has(engine)) { + const suggestion = nearestToolName(trimmed); + return { + ok: false, + unavailable: { + code: "unknown_tool", + tool: trimmed, + ...(suggestion !== undefined ? { suggestion } : {}), + }, + }; + } + if (!known.has(engine)) { + return { + ok: false, + unavailable: { code: "missing_binary", tool: engine }, + }; + } + if (!postFilter.has(engine)) { + if (allow !== undefined && !allow.has(engine)) { + return { + ok: false, + unavailable: { + code: "missing_tool", + tool: engine, + alternatives: rerouteAlternatives(engine), + }, + }; + } + if (deny !== undefined && deny.has(engine)) { + return { + ok: false, + unavailable: { + code: "permission_static", + tool: engine, + alternatives: rerouteAlternatives(engine), + }, + }; + } + } + if (!seen.has(engine)) { + seen.add(engine); + canonical.push(engine); + } + } + return { ok: true, canonical }; +} + +/** + * Mount-time echo helper (run.ts): the stamped dispatch requirements against + * the live mount. A missing stamped tool means the capability snapshot went + * stale between dispatch and mount — never a dispatch-time outcome. + */ +export function checkMountedRequiresTools( + stampedRequires: readonly string[], + mountedToolNames: readonly string[], +): { ok: true } | { ok: false; missing: string[] } { + const mounted = new Set( + mountedToolNames.map((name) => canonicalToolName(name)), + ); + const missing = stampedRequires.filter( + (name) => !mounted.has(canonicalToolName(name)), + ); + return missing.length === 0 ? { ok: true } : { ok: false, missing }; +} + +/** + * One user message per code. Every message ends in exactly one next-action + * sentence — the caller re-dispatches deliberately; there is no auto + * re-dispatch, successor, or retry path. + */ +export function formatCapabilityUnavailable( + unavailable: CapabilityUnavailable, + agentLabel: string, +): string { + const alternatives = + unavailable.alternatives !== undefined && + unavailable.alternatives.length > 0 + ? ` Re-dispatch to one of (${unavailable.alternatives.join(", ")}) or drop the requirement.` + : ` No spawnable director mounts "${unavailable.tool}" — drop the requirement or add a profile that mounts it.`; + switch (unavailable.code) { + case "missing_tool": + return ( + `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its capability allowlist omits it.` + + alternatives + ); + case "permission_static": + return ( + `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its capability denylist blocks it.` + + alternatives + ); + case "missing_binary": + return ( + `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its runtime engine is unavailable in this environment. ` + + `Re-dispatch without requires_tools=["${unavailable.tool}"] or install the backing runtime first.` + ); + case "stale_snapshot": + return ( + `Error: stale_snapshot setup_error (non-continuable) — worker "${agentLabel}" was dispatched requiring "${unavailable.tool}" but the live mount no longer provides it. ` + + `Re-dispatch the worker deliberately with a fresh brief instead of retrying in place.` + ); + case "malformed_profile": { + const reason = + unavailable.suggestion !== undefined + ? ` (${unavailable.suggestion})` + : ""; + return ( + `Error: worker "${agentLabel}" cannot verify requires_tools — profile source "${unavailable.tool}" is malformed${reason}, so capabilities fail closed. ` + + `Fix the profile file and re-dispatch deliberately.` + ); + } + case "unknown_tool": { + const hint = + unavailable.suggestion !== undefined + ? ` Did you mean "${unavailable.suggestion}"?` + : ""; + return ( + `Error: worker "${agentLabel}" requires unknown tool "${unavailable.tool}".${hint} ` + + `Correct requires_tools to a canonical tool name and re-dispatch.` + ); + } + } +} diff --git a/src/subagent/run-requires-tools.test.ts b/src/subagent/run-requires-tools.test.ts new file mode 100644 index 000000000..ef6d56381 --- /dev/null +++ b/src/subagent/run-requires-tools.test.ts @@ -0,0 +1,129 @@ +/** + * runSubAgent mount echo for requires_tools (CL-9476). + * + * Dispatch verifies requires_tools pre-spawn, but the filter or mount may + * shift between dispatch and mount. A stamped tool missing after + * applyCapabilityFilter is a stale snapshot: a setup_error that never + * retries (non-continuable). These tests drive runSubAgent (the real mount + * point) end to end with failing inference — mount decisions run before + * the send, so the echo fires first. + */ + +import { describe, expect, test } from "bun:test"; +import { tmpdir } from "node:os"; +import { mkdtemp } from "node:fs/promises"; +import { join } from "node:path"; + +import { createPermissionGate } from "../permission/gate.js"; +import { runSubAgent } from "./run.js"; +import type { RunSubAgentParams } from "./types.js"; + +const testPermissionGate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: true, + reactorGated: false, +}); + +async function tmpCwd(): Promise { + return mkdtemp(join(tmpdir(), "cl9476-run-requires-tools-")); +} + +function baseParams(cwd: string, baseURL: string): RunSubAgentParams { + return { + cwd, + workdirBase: join(cwd, ".ctx"), + permissionGate: testPermissionGate, + provider: { providerName: "test", baseURL, model: "test-model" }, + description: "requires-tools probe", + prompt: "no-op", + }; +} + +// A local server answering 401 fails the inference send as a +// credential failure, which is never retried — the cycle costs one local +// round trip, and assertions stay timing-independent. +async function withFailingInference( + run: (baseURL: string) => Promise, +): Promise { + const server = Bun.serve({ + port: 0, + fetch: () => + new Response( + JSON.stringify({ error: { message: "requires-tools probe" } }), + { + status: 401, + headers: { "content-type": "application/json" }, + }, + ), + }); + try { + return await run(server.url.origin); + } finally { + server.stop(true); + } +} + +describe("runSubAgent requires_tools mount echo", () => { + test("stamped tool dropped by the filter throws stale_snapshot setup_error", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + requiresTools: ["run_shell"], + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + const message = (error as Error).message; + expect(message).toContain("stale_snapshot"); + expect(message).toContain("run_shell"); + expect(message).toContain("setup_error"); + expect(message).toContain("non-continuable"); + expect(message).toContain("Re-dispatch"); + expect(message).not.toContain('continuable": true'); + }, 15_000); + + test("stamped tool present in the mount passes through to inference", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + requiresTools: ["read_file"], + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain("stale_snapshot"); + }, 15_000); + + test("absent requiresTools leaves the mount path unchanged", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain("stale_snapshot"); + }, 15_000); +}); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index e35b395b5..8e9018390 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -67,6 +67,10 @@ import { } from "./intervention-log.js"; import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normalize.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { + checkMountedRequiresTools, + formatCapabilityUnavailable, +} from "./capability-preflight.js"; import { advertisedToolName, projectToolDefinitions, @@ -768,6 +772,29 @@ async function runSubAgentInner( if (params.capabilities !== undefined) { tools = applyCapabilityFilter(tools, params.capabilities); } + // CL-9476 mount echo: dispatch verified requires_tools pre-spawn, but the + // filter or mount may have shifted since — a stamped tool missing here is + // a stale snapshot, a setup_error that never retries (non-continuable). + // Dispatch-side rejection is the normal path; this throw never fires for + // a dispatch that passed preflight against the same mount. + if (params.requiresTools !== undefined && params.requiresTools.length > 0) { + const mountedNames = tools.map((tool) => tool.definition.name); + const mountedCheck = checkMountedRequiresTools( + params.requiresTools, + mountedNames, + ); + if (!mountedCheck.ok) { + throw new Error( + formatCapabilityUnavailable( + { + code: "stale_snapshot", + tool: mountedCheck.missing[0] ?? "unknown", + }, + params.directorId ?? params.description, + ), + ); + } + } backgroundCollectMounted = tools.some( (tool) => tool.definition.name === "shell_collect", ); diff --git a/src/subagent/session-store.ts b/src/subagent/session-store.ts index 0542ec481..3507d332c 100644 --- a/src/subagent/session-store.ts +++ b/src/subagent/session-store.ts @@ -150,6 +150,14 @@ export interface SubAgentSession { // Once close_agent runs, this flips back to false and the session is a // normal finished record subject to `maxCompleted` like any other. retained?: boolean; + /** + * Canonical tool names this worker hard-required at dispatch (CL-9476). + * Verified pre-spawn; the mount re-checks them and fails the run as a + * stale snapshot when one went missing in between. + */ + requiresTools?: readonly string[]; + /** Profile snapshot revision the dispatch preflight verified against. */ + snapshotRevision?: number; } export interface StartSessionInput { @@ -166,6 +174,10 @@ export interface StartSessionInput { retained?: boolean; /** Catalog provider id for followup admission. */ provider?: string; + /** Canonical tool names this worker hard-required at dispatch (CL-9476). */ + requiresTools?: readonly string[]; + /** Profile snapshot revision the dispatch preflight verified against. */ + snapshotRevision?: number; } export interface SubAgentSessionStoreOptions { @@ -1297,6 +1309,12 @@ export function createSubAgentSessionStore( ? { parentSessionId: input.parentSessionId } : {}), ...(input.provider !== undefined ? { provider: input.provider } : {}), + ...(input.requiresTools !== undefined + ? { requiresTools: [...input.requiresTools] } + : {}), + ...(input.snapshotRevision !== undefined + ? { snapshotRevision: input.snapshotRevision } + : {}), }; sessions.set(id, session); bumpRevision(id); @@ -2317,6 +2335,12 @@ function cloneSession( ? { parentSessionId: session.parentSessionId } : {}), ...(session.provider !== undefined ? { provider: session.provider } : {}), + ...(session.requiresTools !== undefined + ? { requiresTools: [...session.requiresTools] } + : {}), + ...(session.snapshotRevision !== undefined + ? { snapshotRevision: session.snapshotRevision } + : {}), }; } diff --git a/src/subagent/types.ts b/src/subagent/types.ts index bb17fd35a..25335340e 100644 --- a/src/subagent/types.ts +++ b/src/subagent/types.ts @@ -140,6 +140,13 @@ export type RunSubAgentParams = { onProgress?: (info: { description: string; toolName: string }) => void; onRunSettled?: (summary: Readonly) => void; capabilities?: CapabilityFilter; + /** + * Canonical tool names this worker hard-requires (CL-9476). Verified + * pre-spawn by the dispatcher; run.ts re-checks them against the mounted + * tools after the capability filter and fails the run as a stale snapshot + * when one went missing in between. + */ + requiresTools?: readonly string[]; /** * Skill allowlist for the worker's skill_search + use_skill mounts, * resolved by the caller (agent-fleet.ts) as the union of From 5913441ee26ad9b5a7dba36201b45bfdf2e6fdb6 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 29 Sep 2026 11:13:40 -0700 Subject: [PATCH 2/5] refactor(subagent): drop dead malformed branch from capability preflight --- src/agent/profiles.ts | 7 +- src/subagent/capability-preflight.test.ts | 53 +++++++------- src/subagent/capability-preflight.ts | 85 +++++++++++------------ 3 files changed, 69 insertions(+), 76 deletions(-) diff --git a/src/agent/profiles.ts b/src/agent/profiles.ts index 1c7d94d95..cb2135e1b 100644 --- a/src/agent/profiles.ts +++ b/src/agent/profiles.ts @@ -74,8 +74,11 @@ const registry: AgentProfile[] = [...defaultPlugin.agents]; // Load diagnostics: additive over loadAgentProfiles. `revision` stamps the // profile snapshot a dispatch was verified against (agent-fleet records it on // the session); `malformed` names local files that failed to load and why. -// A malformed file never blocks the load — it fails closed only when a -// dispatch's requires_tools preflight must verify against it (CL-9476). +// A malformed file never blocks the load — it is skipped and named here so +// callers can surface it. requires_tools preflight verifies against the +// resolved dispatch capabilities only, never against this list (CL-9476 keeps +// no malformed_profile preflight branch: agent-fleet never plumbed a failing +// source through dispatch, so the branch was dead and has been removed). export interface MalformedAgentProfile { path: string; reason: string; diff --git a/src/subagent/capability-preflight.test.ts b/src/subagent/capability-preflight.test.ts index 717dc795e..5d137e23b 100644 --- a/src/subagent/capability-preflight.test.ts +++ b/src/subagent/capability-preflight.test.ts @@ -74,7 +74,7 @@ describe("preflightCapabilities", () => { expect(result.unavailable.tool).toBe("run_shell"); }); - test("known tool with an absent binary rejects missing_binary", () => { + test("narrowed knownEngines reject missing_binary (test-only seam: production always passes the full catalog)", () => { const result = preflight(["run_shell"], ALLOW_SHELL, []); expect(result.ok).toBe(false); if (result.ok) throw new Error("expected rejection"); @@ -95,23 +95,6 @@ describe("preflightCapabilities", () => { expect(result.unavailable.suggestion).toBe("run_shell"); }); - test("malformed profile source fails closed before per-tool checks", () => { - const result = preflightCapabilities({ - required: ["read_file"], - resolvedFilter: ALLOW_SHELL, - knownEngines: DEFAULT_KNOWN_ENGINES, - agentLabel: "test-worker", - profileSource: { - malformed: true, - path: "/agents/broken.json", - reason: "unexpected token at line 3", - }, - }); - expect(result.ok).toBe(false); - if (result.ok) throw new Error("expected rejection"); - expect(result.unavailable.code).toBe("malformed_profile"); - }); - test("reroute alternatives collapse aliases (shell and run_shell agree)", () => { expect(rerouteAlternatives("shell")).toEqual( rerouteAlternatives("run_shell"), @@ -149,14 +132,6 @@ describe("formatCapabilityUnavailable", () => { code: "stale_snapshot", unavailable: { code: "stale_snapshot", tool: "run_shell" }, }, - { - code: "malformed_profile", - unavailable: { - code: "malformed_profile", - tool: "/agents/broken.json", - suggestion: "unexpected token at line 3", - }, - }, { code: "unknown_tool", unavailable: { @@ -181,9 +156,7 @@ describe("formatCapabilityUnavailable", () => { expect(sentences.length).toBeGreaterThanOrEqual(2); const action = sentences[sentences.length - 1] ?? ""; expect( - /re-dispatch|drop the requirement|install the backing runtime|fix the profile|correct requires_tools/i.test( - action, - ), + /re-dispatch|drop the requirement|correct requires_tools/i.test(action), ).toBe(true); }); } @@ -211,6 +184,28 @@ describe("formatCapabilityUnavailable", () => { expect(message).toContain(alternatives[0] ?? ""); }); + test("missing_tool detail renders a tier fact between fact and action", () => { + const message = formatCapabilityUnavailable( + { + code: "missing_tool", + tool: "spawn_agent", + alternatives: ["skywalker"], + detail: 'Tier 3 leaf directors cannot mount fleet verb "spawn_agent"', + }, + "test-worker", + ); + expect(message.startsWith("Error:")).toBe(true); + expect(message).toContain( + 'Tier 3 leaf directors cannot mount fleet verb "spawn_agent".', + ); + expect(message.indexOf("Tier 3 leaf")).toBeGreaterThan( + message.indexOf("allowlist omits it"), + ); + expect(message.indexOf("Tier 3 leaf")).toBeLessThan( + message.indexOf("Re-dispatch"), + ); + }); + test("unknown_tool message carries the did-you-mean hint", () => { const message = formatCapabilityUnavailable( { code: "unknown_tool", tool: "run_shel", suggestion: "run_shell" }, diff --git a/src/subagent/capability-preflight.ts b/src/subagent/capability-preflight.ts index d8fb0c9a0..0d8c92a6b 100644 --- a/src/subagent/capability-preflight.ts +++ b/src/subagent/capability-preflight.ts @@ -5,8 +5,14 @@ * mounted on the worker or the dispatch is rejected before any session, * telemetry, or worktree exists. Names are canonical engine ids on both sides * (wire/hidden aliases collapse via canonicalToolName), so `shell` and - * `run_shell` are the same requirement. Fail-closed throughout: unknown names, - * unverifiable profiles, and absent binaries all reject. + * `run_shell` are the same requirement. Fail-closed throughout: unknown names + * and allowlist/denylist misses all reject. + * + * `missing_binary` never fires in production: dispatch always passes the full + * catalog as `knownEngines`, so every catalogued engine verifies. Narrowed + * `knownEngines` sets are a test-only seam for simulating an incomplete + * runtime — the branch exists so tests can prove the fail-closed shape, not + * because production probes binaries per engine (most engines are in-process). * * The `stale_snapshot` code is never emitted by preflightCapabilities — it is * the mount-time echo in run.ts (a tool stamped at dispatch is missing from @@ -25,39 +31,37 @@ export type CapabilityUnavailableCode = | "permission_static" | "missing_binary" | "stale_snapshot" - | "malformed_profile" | "unknown_tool"; -export interface CapabilityProfileSource { - /** Directory or file the dispatched profile was loaded from, for messages. */ - path?: string; - /** True when the profile file failed to parse/validate — unverifiable. */ - malformed?: boolean; - /** Loader's reason for the malformed flag, for messages. */ - reason?: string; -} - export interface PreflightCapabilitiesInput { /** Raw requires_tools entries (aliases welcome — canonicalized here). */ required: readonly string[]; /** Resolved dispatch filter; undefined means full mount (everything passes). */ resolvedFilter?: CapabilityFilter | undefined; - /** Canonical engine ids available in this runtime (binary-present). */ + /** + * Canonical engine ids verifiable in this dispatch. Production always + * passes the full catalog (DEFAULT_KNOWN_ENGINES); narrowed sets are a + * test-only seam for simulating an incomplete runtime. + */ knownEngines: readonly string[]; /** Worker label for messages (director id or profile id). */ agentLabel: string; - /** Profile provenance; a malformed source fails closed. */ - profileSource?: CapabilityProfileSource; } export interface CapabilityUnavailable { code: CapabilityUnavailableCode; - /** Canonical tool id (or the raw entry for unknown/malformed). */ + /** Canonical tool id (or the raw entry for unknown). */ tool: string; /** Nearest known name, for the unknown_tool typo guard. */ suggestion?: string; /** Spawnable directors that mount the tool, for reroute hints. */ alternatives?: readonly string[]; + /** + * Extra fact sentence for missing_tool, naming a mount restriction the + * allowlist framing cannot see — e.g. a tier gate that withholds fleet + * verbs from leaves. Rendered between the fact and the action sentence. + */ + detail?: string; } export type CapabilityPreflightResult = @@ -98,14 +102,23 @@ export const KNOWN_CAPABILITY_ENGINES: readonly string[] = [ "submit_result", ]; -/** Production `knownEngines`: every known engine is binary-present. */ +/** + * Dispatch-time `knownEngines`: the full catalog. Dispatch always passes this, + * so every catalogued engine verifies and `missing_binary` never fires in + * production. Narrowed `knownEngines` sets are a test-only seam for + * simulating an incomplete runtime — production probes no binaries per engine + * (most engines are in-process). + */ export const DEFAULT_KNOWN_ENGINES: readonly string[] = KNOWN_CAPABILITY_ENGINES; /** - * Engines mounted outside the capability filter (run.ts mounts manage_tasks + * Engines mounted outside the capability filter (run.ts appends manage_tasks * after filtering), so a requires_tools entry for them passes preflight even - * when the dispatch filter is a narrow allowlist. + * when the dispatch filter is a narrow allowlist. The update_plan alias + * canonicalizes here, so it rides the same exemption — and the mount-time + * echo in run.ts runs after ALL appends, so the stamped entry always matches + * the live mount. */ const POST_FILTER_MOUNTED_ENGINES: readonly string[] = ["manage_tasks"]; @@ -181,19 +194,7 @@ export function rerouteAlternatives(canonical: string): readonly string[] { export function preflightCapabilities( input: PreflightCapabilitiesInput, ): CapabilityPreflightResult { - const { resolvedFilter, knownEngines, profileSource } = input; - if (profileSource?.malformed === true) { - return { - ok: false, - unavailable: { - code: "malformed_profile", - tool: profileSource.path ?? "(profile source)", - ...(profileSource.reason !== undefined - ? { suggestion: profileSource.reason } - : {}), - }, - }; - } + const { resolvedFilter, knownEngines } = input; const known = new Set(knownEngines.map((name) => canonicalToolName(name))); const allow = resolvedFilter?.mode === "allow" @@ -293,11 +294,15 @@ export function formatCapabilityUnavailable( ? ` Re-dispatch to one of (${unavailable.alternatives.join(", ")}) or drop the requirement.` : ` No spawnable director mounts "${unavailable.tool}" — drop the requirement or add a profile that mounts it.`; switch (unavailable.code) { - case "missing_tool": + case "missing_tool": { + const detail = + unavailable.detail !== undefined ? ` ${unavailable.detail}.` : ""; return ( `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its capability allowlist omits it.` + + detail + alternatives ); + } case "permission_static": return ( `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its capability denylist blocks it.` + @@ -305,24 +310,14 @@ export function formatCapabilityUnavailable( ); case "missing_binary": return ( - `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but its runtime engine is unavailable in this environment. ` + - `Re-dispatch without requires_tools=["${unavailable.tool}"] or install the backing runtime first.` + `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but this dispatch cannot verify its runtime engine (not in the dispatch's known-engine set). ` + + `Re-dispatch without requires_tools=["${unavailable.tool}"] or drop the requirement.` ); case "stale_snapshot": return ( `Error: stale_snapshot setup_error (non-continuable) — worker "${agentLabel}" was dispatched requiring "${unavailable.tool}" but the live mount no longer provides it. ` + `Re-dispatch the worker deliberately with a fresh brief instead of retrying in place.` ); - case "malformed_profile": { - const reason = - unavailable.suggestion !== undefined - ? ` (${unavailable.suggestion})` - : ""; - return ( - `Error: worker "${agentLabel}" cannot verify requires_tools — profile source "${unavailable.tool}" is malformed${reason}, so capabilities fail closed. ` + - `Fix the profile file and re-dispatch deliberately.` - ); - } case "unknown_tool": { const hint = unavailable.suggestion !== undefined From d00874396a94bebb024b240049ffc0b8b37a45f5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 29 Sep 2026 11:13:46 -0700 Subject: [PATCH 3/5] feat(subagent): reject tier-gated requires_tools before spawn --- .../agent-fleet-requires-tools.test.ts | 132 +++++++++++++++++- src/subagent/agent-fleet.ts | 54 +++++++ 2 files changed, 185 insertions(+), 1 deletion(-) diff --git a/src/subagent/agent-fleet-requires-tools.test.ts b/src/subagent/agent-fleet-requires-tools.test.ts index 996819070..8386d8f14 100644 --- a/src/subagent/agent-fleet-requires-tools.test.ts +++ b/src/subagent/agent-fleet-requires-tools.test.ts @@ -3,6 +3,7 @@ import { describe, expect, test } from "bun:test"; import { createFleetMailbox, createSpawnAgentTool, + tierGateRequiresTools, type AgentFleetDeps, } from "./agent-fleet.js"; import { unlimitedAdmissionQueue } from "./admission.js"; @@ -31,6 +32,11 @@ const READ_ONLY_PROFILE: AgentProfile = { capabilities: { mode: "allow", tools: ["read_file"] }, }; +const FULL_MOUNT_PROFILE: AgentProfile = { + id: "full-worker", + systemPromptRole: "You do anything.", +}; + function makeDeps( run: (params: RunSubAgentParams) => Promise, capturedTelemetry: TelemetryEvent[], @@ -45,7 +51,7 @@ function makeDeps( sessions, fleetRecords: createFleetMailbox(sessions), admission: unlimitedAdmissionQueue(), - profiles: [READ_ONLY_PROFILE], + profiles: [READ_ONLY_PROFILE, FULL_MOUNT_PROFILE], telemetry: { ...NOOP_TELEMETRY, capture: (event: TelemetryEvent) => { @@ -192,4 +198,128 @@ describe("spawn_agent requires_tools preflight", () => { expect(session?.requiresTools).toBeUndefined(); expect(session?.snapshotRevision).toBeUndefined(); }); + + test("manage_tasks survives dispatch under a narrow allowlist and stamps canonically", async () => { + const telemetry: TelemetryEvent[] = []; + let seenRequires: readonly string[] | undefined; + const deps = makeDeps(async (params) => { + seenRequires = params.requiresTools; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "plan job", + prompt: "track the work", + agent: "read-only-worker", + requires_tools: ["manage_tasks"], + }); + + expect(result.isError).not.toBe(true); + const body = JSON.parse(result.content) as { agent_id: string }; + expect(deps.sessions.get(body.agent_id)?.requiresTools).toEqual([ + "manage_tasks", + ]); + expect(seenRequires).toEqual(["manage_tasks"]); + }); + + test("update_plan alias survives dispatch and stamps manage_tasks", async () => { + const telemetry: TelemetryEvent[] = []; + const deps = makeDeps(async () => ({ report: "done" }), telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "alias plan job", + prompt: "track the work", + agent: "read-only-worker", + requires_tools: ["update_plan"], + }); + + expect(result.isError).not.toBe(true); + const body = JSON.parse(result.content) as { agent_id: string }; + expect(deps.sessions.get(body.agent_id)?.requiresTools).toEqual([ + "manage_tasks", + ]); + }); + + test("leaf requires_tools=[spawn_agent] rejects pre-spawn as missing_tool naming the leaf restriction", async () => { + const telemetry: TelemetryEvent[] = []; + let runCalled = false; + const deps = makeDeps(async () => { + runCalled = true; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "fleet job", + prompt: "spawn more workers", + agent: "full-worker", + requires_tools: ["spawn_agent"], + }); + + expect(result.isError).toBe(true); + expect(result.content.startsWith("Error:")).toBe(true); + expect(result.content).toContain("spawn_agent"); + expect(result.content).toContain("Tier 3 leaf"); + expect(result.content).toMatch(/Re-dispatch|drop the requirement/); + expect(result.content).not.toContain("stale_snapshot"); + expect(runCalled).toBe(false); + expect(deps.sessions.list()).toEqual([]); + expect(telemetry).toEqual([]); + }); +}); + +describe("tierGateRequiresTools", () => { + test("leaf requiring a fleet verb rejects as missing_tool naming the leaf restriction", () => { + const gated = tierGateRequiresTools(["spawn_agent"], "leaf"); + expect(gated?.code).toBe("missing_tool"); + expect(gated?.tool).toBe("spawn_agent"); + expect(gated?.detail).toContain("Tier 3 leaf"); + }); + + test("orchestrator requiring a fleet verb passes", () => { + expect( + tierGateRequiresTools(["spawn_agent"], "orchestrator"), + ).toBeUndefined(); + expect( + tierGateRequiresTools(["spawn_agent"], "nested-orchestrator"), + ).toBeUndefined(); + }); + + test("nested-orchestrator requiring fleet discovery rejects pre-spawn", () => { + const gated = tierGateRequiresTools( + ["search_agents"], + "nested-orchestrator", + ); + expect(gated?.code).toBe("missing_tool"); + expect(gated?.detail).toContain("Tier 2"); + }); + + test("non-leaf requiring the leaf reporting channel rejects pre-spawn", () => { + for (const engine of ["submit_result", "ask_director"]) { + const gated = tierGateRequiresTools([engine], "orchestrator"); + expect(gated?.code).toBe("missing_tool"); + expect(gated?.tool).toBe(engine); + expect(gated?.detail).toContain("Tier 3 leaf workers only"); + } + }); + + test("leaf requiring the leaf reporting channel passes", () => { + expect( + tierGateRequiresTools(["submit_result", "ask_director"], "leaf"), + ).toBeUndefined(); + }); + + test("ordinary tools pass on every tier", () => { + for (const tier of [ + "leaf", + "orchestrator", + "nested-orchestrator", + ] as const) { + expect( + tierGateRequiresTools(["read_file", "manage_tasks"], tier), + ).toBeUndefined(); + } + }); }); diff --git a/src/subagent/agent-fleet.ts b/src/subagent/agent-fleet.ts index 447f34afb..59afd39bf 100644 --- a/src/subagent/agent-fleet.ts +++ b/src/subagent/agent-fleet.ts @@ -72,6 +72,8 @@ import { DEFAULT_KNOWN_ENGINES, formatCapabilityUnavailable, preflightCapabilities, + rerouteAlternatives, + type CapabilityUnavailable, } from "./capability-preflight.js"; import { isCodexProviderName } from "../config/codex-providers.js"; import { buildDispatchBrief, type TaskIntent } from "./report.js"; @@ -107,6 +109,7 @@ import type { DirectorPackage, ModelRole } from "../agent/directors/types.js"; import { SPAWN_AGENT_TOOL_NAME } from "./tool-taxonomy.js"; import { assertCanTargetAgent, + assertTierMayMountFleetVerb, FleetAuthorityError, type FleetNode, type SubagentTier, @@ -723,6 +726,44 @@ function fleetJson(value: unknown): string { return JSON.stringify(value, null, 2); } +/** + * Tier gate for stamped requires_tools (CL-9476). run.ts mounts submit_result + * and ask_director on Tier 3 leaves only, and fleet verbs on orchestrator + * tiers only — a requirement the dispatch tier can never mount rejects here + * as missing_tool (pre-spawn) instead of surviving to the mount-time stale + * echo. Returns the rejection, or undefined when the tier mounts everything + * required. + */ +export function tierGateRequiresTools( + canonical: readonly string[], + tier: SubagentTier, +): CapabilityUnavailable | undefined { + for (const engine of canonical) { + if ( + (engine === "submit_result" || engine === "ask_director") && + tier !== "leaf" + ) { + return { + code: "missing_tool", + tool: engine, + detail: `"${engine}" mounts on Tier 3 leaf workers only, never on ${tier} directors`, + }; + } + try { + assertTierMayMountFleetVerb(tier, engine); + } catch (error) { + if (!(error instanceof FleetAuthorityError)) throw error; + return { + code: "missing_tool", + tool: engine, + alternatives: rerouteAlternatives(engine), + detail: error.message.split(". ")[0] ?? error.message, + }; + } + } + return undefined; +} + interface ResolvedAgentDispatch { directorId: string; agentLabel: string; @@ -1106,6 +1147,19 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { ); } requiresTools = preflight.canonical; + // Tier-gated mounts (fleet verbs, the Tier 3 leaf reporting channel) + // reject here as missing_tool when the dispatch tier can never mount + // them — pre-spawn, never surviving to the mount-time stale echo. + const dispatchTier: SubagentTier = resolved.orchestrator + ? (resolved.orchestratorTier ?? resolved.pkg?.tier ?? "orchestrator") + : "leaf"; + const gated = tierGateRequiresTools(preflight.canonical, dispatchTier); + if (gated !== undefined) { + return fleetResult( + call.id, + formatCapabilityUnavailable(gated, resolved.agentLabel), + ); + } snapshotRevision = currentProfileSnapshotRevision(); } From b2a596619fe33081b4dc4512c4832dc9be5576d2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 29 Sep 2026 11:13:49 -0700 Subject: [PATCH 4/5] fix(subagent): check requires_tools after all run mounts append --- src/subagent/run-requires-tools.test.ts | 38 ++++++++++++++++++++ src/subagent/run.ts | 48 +++++++++++++------------ 2 files changed, 63 insertions(+), 23 deletions(-) diff --git a/src/subagent/run-requires-tools.test.ts b/src/subagent/run-requires-tools.test.ts index ef6d56381..45fd43b25 100644 --- a/src/subagent/run-requires-tools.test.ts +++ b/src/subagent/run-requires-tools.test.ts @@ -109,6 +109,44 @@ describe("runSubAgent requires_tools mount echo", () => { expect((error as Error).message).not.toContain("stale_snapshot"); }, 15_000); + test("stamped manage_tasks survives the echo under a narrow allowlist", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + requiresTools: ["manage_tasks"], + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain("stale_snapshot"); + }, 15_000); + + test("stamped update_plan alias survives the echo under a narrow allowlist", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + requiresTools: ["update_plan"], + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain("stale_snapshot"); + }, 15_000); + test("absent requiresTools leaves the mount path unchanged", async () => { const cwd = await tmpCwd(); const error = await withFailingInference(async (baseURL) => { diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 8e9018390..840934a46 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -772,29 +772,6 @@ async function runSubAgentInner( if (params.capabilities !== undefined) { tools = applyCapabilityFilter(tools, params.capabilities); } - // CL-9476 mount echo: dispatch verified requires_tools pre-spawn, but the - // filter or mount may have shifted since — a stamped tool missing here is - // a stale snapshot, a setup_error that never retries (non-continuable). - // Dispatch-side rejection is the normal path; this throw never fires for - // a dispatch that passed preflight against the same mount. - if (params.requiresTools !== undefined && params.requiresTools.length > 0) { - const mountedNames = tools.map((tool) => tool.definition.name); - const mountedCheck = checkMountedRequiresTools( - params.requiresTools, - mountedNames, - ); - if (!mountedCheck.ok) { - throw new Error( - formatCapabilityUnavailable( - { - code: "stale_snapshot", - tool: mountedCheck.missing[0] ?? "unknown", - }, - params.directorId ?? params.description, - ), - ); - } - } backgroundCollectMounted = tools.some( (tool) => tool.definition.name === "shell_collect", ); @@ -1022,6 +999,31 @@ async function runSubAgentInner( ]; } + // CL-9476 mount echo: dispatch verified requires_tools pre-spawn, but the + // filter or mount may have shifted since — a stamped tool missing here is + // a stale snapshot, a setup_error that never retries (non-continuable). + // Dispatch-side rejection is the normal path. This check runs after ALL + // mounts (manage_tasks, leaf submit_result/ask_director, fleet verbs), so + // a dispatch that passed preflight against the same mount never throws. + if (params.requiresTools !== undefined && params.requiresTools.length > 0) { + const mountedNames = tools.map((tool) => tool.definition.name); + const mountedCheck = checkMountedRequiresTools( + params.requiresTools, + mountedNames, + ); + if (!mountedCheck.ok) { + throw new Error( + formatCapabilityUnavailable( + { + code: "stale_snapshot", + tool: mountedCheck.missing[0] ?? "unknown", + }, + params.directorId ?? params.description, + ), + ); + } + } + tools = wrapAgentToolsWithResultTruncation(tools, { getBlobWriter: () => childBlobWriter, getContextDir: () => childContextDir, From 7eaeaddb2674f1a6544170791e348225fa071c85 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 29 Sep 2026 12:31:22 -0700 Subject: [PATCH 5/5] fix(subagent): address requires_tools review nits without weakening fail-closed --- .../agent-fleet-requires-tools.test.ts | 79 ++++++++++++++++++- src/subagent/agent-fleet.ts | 41 ++++++---- src/subagent/capability-preflight.test.ts | 38 +++++++++ src/subagent/capability-preflight.ts | 50 +++++++++--- src/subagent/run-requires-tools.test.ts | 22 ++++++ src/subagent/run.ts | 1 + src/subagent/session-store.ts | 10 --- 7 files changed, 203 insertions(+), 38 deletions(-) diff --git a/src/subagent/agent-fleet-requires-tools.test.ts b/src/subagent/agent-fleet-requires-tools.test.ts index 8386d8f14..6447ee21a 100644 --- a/src/subagent/agent-fleet-requires-tools.test.ts +++ b/src/subagent/agent-fleet-requires-tools.test.ts @@ -6,6 +6,7 @@ import { tierGateRequiresTools, type AgentFleetDeps, } from "./agent-fleet.js"; +import { formatCapabilityUnavailable } from "./capability-preflight.js"; import { unlimitedAdmissionQueue } from "./admission.js"; import { createPermissionGate } from "../permission/gate.js"; import { NOOP_TELEMETRY, type TelemetryEvent } from "../telemetry/index.js"; @@ -157,7 +158,7 @@ describe("spawn_agent requires_tools preflight", () => { }; const session = deps.sessions.get(body.agent_id); expect(session?.requiresTools).toEqual(["read_file"]); - expect(typeof session?.snapshotRevision).toBe("number"); + expect("snapshotRevision" in (session ?? {})).toBe(false); expect(seenRequires).toEqual(["read_file"]); expect(telemetry).toContain("subagent_start"); }); @@ -196,7 +197,7 @@ describe("spawn_agent requires_tools preflight", () => { const body = JSON.parse(result.content) as { agent_id: string }; const session = deps.sessions.get(body.agent_id); expect(session?.requiresTools).toBeUndefined(); - expect(session?.snapshotRevision).toBeUndefined(); + expect("snapshotRevision" in (session ?? {})).toBe(false); }); test("manage_tasks survives dispatch under a narrow allowlist and stamps canonically", async () => { @@ -268,6 +269,62 @@ describe("spawn_agent requires_tools preflight", () => { expect(deps.sessions.list()).toEqual([]); expect(telemetry).toEqual([]); }); + + test("leaf requires_tools=[submit_result] passes preflight under a narrow allowlist (leaf reporting channel)", async () => { + const telemetry: TelemetryEvent[] = []; + let runCalled = false; + let seenRequires: readonly string[] | undefined; + let seenTier: unknown; + const deps = makeDeps(async (params) => { + runCalled = true; + seenRequires = params.requiresTools; + seenTier = params.tier; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "leaf report job", + prompt: "read a file and report", + agent: "read-only-worker", + requires_tools: ["submit_result"], + }); + + expect(result.isError).not.toBe(true); + expect(runCalled).toBe(true); + const body = JSON.parse(result.content) as { agent_id: string }; + expect(deps.sessions.get(body.agent_id)?.requiresTools).toEqual([ + "submit_result", + ]); + expect(seenRequires).toEqual(["submit_result"]); + expect(seenTier).toBe("leaf"); + }); + + test("whitespace-only requires_tools rejects fail-closed with no session, telemetry, or run", async () => { + for (const requiresTools of [[" "], ["read_file", " "]]) { + const telemetry: TelemetryEvent[] = []; + let runCalled = false; + const deps = makeDeps(async () => { + runCalled = true; + return { report: "done" }; + }, telemetry); + const spawn = createSpawnAgentTool(deps); + + const result = await callSpawn(spawn, { + description: "blank job", + prompt: "do it", + agent: "read-only-worker", + requires_tools: requiresTools, + }); + + expect(result.isError).toBe(true); + expect(result.content.startsWith("Error:")).toBe(true); + expect(result.content).toContain("non-empty tool names"); + expect(runCalled).toBe(false); + expect(deps.sessions.list()).toEqual([]); + expect(telemetry).toEqual([]); + } + }); }); describe("tierGateRequiresTools", () => { @@ -305,6 +362,24 @@ describe("tierGateRequiresTools", () => { } }); + test("tier-gated leaf channel names Tier 3 leaves instead of claiming no director mounts it", () => { + const gated = tierGateRequiresTools(["submit_result"], "orchestrator"); + expect(gated?.alternatives ?? []).toEqual([ + "bruckheimer", + "builder", + "counsel", + ]); + const message = formatCapabilityUnavailable( + gated ?? { code: "missing_tool", tool: "submit_result" }, + "test-orchestrator", + ); + expect(message).toContain("Tier 3 leaf workers only"); + expect(message).toContain( + "Re-dispatch to one of (bruckheimer, builder, counsel)", + ); + expect(message).not.toContain("No spawnable director mounts"); + }); + test("leaf requiring the leaf reporting channel passes", () => { expect( tierGateRequiresTools(["submit_result", "ask_director"], "leaf"), diff --git a/src/subagent/agent-fleet.ts b/src/subagent/agent-fleet.ts index 59afd39bf..9694703a2 100644 --- a/src/subagent/agent-fleet.ts +++ b/src/subagent/agent-fleet.ts @@ -67,10 +67,10 @@ import { type ReasoningEffort, } from "../provider/reasoning-effort.js"; import type { AgentProfile, CapabilityFilter } from "../agent/profiles.js"; -import { currentProfileSnapshotRevision } from "../agent/profiles.js"; import { DEFAULT_KNOWN_ENGINES, formatCapabilityUnavailable, + leafTierAlternatives, preflightCapabilities, rerouteAlternatives, type CapabilityUnavailable, @@ -746,6 +746,7 @@ export function tierGateRequiresTools( return { code: "missing_tool", tool: engine, + alternatives: leafTierAlternatives(), detail: `"${engine}" mounts on Tier 3 leaf workers only, never on ${tier} directors`, }; } @@ -1035,9 +1036,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { const doNot = rawDoNot?.map((d) => d.trim()).filter((d) => d.length > 0) ?? []; const reportFocus = rawReportFocus?.trim(); - const requiresToolsRaw = - rawRequiresTools?.map((t) => t.trim()).filter((t) => t.length > 0) ?? - []; let provider: SubAgentProvider = resolveDep(deps.provider); const parentEffort = provider.reasoningEffort; @@ -1126,8 +1124,23 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { // CL-9476: fail closed before any session, telemetry, or worktree exists. // A rejection returns here — no auto re-dispatch, no successor, no retry. + // The tier below is the single derivation shared by the tier gate and + // the run mount: run.ts mounts the leaf reporting channel exactly when + // tier is "leaf", so gating on any other value would let a requirement + // pass here and die as a stale snapshot at mount (or vice versa). + const dispatchTier: SubagentTier = resolved.orchestrator + ? (resolved.orchestratorTier ?? resolved.pkg?.tier ?? "orchestrator") + : "leaf"; let requiresTools: readonly string[] | undefined; - let snapshotRevision: number | undefined; + const rawEntries = rawRequiresTools ?? []; + const blankEntry = rawEntries.find((t) => t.trim().length === 0); + if (blankEntry !== undefined) { + return fleetResult( + call.id, + `Error: spawn_agent requires_tools entries must be non-empty tool names — got a blank entry. Correct requires_tools to canonical tool names and re-dispatch.`, + ); + } + const requiresToolsRaw = rawEntries.map((t) => t.trim()); if (requiresToolsRaw.length > 0) { const preflight = preflightCapabilities({ required: requiresToolsRaw, @@ -1150,9 +1163,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { // Tier-gated mounts (fleet verbs, the Tier 3 leaf reporting channel) // reject here as missing_tool when the dispatch tier can never mount // them — pre-spawn, never surviving to the mount-time stale echo. - const dispatchTier: SubagentTier = resolved.orchestrator - ? (resolved.orchestratorTier ?? resolved.pkg?.tier ?? "orchestrator") - : "leaf"; const gated = tierGateRequiresTools(preflight.canonical, dispatchTier); if (gated !== undefined) { return fleetResult( @@ -1160,7 +1170,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { formatCapabilityUnavailable(gated, resolved.agentLabel), ); } - snapshotRevision = currentProfileSnapshotRevision(); } const orchestrator = resolved.orchestrator; @@ -1208,7 +1217,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { retained: true, provider: provider.providerName, ...(requiresTools !== undefined ? { requiresTools } : {}), - ...(snapshotRevision !== undefined ? { snapshotRevision } : {}), ...(deps.parentSessionId !== undefined ? { parentSessionId: deps.parentSessionId } : {}), @@ -1540,11 +1548,14 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { ...(deps.deadlineMs !== undefined ? { deadlineMs: deps.deadlineMs } : {}), - ...(resolved.pkg !== undefined - ? { tier: resolved.pkg.tier } - : orchestrator - ? {} - : { tier: "leaf" }), + // Single tier derivation (dispatchTier above): the tier gate and + // the mount agree by construction, so a gated requirement can + // never die as a stale snapshot at mount, or vice versa. An + // orchestrator dispatch without a resolved package carries no + // tier — run.ts fails closed on fleet mounts from there. + ...(resolved.pkg !== undefined || !orchestrator + ? { tier: dispatchTier } + : {}), ...(resolved.pkg?.reportContract?.outputType !== undefined ? { reportType: resolved.pkg.reportContract.outputType } : {}), diff --git a/src/subagent/capability-preflight.test.ts b/src/subagent/capability-preflight.test.ts index 5d137e23b..864a51968 100644 --- a/src/subagent/capability-preflight.test.ts +++ b/src/subagent/capability-preflight.test.ts @@ -4,6 +4,7 @@ import { checkMountedRequiresTools, DEFAULT_KNOWN_ENGINES, formatCapabilityUnavailable, + leafTierAlternatives, preflightCapabilities, rerouteAlternatives, type CapabilityUnavailable, @@ -101,6 +102,29 @@ describe("preflightCapabilities", () => { ); expect(rerouteAlternatives("run_shell").length).toBeGreaterThan(0); }); + + test("reroute alternatives sort before the cap of 3 (read_file has 19 mounting directors)", () => { + const alternatives = rerouteAlternatives("read_file"); + expect(alternatives).toEqual(["bruckheimer", "builder", "counsel"]); + expect(alternatives).toEqual([...alternatives].sort()); + }); + + test("leaf reporting channel passes preflight under a narrow allowlist omitting post-filter mounts", () => { + for (const engine of ["submit_result", "ask_director"]) { + expect(preflight([engine], ALLOW_READ_ONLY)).toEqual({ + ok: true, + canonical: [engine], + }); + } + }); + + test("leafTierAlternatives names sorted Tier 3 leaf directors, never skywalker", () => { + const alternatives = leafTierAlternatives(); + expect(alternatives.length).toBeGreaterThan(0); + expect(alternatives.length).toBeLessThanOrEqual(3); + expect(alternatives).toEqual([...alternatives].sort()); + expect(alternatives).not.toContain("skywalker"); + }); }); describe("formatCapabilityUnavailable", () => { @@ -213,6 +237,20 @@ describe("formatCapabilityUnavailable", () => { ); expect(message).toContain('Did you mean "run_shell"?'); }); + + test("stale_snapshot message names every missing tool, not just the first", () => { + const message = formatCapabilityUnavailable( + { + code: "stale_snapshot", + tool: "run_shell", + tools: ["run_shell", "write_file"], + }, + "test-worker", + ); + expect(message).toContain("stale_snapshot"); + expect(message).toContain("run_shell"); + expect(message).toContain("write_file"); + }); }); describe("checkMountedRequiresTools", () => { diff --git a/src/subagent/capability-preflight.ts b/src/subagent/capability-preflight.ts index 0d8c92a6b..23f1c150e 100644 --- a/src/subagent/capability-preflight.ts +++ b/src/subagent/capability-preflight.ts @@ -52,6 +52,8 @@ export interface CapabilityUnavailable { code: CapabilityUnavailableCode; /** Canonical tool id (or the raw entry for unknown). */ tool: string; + /** Every missing tool, for the multi-missing stale_snapshot echo. */ + tools?: readonly string[]; /** Nearest known name, for the unknown_tool typo guard. */ suggestion?: string; /** Spawnable directors that mount the tool, for reroute hints. */ @@ -114,13 +116,21 @@ export const DEFAULT_KNOWN_ENGINES: readonly string[] = /** * Engines mounted outside the capability filter (run.ts appends manage_tasks - * after filtering), so a requires_tools entry for them passes preflight even - * when the dispatch filter is a narrow allowlist. The update_plan alias - * canonicalizes here, so it rides the same exemption — and the mount-time - * echo in run.ts runs after ALL appends, so the stamped entry always matches - * the live mount. + * after filtering, and mounts the Tier 3 leaf reporting channel + * submit_result/ask_director whenever tier is "leaf"), so a requires_tools + * entry for them passes preflight even when the dispatch filter is a narrow + * allowlist. The update_plan alias canonicalizes here, so it rides the same + * exemption — and the mount-time echo in run.ts runs after ALL appends, so + * the stamped entry always matches the live mount. Fail-closed: the tier gate + * in agent-fleet.ts still rejects submit_result/ask_director on non-leaf + * tiers after this exemption, so the bypass never mounts them where run.ts + * would not. */ -const POST_FILTER_MOUNTED_ENGINES: readonly string[] = ["manage_tasks"]; +const POST_FILTER_MOUNTED_ENGINES: readonly string[] = [ + "manage_tasks", + "submit_result", + "ask_director", +]; /** Alias spellings accepted in requires_tools, for typo suggestions. */ const SUGGESTION_CANDIDATES: readonly string[] = [ @@ -169,7 +179,9 @@ function nearestToolName(raw: string): string | undefined { /** * Spawnable directors (closed set minus primary skywalker) whose mounted * tool set includes `canonical` — derived from packageToCapabilities over - * DIRECTOR_REGISTRY, so the hint tracks the envelopes. Capped for messages. + * DIRECTOR_REGISTRY, so the hint tracks the envelopes. Sorted before the cap + * so the three named are the first alphabetically, not the first in registry + * insertion order. */ export function rerouteAlternatives(canonical: string): readonly string[] { const want = canonicalToolName(canonical); @@ -186,9 +198,23 @@ export function rerouteAlternatives(canonical: string): readonly string[] { } else if (!capabilities.tools.some((t) => canonicalToolName(t) === want)) { out.push(pkg.id); } - if (out.length >= 3) break; } - return out.sort(); + return out.sort().slice(0, 3); +} + +/** + * Tier-3 leaf directors (closed set, skywalker excluded) for the tier-gate + * hint when requires_tools names the leaf reporting channel on a non-leaf + * tier. submit_result/ask_director mount post-filter, so no envelope mentions + * them and rerouteAlternatives would report none — this names the directors + * that actually mount them instead of the allowlist-miss fallback. + */ +export function leafTierAlternatives(): readonly string[] { + return Object.values(DIRECTOR_REGISTRY) + .filter((pkg) => pkg.id !== "skywalker" && pkg.tier === "leaf") + .map((pkg) => pkg.id) + .sort() + .slice(0, 3); } export function preflightCapabilities( @@ -313,11 +339,13 @@ export function formatCapabilityUnavailable( `Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but this dispatch cannot verify its runtime engine (not in the dispatch's known-engine set). ` + `Re-dispatch without requires_tools=["${unavailable.tool}"] or drop the requirement.` ); - case "stale_snapshot": + case "stale_snapshot": { + const missing = unavailable.tools ?? [unavailable.tool]; return ( - `Error: stale_snapshot setup_error (non-continuable) — worker "${agentLabel}" was dispatched requiring "${unavailable.tool}" but the live mount no longer provides it. ` + + `Error: stale_snapshot setup_error (non-continuable) — worker "${agentLabel}" was dispatched requiring "${missing.join('", "')}" but the live mount no longer provides ${missing.length === 1 ? "it" : "them"}. ` + `Re-dispatch the worker deliberately with a fresh brief instead of retrying in place.` ); + } case "unknown_tool": { const hint = unavailable.suggestion !== undefined diff --git a/src/subagent/run-requires-tools.test.ts b/src/subagent/run-requires-tools.test.ts index 45fd43b25..d7311da75 100644 --- a/src/subagent/run-requires-tools.test.ts +++ b/src/subagent/run-requires-tools.test.ts @@ -90,6 +90,28 @@ describe("runSubAgent requires_tools mount echo", () => { expect(message).not.toContain('continuable": true'); }, 15_000); + test("two stamped tools dropped by the filter are both named in the stale_snapshot error", async () => { + const cwd = await tmpCwd(); + const error = await withFailingInference(async (baseURL) => { + try { + await runSubAgent({ + ...baseParams(cwd, baseURL), + capabilities: { mode: "allow", tools: ["read_file"] }, + requiresTools: ["run_shell", "write_file"], + }); + } catch (err) { + return err; + } + throw new Error("runSubAgent did not throw"); + }); + + expect(error).toBeInstanceOf(Error); + const message = (error as Error).message; + expect(message).toContain("stale_snapshot"); + expect(message).toContain("run_shell"); + expect(message).toContain("write_file"); + }, 15_000); + test("stamped tool present in the mount passes through to inference", async () => { const cwd = await tmpCwd(); const error = await withFailingInference(async (baseURL) => { diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 840934a46..b1e3e3bad 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -1017,6 +1017,7 @@ async function runSubAgentInner( { code: "stale_snapshot", tool: mountedCheck.missing[0] ?? "unknown", + tools: mountedCheck.missing, }, params.directorId ?? params.description, ), diff --git a/src/subagent/session-store.ts b/src/subagent/session-store.ts index 3507d332c..346df460d 100644 --- a/src/subagent/session-store.ts +++ b/src/subagent/session-store.ts @@ -156,8 +156,6 @@ export interface SubAgentSession { * stale snapshot when one went missing in between. */ requiresTools?: readonly string[]; - /** Profile snapshot revision the dispatch preflight verified against. */ - snapshotRevision?: number; } export interface StartSessionInput { @@ -176,8 +174,6 @@ export interface StartSessionInput { provider?: string; /** Canonical tool names this worker hard-required at dispatch (CL-9476). */ requiresTools?: readonly string[]; - /** Profile snapshot revision the dispatch preflight verified against. */ - snapshotRevision?: number; } export interface SubAgentSessionStoreOptions { @@ -1312,9 +1308,6 @@ export function createSubAgentSessionStore( ...(input.requiresTools !== undefined ? { requiresTools: [...input.requiresTools] } : {}), - ...(input.snapshotRevision !== undefined - ? { snapshotRevision: input.snapshotRevision } - : {}), }; sessions.set(id, session); bumpRevision(id); @@ -2338,9 +2331,6 @@ function cloneSession( ...(session.requiresTools !== undefined ? { requiresTools: [...session.requiresTools] } : {}), - ...(session.snapshotRevision !== undefined - ? { snapshotRevision: session.snapshotRevision } - : {}), }; }