From 99ee6b2ce87d3ab169d76ee29000bfc344413276 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 24 Aug 2026 00:12:06 -0700 Subject: [PATCH] Thread structured stop reasons through sub-agent dispatch (CL-6946 part 2) Replaces prose-matching of forced-stop reports (isXxxSubAgentReport family, per-reason parent hint functions, classifyBriefSalvage(string)) with a structured ForcedStopReason value threaded through runSubAgent's return and the task tool result's detail field. The parent chat director and task-tool dispatch path now classify salvage outcomes and select hint text from that typed value instead of parsing report text. --- CHANGELOG.md | 5 + src/agent/director.test.ts | 16 +- src/agent/director.ts | 13 +- src/perf/permission-subagent-spans.test.ts | 6 +- src/subagent/agent-fleet.test.ts | 40 ++--- src/subagent/agent-fleet.ts | 13 +- src/subagent/brief-dispatch.ts | 57 ++----- src/subagent/index.test.ts | 168 +++++++++----------- src/subagent/index.ts | 13 +- src/subagent/nudge-director.ts | 13 ++ src/subagent/run.ts | 22 ++- src/subagent/stop-policy.ts | 145 ++++------------- src/subagent/task-tool-worktree.test.ts | 10 +- src/subagent/task-tool.ts | 47 ++++-- src/subagent/types.ts | 7 + tests/unit/subagent-session-store.test.ts | 8 +- tests/unit/subagent.test.ts | 56 +++---- tests/unit/telemetry-product-events.test.ts | 2 +- 18 files changed, 302 insertions(+), 339 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e3d0a8d0..12e5a59d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `search_agents` with no supported opt-in. - Removed the default 30-turn leaf sub-agent ceiling; an unset `maxTurns` now runs unbounded (explicit budgets still apply). - Deleted two unenforced orchestrator prompt rules: a "4 workers at once" fan-out cap and a same-agent lane-disjointness rule. +- Sub-agent forced-stop outcomes (turn budget, no-progress, deadline, + cancelled, etc.) are now classified from the structured stop reason the run + reports directly, not by re-parsing the parent-facing report's prose. + Removes the `isXxxSubAgentReport` classifier family and per-reason parent + hint functions in favor of a single structured switch. ## [0.2.108] - 2026-08-24 diff --git a/src/agent/director.test.ts b/src/agent/director.test.ts index 11af0ea4d..c4587758e 100644 --- a/src/agent/director.test.ts +++ b/src/agent/director.test.ts @@ -111,11 +111,16 @@ function taskTurn(id: string): ReactorInboundEvent { // "successful leaf tool.done" progress signal. function taskDoneEvent( callId: string, - options: { isError?: boolean; content?: string } = {}, + options: { isError?: boolean; content?: string; stopReason?: string } = {}, ): ReactorInboundEvent { return { type: "tool.done", - result: { callId, isError: options.isError ?? false, content: options.content ?? "ok" }, + result: { + callId, + isError: options.isError ?? false, + content: options.content ?? "ok", + ...(options.stopReason !== undefined ? { detail: { stopReason: options.stopReason } } : {}), + }, } as unknown as ReactorInboundEvent; } @@ -866,7 +871,7 @@ describe("ChatDirector tool-only loop protection", () => { await director.decide( { type: "tool.done", - result: { callId: "task-1", content: salvage }, + result: { callId: "task-1", content: salvage, detail: { stopReason: "no-ship" } }, } as unknown as ReactorInboundEvent, mockState, capabilities, @@ -1071,7 +1076,10 @@ describe("ChatDirector tool-only loop protection", () => { await director.decide(taskTurn(id), mockState, capabilities); const result = actionsArray( await director.decide( - taskDoneEvent(id, { content: forcedStopReport("no-progress", "x") }), + taskDoneEvent(id, { + content: forcedStopReport("no-progress", "x"), + stopReason: "no-progress", + }), mockState, capabilities, ), diff --git a/src/agent/director.ts b/src/agent/director.ts index 7fc147b5d..2041f6b4b 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -32,6 +32,7 @@ import { import { PRESENT_VIEW_PRIMITIVES_GUIDANCE } from "./tool-schema-normalize.js"; import { isOperatorOriginated } from "./message-provenance.js"; import { classifyBriefSalvage, isHardBlockSalvage } from "../subagent/brief-dispatch.js"; +import type { ForcedStopReason } from "../subagent/stop-policy.js"; import { PRIMARY_SALVAGE_NUDGE } from "./look-tour.js"; // Fired when turnsSinceUserMessage reaches TURNS_SINCE_USER_MESSAGE_BACKSTOP. @@ -933,8 +934,11 @@ class ChatDirectorImpl extends DefaultDirector { if (event.type === "tool.done" && this.pendingTaskCallIds.has(event.result.callId)) { this.pendingTaskCallIds.delete(event.result.callId); - const body = typeof event.result.content === "string" ? event.result.content : ""; - const salvage = classifyBriefSalvage(body); + const detail = event.result.detail as { stopReason?: ForcedStopReason } | undefined; + const salvage = classifyBriefSalvage({ + ...(detail?.stopReason !== undefined ? { stopReason: detail.stopReason } : {}), + wasCancelled: false, + }); if (salvage !== null && isHardBlockSalvage(salvage) && !this.salvageNudgeFired) { this.salvageNudgeFired = true; this.pendingSalvageNudge = PRIMARY_SALVAGE_NUDGE; @@ -958,9 +962,8 @@ class ChatDirectorImpl extends DefaultDirector { // the backstop forever — once the cap is exhausted, leaf successes // stop resetting the interval and the nudge/pause escalation // eventually forces an operator checkpoint. Credit also requires the - // tool result content to actually be a string: non-string content is - // coerced to "" above only for salvage classification (an empty body - // classifies as success), which must not also buy backstop credit. + // tool result content to actually be a string, independent of the + // structured salvage classification above. if ( !event.result.isError && salvage === null && diff --git a/src/perf/permission-subagent-spans.test.ts b/src/perf/permission-subagent-spans.test.ts index 2b1946642..2216456e6 100644 --- a/src/perf/permission-subagent-spans.test.ts +++ b/src/perf/permission-subagent-spans.test.ts @@ -235,7 +235,7 @@ describe("subagent spans", () => { const open = snapshot().filter((s) => s.name === "subagent" && s.endNs === undefined); expect(open).toHaveLength(1); expect(open[0]!.tags?.subagent_id).toBe("call-sa-1"); - return "## Summary\n\nok\n"; + return { report: "## Summary\n\nok\n" }; }, }); if (tool.kind !== "full") throw new Error("expected full tool"); @@ -281,7 +281,7 @@ describe("subagent spans", () => { cwd: "/repo", getWorkdirBase: () => "/repo/.corbits", provider, - run: async () => "## Summary\n\nchild done\n", + run: async () => ({ report: "## Summary\n\nchild done\n" }), }); if (tool.kind !== "full") throw new Error("expected full tool"); @@ -346,7 +346,7 @@ describe("subagent spans", () => { useWorktree: true, run: async () => { runEntered = true; - return "## Summary\n\nshould not run\n"; + return { report: "## Summary\n\nshould not run\n" }; }, }); if (tool.kind !== "full") throw new Error("expected full tool"); diff --git a/src/subagent/agent-fleet.test.ts b/src/subagent/agent-fleet.test.ts index 641eeb199..17e4754eb 100644 --- a/src/subagent/agent-fleet.test.ts +++ b/src/subagent/agent-fleet.test.ts @@ -8,7 +8,7 @@ import { } from "./agent-fleet.js"; import { createSubAgentSessionStore } from "./session-store.js"; import { createPermissionGate } from "../permission/gate.js"; -import type { RunSubAgentParams } from "./types.js"; +import type { RunSubAgentParams, RunSubAgentResult } from "./types.js"; const testPermissionGate = createPermissionGate({ approvals: [], @@ -37,7 +37,7 @@ function deferred(): { } function makeDeps( - run: (params: RunSubAgentParams) => Promise, + run: (params: RunSubAgentParams) => Promise, opts: { cwd?: string } = {}, ): AgentFleetDeps { return { @@ -75,7 +75,7 @@ async function callTool( describe("spawn_agent", () => { test("returns immediately with a running agent_id without waiting for the worker", async () => { - const gate = deferred(); + const gate = deferred(); const deps = makeDeps(async () => gate.promise); const spawn = createSpawnAgentTool(deps); @@ -94,13 +94,17 @@ describe("spawn_agent", () => { // Worker is still pending; store confirms it has not finished. expect(deps.sessions.get(result.agent_id as string)?.status).toBe("running"); - gate.resolve("done"); + gate.resolve({ report: "done" }); }); }); describe("spawn_agent + wait_agents", () => { test("wait_agents on one target returns once it completes while siblings keep running", async () => { - const gates = [deferred(), deferred(), deferred()]; + const gates = [ + deferred(), + deferred(), + deferred(), + ]; let callIndex = 0; const deps = makeDeps(async () => { const i = callIndex++; @@ -116,7 +120,7 @@ describe("spawn_agent + wait_agents", () => { ); const ids = spawned.map((s) => s.agent_id as string); - gates[0]!.resolve("first report"); + gates[0]!.resolve({ report: "first report" }); const waited = await callTool(wait, { targets: [ids[0]], timeout_ms: 5000 }); expect(waited.timed_out).toBe(false); @@ -129,12 +133,12 @@ describe("spawn_agent + wait_agents", () => { expect(deps.sessions.get(ids[1]!)?.status).toBe("running"); expect(deps.sessions.get(ids[2]!)?.status).toBe("running"); - gates[1]!.resolve("second"); - gates[2]!.resolve("third"); + gates[1]!.resolve({ report: "second" }); + gates[2]!.resolve({ report: "third" }); }); test("wait_agents times out on a still-running agent without cancelling it, and can be called again", async () => { - const gate = deferred(); + const gate = deferred(); const deps = makeDeps(async () => gate.promise); const spawn = createSpawnAgentTool(deps); const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords }); @@ -155,7 +159,7 @@ describe("spawn_agent + wait_agents", () => { expect(deps.sessions.get(id)?.status).toBe("running"); // A second wait still works cleanly (either another timeout, or completion). - gate.resolve("finished"); + gate.resolve({ report: "finished" }); const second = await callTool(wait, { targets: [id], timeout_ms: 5000 }); expect(second.timed_out).toBe(false); const secondResults = second.results as { @@ -168,7 +172,7 @@ describe("spawn_agent + wait_agents", () => { }); test("wait_agents with no targets waits on all currently running spawned agents", async () => { - const gates = [deferred(), deferred()]; + const gates = [deferred(), deferred()]; let callIndex = 0; const deps = makeDeps(async () => gates[callIndex++]!.promise); const spawn = createSpawnAgentTool(deps); @@ -177,14 +181,14 @@ describe("spawn_agent + wait_agents", () => { await callTool(spawn, { description: "a", prompt: "do it", intent: "explore" }); await callTool(spawn, { description: "b", prompt: "do it", intent: "explore" }); - gates[0]!.resolve("a done"); + gates[0]!.resolve({ report: "a done" }); const result = await callTool(wait, { timeout_ms: 5000 }); expect(result.timed_out).toBe(false); const results = result.results as { status: string }[]; expect(results).toHaveLength(2); expect(results.some((r) => r.status === "done")).toBe(true); - gates[1]!.resolve("b done"); + gates[1]!.resolve({ report: "b done" }); }); test("reports survive well past the session store's display cap (20) until wait_agents collects them", async () => { @@ -193,7 +197,7 @@ describe("spawn_agent + wait_agents", () => { // them is collected, proving fleetRecords — not the store — is what // wait_agents actually reads from. const COUNT = 25; - const deps = makeDeps(async () => "irrelevant"); + const deps = makeDeps(async () => ({ report: "irrelevant" })); const spawn = createSpawnAgentTool(deps); const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords }); @@ -226,7 +230,7 @@ describe("spawn_agent + wait_agents", () => { describe("spawn_agent write-lane isolation", () => { test("refuses a second concurrent implement-intent spawn against the same cwd", async () => { - const gate = deferred(); + const gate = deferred(); const deps = makeDeps(async () => gate.promise, { cwd: "/repo" }); const spawn = createSpawnAgentTool(deps); @@ -246,11 +250,11 @@ describe("spawn_agent write-lane isolation", () => { expect(second.content).toContain("Error:"); expect(second.content).toContain(first.agent_id as string); - gate.resolve("done"); + gate.resolve({ report: "done" }); }); test("does not refuse a second concurrent explore-intent spawn against the same cwd", async () => { - const deps = makeDeps(async () => "explored", { cwd: "/repo" }); + const deps = makeDeps(async () => ({ report: "explored" }), { cwd: "/repo" }); const spawn = createSpawnAgentTool(deps); const first = await callTool(spawn, { @@ -269,7 +273,7 @@ describe("spawn_agent write-lane isolation", () => { }); test("releases the write lane once the implement worker finishes, allowing another", async () => { - const deps = makeDeps(async () => "built", { cwd: "/repo" }); + const deps = makeDeps(async () => ({ report: "built" }), { cwd: "/repo" }); const spawn = createSpawnAgentTool(deps); const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords }); diff --git a/src/subagent/agent-fleet.ts b/src/subagent/agent-fleet.ts index 3c64cafca..eb329522d 100644 --- a/src/subagent/agent-fleet.ts +++ b/src/subagent/agent-fleet.ts @@ -62,7 +62,12 @@ import { resolveEffortForRole } from "../provider/reasoning-effort.js"; import { isCodexProviderName } from "../config/codex-providers.js"; import { buildDispatchBrief, type TaskIntent } from "./report.js"; import type { SubAgentSessionStore } from "./session-store.js"; -import type { RunSubAgentParams, SubAgentProvider, SubAgentSandboxDeps } from "./types.js"; +import type { + RunSubAgentParams, + RunSubAgentResult, + SubAgentProvider, + SubAgentSandboxDeps, +} from "./types.js"; import { NOOP_TELEMETRY, type Telemetry } from "../telemetry/index.js"; import { classifyAgentName } from "../telemetry/classify.js"; @@ -212,7 +217,7 @@ export type AgentFleetDeps = SubAgentSandboxDeps & { cwd: string; getWorkdirBase: () => string; provider: SubAgentProvider | (() => SubAgentProvider); - run: (params: RunSubAgentParams) => Promise; + run: (params: RunSubAgentParams) => Promise; sessions: SubAgentSessionStore; fleetRecords: FleetRecordsHandle; settings?: Settings | (() => Settings | undefined); @@ -462,8 +467,8 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool { .then((result) => { releaseWriteLane(); if (childCtl.signal.aborted) return; - deps.fleetRecords.resolve(session.id, result); - deps.sessions.complete(session.id, result); + deps.fleetRecords.resolve(session.id, result.report); + deps.sessions.complete(session.id, result.report); }) .catch((err) => { releaseWriteLane(); diff --git a/src/subagent/brief-dispatch.ts b/src/subagent/brief-dispatch.ts index 7cf8d6fbf..7fca171d3 100644 --- a/src/subagent/brief-dispatch.ts +++ b/src/subagent/brief-dispatch.ts @@ -12,23 +12,15 @@ */ import type { TaskIntent } from "./report.js"; -import { - isDeadlineSubAgentReport, - isForcedStopSubAgentReport, - isNeverActedSubAgentReport, - isNeverEditedSubAgentReport, - isNoProgressSubAgentReport, - isNoShipSubAgentReport, - isRepetitionSubAgentReport, - isTurnBudgetSubAgentReport, -} from "./stop-policy.js"; +import type { ForcedStopReason } from "./stop-policy.js"; /** Salvage classes that must not be re-dispatched with an identical brief. */ export type HardBlockSalvage = "no-ship" | "no-progress" | "repetition" | "never-acted" | "never-edited"; -export type BriefSalvageKind = - HardBlockSalvage | "turn-budget" | "deadline" | "stalled" | "cancelled" | "incomplete-report"; +// Every forced-stop reason a leaf can report maps 1:1 onto a salvage kind +// the parent ledger cares about. +export type BriefSalvageKind = ForcedStopReason; export interface TaskBriefFingerprintInput { prompt: string; @@ -64,38 +56,19 @@ export function isHardBlockSalvage(kind: BriefSalvageKind): kind is HardBlockSal return HARD_BLOCK_SALVAGES.has(kind); } -/** True when the worker returned a stall salvage report. */ -export function isStalledSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "stalled"); -} - -/** True when the worker returned a cancel salvage report. */ -export function isCancelledSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "cancelled"); -} - -/** True when the worker returned an incomplete-report salvage (narration, no envelope). */ -export function isIncompleteReportSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "incomplete-report"); -} - /** - * Classify a sub-agent tool result body as a salvage kind the parent ledger cares - * about. Returns null for normal completes (or unrecognized envelopes). + * Classify a completed dispatch as a salvage kind the parent ledger cares + * about, from the structured stop reason the run reported directly — never + * by matching the report body's prose. `wasCancelled` (observed independently, + * e.g. via the parent's own abort signal) takes precedence since a parent + * cancel can race a run that never got to report its own reason. */ -export function classifyBriefSalvage(report: string): BriefSalvageKind | null { - // Order: more specific salvage phrases first. - if (isNoShipSubAgentReport(report)) return "no-ship"; - if (isRepetitionSubAgentReport(report)) return "repetition"; - if (isNeverEditedSubAgentReport(report)) return "never-edited"; - if (isNeverActedSubAgentReport(report)) return "never-acted"; - if (isNoProgressSubAgentReport(report)) return "no-progress"; - if (isTurnBudgetSubAgentReport(report)) return "turn-budget"; - if (isDeadlineSubAgentReport(report)) return "deadline"; - if (isStalledSubAgentReport(report)) return "stalled"; - if (isCancelledSubAgentReport(report)) return "cancelled"; - if (isIncompleteReportSubAgentReport(report)) return "incomplete-report"; - return null; +export function classifyBriefSalvage(input: { + stopReason?: ForcedStopReason; + wasCancelled: boolean; +}): BriefSalvageKind | null { + if (input.wasCancelled) return "cancelled"; + return input.stopReason ?? null; } /** diff --git a/src/subagent/index.test.ts b/src/subagent/index.test.ts index 57e78a497..03971cf38 100644 --- a/src/subagent/index.test.ts +++ b/src/subagent/index.test.ts @@ -20,8 +20,6 @@ import { parseSubAgentReport, repetitionStopDetail, stopReasonFromReport, - appendDeadlineParentHint, - appendNeverActedParentHint, appendSubAgentParentHints, createBriefDispatchLedger, fingerprintTaskBrief, @@ -694,7 +692,7 @@ describe("sub-agent stop helpers", () => { const neverEdited = forcedStopReport("never-edited", "I mapped the files; ready to code next"); expect(neverEdited).toContain("without writing any files"); - expect(appendSubAgentParentHints(neverEdited)).toContain("edit-first"); + expect(appendSubAgentParentHints(neverEdited, "never-edited")).toContain("edit-first"); // Nested agent envelope must not clobber the outer never-acted Summary when // runSubAgent re-parses the forced stop (the common planning-only path). @@ -729,7 +727,7 @@ describe("sub-agent stop helpers", () => { expect(messyFields.summary).toContain("without using any tools"); expect(messyFields.findings.toLowerCase()).toContain("### summary"); - const withHint = appendNeverActedParentHint(reparsed); + const withHint = appendSubAgentParentHints(reparsed, "never-acted"); expect(withHint).toContain("planning/prose only"); expect(withHint).toContain("without using any tools"); @@ -762,13 +760,13 @@ describe("sub-agent stop helpers", () => { expect(deadlineParsed.findings).toContain("Refactored half of gate.ts"); expect(deadlineParsed.blockers).toContain("re-dispatch"); - const deadlineWithHint = appendDeadlineParentHint(deadline); + const deadlineWithHint = appendSubAgentParentHints(deadline, "deadline"); expect(deadlineWithHint).toContain("wall-clock deadline"); expect(deadlineWithHint).toContain("deadline reached"); // Only fires for a deadline report, not for other forced-stop reasons. - expect(appendDeadlineParentHint(forcedStopReport("cancelled", "x"))).not.toContain( - "wall-clock deadline", - ); + expect( + appendSubAgentParentHints(forcedStopReport("cancelled", "x"), "cancelled"), + ).not.toContain("wall-clock deadline"); }); test("forcedStopReport carries a machine-readable Stopped line the parent sees verbatim", () => { @@ -784,7 +782,9 @@ describe("sub-agent stop helpers", () => { const roundTripped = formatSubAgentReport(parseSubAgentReport(repetition)); expect(stopReasonFromReport(roundTripped)).toBe('repetition — window "Groaning. " × 1363'); // Classifiers and hints still fire on the unchanged Summary text. - expect(appendSubAgentParentHints(repetition)).toContain("degenerated into a loop"); + expect(appendSubAgentParentHints(repetition, "repetition")).toContain( + "degenerated into a loop", + ); const cancelled = forcedStopReport("cancelled", "partial", "Session closed"); expect(stopReasonFromReport(cancelled)).toBe("cancelled — Session closed"); @@ -927,7 +927,7 @@ describe("sub-agent stop helpers", () => { expect(parsed.findings).toContain("dig footer/chrome"); expect(parsed.blockers).toContain("will be refused"); expect(parsed.blockers).toContain("not maxTurns alone"); - const hinted = appendSubAgentParentHints(report); + const hinted = appendSubAgentParentHints(report, "repetition"); expect(hinted).toContain("Do not re-dispatch the identical brief"); }); @@ -1364,7 +1364,7 @@ describe("createTaskTool", () => { profiles: [{ id: "leaf" }], run: async () => { await gate; - return report; + return { report }; }, }); @@ -1399,7 +1399,7 @@ describe("createTaskTool", () => { profiles: [{ id: "leaf" }], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, } as Parameters[0] & { maxTurns: number }); @@ -1426,7 +1426,7 @@ describe("createTaskTool", () => { profiles: [{ id: "leaf" }], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1449,7 +1449,7 @@ describe("createTaskTool", () => { profiles: [{ id: "leaf" }], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1488,7 +1488,7 @@ describe("createTaskTool", () => { ], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); await callTask(tool, { @@ -1529,7 +1529,7 @@ describe("createTaskTool", () => { ], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); await callTask(tool, { description: "oauth-profile-inference", prompt: "x", agent: "deep" }); @@ -1555,7 +1555,7 @@ describe("createTaskTool", () => { inference: { mode: "pin", order: [{ provider: "xai/missing", model: "grok-4" }] }, }, ], - run: async () => "done", + run: async () => ({ report: "done" }), }); const out = await callTask(tool, { description: "missing-oauth", @@ -1575,7 +1575,7 @@ describe("createTaskTool", () => { provider, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1596,7 +1596,7 @@ describe("createTaskTool", () => { cwd: "/repo", getWorkdirBase: () => "/repo/.corbits", provider, - run: async () => "done", + run: async () => ({ report: "done" }), }); const result = await callTask(tool, { @@ -1620,7 +1620,7 @@ describe("createTaskTool", () => { profiles: [{ id: "deep", maxTurns: 45 }], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1643,7 +1643,7 @@ describe("createTaskTool", () => { profiles: [{ id: "deep", maxTurns: 45 }], run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1663,7 +1663,10 @@ describe("createTaskTool", () => { cwd: "/repo", getWorkdirBase: () => "/repo/.corbits", provider, - run: async () => forcedStopReport("turn-budget", "partial"), + run: async () => ({ + report: forcedStopReport("turn-budget", "partial"), + stopReason: "turn-budget", + }), }); const result = await callTask(tool, { @@ -1693,7 +1696,7 @@ describe("createTaskTool", () => { inheritMcpTools: () => inherited, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1713,7 +1716,7 @@ describe("createTaskTool", () => { shellEnv: { FOO: "bar" }, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1754,7 +1757,10 @@ describe("createTaskTool", () => { parent.abort(); }); // Injected run returns salvage after cancel-with-progress; task must keep it. - return forcedStopReport("cancelled", "partial from tools"); + return { + report: forcedStopReport("cancelled", "partial from tools"), + stopReason: "cancelled", + }; }, }); const out = await callTask( @@ -1780,7 +1786,7 @@ describe("createTaskTool", () => { run: async () => { const row = sessions.list().find((s) => s.description === "race"); if (row !== undefined) sessions.cancel(row.id, "Cancelled by operator"); - return forcedStopReport("cancelled", "salvaged work"); + return { report: forcedStopReport("cancelled", "salvaged work"), stopReason: "cancelled" }; }, }); const out = await callTask(tool, { description: "race", prompt: "x", intent: "explore" }); @@ -1841,7 +1847,10 @@ describe("createTaskTool", () => { cwd: "/repo", getWorkdirBase: () => "/repo/.corbits", provider, - run: async () => forcedStopReport("cancelled", "Found path in gate.ts"), + run: async () => ({ + report: forcedStopReport("cancelled", "Found path in gate.ts"), + stopReason: "cancelled", + }), }); const out = await callTask(tool, { description: "salvage", prompt: "x", intent: "explore" }); expect(out).toContain("## Summary"); @@ -1897,7 +1906,10 @@ describe("createTaskTool", () => { deadlineMs: 45_000, run: async (params) => { captured = params; - return forcedStopReport("deadline", "partial before wall clock"); + return { + report: forcedStopReport("deadline", "partial before wall clock"), + stopReason: "deadline", + }; }, }); const out = await callTask(tool, { description: "deadline", prompt: "x", intent: "explore" }); @@ -1925,7 +1937,10 @@ describe("createTaskTool", () => { params.signal?.addEventListener("abort", () => resolve(), { once: true }); }); await new Promise((r) => setTimeout(r, 10)); - return forcedStopReport("cancelled", "Found path in gate.ts"); + return { + report: forcedStopReport("cancelled", "Found path in gate.ts"), + stopReason: "cancelled", + }; }, }); const runner = createDynamicToolRunner([task], { defaultMs: 10_000 }); @@ -1956,7 +1971,7 @@ describe("createTaskTool", () => { provider, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -1986,7 +2001,7 @@ describe("createTaskTool", () => { provider, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -2006,7 +2021,7 @@ describe("createTaskTool", () => { cwd: "/repo", getWorkdirBase: () => "/repo/.corbits", provider, - run: async () => "done", + run: async () => ({ report: "done" }), }); const out = await callTask(tool, { description: "bad-intent", @@ -2220,65 +2235,26 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(again.dispatchCount).toBe(1); }); - test("classifyBriefSalvage maps forced-stop envelopes", () => { - expect(classifyBriefSalvage(forcedStopReport("no-progress", "x"))).toBe("no-progress"); - expect(classifyBriefSalvage(forcedStopReport("repetition", "x"))).toBe("repetition"); - expect(classifyBriefSalvage(forcedStopReport("never-acted", "x"))).toBe("never-acted"); - expect(classifyBriefSalvage(forcedStopReport("never-edited", "x"))).toBe("never-edited"); - expect(classifyBriefSalvage(forcedStopReport("no-ship", "x"))).toBe("no-ship"); - expect(classifyBriefSalvage(forcedStopReport("turn-budget", "x"))).toBe("turn-budget"); - expect( - classifyBriefSalvage( - "## Summary\nDone\n\n## Findings\nok\n\n## Blockers\nNone\n\n## Paths\n", - ), - ).toBeNull(); - }); - - test("classifyBriefSalvage maps incomplete-report salvage", () => { - expect(classifyBriefSalvage(forcedStopReport("incomplete-report", "x"))).toBe( - "incomplete-report", + test("classifyBriefSalvage decides purely from the structured stop reason, never from report prose", () => { + // classifyBriefSalvage takes no report text at all — only the structured + // stopReason and an independently-observed wasCancelled flag. + expect(classifyBriefSalvage({ wasCancelled: false })).toBeNull(); + expect(classifyBriefSalvage({ stopReason: "no-progress", wasCancelled: false })).toBe( + "no-progress", ); - }); - - test("CL-6704: a successful Summary containing forced-stop phrases is not classified as a salvage", () => { - const noProgressPhrase = formatSubAgentReport({ - summary: "Investigated the flaky test; root cause is a race, not no progress on our side.", - findings: "Fixed the race in retry logic.", - blockers: "None", - paths: "src/retry.ts", - }); - expect(classifyBriefSalvage(noProgressPhrase)).toBeNull(); - - const cancelledPhrase = formatSubAgentReport({ - summary: "Implemented the cancelled-order refund flow end to end.", - findings: "Added refund handler and tests.", - blockers: "None", - paths: "src/refunds.ts", - }); - expect(classifyBriefSalvage(cancelledPhrase)).toBeNull(); - - const longSilencePhrase = formatSubAgentReport({ - summary: "Reduced UI flicker with a long silence period before re-render.", - findings: "Debounced the re-render.", - blockers: "None", - paths: "src/ui.ts", - }); - expect(classifyBriefSalvage(longSilencePhrase)).toBeNull(); - }); - - test("CL-6704: true forced-stop Summary strings still classify as their salvage kind", () => { - expect(classifyBriefSalvage(forcedStopReport("no-progress", "x"))).toBe("no-progress"); - expect(classifyBriefSalvage(forcedStopReport("cancelled", "x"))).toBe("cancelled"); - expect(classifyBriefSalvage(forcedStopReport("stalled", "x"))).toBe("stalled"); - expect(classifyBriefSalvage(forcedStopReport("deadline", "x"))).toBe("deadline"); + // An operator cancel wins even when the run's own reason disagrees. + expect(classifyBriefSalvage({ stopReason: "no-progress", wasCancelled: true })).toBe( + "cancelled", + ); + expect(classifyBriefSalvage({ stopReason: "deadline", wasCancelled: false })).toBe("deadline"); }); test("turn-budget parent hint flips after re-dispatch threshold", () => { const report = forcedStopReport("turn-budget", "partial"); - const first = appendSubAgentParentHints(report, { dispatchCount: 1 }); + const first = appendSubAgentParentHints(report, "turn-budget", { dispatchCount: 1 }); expect(first).toContain("higher maxTurns"); expect(first).not.toContain("re-dispatch cap"); - const third = appendSubAgentParentHints(report, { + const third = appendSubAgentParentHints(report, "turn-budget", { dispatchCount: TURN_BUDGET_STOP_AFTER_DISPATCHES, }); expect(third).toContain("re-dispatch cap"); @@ -2286,7 +2262,10 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("createTaskTool refuses identical re-dispatch after no-progress salvage", async () => { - const thrash = forcedStopReport("no-progress", "Repeated the same call"); + const thrash = { + report: forcedStopReport("no-progress", "Repeated the same call"), + stopReason: "no-progress" as const, + }; let runs = 0; const sessions = createSubAgentSessionStore(); const tool = createTaskTool({ @@ -2336,7 +2315,10 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("createTaskTool flips turn-budget hint on third same-brief dispatch", async () => { - const budget = forcedStopReport("turn-budget", "partial work"); + const budget = { + report: forcedStopReport("turn-budget", "partial work"), + stopReason: "turn-budget" as const, + }; let runs = 0; const tool = createTaskTool({ permissionGate: testPermissionGate, @@ -2359,8 +2341,13 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("createTaskTool success resets turn-budget retry budget", async () => { - const budget = forcedStopReport("turn-budget", "partial"); - const ok = "## Summary\nDone\n\n## Findings\nok\n\n## Blockers\nNone\n\n## Paths\n"; + const budget = { + report: forcedStopReport("turn-budget", "partial"), + stopReason: "turn-budget" as const, + }; + const ok = { + report: "## Summary\nDone\n\n## Findings\nok\n\n## Blockers\nNone\n\n## Paths\n", + }; let runs = 0; const tool = createTaskTool({ permissionGate: testPermissionGate, @@ -2385,7 +2372,10 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("createTaskTool auth failure does not burn turn-budget count", async () => { - const budget = forcedStopReport("turn-budget", "partial"); + const budget = { + report: forcedStopReport("turn-budget", "partial"), + stopReason: "turn-budget" as const, + }; let runs = 0; const tool = createTaskTool({ permissionGate: testPermissionGate, diff --git a/src/subagent/index.ts b/src/subagent/index.ts index d40c3d3fd..f733ebb2d 100644 --- a/src/subagent/index.ts +++ b/src/subagent/index.ts @@ -47,21 +47,10 @@ export { export { DEFAULT_SUBAGENT_REPEAT_LIMIT, SUBAGENT_DEADLINE_MARGIN_MS, - appendDeadlineParentHint, - appendNeverActedParentHint, - appendNoProgressParentHint, - appendRepetitionParentHint, appendSubAgentParentHints, - appendTurnBudgetParentHint, evaluateSubAgentStop, fingerprintToolCalls, forcedStopReport, - isDeadlineSubAgentReport, - isNeverActedSubAgentReport, - isNeverEditedSubAgentReport, - isNoProgressSubAgentReport, - isRepetitionSubAgentReport, - isTurnBudgetSubAgentReport, nextToolCallStreak, partialTextFromEvent, preferCompletedSubAgentReply, @@ -70,6 +59,7 @@ export { subAgentNoProgress, subAgentTurnLimitExceeded, TURN_BUDGET_STOP_PARENT_HINT, + type ForcedStopReason, type SubAgentCatchOutcome, type SubAgentParentHintOptions, type SubAgentStopReason, @@ -106,6 +96,7 @@ export { export type { NestedDispatchDeps, RunSubAgentParams, + RunSubAgentResult, SubAgentProvider, SubAgentSandboxDeps, } from "./types.js"; diff --git a/src/subagent/nudge-director.ts b/src/subagent/nudge-director.ts index 0ec5cde09..db74e152f 100644 --- a/src/subagent/nudge-director.ts +++ b/src/subagent/nudge-director.ts @@ -29,6 +29,7 @@ import { forcedStopReport, lastText, nextToolCallStreak, + type ForcedStopReason, type ToolCallStreak, } from "./stop-policy.js"; @@ -129,12 +130,21 @@ export class SubAgentDirector extends DefaultDirector { // threshold, so a later threshold change can cite data instead of judgment // (CL-6938). Defaults to a no-op: logging is diagnostic, never required. private interventions: InterventionSink = NOOP_INTERVENTION_SINK; + // Structured stop-reason side channel (CL-6946 part 2): fired synchronously + // whenever this director force-stops, so the caller learns the reason as a + // typed value rather than re-parsing the forcedStopReport prose it returns. + private onForcedStop: (reason: ForcedStopReason) => void = () => {}; /** Route this leaf's stop/nudge decisions to an intervention log. */ observeInterventions(sink: InterventionSink): void { this.interventions = sink; } + /** Route this leaf's forced-stop reason to the caller as a typed value. */ + observeForcedStop(callback: (reason: ForcedStopReason) => void): void { + this.onForcedStop = callback; + } + /** Run state every intervention record carries, for judging it afterwards. */ private interventionState(): { turnsCompleted: number; @@ -276,6 +286,7 @@ export class SubAgentDirector extends DefaultDirector { state: this.interventionState(), detail: "no report envelope after the wrap-up nudge", }); + this.onForcedStop("incomplete-report"); const terminal: ReactorAction[] = [ capabilities.checkpoint("subagent-incomplete-report"), capabilities.reply(forcedStopReport("incomplete-report", this.lastAssistantText)), @@ -335,6 +346,7 @@ export class SubAgentDirector extends DefaultDirector { state: this.interventionState(), ...(detail !== undefined ? { detail } : {}), }); + this.onForcedStop(stop); const terminal: ReactorAction[] = [ capabilities.checkpoint(checkpoint), capabilities.reply(forcedStopReport(stop, lastText(content), detail)), @@ -412,6 +424,7 @@ export class SubAgentDirector extends DefaultDirector { state: this.interventionState(), detail: `no activity for ${Math.round(elapsed / 1000)}s after stall nudge`, }); + this.onForcedStop("stalled"); const terminal: ReactorAction[] = [ capabilities.checkpoint("subagent-stalled"), capabilities.reply( diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 8ad655c70..8c9b1c852 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -102,6 +102,7 @@ import { preferCompletedSubAgentReply, resolveSubAgentCatchOutcome, resolveSubAgentDeadlineMs, + type ForcedStopReason, } from "./stop-policy.js"; import { SubAgentDirector } from "./nudge-director.js"; import { assertTierMayMountFleetVerb } from "./authority.js"; @@ -120,7 +121,7 @@ import { import { createTaskTool } from "./task-tool.js"; import { createFleetRecords, createSpawnAgentTool, createWaitAgentsTool } from "./agent-fleet.js"; import { createSubAgentSessionStore } from "./session-store.js"; -import type { RunSubAgentParams, SubAgentProvider } from "./types.js"; +import type { RunSubAgentParams, RunSubAgentResult, SubAgentProvider } from "./types.js"; import type { TaskIntent } from "./report.js"; import { runWithSubAgentIdentity } from "./identity-context.js"; @@ -327,7 +328,7 @@ const submitResultDefinition: ToolDefinition = { // (isolated mode, see task-tool.ts's useWorktree) — either way this loop // gets its own posix tool instances and its own git-backed context store so // the two loops never trample each other's state. -export async function runSubAgent(params: RunSubAgentParams): Promise { +export async function runSubAgent(params: RunSubAgentParams): Promise { await seedPricingMetadataFromCache({ cachePath: defaultPricingCachePath(), }); @@ -604,6 +605,10 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { // Assigned once the leaf's trace dir exists; the director factory closes // over this binding and only fires after that point. let interventions: InterventionSink = NOOP_INTERVENTION_SINK; + // Set by the director when it force-stops via capabilities.reply (the + // normal agent.send success path) — carried into the returned result + // instead of being re-derived by parsing the report text. + let directorForcedStopReason: ForcedStopReason | undefined; let agentHandle: Awaited> | null = null; const requestContinuation = (): void => { try { @@ -642,6 +647,9 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { params.intent === "implement", shouldRequireEvidence(params), ); + director.observeForcedStop((reason) => { + directorForcedStopReason = reason; + }); director.observeInterventions((event) => { interventions(event); }); @@ -966,7 +974,10 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { // Normalize into the structured envelope so the parent always gets a // consistent shape even when the model rambling-returns free-form prose. const report = formatSubAgentReport(parseSubAgentReport(reply)); - return appendActivitySummary(report, toolNamesUsed); + return { + report: appendActivitySummary(report, toolNamesUsed), + ...(directorForcedStopReason !== undefined ? { stopReason: directorForcedStopReason } : {}), + }; } catch (err) { if (isSubAgentCancelError(err, runController.signal)) { // Close the recorder against the dead cycle before its inference.error @@ -1050,7 +1061,10 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { state: { totalToolCalls: toolNamesUsed.length }, ...(detail !== undefined ? { detail } : {}), }); - return appendActivitySummary(forcedStopReport(reason, partial, detail), toolNamesUsed); + return { + report: appendActivitySummary(forcedStopReport(reason, partial, detail), toolNamesUsed), + stopReason: reason, + }; } } throw err; diff --git a/src/subagent/stop-policy.ts b/src/subagent/stop-policy.ts index 193183d8f..40511bade 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -7,12 +7,7 @@ import type { ReactorEmittedEvent } from "@intx/inference"; import { onTurnBoundary } from "../agent/reactor-events.js"; import { detectSequencePeriod, type SequencePeriodCheck } from "../util/period-detection.js"; import { evaluateThrashStop, type ThrashConfig, type ThrashState } from "./thrash.js"; -import { - demoteNestedReportHeadings, - formatSubAgentReport, - hasReportEnvelope, - parseSubAgentReport, -} from "./report.js"; +import { demoteNestedReportHeadings, formatSubAgentReport, hasReportEnvelope } from "./report.js"; // Consecutive identical tool-call fingerprints before a leaf is forced to // stop. Mirrors IDENTICAL_REPEAT_MIN below (the director-level period-1 @@ -435,13 +430,10 @@ export type ForcedStopReason = | "repetition" | "incomplete-report"; -// Exact Summary text for each forced-stop reason. This is the single source -// of truth for both forcedStopReport (the producer) and the isXxxSubAgentReport -// classifiers (the consumers) — CL-6704: classifying on a free-text substring -// like "no progress" or "cancelled" hard-blocks a SUCCESSFUL report whose -// Summary happens to contain that phrase. Matching the exact string a forced -// stop actually produces closes that false-positive path without a report -// schema change (a typed marker would need one; see CL-6786, out of scope). +// Exact Summary text rendered for each forced-stop reason. Human-facing only — +// forcedStopReport is the sole reader; the parent classifies outcomes from the +// structured ForcedStopReason value itself (see run.ts/task-tool.ts), never by +// parsing this text back out of the report. const FORCED_STOP_SUMMARIES: Record = { "no-progress": "Stopped: repeated the same tool calls with no progress.", "no-ship": "Stopped: implement intent searched many files without writing any.", @@ -507,42 +499,6 @@ export function forcedStopReport( }); } -/** - * True when a report's Summary is exactly the forced-stop text for `reason` - * (CL-6704: exact match, not a free-text substring — a successful report - * whose Summary happens to mention the same words must not classify as a - * forced stop). - */ -export function isForcedStopSubAgentReport(report: string, reason: ForcedStopReason): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary === FORCED_STOP_SUMMARIES[reason]; -} - -/** True when the worker returned a turn-budget salvage report for the parent. */ -export function isTurnBudgetSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "turn-budget"); -} - -/** True when the worker returned a never-acted salvage report for the parent. */ -export function isNeverActedSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "never-acted"); -} - -/** True when implement intent finished without any write/edit tools. */ -export function isNeverEditedSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "never-edited"); -} - -/** True when the worker returned a deadline salvage report for the parent. */ -export function isDeadlineSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "deadline"); -} - -/** True when the worker returned a streamed-repetition salvage report. */ -export function isRepetitionSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "repetition"); -} - const TURN_BUDGET_PARENT_HINT = "[Sub-agent hit its turn budget before finishing. Continue from Findings rather than redoing completed work; re-dispatch with continuation context and a higher maxTurns if more work is warranted.]"; @@ -583,70 +539,37 @@ export interface SubAgentParentHintOptions { turnBudgetStopAfterDispatches?: number; } -export function appendTurnBudgetParentHint( - report: string, - options: SubAgentParentHintOptions = {}, -): string { - if (!isTurnBudgetSubAgentReport(report)) return report; - const stopAfter = options.turnBudgetStopAfterDispatches ?? 3; - const count = options.dispatchCount ?? 1; - const hint = count >= stopAfter ? TURN_BUDGET_STOP_PARENT_HINT : TURN_BUDGET_PARENT_HINT; - return `${hint}\n\n${report}`; -} - -export function appendNeverActedParentHint(report: string): string { - if (!isNeverActedSubAgentReport(report)) return report; - return `${NEVER_ACTED_PARENT_HINT}\n\n${report}`; -} - -export function appendNeverEditedParentHint(report: string): string { - if (!isNeverEditedSubAgentReport(report)) return report; - return `${NEVER_EDITED_PARENT_HINT}\n\n${report}`; -} - -export function appendDeadlineParentHint(report: string): string { - if (!isDeadlineSubAgentReport(report)) return report; - return `${DEADLINE_PARENT_HINT}\n\n${report}`; -} - -/** True when the worker returned a no-ship (search-tour) salvage report. */ -export function isNoShipSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "no-ship"); -} - -export function appendNoShipParentHint(report: string): string { - if (!isNoShipSubAgentReport(report)) return report; - return `${NO_SHIP_PARENT_HINT}\n\n${report}`; -} - -export function appendRepetitionParentHint(report: string): string { - if (!isRepetitionSubAgentReport(report)) return report; - return `${REPETITION_PARENT_HINT}\n\n${report}`; -} - -/** True when the worker returned a no-progress salvage report. */ -export function isNoProgressSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "no-progress"); -} - -export function appendNoProgressParentHint(report: string): string { - if (!isNoProgressSubAgentReport(report)) return report; - return `${NO_PROGRESS_PARENT_HINT}\n\n${report}`; -} - -/** Stack parent-visible salvage hints for budget / never-acted / deadline / repetition / no-progress. */ +/** + * Prepend the parent-facing salvage hint for `reason`, chosen from the + * structured ForcedStopReason the run reported directly — never by parsing + * `report`'s prose. Reasons with no dedicated hint (cancelled, stalled, + * incomplete-report, or a normal complete) pass `report` through unchanged. + */ export function appendSubAgentParentHints( report: string, + reason: ForcedStopReason | undefined, options: SubAgentParentHintOptions = {}, ): string { - return appendDeadlineParentHint( - appendNeverEditedParentHint( - appendNeverActedParentHint( - appendTurnBudgetParentHint( - appendNoProgressParentHint(appendNoShipParentHint(appendRepetitionParentHint(report))), - options, - ), - ), - ), - ); + switch (reason) { + case "turn-budget": { + const stopAfter = options.turnBudgetStopAfterDispatches ?? 3; + const count = options.dispatchCount ?? 1; + const hint = count >= stopAfter ? TURN_BUDGET_STOP_PARENT_HINT : TURN_BUDGET_PARENT_HINT; + return `${hint}\n\n${report}`; + } + case "never-acted": + return `${NEVER_ACTED_PARENT_HINT}\n\n${report}`; + case "never-edited": + return `${NEVER_EDITED_PARENT_HINT}\n\n${report}`; + case "deadline": + return `${DEADLINE_PARENT_HINT}\n\n${report}`; + case "no-ship": + return `${NO_SHIP_PARENT_HINT}\n\n${report}`; + case "repetition": + return `${REPETITION_PARENT_HINT}\n\n${report}`; + case "no-progress": + return `${NO_PROGRESS_PARENT_HINT}\n\n${report}`; + default: + return report; + } } diff --git a/src/subagent/task-tool-worktree.test.ts b/src/subagent/task-tool-worktree.test.ts index 4025e7243..9d58a7740 100644 --- a/src/subagent/task-tool-worktree.test.ts +++ b/src/subagent/task-tool-worktree.test.ts @@ -71,7 +71,7 @@ describe("createTaskTool worktree isolation", () => { useWorktree: true, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -106,7 +106,7 @@ describe("createTaskTool worktree isolation", () => { provider, run: async (params) => { captured = params; - return "done"; + return { report: "done" }; }, }); @@ -130,7 +130,7 @@ describe("createTaskTool worktree isolation", () => { useWorktree: true, run: async () => { ran = true; - return "done"; + return { report: "done" }; }, }); @@ -162,7 +162,7 @@ describe("createTaskTool worktree isolation", () => { worktreePath = params.cwd; // Simulate the sub-agent leaving uncommitted work behind. await writeFile(join(params.cwd, "new-file.txt"), "unfinished work"); - return "done"; + return { report: "done" }; }, }); @@ -203,7 +203,7 @@ describe("createTaskTool worktree isolation", () => { await writeFile(join(params.cwd, "wip.txt"), "half-finished change"); await run("git", ["add", "."], { cwd: params.cwd }); await run("git", ["stash"], { cwd: params.cwd }); - return "done"; + return { report: "done" }; }, }); diff --git a/src/subagent/task-tool.ts b/src/subagent/task-tool.ts index b654583db..736cdda2b 100644 --- a/src/subagent/task-tool.ts +++ b/src/subagent/task-tool.ts @@ -35,7 +35,7 @@ import { import { isCodexProviderName } from "../config/codex-providers.js"; import { DEFAULT_CANCEL_REASON, type SubAgentSessionStore } from "./session-store.js"; import { buildDispatchBrief, type TaskIntent } from "./report.js"; -import { appendSubAgentParentHints } from "./stop-policy.js"; +import { appendSubAgentParentHints, type ForcedStopReason } from "./stop-policy.js"; import { classifyBriefSalvage, createBriefDispatchLedger, @@ -55,6 +55,7 @@ import { join } from "node:path"; import type { NestedDispatchDeps, RunSubAgentParams, + RunSubAgentResult, SubAgentProvider, SubAgentSandboxDeps, } from "./types.js"; @@ -149,7 +150,7 @@ export type TaskToolDeps = SubAgentSandboxDeps & { provider: SubAgentProvider | (() => SubAgentProvider); // Required runner — inject runSubAgent in production, a mock in tests. // Keeping this required (no default import of run) breaks the run↔task-tool cycle. - run: (params: RunSubAgentParams) => Promise; + run: (params: RunSubAgentParams) => Promise; onEvent?: (event: ReactorEmittedEvent) => void; onProgress?: (info: { description: string; toolName: string }) => void; // When set, each spawn is recorded as an inspectable session (identity, @@ -192,9 +193,20 @@ export type TaskToolDeps = SubAgentSandboxDeps & { telemetry?: Telemetry; }; -function taskToolResult(callId: string, content: string): ToolResult { +function taskToolResult( + callId: string, + content: string, + stopReason?: ForcedStopReason, +): ToolResult { const isError = content.startsWith("Error:") || content.startsWith("Error "); - return { callId, content, ...(isError ? { isError: true } : {}) }; + return { + callId, + content, + ...(isError ? { isError: true } : {}), + // Structured stop-reason side channel (CL-6946 part 2): the parent chat + // director classifies salvage outcomes from this, never from `content`. + ...(stopReason !== undefined ? { detail: { stopReason } } : {}), + }; } type RequiredTaskField = "description" | "prompt"; @@ -842,7 +854,10 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { const wasCancelled = childCtl.signal.aborted || (session !== undefined && deps.sessions?.get(session.id)?.status === "cancelled"); - const salvage = classifyBriefSalvage(result); + const salvage = classifyBriefSalvage({ + ...(result.stopReason !== undefined ? { stopReason: result.stopReason } : {}), + wasCancelled, + }); briefLedger.recordOutcome(fingerprint, salvage); // Prefer the last provider/model that actually served inference // (captured off inference.done above) over the pre-dispatch @@ -862,15 +877,27 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { if (session !== undefined && deps.sessions?.get(session.id)?.status === "running") { deps.sessions.cancel(session.id, cancelReason(childCtl.signal)); } - const reported = appendSubAgentParentHints(result, hintOptions); + const reported = appendSubAgentParentHints( + result.report, + result.stopReason, + hintOptions, + ); return await finishWithWorktree( - taskToolResult(call.id, `Sub-agent "${description}" reported:\n\n${reported}`), + taskToolResult( + call.id, + `Sub-agent "${description}" reported:\n\n${reported}`, + salvage ?? undefined, + ), ); } - if (session !== undefined) deps.sessions?.complete(session.id, result); - const reported = appendSubAgentParentHints(result, hintOptions); + if (session !== undefined) deps.sessions?.complete(session.id, result.report); + const reported = appendSubAgentParentHints(result.report, result.stopReason, hintOptions); return await finishWithWorktree( - taskToolResult(call.id, `Sub-agent "${description}" reported:\n\n${reported}`), + taskToolResult( + call.id, + `Sub-agent "${description}" reported:\n\n${reported}`, + salvage ?? undefined, + ), ); } catch (err) { if ( diff --git a/src/subagent/types.ts b/src/subagent/types.ts index 77d4c3030..9c7da2ad8 100644 --- a/src/subagent/types.ts +++ b/src/subagent/types.ts @@ -18,6 +18,7 @@ import type { ReasoningEffort } from "../provider/reasoning-effort.js"; import type { SubAgentSessionStore } from "./session-store.js"; import type { TaskIntent } from "./report.js"; import type { SubagentTier } from "../agent/directors/types.js"; +import type { ForcedStopReason } from "./stop-policy.js"; export interface SubAgentProvider { providerName: string; @@ -148,3 +149,9 @@ export type RunSubAgentParams = { /** DirectorPackage.reportContract.outputType, when the resolved leaf declares one. */ reportType?: OutputType; } & SubAgentSandboxDeps; + +/** runSubAgent's result: the parent-facing report plus, when force-stopped, the structured reason why (CL-6946 part 2) — classify outcomes from `stopReason`, never by parsing `report`. */ +export interface RunSubAgentResult { + report: string; + stopReason?: ForcedStopReason; +} diff --git a/tests/unit/subagent-session-store.test.ts b/tests/unit/subagent-session-store.test.ts index ad9a9cf45..beede85f5 100644 --- a/tests/unit/subagent-session-store.test.ts +++ b/tests/unit/subagent-session-store.test.ts @@ -304,7 +304,7 @@ describe("createTaskTool session recording", () => { sessions: store, run: async (params) => { params.onEvent?.(event("inference.text.delta", { token: "working" })); - return "## Summary\nDone."; + return { report: "## Summary\nDone." }; }, }); const out = await call(tool, { @@ -351,7 +351,7 @@ describe("createTaskTool session recording", () => { cwd: process.cwd(), getWorkdirBase: () => "/tmp", provider, - run: async () => "ok", + run: async () => ({ report: "ok" }), }); const out = await call(tool, { description: "no store", prompt: "x", intent: "explore" }); expect(out).toContain("ok"); @@ -394,7 +394,7 @@ describe("createTaskTool session recording", () => { store.cancel(id!, "Cancelled from Agents strip"); }); }); - return "should not complete"; + return { report: "should not complete" }; }, }); const out = await call(tool, { @@ -428,7 +428,7 @@ describe("createTaskTool session recording", () => { ); queueMicrotask(() => parent.abort()); }); - return "nope"; + return { report: "nope" }; }, }); const out = await call( diff --git a/tests/unit/subagent.test.ts b/tests/unit/subagent.test.ts index 727e8fa5e..a78df4283 100644 --- a/tests/unit/subagent.test.ts +++ b/tests/unit/subagent.test.ts @@ -54,7 +54,7 @@ test("handler rejects empty description or prompt, naming only the empty field", cwd: "/repo", getWorkdirBase: () => "/repo/.ctx", provider, - run: async () => "should not run", + run: async () => ({ report: "should not run" }), }); const emptyDesc = await callHandler(tool, { description: "", prompt: "do it" }); expect(emptyDesc).toContain("Error: task requires a non-empty description"); @@ -72,7 +72,7 @@ test("handler rejects missing required fields, naming only the missing ones", as cwd: "/repo", getWorkdirBase: () => "/repo/.ctx", provider, - run: async () => "should not run", + run: async () => ({ report: "should not run" }), }); const missingPrompt = await callHandler(tool, { description: "Add GET /health route" }); expect(missingPrompt).toContain( @@ -103,7 +103,7 @@ test("generic leaf gets role-default medium even when parent effort is high", as provider: { ...provider, reasoningEffort: "high" }, run: async (params) => { receivedEffort = params; - return "done"; + return { report: "done" }; }, }); @@ -122,7 +122,7 @@ test("a provider getter is resolved at spawn time, so a live switch reaches suba provider: () => current, run: async (params) => { received = params; - return "done"; + return { report: "done" }; }, }); @@ -144,7 +144,7 @@ test("handler forwards trimmed args to the runner and wraps the result", async ( provider, run: async (params) => { received = params; - return "found three callers in foo.ts"; + return { report: "found three callers in foo.ts" }; }, }); @@ -201,7 +201,7 @@ test("unknown agent id fails closed instead of silent generic fall-through", asy profiles: [{ id: "greybeard", systemPromptRole: "You are greybeard." }], run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); const result = await callHandler(tool, { @@ -226,7 +226,7 @@ test("unknown agent id fails closed when no profiles are loaded", async () => { provider, run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); // Non-director ids still require profiles; directors resolve from the closed registry. @@ -249,7 +249,7 @@ test("closed director resolves without profiles loaded", async () => { provider, run: async (params) => { received = params; - return "ok"; + return { report: "ok" }; }, }); const result = await callHandler(tool, { @@ -271,7 +271,7 @@ test("intent maps to closed director without profiles", async () => { provider, run: async (params) => { received = params; - return "ok"; + return { report: "ok" }; }, }); const result = await callHandler(tool, { @@ -297,7 +297,7 @@ test("intent general is refused (no general director)", async () => { provider, run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); const result = await callHandler(tool, { @@ -319,7 +319,7 @@ test("bare task without agent or intent is refused (no catch-all worker)", async provider, run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); const result = await callHandler(tool, { @@ -341,7 +341,7 @@ test("spawnAllowlist rejects children outside the parent director matrix", async spawnAllowlist: ["intern", "explore", "critique"], run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); const denied = await callHandler(tool, { @@ -371,7 +371,7 @@ test("task refuses skywalker as a spawned worker", async () => { provider, run: async () => { ran = true; - return "should not run"; + return { report: "should not run" }; }, }); const result = await callHandler(tool, { @@ -394,7 +394,7 @@ test("greybeard nestedDispatch carries spawn allowlist into nested task", async provider, run: async (params) => { nestedAllow = params.nestedDispatch?.spawnAllowlist; - return "reviewed"; + return { report: "reviewed" }; }, }); await callHandler(tool, { @@ -421,7 +421,7 @@ test("orchestrator profile installs nestedDispatch so task can be re-dispatched" ], run: async (params) => { received = params; - return "coordinated"; + return { report: "coordinated" }; }, }); await callHandler(tool, { @@ -453,7 +453,7 @@ test("nested dispatch forwards the external sink, not the orchestrator recorder" type: "inference.text.delta", data: { token: "grandchild" }, } as ReactorEmittedEvent); - return "coordinated"; + return { report: "coordinated" }; }, }); await callHandler(tool, { description: "fan out", prompt: "dispatch", agent: "dispatch" }); @@ -475,7 +475,7 @@ test("allowOrchestrator false strips orchestrator even when the profile is marke profiles: [{ id: "dispatch", orchestrator: true }], run: async (params) => { received = params; - return "leaf"; + return { report: "leaf" }; }, }); await callHandler(tool, { @@ -514,7 +514,7 @@ test("handler injects context and goals into runner params when provided", async provider, run: async (params) => { received = params; - return "task completed"; + return { report: "task completed" }; }, }); @@ -543,7 +543,7 @@ test("handler omits context and goals when empty", async () => { provider, run: async (params) => { receivedNoContext = params; - return "done"; + return { report: "done" }; }, }); @@ -554,7 +554,7 @@ test("handler omits context and goals when empty", async () => { provider, run: async (params) => { receivedEmptyContext = params; - return "done"; + return { report: "done" }; }, }); @@ -708,7 +708,7 @@ test("a profile-resolved provider carries the bifrost virtual-key marker", async ], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); @@ -757,7 +757,7 @@ describe("createTaskTool profile resolution", () => { ], run: async () => { runs += 1; - return "should-not-be-called"; + return { report: "should-not-be-called" }; }, }); @@ -796,7 +796,7 @@ describe("createTaskTool profile resolution", () => { ], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); @@ -834,7 +834,7 @@ describe("createTaskTool profile resolution", () => { ], run: async () => { runs += 1; - return "should-not-be-called"; + return { report: "should-not-be-called" }; }, }); @@ -873,7 +873,7 @@ describe("createTaskTool profile resolution", () => { ], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); @@ -906,7 +906,7 @@ describe("createTaskTool profile resolution", () => { ], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); @@ -938,7 +938,7 @@ describe("createTaskTool profile resolution", () => { ], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); @@ -965,7 +965,7 @@ describe("createTaskTool profile resolution", () => { profiles: [{ id: "karen", systemPromptRole: "You are karen.", orchestrator: true }], run: async (params) => { received = params; - return "ran"; + return { report: "ran" }; }, }); diff --git a/tests/unit/telemetry-product-events.test.ts b/tests/unit/telemetry-product-events.test.ts index 0709d1cdf..7d254bb4d 100644 --- a/tests/unit/telemetry-product-events.test.ts +++ b/tests/unit/telemetry-product-events.test.ts @@ -197,7 +197,7 @@ test('subagent events bucket a project-defined profile id to "custom"', async () profiles: [ { id: "acmecorp-release-captain", description: "release", systemPromptRole: "release" }, ], - run: async () => "done", + run: async () => ({ report: "done" }), telemetry, }); if (tool.kind !== "full") throw new Error(`expected full tool, got ${tool.kind}`);