diff --git a/packages/cli/src/commands/review/check-coverage.test.ts b/packages/cli/src/commands/review/check-coverage.test.ts index a791e878c15..8992b26f9ae 100644 --- a/packages/cli/src/commands/review/check-coverage.test.ts +++ b/packages/cli/src/commands/review/check-coverage.test.ts @@ -168,9 +168,20 @@ function transcript( * credit narrows to its ranged reads. */ range?: [number, number]; + /** + * Launch-path provenance stamped on the records — `workflow` for a + * workflow agent() dispatch, which shares this directory but is not an + * agent this review launched. + */ + agentKind?: string; } = {}, ): void { - const base = { agentId: id, agentName: 'general-purpose', sessionId: 'S1' }; + const base = { + agentId: id, + agentName: 'general-purpose', + sessionId: 'S1', + ...(opts.agentKind ? { agentKind: opts.agentKind } : {}), + }; const pointedAtBriefs = [ ...launchPrompt.matchAll(/read_file\(file_path="([^"]*\.brief\.md)"\)/g), ].map((m) => m[1]); @@ -433,6 +444,56 @@ describe('coverage — from the harness, not from the caller', () => { expect(r.ok).toBe(false); }); + it('ignores workflow-dispatch transcripts, however they read', () => { + // Workflow agent() dispatches write their transcripts into this same + // directory. A pre-launch failure or an abandoned attempt leaves a + // seed-only record — zero tool calls — that used to be classified idle + // BEFORE the "not launched by this review" escape could see it, failing + // a compliant review. Provenance, not prompt shape, decides. + transcript('a1', good(1), { calls: 2 }); + transcript('a2', good(2), { calls: 2 }); + // Seed-only: the prompt carries no diff path. + transcript('workflow-agent-deadbeef01', 'review the widget cache', { + calls: 0, + agentKind: 'workflow', + }); + // Prompt that happens to name the diff — `given` would be true, and + // zero tool calls would read as an idled review agent. + transcript('workflow-agent-deadbeef02', `read ${DIFF} and summarize`, { + calls: 0, + agentKind: 'workflow', + }); + // Prompt that happens to say `chunk 1 of 2` — would otherwise be + // adopted as that chunk's agent and reported blind. + transcript('workflow-agent-deadbeef03', 'chunk 1 of 2 of my own work', { + calls: 0, + agentKind: 'workflow', + }); + + const r = coverageFromTranscripts(plan(), ENV); + expect(r.ok).toBe(true); + expect(r.idleAgents).toEqual([]); + expect(r.blindAgents).toEqual([]); + // The foreign records do not even count as agents of this review: + // the two chunk agents plus the roster's test-matrix stand-in, and + // none of the three workflow transcripts. + expect(r.agents).toBe(3); + }); + + it('skips zero-tool-call records this review never launched', () => { + // The "not launched" escape runs BEFORE the idle classification: a + // foreign record with no diff in its prompt owes the review nothing, + // whatever it did or did not call — including agents the review's own + // agents spawned. + transcript('a1', good(1), { calls: 2 }); + transcript('a2', good(2), { calls: 2 }); + transcript('nested-spawn', 'check the build logs', { calls: 0 }); + + const r = coverageFromTranscripts(plan(), ENV); + expect(r.ok).toBe(true); + expect(r.idleAgents).toEqual([]); + }); + it('names a blind launch as itself — the prompt is the defect, not the agent', () => { // The real failure, 23 times over: the agent was handed a description of a // chunk it had no way to open. Calling this a whiff sends the reader off to @@ -1981,6 +2042,35 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', () expect(gapText(r)).not.toMatch(/verification/); }); + it('ignores workflow-dispatch transcripts, however they match the floor', () => { + // Same provenance fence as coverageFromTranscripts, asserted where it + // bites: a workflow agent() dispatch launched with the VERBATIM built + // prompt, which opened the brief and read the findings file, is the + // exact record shape that satisfies the verifier floor. Without the + // filter this one record flips the gate to ok while nothing this + // review launched ever verified anything. + const p = plan(); + step45(p, 'reverse-audit'); // Step 5 compliant; verification is the subject + const key = 'verify--abc123def456'; + step45(p, key, { findings: true, launch: false }); + const recordedPrompt = readFileSync( + join(promptRecordDir(p), `${key}.txt`), + 'utf8', + ); + transcript('workflow-agent-deadbeef01', recordedPrompt, { + calls: 2, + opens: [briefPath(p, key), findingsFilePath(p, key)], + agentKind: 'workflow', + }); + + const r = verificationGaps(p, { postsFindings: true }, ENV); + expect(r.ok).toBe(false); + expect(r.unverifiedFindings).toBe(true); + expect(gapText(r)).toMatch( + /verification — its prompt was built, but no agent was launched with it/, + ); + }); + it('flags a verifier built but whose agent never opened its brief', () => { const p = plan(); step45(p, 'reverse-audit'); diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 883de9cc898..9818bd28ed5 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -380,7 +380,16 @@ export function coverageFromTranscripts( env: NodeJS.ProcessEnv = process.env, ): CoverageFromTranscripts { const { plan, mtimeMs } = readPlan(planPath); - const records = readTranscripts(mtimeMs, env, plan.diffPathAbsolute); + const records = readTranscripts(mtimeMs, env, plan.diffPathAbsolute).filter( + // Workflow `agent()` dispatches write their transcripts into this same + // directory, but they are not agents this review launched. A seed-only + // workflow record made zero tool calls and would be classified idle + // before the "not launched by this review" escape below could see it, + // and a workflow prompt that happens to say `chunk N of M` would be + // adopted as that chunk's agent. Provenance, not prompt shape, decides + // whose evidence this gate reads. + (rec) => rec.agentKind !== 'workflow', + ); const built = readRecordedPrompts(planPath); const blindAgents: string[] = []; @@ -565,6 +574,16 @@ export function coverageFromTranscripts( continue; // Its silence proves nothing about the diff; the prompt failed. } + // Not a diff reader, and not required to be. Two review agents legitimately + // never open the diff — Build & Test runs the build, Issue Fidelity reads the + // issue — and the session's transcript directory also holds agents this review + // did not launch, including ones its own agents spawned. None of them owes the + // diff anything; none of them may be credited with having read it either. + // Runs BEFORE the idle check, on purpose: a zero-tool-call record this + // review never launched is not a review agent that idled, and classifying + // it as one is how a foreign transcript fails a compliant review. + if (!given) continue; + // Did it work? Zero successful tool calls means it read nothing — whatever // its prose says. This is checked BEFORE the Uncoverable claim below, and the // order is load-bearing: `Uncoverable: chunk N` is a line the prompt hands the @@ -576,13 +595,6 @@ export function coverageFromTranscripts( continue; } - // Not a diff reader, and not required to be. Two review agents legitimately - // never open the diff — Build & Test runs the build, Issue Fidelity reads the - // issue — and the session's transcript directory also holds agents this review - // did not launch, including ones its own agents spawned. None of them owes the - // diff anything; none of them may be credited with having read it either. - if (!given) continue; - // The prompt the CLI built for this chunk, against the prompt the harness // recorded the agent being launched with. Nothing else in the run can see the // difference: a paraphrase keeps the diff path, so every other check passes. @@ -1327,7 +1339,12 @@ export function verificationGaps( env: NodeJS.ProcessEnv = process.env, ): VerificationReport { const { plan, mtimeMs } = readPlan(planPath); - const records = readTranscripts(mtimeMs, env, plan.diffPathAbsolute); + // Same provenance fence as coverageFromTranscripts: workflow dispatches + // share the transcript directory but are not agents this review launched, + // so they neither satisfy a delivery floor nor count against it. + const records = readTranscripts(mtimeMs, env, plan.diffPathAbsolute).filter( + (rec) => rec.agentKind !== 'workflow', + ); const built = readRecordedPrompts(planPath); const gaps: VerificationReport['gaps'] = []; const remediation: string[] = []; diff --git a/packages/cli/src/commands/review/lib/transcripts.test.ts b/packages/cli/src/commands/review/lib/transcripts.test.ts index 876924081f7..d6b62a651e4 100644 --- a/packages/cli/src/commands/review/lib/transcripts.test.ts +++ b/packages/cli/src/commands/review/lib/transcripts.test.ts @@ -135,6 +135,28 @@ describe('readTranscripts — defensive parsing', () => { expect(recs[0].launchPrompt).toBe('chunk 1 of 1'); }); + it('reads the launch-path provenance stamped on the records', () => { + // Workflow agent() dispatches share this directory with Agent-tool + // launches; readers filter on the stamped field, so the parse must + // surface it. + const b = { + agentId: 'workflow-agent-deadbeef', + agentName: 'workflow-agent', + agentKind: 'workflow', + sessionId: 'S1', + }; + file( + 'agent-workflow-agent-deadbeef.jsonl', + JSON.stringify({ + ...b, + type: 'user', + message: { role: 'user', parts: [{ text: 'do the thing' }] }, + }) + '\n', + ); + const [rec] = readTranscripts(undefined, ENV); + expect(rec.agentKind).toBe('workflow'); + }); + it('counts only successful tool calls', () => { const b = { agentId: 'a1', agentName: 'general-purpose', sessionId: 'S1' }; const call = { diff --git a/packages/cli/src/commands/review/lib/transcripts.ts b/packages/cli/src/commands/review/lib/transcripts.ts index c0193968cad..e992ffa8e58 100644 --- a/packages/cli/src/commands/review/lib/transcripts.ts +++ b/packages/cli/src/commands/review/lib/transcripts.ts @@ -45,6 +45,12 @@ import { join } from 'node:path'; export interface AgentRecord { agentId: string; agentName: string; + /** + * Launch-path provenance stamped on the records, when present. + * `'workflow'` marks a workflow `agent()` dispatch — a transcript that + * shares this directory but is not an agent the review launched. + */ + agentKind?: string; /** The prompt the agent was launched with — the transcript's first record. */ launchPrompt: string; /** Tool calls that came back without an error. */ @@ -211,6 +217,7 @@ function parseTranscript(file: string, diffPath?: string): AgentRecord | null { let agentId = ''; let agentName = ''; + let agentKind = ''; let launchPrompt = ''; let finalText = ''; let successfulToolCalls = 0; @@ -245,6 +252,9 @@ function parseTranscript(file: string, diffPath?: string): AgentRecord | null { if (!agentName && typeof rec['agentName'] === 'string') { agentName = rec['agentName']; } + if (!agentKind && typeof rec['agentKind'] === 'string') { + agentKind = rec['agentKind']; + } const type = rec['type']; @@ -325,6 +335,7 @@ function parseTranscript(file: string, diffPath?: string): AgentRecord | null { return { agentId, agentName, + ...(agentKind ? { agentKind } : {}), launchPrompt, successfulToolCalls, diffToolCalls, diff --git a/packages/core/src/agents/agent-transcript.ts b/packages/core/src/agents/agent-transcript.ts index a96eb3475ec..7bceb7cacd5 100644 --- a/packages/core/src/agents/agent-transcript.ts +++ b/packages/core/src/agents/agent-transcript.ts @@ -287,6 +287,13 @@ export interface AttachJsonlOptions { agentId: string; /** Display name (subagent type), e.g. "explore". */ agentName?: string; + /** + * Launch path provenance, stamped on every record — see + * `ChatRecord.agentKind`. Workflow dispatches pass `'workflow'` so + * readers of the shared subagents directory can tell them apart from + * Agent-tool launches. + */ + agentKind?: 'workflow'; /** UI hint. */ agentColor?: string; /** Parent user-session UUID — recorded as `sessionId` on every record. */ @@ -397,6 +404,7 @@ export function attachJsonlTranscriptWriter( agentId: options.agentId, agentName: options.agentName, agentColor: options.agentColor, + agentKind: options.agentKind, isSidechain: true, }); diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 8af40bbd9b1..bbaba21fa21 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -7,7 +7,10 @@ // T7 (PR #4732 R1): the `vi as vitest` alias diverges from every other // test file in the repo. Use `vi` directly. import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import * as fsSync from 'node:fs'; import * as os from 'node:os'; +import * as path from 'node:path'; import { WorkflowOrchestrator, WorkflowExecutionError, @@ -125,6 +128,29 @@ vi.mock('../../services/gitWorktreeService.js', async (importOriginal) => { }; }); +// The transcript-pairing warns are the only record some terminal failures +// leave of their attempt id (the verbatim errors cannot carry it in the +// message), so the pairing tests capture them directly instead of enabling +// the debug log file. +const debugLogRecorder = vi.hoisted(() => ({ + warn: vi.fn(), +})); + +vi.mock('../../utils/debugLogger.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + createDebugLogger: () => ({ + isEnabled: () => true, + debug: () => {}, + info: () => {}, + warn: (...args: unknown[]) => debugLogRecorder.warn(...args), + error: () => {}, + }), + }; +}); + vi.mock('./agent-headless.js', () => ({ AgentHeadless: { create: async ( @@ -1698,6 +1724,7 @@ describe('createProductionDispatch', () => { nextFinalText.value = undefined; nextTerminateMode.value = 'GOAL'; nextExecuteHook.value = undefined; + debugLogRecorder.warn.mockClear(); }); it('routes calls through AgentHeadless and returns getFinalText', async () => { @@ -1710,6 +1737,720 @@ describe('createProductionDispatch', () => { expect(created[0]!.agentId).toMatch(/^workflow-agent-[0-9a-f]{16}$/); }); + // --- per-dispatch transcript ------------------------------------------ + // + // Workflow subagents were the only agents in the product that left no + // record on disk. These cases pin the SHAPE, not just the existence of a + // file: a consumer of `/subagents//` answers "what + // did this agent actually do" by reading the launch prompt out of the + // first record and pairing functionCall/functionResponse by callId. A file + // that exists but carries neither answers nothing. + describe('dispatch transcript', () => { + let projectDir: string; + + beforeEach(() => { + projectDir = fsSync.mkdtempSync(path.join(os.tmpdir(), 'wf-tx-')); + }); + + afterEach(() => { + fsSync.rmSync(projectDir, { recursive: true, force: true }); + }); + + const SESSION = 'sess-transcript-test'; + + function transcriptConfig(): Config { + return { + storage: { getProjectDir: () => projectDir }, + getSessionId: () => SESSION, + getProjectRoot: () => projectDir, + getCliVersion: () => '0.0.0-test', + } as unknown as Config; + } + + function transcriptFiles(): string[] { + const dir = path.join(projectDir, 'subagents', SESSION); + if (!fsSync.existsSync(dir)) return []; + return fsSync + .readdirSync(dir) + .filter((f) => f.endsWith('.jsonl')) + .map((f) => path.join(dir, f)); + } + + function recordsOf(file: string): Array> { + return fsSync + .readFileSync(file, 'utf8') + .split('\n') + .filter((l) => l.trim()) + .map((l) => JSON.parse(l) as Record); + } + + // Override-path dispatch routes through SubagentManager.createAgentHeadless + // rather than AgentHeadless.create, so exercising the transcript there + // needs a config that merges the subagent-manager stub with the + // transcript methods (the override-path suite's fakeConfigWithMgr has no + // getProjectRoot, so attach throws there and is swallowed). The stub's + // execute emits a tool-call pair on the emitter the stall wrapper + // forwards into createAgentHeadless. + function overrideTranscriptConfig( + resolveName: (name: string) => string | null, + ): Config { + return { + storage: { getProjectDir: () => projectDir }, + getSessionId: () => SESSION, + getProjectRoot: () => projectDir, + getCliVersion: () => '0.0.0-test', + getSubagentManager: () => ({ + findSubagentByName: async (name: string) => { + const canonical = resolveName(name); + return canonical === null + ? null + : { + name: canonical, + description: 'stub subagent', + systemPrompt: 'You are a stub.', + level: 'builtin', + }; + }, + createAgentHeadless: async ( + _subagentConfig: unknown, + _runtimeContext: unknown, + options?: { eventEmitter?: unknown }, + ) => + makeStubSubagent({ + execute: async () => { + const ee = options?.eventEmitter as + | AgentEventEmitter + | undefined; + ee?.emit(AgentEventType.TOOL_CALL, { + subagentId: 'workflow-agent', + round: 1, + callId: 'call-override-1', + name: 'glob', + args: { pattern: '**/*.ts' }, + description: 'find files', + timestamp: Date.now(), + }); + ee?.emit(AgentEventType.TOOL_RESPONSES_FINALIZED, { + subagentId: 'workflow-agent', + round: 1, + responses: [ + { + callId: 'call-override-1', + responseParts: [ + { + functionResponse: { + id: 'call-override-1', + name: 'glob', + response: { output: 'src/index.ts' }, + }, + }, + ], + }, + ], + timestamp: Date.now(), + }); + }, + finalText: () => 'override-output', + terminateMode: () => nextTerminateMode.value, + }), + }), + } as unknown as Config; + } + + it('writes one transcript per dispatch, opening with the launch prompt', async () => { + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('review the diff', { label: 'reviewer' }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + // Named for the same id the dispatch reports in its terminal error, so + // a failure message leads straight to the record. + expect(path.basename(files[0])).toMatch( + /^agent-workflow-agent-[0-9a-f]{16}\.jsonl$/, + ); + + const records = recordsOf(files[0]); + const first = records[0]; + expect(first['type']).toBe('user'); + expect(first['agentId']).toBe(created[0]!.agentId); + expect(first['agentName']).toBe('reviewer'); + expect(first['sessionId']).toBe(SESSION); + // Provenance: readers of the shared subagents directory — the + // /review coverage gate — filter workflow dispatches on this field. + expect(first['agentKind']).toBe('workflow'); + // The audit annotations the feature exists to carry — un-pinned, a + // rebuild of the attach options could drop them without a red test. + expect(first['cwd']).toBe(projectDir); + expect(first['version']).toBe('0.0.0-test'); + // The prompt is written at attach time, before the model has produced + // anything — which is what makes it evidence the agent cannot revise. + expect( + (first['message'] as { parts: Array<{ text?: string }> }).parts[0].text, + ).toBe('review the diff'); + }); + + it("records the agent's tool calls, pairable by callId", async () => { + nextExecuteHook.value = async (emitter) => { + emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: 'workflow-agent', + round: 1, + callId: 'call-1', + name: 'read_file', + args: { absolute_path: '/repo/diff.txt', offset: 0, limit: 40 }, + description: 'read the diff', + timestamp: Date.now(), + }); + emitter.emit(AgentEventType.TOOL_RESPONSES_FINALIZED, { + subagentId: 'workflow-agent', + round: 1, + responses: [ + { + callId: 'call-1', + responseParts: [ + { + functionResponse: { + id: 'call-1', + name: 'read_file', + response: { output: 'diff text' }, + }, + }, + ], + }, + ], + timestamp: Date.now(), + }); + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('read it', { label: 'reader' }); + + const records = recordsOf(transcriptFiles()[0]); + const call = records.find((r) => + ( + r['message'] as { parts?: Array<{ functionCall?: unknown }> } + )?.parts?.some((p) => p.functionCall !== undefined), + ); + const result = records.find((r) => r['type'] === 'tool_result'); + expect(call).toBeDefined(); + expect(result).toBeDefined(); + + const fc = ( + call!['message'] as { + parts: Array<{ + functionCall?: { id: string; name: string; args: unknown }; + }>; + } + ).parts[0].functionCall!; + expect(fc.id).toBe('call-1'); + expect(fc.name).toBe('read_file'); + // The args are what let a reader ask "which file, which lines" rather + // than only "it called something". + expect(fc.args).toEqual({ + absolute_path: '/repo/diff.txt', + offset: 0, + limit: 40, + }); + expect((result!['toolCallResult'] as { callId: string }).callId).toBe( + 'call-1', + ); + }); + + // A retried dispatch is two agent runs, and a reader that saw one merged + // record could not tell a stall from an agent that behaved oddly. The id + // is minted per attempt precisely so each leaves its own file. + it('writes a separate transcript for each stall retry attempt', async () => { + let attempt = 0; + nextExecuteHook.value = async (emitter, signal) => { + attempt += 1; + if (attempt > 1) { + nextTerminateMode.value = 'GOAL'; + return; + } + nextTerminateMode.value = 'CANCELLED'; + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'workflow-agent', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + await new Promise((resolve) => { + if (signal?.aborted) { + resolve(); + return; + } + signal?.addEventListener('abort', () => resolve(), { once: true }); + }); + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('stall once', { label: 'staller', stallMs: 20 }); + + expect(attempt).toBe(2); + const files = transcriptFiles(); + expect(files).toHaveLength(2); + // Each is self-describing: both carry the launch prompt, so neither is + // a fragment that only makes sense beside the other. + for (const file of files) { + expect(recordsOf(file)[0]['type']).toBe('user'); + } + }); + + // The transcript is audit metadata. A dispatch that would have succeeded + // must not fail because the record could not be written — which is also + // why every other test in this file, whose fakeConfig() has none of the + // methods used above, still passes. + it('does not fail the dispatch when the transcript cannot be written', async () => { + const broken = { + storage: { + getProjectDir: () => { + throw new Error('no project dir'); + }, + }, + getSessionId: () => SESSION, + getProjectRoot: () => projectDir, + getCliVersion: () => '0.0.0-test', + } as unknown as Config; + + const dispatch = createProductionDispatch(broken); + await expect(dispatch('still works', { label: 'x' })).resolves.toBe( + 'headless-said:still works', + ); + expect(transcriptFiles()).toHaveLength(0); + }); + + // Both fallback branches set: the script-chosen label must win over the + // model-authored agentType, or the transcript-to-script-line name + // matching the attach comment promises breaks. agentType forces the + // override path, so this also exercises the fallback chain there. + it('records the label over agentType when both are set', async () => { + const config = overrideTranscriptConfig((name) => + name.toLowerCase() === 'explore' ? 'Explore' : null, + ); + const dispatch = createProductionDispatch(config); + await dispatch('review the diff', { + label: 'reviewer', + agentType: 'Explore', + }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('reviewer'); + }); + + it('falls back to the constant agentName when neither option is set', async () => { + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('plain run', {}); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('workflow-agent'); + }); + + // One attach point covers both dispatch paths — the override path must + // produce the same record shape through the emitter the stall wrapper + // forwards into createAgentHeadless: launch prompt first, tool calls + // pairable by callId. + it('records override-path dispatches through the same attach point', async () => { + const config = overrideTranscriptConfig((name) => + name.toLowerCase() === 'explore' ? 'Explore' : null, + ); + const dispatch = createProductionDispatch(config); + await dispatch('find foo', { agentType: 'Explore', label: 'e' }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + const records = recordsOf(files[0]); + const first = records[0]; + expect(first['type']).toBe('user'); + expect(first['agentName']).toBe('e'); + expect( + (first['message'] as { parts: Array<{ text?: string }> }).parts[0].text, + ).toBe('find foo'); + + const call = records.find((r) => + ( + r['message'] as { parts?: Array<{ functionCall?: unknown }> } + )?.parts?.some((p) => p.functionCall !== undefined), + ); + const result = records.find((r) => r['type'] === 'tool_result'); + expect(call).toBeDefined(); + expect(result).toBeDefined(); + expect( + ( + ( + call!['message'] as { + parts: Array<{ functionCall?: { id?: string } }>; + } + ).parts[0].functionCall as { id: string } | undefined + )?.id, + ).toBe('call-override-1'); + expect((result!['toolCallResult'] as { callId: string }).callId).toBe( + 'call-override-1', + ); + }); + + // Subagent resolution is case-insensitive, and the Agent tool records + // the canonical SubagentConfig name — a lowercase model-authored + // agentType must land on the same canonical name or the two launch + // paths split any reader joining on agentName. + it('records the canonical subagent name when only agentType is set', async () => { + const config = overrideTranscriptConfig((name) => + name.toLowerCase() === 'explore' ? 'Explore' : null, + ); + const dispatch = createProductionDispatch(config); + await dispatch('review X', { agentType: 'explore' }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('Explore'); + }); + + // Resolution fallbacks: an unregistered agentType (a model typo or a + // custom name) must still record the raw string, and an unavailable + // manager must lose only the name resolution — never the transcript. + it('records the raw agentType when it is not a registered subagent', async () => { + const config = overrideTranscriptConfig(() => null); + const dispatch = createProductionDispatch(config); + await expect( + dispatch('find foo', { agentType: 'NoSuchAgent' }), + ).rejects.toThrow(/agent type 'NoSuchAgent' not found/); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('NoSuchAgent'); + }); + + it('still writes the transcript when subagent resolution is unavailable', async () => { + // transcriptConfig() has no getSubagentManager: the attach's + // best-effort resolution swallows the throw and falls back to the raw + // name; the override path's authoritative lookup then rejects. + const dispatch = createProductionDispatch(transcriptConfig()); + await expect( + dispatch('find foo', { agentType: 'Explore' }), + ).rejects.toThrow(); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('Explore'); + }); + + // R2-4: detach is the only path that removes the writer's listeners — + // if it regresses, events emitted on the attempt emitter after the + // dispatch settles still grow the transcript, and nothing fails. + it('detaches the transcript writer when the dispatch settles', async () => { + let captured: AgentEventEmitter | undefined; + nextExecuteHook.value = async (emitter) => { + captured = emitter; + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('settle', { label: 'd' }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + const before = recordsOf(files[0]).length; + captured!.emit(AgentEventType.TOOL_CALL, { + subagentId: 'workflow-agent', + round: 1, + callId: 'call-post-detach', + name: 'glob', + args: { pattern: '**/*.ts' }, + description: 'emitted after detach', + timestamp: Date.now(), + }); + expect(recordsOf(files[0])).toHaveLength(before); + }); + + // All three attempts stalled, so three transcripts are on disk; the + // terminal error must name every attempt's id or the files cannot be + // paired with the failure. + it('names every attempt id in the stall-abandoned error', async () => { + nextExecuteHook.value = async (emitter, signal) => { + nextTerminateMode.value = 'CANCELLED'; + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'workflow-agent', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + await new Promise((resolve) => { + if (signal?.aborted) { + resolve(); + return; + } + signal?.addEventListener('abort', () => resolve(), { once: true }); + }); + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + let caught: unknown; + try { + await dispatch('stall forever', { label: 'staller', stallMs: 20 }); + } catch (error) { + caught = error; + } + + const files = transcriptFiles(); + expect(files).toHaveLength(3); + expect(String(caught)).toMatch(/stalled on all 3 attempts/); + for (const file of files) { + const id = path.basename(file, '.jsonl').slice('agent-'.length); + expect(String(caught)).toContain(id); + } + }); + + // A mixed retry (stall, then a deterministic failure) also leaves one + // transcript per attempt; the terminal error must name every attempt's + // id, not just the last attempt's. + it('names every attempt id when a retry ends in a non-stall failure', async () => { + let attempt = 0; + nextExecuteHook.value = async (emitter, signal) => { + attempt += 1; + if (attempt > 1) { + nextTerminateMode.value = 'MAX_TURNS'; + return; + } + nextTerminateMode.value = 'CANCELLED'; + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'workflow-agent', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + await new Promise((resolve) => { + if (signal?.aborted) { + resolve(); + return; + } + signal?.addEventListener('abort', () => resolve(), { once: true }); + }); + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + let caught: unknown; + try { + await dispatch('stall then burn out', { + label: 'staller', + stallMs: 20, + }); + } catch (error) { + caught = error; + } + + const files = transcriptFiles(); + expect(files).toHaveLength(2); + expect(String(caught)).toContain('MAX_TURNS'); + for (const file of files) { + const id = path.basename(file, '.jsonl').slice('agent-'.length); + expect(String(caught)).toContain(id); + } + }); + + // Every other transcript test runs in a non-git tmp dir, so + // getCachedGitBranch returns undefined and JSON.stringify omits the key — + // the gitBranch option line could be dropped with no red test. Real + // dispatches run inside git checkouts, so pin the annotation against a + // real repo (one commit: rev-parse fails on an unborn HEAD). + it('records the git branch of the project root', async () => { + const gitCmd = (...args: string[]): string => + execFileSync('git', args, { cwd: projectDir, encoding: 'utf8' }); + gitCmd('init', '-q', '-b', 'wf-fixture-branch'); + gitCmd('config', 'user.email', 'test@example.com'); + gitCmd('config', 'user.name', 'Test'); + gitCmd('config', 'commit.gpgsign', 'false'); + fsSync.writeFileSync(path.join(projectDir, 'a.txt'), 'seed\n'); + gitCmd('add', '.'); + gitCmd('commit', '-q', '-m', 'init'); + + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('branch run', { label: 'b' }); + + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['gitBranch']).toBe('wf-fixture-branch'); + }); + + // A padded label must not make the transcript name disagree with the + // run's own displayed name — the dispatch trims once at the top and + // every surface (launch name, transcript agentName) reads the same + // normalized value. + it('trims the label once so every surface shows the same name', async () => { + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('padded', { label: ' reviewer ' }); + + expect(created[0]!.name).toBe('reviewer'); + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(recordsOf(files[0])[0]['agentName']).toBe('reviewer'); + }); + + it('maps an empty-after-trim label to the default name everywhere', async () => { + const dispatch = createProductionDispatch(transcriptConfig()); + await dispatch('blank', { label: ' ' }); + + expect(created[0]!.name).toBe('workflow-agent'); + expect(recordsOf(transcriptFiles()[0])[0]['agentName']).toBe( + 'workflow-agent', + ); + }); + + // agentType is resolved RAW, exactly the way the Agent tool resolves + // subagent_type — a padded model-authored name is not a definition. + // Both launch paths must agree on the same input. + it('does not trim a padded agentType (Agent-tool parity)', async () => { + const config = overrideTranscriptConfig((name) => + name.toLowerCase() === 'explore' ? 'Explore' : null, + ); + const dispatch = createProductionDispatch(config); + await expect( + dispatch('find foo', { agentType: ' Explore ' }), + ).rejects.toThrow(/agent type ' Explore ' not found/); + + // The transcript records what was attempted, untrimmed. + expect(recordsOf(transcriptFiles()[0])[0]['agentName']).toBe(' Explore '); + }); + + // DOMException's `message` is getter-only — the attempt-id append must + // SKIP it rather than force an assignment that throws a TypeError and + // replaces the very error the sandbox classifies by `name === + // 'AbortError'`. The pairing rides the warn log instead. + it('preserves a DOMException terminal error and pairs by warn instead', async () => { + let attempt = 0; + const boom = new DOMException('aborted', 'AbortError'); + nextExecuteHook.value = async (emitter, signal) => { + attempt += 1; + if (attempt === 1) { + nextTerminateMode.value = 'CANCELLED'; + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'workflow-agent', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + await new Promise((resolve) => { + if (signal?.aborted) { + resolve(); + return; + } + signal?.addEventListener('abort', () => resolve(), { + once: true, + }); + }); + return; + } + throw boom; + }; + + const dispatch = createProductionDispatch(transcriptConfig()); + let caught: unknown; + try { + await dispatch('stall then abort', { label: 's', stallMs: 20 }); + } catch (error) { + caught = error; + } + + // Identity AND text intact — the rethrow is the same object, not a + // TypeError from a forced assignment and not a rewrap. + expect(caught).toBe(boom); + expect((caught as DOMException).name).toBe('AbortError'); + expect((caught as DOMException).message).toBe('aborted'); + // The multi-attempt pairing was not dropped by the skip — it was + // logged instead, naming every attempt's id — derived here from the + // transcript file names exactly like the sibling tests. + const warns = debugLogRecorder.warn.mock.calls.map((c) => String(c[0])); + const pairingWarn = warns.find((w) => w.includes('Attempt ids:')); + expect(pairingWarn).toBeDefined(); + const files = transcriptFiles(); + expect(files).toHaveLength(2); + for (const file of files) { + const id = path.basename(file, '.jsonl').slice('agent-'.length); + expect(pairingWarn).toContain(id); + } + }); + + // Errors rethrown raw from the agent loop (the production ERROR path: + // AgentHeadless.execute sets terminateMode=ERROR and rethrows) name no + // attempt id, and with one attempt nothing else logs the pairing — the + // dispatch catch must, or the transcript is orphaned. + it('pairs a single-attempt raw-rethrow failure with its transcript in the warn log', async () => { + nextExecuteThrow.value = new Error('reasoning-loop boom'); + try { + const dispatch = createProductionDispatch(transcriptConfig()); + await expect( + dispatch('doomed run', { label: 'doomed' }), + ).rejects.toThrow(/reasoning-loop boom/); + + const pairing = debugLogRecorder.warn.mock.calls + .map((c) => String(c[0])) + .find((m) => m.includes('single-attempt terminal failure')); + expect(pairing).toBeDefined(); + const id = pairing!.match( + /for (workflow-agent-[0-9a-f]{16}); transcript: /, + )?.[1]; + expect(id).toBeDefined(); + const files = transcriptFiles(); + expect(files).toHaveLength(1); + expect(path.basename(files[0])).toBe(`agent-${id}.jsonl`); + } finally { + nextExecuteThrow.value = null; + } + }); + + // A parent abort ending a mixed retry must name every attempt's id on + // the propagated error, so both transcripts stay pairable (this case + // lived in workflow-stall.test.ts while the decoration was an option + // of runStallResilient; the decoration now belongs to the dispatch). + it('names every attempt id when a parent abort ends a mixed retry', async () => { + let attempt = 0; + let sawSecondAttempt!: () => void; + const secondAttempt = new Promise((resolve) => { + sawSecondAttempt = resolve; + }); + nextExecuteHook.value = async (emitter, signal) => { + attempt += 1; + if (attempt === 2) sawSecondAttempt(); + nextTerminateMode.value = 'CANCELLED'; + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'workflow-agent', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + await new Promise((resolve) => { + if (signal?.aborted) { + resolve(); + return; + } + signal?.addEventListener('abort', () => resolve(), { once: true }); + }); + }; + + const parent = new AbortController(); + const dispatch = createProductionDispatch( + transcriptConfig(), + parent.signal, + ); + const run = dispatch('cancel mid-retry', { label: 'c', stallMs: 20 }); + await secondAttempt; + parent.abort('user-cancel'); + let caught: unknown; + try { + await run; + } catch (error) { + caught = error; + } + + expect(attempt).toBe(2); + expect(String(caught)).toContain('CANCELLED'); + const files = transcriptFiles(); + expect(files).toHaveLength(2); + for (const file of files) { + const id = path.basename(file, '.jsonl').slice('agent-'.length); + expect(String(caught)).toContain(id); + } + }); + }); + it('does not suppress env bootstrap with an empty initial history', async () => { const dispatch = createProductionDispatch(fakeConfig()); await dispatch('hello', { label: 'h1' }); @@ -2481,11 +3222,50 @@ describe('WorkflowOrchestrator P2 — parallel() / pipeline() / caps', () => { // each test wires a fake Config whose `getSubagentManager()` returns a // stub matching just enough surface (findSubagentByName + // createAgentHeadless) for the path under test. +// The minimal subagent surface the override path consumes after +// createAgentHeadless returns. Both override-path fixtures build theirs +// through this helper, so when the production path starts reading a new +// method off the subagent it lands in ONE place instead of two stubs +// drifting apart (the surface already grew once under review pressure, +// when getExecutionSummary was added). +function makeStubSubagent(behaviour: { + execute: (ctx: unknown, signal?: AbortSignal) => Promise; + finalText: () => string; + terminateMode: () => unknown; + outputTokens?: () => number; + onDispose?: () => void; +}): { + subagent: { + execute: (ctx: unknown, signal?: AbortSignal) => Promise; + getFinalText: () => string; + getTerminateMode: () => unknown; + getExecutionSummary: () => { outputTokens: number }; + }; + dispose: () => Promise; +} { + return { + subagent: { + execute: behaviour.execute, + getFinalText: behaviour.finalText, + getTerminateMode: behaviour.terminateMode, + // R1 (#1): production dispatch reads this in reportTokens regardless + // of terminate mode — every stub must expose it. + getExecutionSummary: () => ({ + outputTokens: behaviour.outputTokens?.() ?? 0, + }), + }, + dispose: async () => { + behaviour.onDispose?.(); + }, + }; +} + describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', () => { // Reset GitWorktreeService stub state between tests so an override set // by one test does not bleed into the next (mockImplementation is // persistent; the per-test overrides below rely on a clean baseline). beforeEach(async () => { + debugLogRecorder.warn.mockClear(); const { GitWorktreeService } = await import( '../../services/gitWorktreeService.js' ); @@ -2531,7 +3311,13 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( runWithEmitter?: (emitter: { emit(event: string, payload: unknown): void; }) => void; + // When set, execute() waits for the abort signal instead of returning + // — the stall-watchdog tests need an attempt that hangs. + waitForSignal?: boolean; }>; + // Transcript accessors the dispatch's attach reads; absent on most + // override-path tests, where attach is swallowed by design. + transcript?: { projectDir: string; sessionId: string }; }): { config: Config; calls: StubSubagentCall[]; @@ -2554,9 +3340,17 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( // P3 R2 self-review: isolation:'worktree' provisioning reads // these methods. Provide deterministic returns so the tests can // drive GitWorktreeService stubs without re-deriving cwd. - getTargetDir: () => '/fake/repo', - getSessionId: () => 'sess_fake_test_id', + getTargetDir: () => opts.transcript?.projectDir ?? '/fake/repo', + getSessionId: () => opts.transcript?.sessionId ?? 'sess_fake_test_id', getWorktreeSymlinkDirectories: () => [], + // Transcript attach accessors — present only for the pairing tests. + ...(opts.transcript + ? { + storage: { getProjectDir: () => opts.transcript!.projectDir }, + getProjectRoot: () => opts.transcript!.projectDir, + getCliVersion: () => '0.0.0-test', + } + : {}), getSubagentManager: () => ({ findSubagentByName: opts.findSubagentByName ?? (async () => null), createAgentHeadless: async ( @@ -2581,54 +3375,47 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( | { on(event: string, cb: (payload: unknown) => void): void } | undefined, ); - const finalText = outcome.finalText; - const terminateMode = outcome.terminateMode; - return { - subagent: { - execute: async ( - _ctx: unknown, - signal?: AbortSignal, - ): Promise => { - const { getCurrentAgentId } = await import( - './agent-context.js' + return makeStubSubagent({ + execute: async ( + _ctx: unknown, + signal?: AbortSignal, + ): Promise => { + const { getCurrentAgentId } = await import('./agent-context.js'); + call.executeAgentId = getCurrentAgentId(); + if (outcome.runWithEmitter && options?.eventEmitter) { + outcome.runWithEmitter( + options.eventEmitter as { + emit(event: string, payload: unknown): void; + }, ); - call.executeAgentId = getCurrentAgentId(); - if (outcome.runWithEmitter && options?.eventEmitter) { - outcome.runWithEmitter( - options.eventEmitter as { - emit(event: string, payload: unknown): void; - }, - ); - } - // R3 (wenshao #6): honor `nextExecuteThrow` on the - // override-path stub too, so the override-path sibling - // of the throw-path test (test name "R3 #6: override- - // path records tokens...") can reproduce the real - // AgentHeadless.execute() throw against the override - // dispatch site. - if (nextExecuteThrow.value) { - throw nextExecuteThrow.value; - } - // Honor signal abort if it fires. - if (signal?.aborted) return; - }, - getFinalText: () => finalText, - getTerminateMode: () => terminateMode, - // R1 (#1): expose `getExecutionSummary` on the override- - // path subagent stub. Production dispatch reads it in - // `reportTokens` regardless of terminate mode, so the - // schema-mode early return (Critical #1) and the - // schema-mode failure paths (Critical #3) both need - // this surface. Defaults to 0; tests that observe - // budget-recording set `nextOutputTokens.value` first. - getExecutionSummary: () => ({ - outputTokens: nextOutputTokens.value, - }), + } + // R3 (wenshao #6): honor `nextExecuteThrow` on the + // override-path stub too, so the override-path sibling + // of the throw-path test (test name "R3 #6: override- + // path records tokens...") can reproduce the real + // AgentHeadless.execute() throw against the override + // dispatch site. + if (nextExecuteThrow.value) { + throw nextExecuteThrow.value; + } + if (outcome.waitForSignal && signal) { + await new Promise((resolve) => { + if (signal.aborted) return resolve(); + signal.addEventListener('abort', () => resolve(), { + once: true, + }); + }); + } + // Honor signal abort if it fires. + if (signal?.aborted) return; }, - dispose: async () => { + finalText: () => outcome.finalText, + terminateMode: () => outcome.terminateMode, + outputTokens: () => nextOutputTokens.value, + onDispose: () => { disposed += 1; }, - }; + }); }, }), } as unknown as Config; @@ -2737,6 +3524,79 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( ); }); + // The pre-launch rejections keep upstream-verbatim text, so their pairing + // warn is the only record tying the failure to the seeded transcript — + // pin both sites under a transcript-capable config, mirroring the schema + // content pairing test. + it("pairs the isolation:'remote' pre-launch failure with its transcript in the warn log", async () => { + const projectDir = fsSync.mkdtempSync(path.join(os.tmpdir(), 'wf-pre-')); + try { + const { config } = fakeConfigWithMgr({ + transcript: { projectDir, sessionId: 'sess-prelaunch' }, + onCreate: async () => ({ finalText: '', terminateMode: 'GOAL' }), + }); + const dispatch = createProductionDispatch(config); + await expect( + dispatch('do something', { isolation: 'remote' }), + ).rejects.toThrow(/is not available in this build/); + + const pairing = debugLogRecorder.warn.mock.calls + .map((c) => String(c[0])) + .find((m) => + m.includes('pre-launch failure (remote isolation unavailable)'), + ); + expect(pairing).toBeDefined(); + const id = pairing!.match( + /for (workflow-agent-[0-9a-f]{16}); transcript: /, + )?.[1]; + expect(id).toBeDefined(); + const transcript = path.join( + projectDir, + 'subagents', + 'sess-prelaunch', + `agent-${id}.jsonl`, + ); + expect(pairing).toContain(transcript); + expect(fsSync.existsSync(transcript)).toBe(true); + } finally { + fsSync.rmSync(projectDir, { recursive: true, force: true }); + } + }); + + it('pairs the agent-type-not-found pre-launch failure with its transcript in the warn log', async () => { + const projectDir = fsSync.mkdtempSync(path.join(os.tmpdir(), 'wf-pre-')); + try { + const { config } = fakeConfigWithMgr({ + findSubagentByName: async () => null, + transcript: { projectDir, sessionId: 'sess-prelaunch' }, + onCreate: async () => ({ finalText: '', terminateMode: 'GOAL' }), + }); + const dispatch = createProductionDispatch(config); + await expect( + dispatch('do something', { agentType: 'NotARealAgent' }), + ).rejects.toThrow(/agent type 'NotARealAgent' not found/); + + const pairing = debugLogRecorder.warn.mock.calls + .map((c) => String(c[0])) + .find((m) => m.includes('pre-launch failure (agent type not found)')); + expect(pairing).toBeDefined(); + const id = pairing!.match( + /for (workflow-agent-[0-9a-f]{16}); transcript: /, + )?.[1]; + expect(id).toBeDefined(); + const transcript = path.join( + projectDir, + 'subagents', + 'sess-prelaunch', + `agent-${id}.jsonl`, + ); + expect(pairing).toContain(transcript); + expect(fsSync.existsSync(transcript)).toBe(true); + } finally { + fsSync.rmSync(projectDir, { recursive: true, force: true }); + } + }); + it('floor disallowedTools always unioned (agentType cannot re-enable them)', async () => { const { config, calls } = fakeConfigWithMgr({ findSubagentByName: async () => ({ @@ -2941,6 +3801,127 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( ); }); + // The verbatim schema content errors cannot carry the attempt id in the + // message — the pairing warn is the only record of it, so it must + // actually name the id and the transcript path (with a transcript-capable + // config; the default fakeConfigWithMgr only reaches the path-less catch + // branch). + it('pairs the verbatim schema content failure with its transcript in the warn log', async () => { + const projectDir = fsSync.mkdtempSync(path.join(os.tmpdir(), 'wf-pair-')); + try { + const { config } = fakeConfigWithMgr({ + transcript: { projectDir, sessionId: 'sess-pair' }, + onCreate: async () => ({ + finalText: 'plain-text answer the script will discard', + terminateMode: 'GOAL', + }), + }); + const dispatch = createProductionDispatch(config); + await expect( + dispatch('extract', { schema: { type: 'object' } }), + ).rejects.toThrow(/no validation attempt/); + + const pairing = debugLogRecorder.warn.mock.calls + .map((c) => String(c[0])) + .find((m) => m.includes('schema content failure')); + expect(pairing).toBeDefined(); + const id = pairing!.match( + /for (workflow-agent-[0-9a-f]{16}); transcript: /, + )?.[1]; + expect(id).toBeDefined(); + const transcript = path.join( + projectDir, + 'subagents', + 'sess-pair', + `agent-${id}.jsonl`, + ); + expect(pairing).toContain(transcript); + expect(fsSync.existsSync(transcript)).toBe(true); + } finally { + fsSync.rmSync(projectDir, { recursive: true, force: true }); + } + }); + + // A mixed retry whose second attempt ends in a verbatim schema content + // failure must NOT gain the attempt-id suffix — upstream's exact string + // is what scripts branch on. The multi-attempt pairing rides the warn + // log instead of the message. + it('keeps the verbatim schema content error verbatim across a stall retry', async () => { + let attempt = 0; + const { config } = fakeConfigWithMgr({ + onCreate: async () => { + attempt += 1; + if (attempt === 1) { + return { + finalText: '', + terminateMode: 'CANCELLED', + waitForSignal: true, + runWithEmitter: (emitter) => { + // Arm the stall watchdog, then hang until it aborts. + emitter.emit(AgentEventType.ROUND_START, { + subagentId: 'sub', + round: 1, + promptId: 'prompt-1', + timestamp: Date.now(), + }); + }, + }; + } + return { + finalText: '', + terminateMode: 'CANCELLED', + runWithEmitter: (emitter) => { + for (let i = 1; i <= 3; i++) { + emitter.emit('tool_call', { + subagentId: 'sub', + round: i, + callId: `c${i}`, + name: 'structured_output', + args: { bad: 'shape' }, + description: '', + isOutputMarkdown: false, + timestamp: i, + }); + emitter.emit('tool_result', { + subagentId: 'sub', + round: i, + callId: `c${i}`, + name: 'structured_output', + success: false, + error: 'validation failed', + responseParts: [], + resultDisplay: '', + durationMs: 1, + timestamp: i, + }); + } + }, + }; + }, + }); + const dispatch = createProductionDispatch(config); + let caught: unknown; + try { + await dispatch('extract', { + schema: { type: 'object' }, + stallMs: 20, + }); + } catch (e) { + caught = e; + } + + expect(attempt).toBe(2); + // Verbatim-equality preserved: no ' Attempt ids: ...' suffix. + expect((caught as Error).message).toBe( + 'subagent completed without calling StructuredOutput (after 2 in-conversation nudges).', + ); + // The pairing the message cannot carry was logged instead. + const warns = debugLogRecorder.warn.mock.calls.map((c) => String(c[0])); + expect( + warns.some((w) => w.includes('terminal error cannot carry attempt ids')), + ).toBe(true); + }); + it('schema-mode attaches an event emitter to the subagent', async () => { const { config, calls } = fakeConfigWithMgr({ onCreate: async (_call, _ee) => ({ diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index 66b425f1941..a6675954280 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -46,7 +46,14 @@ import { import { FileDiscoveryService } from '../../services/fileDiscoveryService.js'; import { WorkspaceContext } from '../../utils/workspaceContext.js'; import { SyntheticOutputTool } from '../../tools/syntheticOutput.js'; -import { rebuildToolRegistryOnOverride } from '../../tools/agent/agent.js'; +import { + getCachedGitBranch, + rebuildToolRegistryOnOverride, +} from '../../tools/agent/agent.js'; +import { + attachJsonlTranscriptWriter, + getAgentJsonlPath, +} from '../agent-transcript.js'; import { toModelVisibleSubagentResult } from '../subagent-result.js'; import { SUBAGENT_PLAN_LIFECYCLE_TOOLS } from './subagent-plan-tool-policy.js'; import { runWithAgentContext } from './agent-context.js'; @@ -380,7 +387,18 @@ export function createProductionDispatch( onTokens?: (outputTokens: number, opts: WorkflowAgentOpts) => void, bridgeApprovalEvents?: (emitter: AgentEventEmitter) => () => void, ): WorkflowAgentDispatch { - return async (prompt, opts) => { + return async (prompt, rawOpts) => { + // Normalize the script-chosen label ONCE and hand the trimmed value to + // every surface that shows it — the fast-path agent name, the override + // path's ephemeral config, the stall/abandoned error, and the + // transcript's `agentName`. A padded label must not make the transcript + // name disagree with the run's own displayed name; an empty-after-trim + // label becomes `undefined` so every `?? 'workflow-agent'` fallback + // agrees too. + const opts = + typeof rawOpts.label === 'string' + ? { ...rawOpts, label: rawOpts.label.trim() || undefined } + : rawOpts; // P-stall: wrap the single-attempt dispatch in the stall watchdog + // retry loop. The wrapper owns the per-attempt AbortController + // AgentEventEmitter; it chains the caller's `signal` into the @@ -392,31 +410,224 @@ export function createProductionDispatch( const stallMs = resolveStallMs( typeof opts.stallMs === 'number' ? opts.stallMs : undefined, ); - return runStallResilient( - async (attemptSignal, emitter) => { - const cleanupApprovalBridge = bridgeApprovalEvents?.(emitter); - try { - return await runSingleDispatch( - config, - prompt, - opts, - attemptSignal, - emitter, - onTokens, - ); - } finally { - cleanupApprovalBridge?.(); + const attemptIds: string[] = []; + try { + return await runStallResilient( + async (attemptSignal, emitter) => { + // Minted here rather than inside the dispatch so this attempt's + // transcript is named for the same id the dispatch reports in its + // terminal errors — and so the writer is attached to the emitter + // BEFORE any agent event can fire on it. Terminal errors name this + // id directly except the failures whose text must stay + // upstream-verbatim — the two schema content failures and the two + // pre-launch rejections — which log the pairing instead (see + // `warnTranscriptPairing`), and errors rethrown raw from the agent + // loop, which name no id and are paired by the catch below; when + // the run ends after multiple attempts, the error names every + // attempt's id. + const workflowAgentId = `workflow-agent-${randomBytes(8).toString('hex')}`; + attemptIds.push(workflowAgentId); + const cleanupApprovalBridge = bridgeApprovalEvents?.(emitter); + const { detach: detachTranscript, resolvedSubagent } = + await attachDispatchTranscript( + config, + workflowAgentId, + prompt, + opts, + emitter, + ); + try { + return await runSingleDispatch( + config, + prompt, + opts, + attemptSignal, + emitter, + workflowAgentId, + resolvedSubagent, + onTokens, + ); + } finally { + detachTranscript(); + cleanupApprovalBridge?.(); + } + }, + { stallMs, signal, label: opts.label }, + ); + } catch (err) { + // Earlier attempts already left transcripts on disk; when more than + // one attempt ran, name every attempt on whatever error terminates + // the run so no record is left unpairable. A single-attempt failure + // usually names its own id — or logs it at the throw site, for the + // verbatim errors — but errors rethrown raw from the agent loop + // (model-client failures, create / provisioning throws) do neither, + // so log the pairing here. These warns ride the debug log file, + // which lands on disk only when debug file logging is enabled + // (the CLI's `--debug`). + if (attemptIds.length > 1) { + appendAttemptDetail(config, err, attemptIds); + } else { + const id = attemptIds[0]!; + const paired = + err instanceof Error && + (verbatimTerminalErrors.has(err) || err.message.includes(id)); + if (!paired) { + warnTranscriptPairing(config, id, 'single-attempt terminal failure'); } - }, + } + throw err; + } + }; +} + +/** + * Attach the harness's own per-subagent transcript to one dispatch. + * + * Workflow subagents were the only agents in the product that left no record. + * `attachJsonlTranscriptWriter` has three other callers — the Agent tool + * (foreground and background) and the background-agent resume path, which is + * the only consumer of the writer's append/parent-uuid resume options — + * while workflow dispatch goes straight to + * `AgentHeadless.create` / `createAgentHeadless`, so nothing landed in + * `/subagents//`. Everything that reads that directory + * to answer "what did this agent actually do" was blind on the workflow path: + * post-mortem of a failed run, cost accounting, and any gate built on tool + * calls rather than on the agent's own prose (which is precisely the evidence + * an agent cannot forge — a whiffing agent still writes plausible text, but it + * makes no tool call). + * + * One attach point covers both dispatch paths because `runStallResilient` + * builds one `AgentEventEmitter` per attempt and hands the same instance to + * the fast path and the override path alike. + * + * Per ATTEMPT, not per `agent()` call: the caller mints one id per attempt, + * so a stall retry writes its own transcript rather than appending to the + * abandoned one. Three attempts leave three records, each self-describing — + * which is what makes a retry legible after the fact instead of looking like + * one agent that behaved strangely. + * + * Best-effort by construction. A transcript is audit metadata; an unwritable + * or full disk must not fail a dispatch that would otherwise have succeeded, + * so both the attach and the returned cleanup swallow their errors. The + * writer opens its fd lazily, and the seeded launch-prompt record is written + * at attach time — so a dispatch with a NON-EMPTY prompt materializes its + * file immediately, including dispatches that fail before the agent + * launches. Two cases leave an attempt id with no file: an empty prompt + * seeds nothing (the writer skips empty text), and an unwritable transcript + * directory degrades to a failed open the attach never learns about. + * Terminal errors may therefore name attempt ids whose record did not + * materialize; an id still keys whatever file DOES exist. + */ +async function attachDispatchTranscript( + config: Config, + workflowAgentId: string, + prompt: string, + opts: WorkflowAgentOpts, + emitter: AgentEventEmitter, +): Promise<{ + detach: () => void; + /** + * The definition resolved for `agentName`, when the attach resolved one — + * threaded into the override path so one attempt pays one definition- + * directory scan instead of two. `undefined` means the override path must + * resolve itself: a label-only attach skips resolution, and a failed + * best-effort resolution must still surface the authoritative not-found + * throw there. + */ + resolvedSubagent: SubagentConfig | undefined; +}> { + let cleanup: (() => void) | undefined; + let resolvedSubagent: SubagentConfig | undefined; + try { + const sessionId = config.getSessionId(); + const projectRoot = config.getProjectRoot(); + // `label` first: it is the identity the script chose and the one the run + // shows in progress output, so a reader matching a transcript to a line + // of the script has the same name in both places. `agentType` is the + // fallback that still says something; the constant is the last resort. + // Both arrive normalized (label trimmed by the dispatch closure) or raw — + // `agentType` is deliberately NOT trimmed so it resolves exactly the way + // the Agent tool resolves it. + const label = typeof opts.label === 'string' ? opts.label : ''; + const agentType = typeof opts.agentType === 'string' ? opts.agentType : ''; + let agentName: string; + if (label) { + agentName = label; + } else if (agentType) { + resolvedSubagent = + (await resolveSubagentForTranscript(config, agentType)) ?? undefined; + agentName = resolvedSubagent?.name || agentType; + } else { + agentName = 'workflow-agent'; + } + ({ cleanup } = attachJsonlTranscriptWriter( + emitter, + getAgentJsonlPath( + config.storage.getProjectDir(), + sessionId, + workflowAgentId, + ), { - stallMs, - signal, - label: typeof opts.label === 'string' ? opts.label : undefined, + agentId: workflowAgentId, + agentName, + // Provenance for readers of the shared subagents directory — the + // /review coverage gate filters on it so a workflow dispatch is + // never judged as an agent the review launched. + agentKind: 'workflow', + sessionId, + cwd: projectRoot, + version: config.getCliVersion() || 'unknown', + gitBranch: getCachedGitBranch(projectRoot), + // Seeds the first `user` record, written before the model has said + // anything — so the transcript states what the agent was asked to do + // without a reader needing the script that asked it. + initialUserPrompt: prompt, }, + )); + } catch (error) { + debugLogger.warn( + `[workflow] transcript attach failed for ${workflowAgentId}: ${error}`, ); + return { detach: () => {}, resolvedSubagent }; + } + const detach = cleanup; + return { + detach: () => { + try { + detach(); + } catch (error) { + debugLogger.warn( + `[workflow] transcript cleanup failed for ${workflowAgentId}: ${error}`, + ); + } + }, + resolvedSubagent, }; } +/** + * Subagent resolution is case-insensitive, so a model-authored `agentType` + * can differ in case from the canonical `SubagentConfig.name` the Agent tool + * records for the same definition. Resolve so transcripts from both launch + * paths join on `agentName`. Best-effort: errors are swallowed and the + * caller falls back to the raw string — the name is audit metadata, and the + * override path's authoritative lookup still runs when this yields nothing. + */ +async function resolveSubagentForTranscript( + config: Config, + agentType: string, +): Promise { + try { + return await config.getSubagentManager().findSubagentByName(agentType); + } catch (error) { + debugLogger.warn( + `[workflow] transcript agentName resolution failed for ` + + `${sanitizeForErrorMessage(agentType)}: ${error}`, + ); + return null; + } +} + /** * One single-attempt production dispatch. Receives the per-attempt abort * signal (the stall wrapper chains the parent signal into it + the watchdog @@ -431,12 +642,23 @@ async function runSingleDispatch( opts: WorkflowAgentOpts, attemptSignal: AbortSignal, emitter: AgentEventEmitter, + /** + * This attempt's agent id, minted by the caller so the transcript writer + * could be attached to `emitter` before the agent exists. Also the id this + * function names in its non-GOAL terminal error. + */ + workflowAgentId: string, + /** + * The subagent definition the transcript attach already resolved for this + * attempt, when it resolved one — forwarded to the override path so the + * attempt's definition lookup runs once, not twice. + */ + resolvedSubagent: SubagentConfig | undefined, onTokens?: (outputTokens: number, opts: WorkflowAgentOpts) => void, ): Promise { const { AgentHeadless, ContextState } = await import('./agent-headless.js'); const ctx = new ContextState(); ctx.set('task_prompt', prompt); - const workflowAgentId = `workflow-agent-${randomBytes(8).toString('hex')}`; debugLogger.debug(`[workflow] Dispatch ${workflowAgentId}`); if ( @@ -507,6 +729,7 @@ async function runSingleDispatch( opts, attemptSignal, workflowAgentId, + resolvedSubagent, onTokens, emitter, ); @@ -580,6 +803,13 @@ async function runOverridePath( opts: WorkflowAgentOpts, signal: AbortSignal | undefined, workflowAgentId: string, + /** + * The subagent definition the transcript attach already resolved for this + * attempt, when it resolved one (see `attachDispatchTranscript`). Reused + * here so an unlabeled-agentType dispatch scans the definition directories + * once per attempt instead of twice; `undefined` resolves now. + */ + resolvedSubagent: SubagentConfig | undefined, /** * P5: forwarded from createProductionDispatch. The override path * builds its own AgentHeadless and runs subagent.execute(); the @@ -598,32 +828,53 @@ async function runOverridePath( if (opts.isolation === 'remote') { // Error message verbatim from upstream Claude Code 2.1.168 strings. // Match for parity so scripts written against either runtime see the - // same text and can branch on it. - throw new Error( + // same text and can branch on it. The transcript seed is already on + // disk at this point, so log the pairing — the message stays verbatim. + warnTranscriptPairing( + config, + workflowAgentId, + 'pre-launch failure (remote isolation unavailable)', + ); + const err = new Error( "agent({isolation:'remote'}) is not available in this build.", ); + verbatimTerminalErrors.add(err); + throw err; } const subagentMgr = config.getSubagentManager(); let baseConfig: SubagentConfig; if (opts.agentType !== undefined) { - const resolved = await subagentMgr.findSubagentByName(opts.agentType); + // Resolved RAW — no trim — exactly the way the Agent tool resolves + // `subagent_type`, so both launch paths agree on the same input. The + // transcript attach resolved the same raw string for `agentName`. + const resolved = + resolvedSubagent ?? + (await subagentMgr.findSubagentByName(opts.agentType)); if (!resolved) { // Error message verbatim from upstream Claude Code 2.1.168 strings: // "agent({agentType}): agent type '{name}' not found". Match for // user-visible parity so scripts authored against either runtime see - // the same error text. + // the same error text. The transcript seed is already on disk, so + // log the pairing — the message stays verbatim. // - // SECURITY (P3 R2 self-review): sanitize opts.agentType before + // SECURITY (P3 R2 self-review): sanitize the agentType before // interpolation. The string is model-authored; an attacker model // could embed CRLF / control characters that fragment the error // message in logs / display / OTLP traces. Replace control chars // with a single space so the error stays single-line. + warnTranscriptPairing( + config, + workflowAgentId, + 'pre-launch failure (agent type not found)', + ); const safeAgentType = sanitizeForErrorMessage(opts.agentType); - throw new Error( + const err = new Error( `agent({agentType}): agent type '${safeAgentType}' not found.`, ); + verbatimTerminalErrors.add(err); + throw err; } baseConfig = resolved; } else { @@ -862,14 +1113,28 @@ async function runOverridePath( // factually correct only for (b). if (schemaState.attempts > 2) { // Error message verbatim from upstream Claude Code 2.1.168 strings. - throw new Error( + warnTranscriptPairing( + config, + workflowAgentId, + 'schema content failure', + ); + const err = new Error( 'subagent completed without calling StructuredOutput (after 2 in-conversation nudges).', ); + verbatimTerminalErrors.add(err); + throw err; } - throw new Error( + warnTranscriptPairing( + config, + workflowAgentId, + 'schema content failure', + ); + const err = new Error( 'subagent completed without calling structured_output ' + '(no validation attempt — model produced plain-text content).', ); + verbatimTerminalErrors.add(err); + throw err; } // Non-schema mode. @@ -937,6 +1202,82 @@ async function runOverridePath( } } +/** + * Terminal errors whose message must remain upstream-verbatim so scripts + * authored against either runtime can branch on the exact text — the two + * schema content failures and the two pre-launch rejections. Marked at the + * throw site so any id pairing is logged instead of appended (see + * `appendAttemptDetail`), and read by the dispatch catch to avoid logging + * a pairing the throw site already logged. + */ +const verbatimTerminalErrors = new WeakSet(); + +/** + * Log the id↔transcript pairing for a terminal error whose message cannot + * carry the attempt's id: the four upstream-verbatim failures (two schema + * content, two pre-launch) keep their text for user-visible parity, the + * multi-attempt fallback lands here when the detail cannot be appended in + * place, and the dispatch catch lands here for a single-attempt failure + * that names no id. Rides `debugLogger.warn`, which lands on disk only + * when debug file logging is enabled (the CLI's `--debug`). The transcript + * file is named for the id, so this is what lets an operator match such a + * failure to its record. + */ +function warnTranscriptPairing( + config: Config, + workflowAgentId: string, + reason: string, +): void { + let transcript = '(path unavailable)'; + try { + transcript = getAgentJsonlPath( + config.storage.getProjectDir(), + config.getSessionId(), + workflowAgentId, + ); + } catch { + // Best-effort pairing — a storage accessor failure must not replace the + // terminal error this warn accompanies. + } + debugLogger.warn( + `[workflow] ${reason} for ${workflowAgentId}; transcript: ${transcript}`, + ); +} + +/** + * Name every attempt's id on the run's terminating error, in place. + * Mutation (not a rewrap) preserves error identity — the sandbox classifies + * cancellations by `name === 'AbortError'`. + * + * The append is skipped for errors whose text must stay upstream-verbatim + * (marked at the throw site), errors without a writable own `message` + * (DOMException's is getter-only — forcing the assignment would throw a + * TypeError and replace the very error the sandbox classifies), and + * non-Error rejections. The pairing then rides the warn log instead — + * which lands on disk only when debug file logging is enabled (the CLI's + * `--debug`) — so a default session records no pairing for the skip. + */ +function appendAttemptDetail( + config: Config, + err: unknown, + attemptIds: string[], +): void { + const detail = `Attempt ids: ${attemptIds.join(', ')}.`; + if ( + err instanceof Error && + !verbatimTerminalErrors.has(err) && + Object.getOwnPropertyDescriptor(err, 'message')?.writable + ) { + err.message = `${err.message} ${detail}`; + return; + } + warnTranscriptPairing( + config, + attemptIds[attemptIds.length - 1]!, + `terminal error cannot carry attempt ids — ${detail}`, + ); +} + /** * Result of a worktree-isolation cleanup: nothing when the worktree was * cleanly removed (no changes and no unmerged commits), or the path / diff --git a/packages/core/src/agents/runtime/workflow-stall.test.ts b/packages/core/src/agents/runtime/workflow-stall.test.ts index 65355b2702c..3c13d02c406 100644 --- a/packages/core/src/agents/runtime/workflow-stall.test.ts +++ b/packages/core/src/agents/runtime/workflow-stall.test.ts @@ -221,6 +221,11 @@ describe('runStallResilient', () => { expect(String(caught)).toMatch(/MAX_TURNS/); }); + // The attempt-id pairing the run leaves beside its transcripts is applied + // by the PRODUCTION DISPATCH (createProductionDispatch), not by this + // generic retry wrapper — see the transcript suite in + // workflow-orchestrator.test.ts for those cases. + it('does NOT retry on parent abort (propagates)', async () => { const parent = new AbortController(); let calls = 0; diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index 24bb49550f8..d87f4f01107 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -363,6 +363,14 @@ export interface ChatRecord { agentId?: string; /** Display name for the subagent (e.g. "Explore"). */ agentName?: string; + /** + * Launch path that produced this subagent transcript, when it was not + * the Agent tool. `'workflow'` marks workflow `agent()` dispatches, which + * share `/subagents//` with Agent-tool launches; + * readers of that directory (e.g. the /review coverage gate) filter on it + * to tell the two populations apart. + */ + agentKind?: 'workflow'; /** UI hint for tools rendering subagent transcripts. */ agentColor?: string; /** True for records produced by a subagent (a sidechain off the parent session). */ diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 95470a27948..ddee2ce36ac 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -139,8 +139,14 @@ import type { AuthOverrides } from '../../models/content-generator-config.js'; // background) starts. Branches don't change within a process under normal // use; the transcript annotation is best-effort audit metadata, so a stale // value after a user `git checkout` mid-session is acceptable. +// +// Exported so the workflow dispatch path (workflow-orchestrator.ts) annotates +// its transcripts from the SAME cache instead of opening a second one — the +// whole point of the memo is one `git rev-parse` per cwd per process, and a +// private copy per launch path would quietly restore the per-launch execSync +// for workflow subagents. const gitBranchCache = new Map(); -function getCachedGitBranch(cwd: string): string | undefined { +export function getCachedGitBranch(cwd: string): string | undefined { if (gitBranchCache.has(cwd)) return gitBranchCache.get(cwd); const branch = getGitBranch(cwd); gitBranchCache.set(cwd, branch);