Skip to content

Commit 7eaeadd

Browse files
committed
fix(subagent): address requires_tools review nits without weakening fail-closed
1 parent b2a5966 commit 7eaeadd

7 files changed

Lines changed: 203 additions & 38 deletions

‎src/subagent/agent-fleet-requires-tools.test.ts‎

Lines changed: 77 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
tierGateRequiresTools,
77
type AgentFleetDeps,
88
} from "./agent-fleet.js";
9+
import { formatCapabilityUnavailable } from "./capability-preflight.js";
910
import { unlimitedAdmissionQueue } from "./admission.js";
1011
import { createPermissionGate } from "../permission/gate.js";
1112
import { NOOP_TELEMETRY, type TelemetryEvent } from "../telemetry/index.js";
@@ -157,7 +158,7 @@ describe("spawn_agent requires_tools preflight", () => {
157158
};
158159
const session = deps.sessions.get(body.agent_id);
159160
expect(session?.requiresTools).toEqual(["read_file"]);
160-
expect(typeof session?.snapshotRevision).toBe("number");
161+
expect("snapshotRevision" in (session ?? {})).toBe(false);
161162
expect(seenRequires).toEqual(["read_file"]);
162163
expect(telemetry).toContain("subagent_start");
163164
});
@@ -196,7 +197,7 @@ describe("spawn_agent requires_tools preflight", () => {
196197
const body = JSON.parse(result.content) as { agent_id: string };
197198
const session = deps.sessions.get(body.agent_id);
198199
expect(session?.requiresTools).toBeUndefined();
199-
expect(session?.snapshotRevision).toBeUndefined();
200+
expect("snapshotRevision" in (session ?? {})).toBe(false);
200201
});
201202

202203
test("manage_tasks survives dispatch under a narrow allowlist and stamps canonically", async () => {
@@ -268,6 +269,62 @@ describe("spawn_agent requires_tools preflight", () => {
268269
expect(deps.sessions.list()).toEqual([]);
269270
expect(telemetry).toEqual([]);
270271
});
272+
273+
test("leaf requires_tools=[submit_result] passes preflight under a narrow allowlist (leaf reporting channel)", async () => {
274+
const telemetry: TelemetryEvent[] = [];
275+
let runCalled = false;
276+
let seenRequires: readonly string[] | undefined;
277+
let seenTier: unknown;
278+
const deps = makeDeps(async (params) => {
279+
runCalled = true;
280+
seenRequires = params.requiresTools;
281+
seenTier = params.tier;
282+
return { report: "done" };
283+
}, telemetry);
284+
const spawn = createSpawnAgentTool(deps);
285+
286+
const result = await callSpawn(spawn, {
287+
description: "leaf report job",
288+
prompt: "read a file and report",
289+
agent: "read-only-worker",
290+
requires_tools: ["submit_result"],
291+
});
292+
293+
expect(result.isError).not.toBe(true);
294+
expect(runCalled).toBe(true);
295+
const body = JSON.parse(result.content) as { agent_id: string };
296+
expect(deps.sessions.get(body.agent_id)?.requiresTools).toEqual([
297+
"submit_result",
298+
]);
299+
expect(seenRequires).toEqual(["submit_result"]);
300+
expect(seenTier).toBe("leaf");
301+
});
302+
303+
test("whitespace-only requires_tools rejects fail-closed with no session, telemetry, or run", async () => {
304+
for (const requiresTools of [[" "], ["read_file", " "]]) {
305+
const telemetry: TelemetryEvent[] = [];
306+
let runCalled = false;
307+
const deps = makeDeps(async () => {
308+
runCalled = true;
309+
return { report: "done" };
310+
}, telemetry);
311+
const spawn = createSpawnAgentTool(deps);
312+
313+
const result = await callSpawn(spawn, {
314+
description: "blank job",
315+
prompt: "do it",
316+
agent: "read-only-worker",
317+
requires_tools: requiresTools,
318+
});
319+
320+
expect(result.isError).toBe(true);
321+
expect(result.content.startsWith("Error:")).toBe(true);
322+
expect(result.content).toContain("non-empty tool names");
323+
expect(runCalled).toBe(false);
324+
expect(deps.sessions.list()).toEqual([]);
325+
expect(telemetry).toEqual([]);
326+
}
327+
});
271328
});
272329

273330
describe("tierGateRequiresTools", () => {
@@ -305,6 +362,24 @@ describe("tierGateRequiresTools", () => {
305362
}
306363
});
307364

365+
test("tier-gated leaf channel names Tier 3 leaves instead of claiming no director mounts it", () => {
366+
const gated = tierGateRequiresTools(["submit_result"], "orchestrator");
367+
expect(gated?.alternatives ?? []).toEqual([
368+
"bruckheimer",
369+
"builder",
370+
"counsel",
371+
]);
372+
const message = formatCapabilityUnavailable(
373+
gated ?? { code: "missing_tool", tool: "submit_result" },
374+
"test-orchestrator",
375+
);
376+
expect(message).toContain("Tier 3 leaf workers only");
377+
expect(message).toContain(
378+
"Re-dispatch to one of (bruckheimer, builder, counsel)",
379+
);
380+
expect(message).not.toContain("No spawnable director mounts");
381+
});
382+
308383
test("leaf requiring the leaf reporting channel passes", () => {
309384
expect(
310385
tierGateRequiresTools(["submit_result", "ask_director"], "leaf"),

‎src/subagent/agent-fleet.ts‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,10 @@ import {
6767
type ReasoningEffort,
6868
} from "../provider/reasoning-effort.js";
6969
import type { AgentProfile, CapabilityFilter } from "../agent/profiles.js";
70-
import { currentProfileSnapshotRevision } from "../agent/profiles.js";
7170
import {
7271
DEFAULT_KNOWN_ENGINES,
7372
formatCapabilityUnavailable,
73+
leafTierAlternatives,
7474
preflightCapabilities,
7575
rerouteAlternatives,
7676
type CapabilityUnavailable,
@@ -746,6 +746,7 @@ export function tierGateRequiresTools(
746746
return {
747747
code: "missing_tool",
748748
tool: engine,
749+
alternatives: leafTierAlternatives(),
749750
detail: `"${engine}" mounts on Tier 3 leaf workers only, never on ${tier} directors`,
750751
};
751752
}
@@ -1035,9 +1036,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
10351036
const doNot =
10361037
rawDoNot?.map((d) => d.trim()).filter((d) => d.length > 0) ?? [];
10371038
const reportFocus = rawReportFocus?.trim();
1038-
const requiresToolsRaw =
1039-
rawRequiresTools?.map((t) => t.trim()).filter((t) => t.length > 0) ??
1040-
[];
10411039

10421040
let provider: SubAgentProvider = resolveDep(deps.provider);
10431041
const parentEffort = provider.reasoningEffort;
@@ -1126,8 +1124,23 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
11261124

11271125
// CL-9476: fail closed before any session, telemetry, or worktree exists.
11281126
// A rejection returns here — no auto re-dispatch, no successor, no retry.
1127+
// The tier below is the single derivation shared by the tier gate and
1128+
// the run mount: run.ts mounts the leaf reporting channel exactly when
1129+
// tier is "leaf", so gating on any other value would let a requirement
1130+
// pass here and die as a stale snapshot at mount (or vice versa).
1131+
const dispatchTier: SubagentTier = resolved.orchestrator
1132+
? (resolved.orchestratorTier ?? resolved.pkg?.tier ?? "orchestrator")
1133+
: "leaf";
11291134
let requiresTools: readonly string[] | undefined;
1130-
let snapshotRevision: number | undefined;
1135+
const rawEntries = rawRequiresTools ?? [];
1136+
const blankEntry = rawEntries.find((t) => t.trim().length === 0);
1137+
if (blankEntry !== undefined) {
1138+
return fleetResult(
1139+
call.id,
1140+
`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.`,
1141+
);
1142+
}
1143+
const requiresToolsRaw = rawEntries.map((t) => t.trim());
11311144
if (requiresToolsRaw.length > 0) {
11321145
const preflight = preflightCapabilities({
11331146
required: requiresToolsRaw,
@@ -1150,17 +1163,13 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
11501163
// Tier-gated mounts (fleet verbs, the Tier 3 leaf reporting channel)
11511164
// reject here as missing_tool when the dispatch tier can never mount
11521165
// them — pre-spawn, never surviving to the mount-time stale echo.
1153-
const dispatchTier: SubagentTier = resolved.orchestrator
1154-
? (resolved.orchestratorTier ?? resolved.pkg?.tier ?? "orchestrator")
1155-
: "leaf";
11561166
const gated = tierGateRequiresTools(preflight.canonical, dispatchTier);
11571167
if (gated !== undefined) {
11581168
return fleetResult(
11591169
call.id,
11601170
formatCapabilityUnavailable(gated, resolved.agentLabel),
11611171
);
11621172
}
1163-
snapshotRevision = currentProfileSnapshotRevision();
11641173
}
11651174

11661175
const orchestrator = resolved.orchestrator;
@@ -1208,7 +1217,6 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
12081217
retained: true,
12091218
provider: provider.providerName,
12101219
...(requiresTools !== undefined ? { requiresTools } : {}),
1211-
...(snapshotRevision !== undefined ? { snapshotRevision } : {}),
12121220
...(deps.parentSessionId !== undefined
12131221
? { parentSessionId: deps.parentSessionId }
12141222
: {}),
@@ -1540,11 +1548,14 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
15401548
...(deps.deadlineMs !== undefined
15411549
? { deadlineMs: deps.deadlineMs }
15421550
: {}),
1543-
...(resolved.pkg !== undefined
1544-
? { tier: resolved.pkg.tier }
1545-
: orchestrator
1546-
? {}
1547-
: { tier: "leaf" }),
1551+
// Single tier derivation (dispatchTier above): the tier gate and
1552+
// the mount agree by construction, so a gated requirement can
1553+
// never die as a stale snapshot at mount, or vice versa. An
1554+
// orchestrator dispatch without a resolved package carries no
1555+
// tier — run.ts fails closed on fleet mounts from there.
1556+
...(resolved.pkg !== undefined || !orchestrator
1557+
? { tier: dispatchTier }
1558+
: {}),
15481559
...(resolved.pkg?.reportContract?.outputType !== undefined
15491560
? { reportType: resolved.pkg.reportContract.outputType }
15501561
: {}),

‎src/subagent/capability-preflight.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
checkMountedRequiresTools,
55
DEFAULT_KNOWN_ENGINES,
66
formatCapabilityUnavailable,
7+
leafTierAlternatives,
78
preflightCapabilities,
89
rerouteAlternatives,
910
type CapabilityUnavailable,
@@ -101,6 +102,29 @@ describe("preflightCapabilities", () => {
101102
);
102103
expect(rerouteAlternatives("run_shell").length).toBeGreaterThan(0);
103104
});
105+
106+
test("reroute alternatives sort before the cap of 3 (read_file has 19 mounting directors)", () => {
107+
const alternatives = rerouteAlternatives("read_file");
108+
expect(alternatives).toEqual(["bruckheimer", "builder", "counsel"]);
109+
expect(alternatives).toEqual([...alternatives].sort());
110+
});
111+
112+
test("leaf reporting channel passes preflight under a narrow allowlist omitting post-filter mounts", () => {
113+
for (const engine of ["submit_result", "ask_director"]) {
114+
expect(preflight([engine], ALLOW_READ_ONLY)).toEqual({
115+
ok: true,
116+
canonical: [engine],
117+
});
118+
}
119+
});
120+
121+
test("leafTierAlternatives names sorted Tier 3 leaf directors, never skywalker", () => {
122+
const alternatives = leafTierAlternatives();
123+
expect(alternatives.length).toBeGreaterThan(0);
124+
expect(alternatives.length).toBeLessThanOrEqual(3);
125+
expect(alternatives).toEqual([...alternatives].sort());
126+
expect(alternatives).not.toContain("skywalker");
127+
});
104128
});
105129

106130
describe("formatCapabilityUnavailable", () => {
@@ -213,6 +237,20 @@ describe("formatCapabilityUnavailable", () => {
213237
);
214238
expect(message).toContain('Did you mean "run_shell"?');
215239
});
240+
241+
test("stale_snapshot message names every missing tool, not just the first", () => {
242+
const message = formatCapabilityUnavailable(
243+
{
244+
code: "stale_snapshot",
245+
tool: "run_shell",
246+
tools: ["run_shell", "write_file"],
247+
},
248+
"test-worker",
249+
);
250+
expect(message).toContain("stale_snapshot");
251+
expect(message).toContain("run_shell");
252+
expect(message).toContain("write_file");
253+
});
216254
});
217255

218256
describe("checkMountedRequiresTools", () => {

‎src/subagent/capability-preflight.ts‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ export interface CapabilityUnavailable {
5252
code: CapabilityUnavailableCode;
5353
/** Canonical tool id (or the raw entry for unknown). */
5454
tool: string;
55+
/** Every missing tool, for the multi-missing stale_snapshot echo. */
56+
tools?: readonly string[];
5557
/** Nearest known name, for the unknown_tool typo guard. */
5658
suggestion?: string;
5759
/** Spawnable directors that mount the tool, for reroute hints. */
@@ -114,13 +116,21 @@ export const DEFAULT_KNOWN_ENGINES: readonly string[] =
114116

115117
/**
116118
* Engines mounted outside the capability filter (run.ts appends manage_tasks
117-
* after filtering), so a requires_tools entry for them passes preflight even
118-
* when the dispatch filter is a narrow allowlist. The update_plan alias
119-
* canonicalizes here, so it rides the same exemption — and the mount-time
120-
* echo in run.ts runs after ALL appends, so the stamped entry always matches
121-
* the live mount.
119+
* after filtering, and mounts the Tier 3 leaf reporting channel
120+
* submit_result/ask_director whenever tier is "leaf"), so a requires_tools
121+
* entry for them passes preflight even when the dispatch filter is a narrow
122+
* allowlist. The update_plan alias canonicalizes here, so it rides the same
123+
* exemption — and the mount-time echo in run.ts runs after ALL appends, so
124+
* the stamped entry always matches the live mount. Fail-closed: the tier gate
125+
* in agent-fleet.ts still rejects submit_result/ask_director on non-leaf
126+
* tiers after this exemption, so the bypass never mounts them where run.ts
127+
* would not.
122128
*/
123-
const POST_FILTER_MOUNTED_ENGINES: readonly string[] = ["manage_tasks"];
129+
const POST_FILTER_MOUNTED_ENGINES: readonly string[] = [
130+
"manage_tasks",
131+
"submit_result",
132+
"ask_director",
133+
];
124134

125135
/** Alias spellings accepted in requires_tools, for typo suggestions. */
126136
const SUGGESTION_CANDIDATES: readonly string[] = [
@@ -169,7 +179,9 @@ function nearestToolName(raw: string): string | undefined {
169179
/**
170180
* Spawnable directors (closed set minus primary skywalker) whose mounted
171181
* tool set includes `canonical` — derived from packageToCapabilities over
172-
* DIRECTOR_REGISTRY, so the hint tracks the envelopes. Capped for messages.
182+
* DIRECTOR_REGISTRY, so the hint tracks the envelopes. Sorted before the cap
183+
* so the three named are the first alphabetically, not the first in registry
184+
* insertion order.
173185
*/
174186
export function rerouteAlternatives(canonical: string): readonly string[] {
175187
const want = canonicalToolName(canonical);
@@ -186,9 +198,23 @@ export function rerouteAlternatives(canonical: string): readonly string[] {
186198
} else if (!capabilities.tools.some((t) => canonicalToolName(t) === want)) {
187199
out.push(pkg.id);
188200
}
189-
if (out.length >= 3) break;
190201
}
191-
return out.sort();
202+
return out.sort().slice(0, 3);
203+
}
204+
205+
/**
206+
* Tier-3 leaf directors (closed set, skywalker excluded) for the tier-gate
207+
* hint when requires_tools names the leaf reporting channel on a non-leaf
208+
* tier. submit_result/ask_director mount post-filter, so no envelope mentions
209+
* them and rerouteAlternatives would report none — this names the directors
210+
* that actually mount them instead of the allowlist-miss fallback.
211+
*/
212+
export function leafTierAlternatives(): readonly string[] {
213+
return Object.values(DIRECTOR_REGISTRY)
214+
.filter((pkg) => pkg.id !== "skywalker" && pkg.tier === "leaf")
215+
.map((pkg) => pkg.id)
216+
.sort()
217+
.slice(0, 3);
192218
}
193219

194220
export function preflightCapabilities(
@@ -313,11 +339,13 @@ export function formatCapabilityUnavailable(
313339
`Error: worker "${agentLabel}" requires tool "${unavailable.tool}" but this dispatch cannot verify its runtime engine (not in the dispatch's known-engine set). ` +
314340
`Re-dispatch without requires_tools=["${unavailable.tool}"] or drop the requirement.`
315341
);
316-
case "stale_snapshot":
342+
case "stale_snapshot": {
343+
const missing = unavailable.tools ?? [unavailable.tool];
317344
return (
318-
`Error: stale_snapshot setup_error (non-continuable) — worker "${agentLabel}" was dispatched requiring "${unavailable.tool}" but the live mount no longer provides it. ` +
345+
`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"}. ` +
319346
`Re-dispatch the worker deliberately with a fresh brief instead of retrying in place.`
320347
);
348+
}
321349
case "unknown_tool": {
322350
const hint =
323351
unavailable.suggestion !== undefined

‎src/subagent/run-requires-tools.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,28 @@ describe("runSubAgent requires_tools mount echo", () => {
9090
expect(message).not.toContain('continuable": true');
9191
}, 15_000);
9292

93+
test("two stamped tools dropped by the filter are both named in the stale_snapshot error", async () => {
94+
const cwd = await tmpCwd();
95+
const error = await withFailingInference(async (baseURL) => {
96+
try {
97+
await runSubAgent({
98+
...baseParams(cwd, baseURL),
99+
capabilities: { mode: "allow", tools: ["read_file"] },
100+
requiresTools: ["run_shell", "write_file"],
101+
});
102+
} catch (err) {
103+
return err;
104+
}
105+
throw new Error("runSubAgent did not throw");
106+
});
107+
108+
expect(error).toBeInstanceOf(Error);
109+
const message = (error as Error).message;
110+
expect(message).toContain("stale_snapshot");
111+
expect(message).toContain("run_shell");
112+
expect(message).toContain("write_file");
113+
}, 15_000);
114+
93115
test("stamped tool present in the mount passes through to inference", async () => {
94116
const cwd = await tmpCwd();
95117
const error = await withFailingInference(async (baseURL) => {

0 commit comments

Comments
 (0)