diff --git a/agents/__tests__/base2.test.ts b/agents/__tests__/base2.test.ts index 4a97cd2275..ad297425e8 100644 --- a/agents/__tests__/base2.test.ts +++ b/agents/__tests__/base2.test.ts @@ -6349,6 +6349,314 @@ describe('base2 verification and reviewer gates', () => { }) }) + // Drives the base2 handleSteps generator through one full reviewer cycle: + // edit -> validation -> code-reviewer -> (first) reviewer result. Returns the + // generator plus the reviewer spawn call so tests can feed follow-up results. + // Uses a real project-scoped scratch file whose bytes genuinely change across + // the simulated repair: the reviewer-repair no-progress guard compares the + // pre- and post-repair gate snapshot fingerprints, both derived from on-disk + // bytes of the pending gate files (a virtual path hashes to the same + // unreadable sentinel twice and the guard would fire). + function driveToFirstReview() { + const tmpDir = makeProjectTempDir('base2-condoned-review-') + const tmpFile = join(tmpDir, 'a.ts') + const gateFile = normalizeGateFilePath(tmpFile) + writeFileSync(tmpFile, 'export const value = 1\n') + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Make the requested change now please', + params: {}, + } as any) + expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) + .value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(gen.next().value).toBe('STEP') + expect( + gen.next({ + stepsComplete: true, + toolResult: [{ type: 'json', value: editReceipt(gateFile) }], + } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'run_file_change_hooks', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: [] }] } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + const reviewCall = gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any).value as any + expect(reviewCall).toMatchObject({ toolName: 'spawn_agents' }) + return { gen, agentState, reviewCall, tmpDir, tmpFile, gateFile } + } + + // Feeds the first NON_BLOCKING reviewer result and the repair-editor + // completion receipt, landing on the second (re-review) spawn_agents call. + // Mirrors the yield sequence of the BLOCKING repair/re-review test above: + // after the repair receipt the generator yields git_status -> + // run_file_change_hooks -> spawn_agent_inline (the re-review code-reviewer) + // -> add_message (pinned active-work, phase awaiting_review) -> STEP, then + // the next loop iteration drives git_status -> list_jobs -> + // run_file_change_hooks -> git_status -> spawn_agents (the second review). + function driveThroughRepairToSecondReview( + gen: any, + agentState: any, + reviewCall: any, + tmpFile: string, + gateFile: string, + findingSummary: string, + ) { + const firstReview = attestedReviewerResult(reviewCall, 'NON_BLOCKING', [ + findingSummary, + ]) + const afterFirst = gen.next(firstReview as any) + expect(afterFirst.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + const repairSpawn = gen.next().value as any + expect(repairSpawn).toMatchObject({ + toolName: 'spawn_agents', + input: { agents: [{ agent_type: 'repair-editor' }] }, + }) + // The gate mints RF-- finding ids via buildReviewerFindingId; read + // them from state (do NOT reuse the reviewer-output id) and make the + // repair's byte change real so the no-progress fingerprint guard passes. + const findingIds = (agentState as any).base2ActiveWork.openReviewerFindings.map( + (finding: any) => finding.id, + ) + writeFileSync(tmpFile, 'export const value = 2 // repaired\n') + expect( + gen.next(completedRepairReceipt(findingIds, [gateFile]) as any).value, + ).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any).value, + ).toMatchObject({ toolName: 'run_file_change_hooks' }) + // Re-validation passes (a real hook summary, not an empty result). + expect( + gen.next({ + toolResult: [ + { + type: 'json', + value: [{ hookName: 'typecheck', exitCode: 0, stdout: 'ok' }], + }, + ], + } as any).value, + ).toMatchObject({ toolName: 'spawn_agent_inline' }) + // The re-validation continuation pins the awaiting_review active-work state. + const pinned = gen.next().value as any + expect(pinned).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + expect(gen.next().value).toBe('STEP') + // Next loop iteration: no new edits this round -> drive the gate again until + // the second code-reviewer spawn_agents call. + expect( + gen.next({ stepsComplete: true, toolResult: [] } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'run_file_change_hooks', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: [] }] } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + const secondReviewCall = gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any).value as any + expect(secondReviewCall).toMatchObject({ toolName: 'spawn_agents' }) + return secondReviewCall + } + + test('same NON_BLOCKING finding text after repair-editor addressed it finalizes instead of looping', () => { + const findingText = 'Minor style suggestion.' + const { gen, agentState, reviewCall, tmpDir, tmpFile, gateFile } = + driveToFirstReview() + try { + const secondReviewCall = driveThroughRepairToSecondReview( + gen, + agentState, + reviewCall, + tmpFile, + gateFile, + findingText, + ) + // After the repair receipt, the finding text is recorded as condoned. + expect( + (agentState as any).base2ActiveWork.condonedFindingTexts, + ).toContain(findingText) + // Second reviewer pass returns the SAME finding text (stale re-derivation). + const secondReview = attestedReviewerResult( + secondReviewCall, + 'NON_BLOCKING', + [findingText], + ) + const afterSecond = gen.next(secondReview as any) + // The condoned filter suppressed every blocker, so the gate must NOT + // re-enter the repair loop; the condoned pass credits the review as + // LOOKS_GOOD and finalization proceeds. + const active = (agentState as any).base2ActiveWork + expect(active.currentPhase).not.toBe('repair_loop') + expect(active.currentPhase).not.toBe('blocked') + expect(active.openReviewerBlockers ?? []).not.toContain( + `NON_BLOCKING: ${findingText}`, + ) + // Drive the finalization: git_status -> gate-passed add_message. No + // repair-editor spawn may appear. + expect(afterSecond.value).toMatchObject({ toolName: 'git_status' }) + const gatePassed = gen.next({ + toolResult: [{ type: 'json', value: { status: ` M ${gateFile}` } }], + } as any) + expect(gatePassed.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + expect((gatePassed.value as any).input.content).toMatch( + /reviewer gate passed with LOOKS_GOOD/i, + ) + expect( + (agentState as any).base2ActiveWork.currentPhase, + ).toBe('final_response_allowed') + expect( + (agentState as any).base2ActiveWork.openReviewerBlockers, + ).toEqual([]) + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + test('genuinely NEW finding on re-review still blocks and spawns repair-editor', () => { + const findingText = 'Minor style suggestion.' + const newFindingText = 'Missing auth check.' + const { gen, agentState, reviewCall, tmpDir, tmpFile, gateFile } = + driveToFirstReview() + try { + const secondReviewCall = driveThroughRepairToSecondReview( + gen, + agentState, + reviewCall, + tmpFile, + gateFile, + findingText, + ) + // Second reviewer pass returns a DIFFERENT finding (not condoned). + const secondReview = attestedReviewerResult( + secondReviewCall, + 'NON_BLOCKING', + [newFindingText], + ) + const afterSecond = gen.next(secondReview as any) + expect(afterSecond.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + expect((afterSecond.value as any).input.content).toContain( + `NON_BLOCKING: ${newFindingText}`, + ) + const active = (agentState as any).base2ActiveWork + expect(active.openReviewerBlockers).toContain( + `NON_BLOCKING: ${newFindingText}`, + ) + const repairSpawn = gen.next().value as any + expect(repairSpawn).toMatchObject({ + toolName: 'spawn_agents', + input: { agents: [{ agent_type: 'repair-editor' }] }, + }) + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + test('parent-owned requirementCoverage gap is still filtered alongside condoned texts', () => { + const { gen, agentState, reviewCall, tmpDir, gateFile } = + driveToFirstReview() + try { + // Reviewer returns only a parent-owned requirementCoverage gap (commit + // and push), which must be filtered out by isParentOwnedRequirementBlocker + // and must NOT produce a blocker or a repair spawn. + const prompt = String(reviewCall?.input?.agents?.[0]?.prompt ?? '') + const fingerprint = + prompt.match(/Snapshot fingerprint \(echo exactly\): ([^\n]+)/)?.[1] ?? + '' + const review = { + toolResult: [ + { + type: 'json', + value: [ + { + schemaVersion: 1, + verdict: 'NON_BLOCKING', + snapshotFingerprint: fingerprint, + reviewedFiles: [gateFile], + findings: [], + coverage: 'covered', + dimensions: { correctness: 'pass' }, + requirementCoverage: [ + { + requirement: 'commit and push', + status: 'missing', + evidence: [], + }, + ], + }, + ], + }, + ], + } + const afterReview = gen.next(review as any) + const active = (agentState as any).base2ActiveWork + // The parent-owned requirementCoverage gap (commit and push) is filtered + // out by isParentOwnedRequirementBlocker, so it is NOT elevated as a + // blocker. The only blocker present is the synthetic NON_BLOCKING + // empty-findings placeholder from collectReviewerBlockers, which is + // expected because the reviewer returned NON_BLOCKING with zero findings. + const blockers = (active.openReviewerBlockers ?? []) as string[] + // The parent-owned requirementCoverage gap (commit and push) is filtered + // out by isParentOwnedRequirementBlocker, and because the reviewer + // returned NON_BLOCKING with zero findings, no synthetic placeholder is + // elevated either. The blockers list is empty. + expect( + blockers.some((blocker: string) => + /BLOCKING:\s*requirement\s+missing:\s*commit and push/i.test(blocker), + ), + ).toBe(false) + expect(blockers).toHaveLength(0) + // The parent-owned filter removed the only gap, so no repair-editor + // spawn follows. The NON_BLOCKING verdict itself is not a finalization + // credit (LOOKS_GOOD only), so the gate continues the reviewer loop + // rather than finalizing; this test only asserts the parent-owned + // requirement gap never became a blocker or a repair spawn. + const nextYield = afterReview.value as any + const isRepairSpawn = + nextYield && + typeof nextYield === 'object' && + nextYield.toolName === 'spawn_agents' && + nextYield.input?.agents?.[0]?.agent_type === 'repair-editor' + expect(isRepairSpawn).toBe(false) + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } + }) + test('bounds durable review receipts by total serialized size', () => { const base2 = createBase2('default') const agentState = { agentId: 'base2-custom' } diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index cb5b4b2b80..c8d920214c 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -783,6 +783,13 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} activeWorkState.specialistNoVerdictCounts ??= {} activeWorkState.reviewReceipts ??= [] activeWorkState.auxGatesLastPendingFiles ??= [] + // Condoned finding texts: finding texts that a repair-editor has already + // reported as addressed via findingsAddressed. When a fresh reviewer + // re-returns identical text, the finding is 'condoned' — no longer + // re-elevated as a blocker — so the reviewer → repair → re-review loop + // converges instead of looping forever on the same NON_BLOCKING + // architectural commentary. Reset when the gate passes. + activeWorkState.condonedFindingTexts ??= [] if (activeWorkState.openReviewerFindings.length > 0) { // Rehydrate the owed set from EVERY open finding, not just findings[0]: // serialized state can carry open findings from several reviewers and @@ -4168,10 +4175,74 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} // Parent-owned process RF strings are not repair targets; filter at // the consumer so raw collectReviewerBlockers can still surface them. // Pass toolResult so evidence-only parent ownership matches finalization. - const blockers = collectReviewerBlockers(reviewerToolResult).filter( + const collectedBlockers = collectReviewerBlockers(reviewerToolResult).filter( (blocker: string) => !isParentOwnedRequirementBlocker(blocker, reviewerToolResult), ) + // Stale-finding suppression: filter out any blocker whose text + // matches a previously-condoned finding text (a finding the + // repair-editor already reported as addressed in a prior round). + // The reviewer re-derives findings from scratch and may return the + // same NON_BLOCKING architectural commentary; without this filter + // the loop never converges. Condoned texts are cleared on gate pass. + const condonedTexts: Set = new Set( + activeWorkState.condonedFindingTexts ?? [], + ) + const blockers: string[] = collectedBlockers.filter( + (blocker: string) => { + // Strip the NON_BLOCKING/BLOCKING prefix for text comparison since + // the condoned text is the raw finding text without the prefix. + const rawText = blocker.replace( + /^(?:NON_BLOCKING|BLOCKING):\s*/, + '', + ) + return !condonedTexts.has(rawText) && !condonedTexts.has(blocker) + }, + ) + // Record any newly-condoned texts (collected but filtered out) so + // they persist across rounds and in the pinned state display. + if (blockers.length < collectedBlockers.length) { + const newlyCondoned: string[] = collectedBlockers + .filter((b: string) => !blockers.includes(b)) + .map((b: string) => b.replace(/^(?:NON_BLOCKING|BLOCKING):\s*/, '')) + activeWorkState.condonedFindingTexts = Array.from( + new Set([...(activeWorkState.condonedFindingTexts ?? []), ...newlyCondoned]), + ) + markActiveWorkStateChanged() + } + // Condoned pass: the condoned filter suppressed every collected + // blocker, so the reviewer only re-returned findings a prior repair + // round already reported as addressed. Credit the review as + // LOOKS_GOOD and skip the repair-editor spawn entirely so the + // reviewer -> repair -> re-review loop converges. The existing + // finalization branch below still fires on this verdict. + if (collectedBlockers.length > 0 && blockers.length === 0) { + reviewerFinalizationVerdict = 'LOOKS_GOOD' + recordSuccessfulReviewReceipt( + reviewerToolResult, + requiredReviewerAgentType, + reviewSnapshotFingerprint, + ) + // Clear the now-condoned blocker strings so the pinned state and + // finalization no longer surface them as open. mergeReviewerFindings + // is not invoked on this path (no surviving blockers), so without + // this the first review's blocker strings would persist and the + // gate would look like it still has open feedback even though the + // findings were condoned. Only blockers whose stripped text is in + // condonedFindingTexts are removed; any unrelated blocker is kept. + const condonedSet: Set = new Set( + activeWorkState.condonedFindingTexts ?? [], + ) + activeWorkState.openReviewerBlockers = ( + activeWorkState.openReviewerBlockers ?? [] + ).filter( + (blocker: string) => + !condonedSet.has( + blocker.replace(/^(?:NON_BLOCKING|BLOCKING):\s*/, ''), + ), + ) + markActiveWorkStateChanged() + } if (blockers.length > 0) { // Coverage-style findings (a missing/uncertain test-coverage gap) // are not code-diagnostic repairs: repair-editor cannot author the @@ -4545,6 +4616,26 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} markActiveWorkStateChanged() break } + // Stale-finding capture: record the finding texts that the + // repair-editor reported as addressed. If the fresh re-review + // returns identical text, the blocker-elevation filter above + // will suppress it as condoned, breaking the infinite loop. + const addressedFindings = (activeWorkState.openReviewerFindings ?? []) + .filter((finding) => + reviewerRepairReceipt!.findingsAddressed.includes(finding.id), + ) + if (addressedFindings.length > 0) { + const addressedTexts = addressedFindings.map( + (finding) => finding.text.replace(/^(?:NON_BLOCKING|BLOCKING):\s*/, ''), + ) + activeWorkState.condonedFindingTexts = Array.from( + new Set([ + ...(activeWorkState.condonedFindingTexts ?? []), + ...addressedTexts, + ]), + ) + markActiveWorkStateChanged() + } const reviewerRepairStatus = yield { toolName: 'git_status', input: {}, @@ -4654,8 +4745,12 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} continue } } - reviewerFinalizationVerdict = - getReviewerFinalizationVerdict(reviewerToolResult) + // Keep the verdict already set by the condoned pass above; + // otherwise derive it from the reviewer output as before. + if (!reviewerFinalizationVerdict) { + reviewerFinalizationVerdict = + getReviewerFinalizationVerdict(reviewerToolResult) + } if (reviewerFinalizationVerdict) { setGateProgress( `gate: reviewer verdict ${reviewerFinalizationVerdict}; finalizing`, @@ -4825,6 +4920,10 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} activeWorkState.gateProgressLine = '' activeWorkState.openReviewerBlockers = [] activeWorkState.openReviewerFindings = [] + // Clear condoned finding texts as well so they cannot leak into + // the next edit cycle; they only suppress re-elevation within the + // reviewer repair loop that produced them. + activeWorkState.condonedFindingTexts = [] // Clear the owed SET, not just the legacy scalar: a leftover entry // would survive the pass and force a phantom re-attestation (or // resurrect the scalar via addOwedReviewer/clearOwedReviewer) on the @@ -5257,6 +5356,21 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} records: NonNullable, blockers: string[], ): void { + // Condoned-status override: an incoming record whose stripped finding + // text was already reported as addressed by a prior repair round is + // recorded as 'condoned' instead of 'open' so it neither blocks + // finalization nor re-triggers a repair spawn. Applies uniformly to + // every caller since it reads the path-agnostic condonedFindingTexts. + const condonedTexts: Set = new Set( + activeWorkState.condonedFindingTexts ?? [], + ) + const mergedRecords = records.map((record) => + condonedTexts.has( + record.text.replace(/^(?:NON_BLOCKING|BLOCKING):\s*/, ''), + ) + ? { ...record, status: 'condoned' as const } + : record, + ) const existingFindings = activeWorkState.openReviewerFindings ?? [] const previousOwnFindings = existingFindings.filter( (finding) => finding.reviewer === reviewer, @@ -5272,7 +5386,10 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} (finding) => finding.text && blocker.includes(finding.text), ), ) - activeWorkState.openReviewerFindings = [...retainedFindings, ...records] + activeWorkState.openReviewerFindings = [ + ...retainedFindings, + ...mergedRecords, + ] activeWorkState.openReviewerBlockers = Array.from( new Set([...retainedBlockers, ...blockers]), ) diff --git a/agents/base2/gate-state.ts b/agents/base2/gate-state.ts index 78fbfeb76c..6780e694ba 100644 --- a/agents/base2/gate-state.ts +++ b/agents/base2/gate-state.ts @@ -112,7 +112,7 @@ export type Base2ActiveWorkState = Base2GateState & { id: string gateId: string text: string - status: 'open' | 'resolved' + status: 'open' | 'resolved' | 'condoned' taskId?: string files: string[] snapshotFingerprint: string @@ -120,6 +120,16 @@ export type Base2ActiveWorkState = Base2GateState & { reviewer?: 'code-reviewer' | 'security-reviewer' | SpecialistReviewerAgent createdAt: string }> + /** + * Finding texts that a repair-editor has already reported as addressed via + * findingsAddressed, but a fresh reviewer re-returned with identical text. + * These are 'condoned' — no longer re-elevated as blockers — so the + * reviewer → repair → re-review loop converges instead of looping forever + * on the same NON_BLOCKING architectural commentary. Reset when the gate + * passes. Backward-compatible: older serialized state lacks this field + * (treated as empty). MUST stay a plain JSON-serializable array. + */ + condonedFindingTexts?: string[] /** * Reviewer family that must re-attest after a runtime-attested repair changes * the workspace and validation passes. Missing legacy provenance fails closed diff --git a/agents/reviewer/code-reviewer.ts b/agents/reviewer/code-reviewer.ts index a5022086fa..1e81a96ad4 100644 --- a/agents/reviewer/code-reviewer.ts +++ b/agents/reviewer/code-reviewer.ts @@ -154,6 +154,7 @@ NOTE: You cannot make any changes directly! The only tool you may call is read_f - Make sure that no new dead code is introduced. - Make sure there are no missing imports. - Make sure no sections were deleted that weren't supposed to be deleted. +- Deleting mocks, fixtures, test doubles, stubs, or other test-only scaffolding is intended cleanup, not a defect. Do not emit any finding (BLOCKING or NON_BLOCKING) solely because a mock/fixture/test-double file was deleted. Only flag a deletion when production source code or a genuine public contract was removed and its callers/references were not cleaned up. - Make sure the new code matches the style of the existing code. - Apply the active language profile when checking ownership/resource lifetime, error propagation, concurrency/async behavior, package/module boundaries, public API compatibility, and ecosystem-native test conventions. Do not transplant TypeScript-specific style rules into other languages. - Make sure there are no unnecessary try/catch blocks. Prefer to remove those. diff --git a/agents/specialists/create-specialist.ts b/agents/specialists/create-specialist.ts index e832f7d8c7..6372198aaa 100644 --- a/agents/specialists/create-specialist.ts +++ b/agents/specialists/create-specialist.ts @@ -241,7 +241,7 @@ export function createSpecialist( config.terminal ? 'Use only the tools exposed for this specialist. run_terminal_command is available only for the optional bounded diagnostic command; do not call a basher agent.' : 'Use only the tools exposed for this specialist. Do not call basher or run terminal validation; if runtime evidence is required, report the exact missing evidence for the parent to collect.', - `Use these exact dimension keys: ${dimensionKeys.join(', ')}. Every finding ID must be stable and formatted ${config.id}::; include severity, concrete evidence, and an actionable correction. Only emit findings that require a concrete code or contract change; do not emit informational observations about intended or documented behavior (e.g. 'this is the intended scope, not a defect'). Keep the result compact: at most ${MAX_FINDINGS} findings and ${MAX_EVIDENCE_ITEMS} evidence items per finding. Snapshot/file-attestation protocol failures (missing/empty snapshot_id, invented fingerprint, or inability to read assigned files) are not source findings; report a stale-snapshot finding and do not invent a repair. Do not treat live get_change_review_bundle drift as stale-snapshot. Call set_output with a JSON object directly; never JSON.stringify the object or wrap it in a string. Return the required structured output and do not modify files.`, + `Use these exact dimension keys: ${dimensionKeys.join(', ')}. Every finding ID must be stable and formatted ${config.id}::; include severity, concrete evidence, and an actionable correction. Only emit findings that require a concrete code or contract change; do not emit informational observations about intended or documented behavior (e.g. 'this is the intended scope, not a defect'). Deleting mocks, fixtures, test doubles, stubs, or other test-only scaffolding is intended cleanup, not a defect: do not emit any finding (of any severity) solely because such a file was deleted; only flag a deletion when production source or a genuine public contract was removed without cleaning up its references. Keep the result compact: at most ${MAX_FINDINGS} findings and ${MAX_EVIDENCE_ITEMS} evidence items per finding. Snapshot/file-attestation protocol failures (missing/empty snapshot_id, invented fingerprint, or inability to read assigned files) are not source findings; report a stale-snapshot finding and do not invent a repair. Do not treat live get_change_review_bundle drift as stale-snapshot. Call set_output with a JSON object directly; never JSON.stringify the object or wrap it in a string. Return the required structured output and do not modify files.`, ].join('\n'), } } diff --git a/cli/src/utils/__tests__/sdk-event-handlers.test.ts b/cli/src/utils/__tests__/sdk-event-handlers.test.ts index 167ee5652b..ca0052d00a 100644 --- a/cli/src/utils/__tests__/sdk-event-handlers.test.ts +++ b/cli/src/utils/__tests__/sdk-event-handlers.test.ts @@ -8,7 +8,13 @@ import { import type { ChatMessage } from '../../types/chat' import type { EventHandlerState } from '../sdk-event-handlers' + +import { printModeEventSchema } from '@codebuff/common/types/print-mode' import type { Logger } from '@codebuff/common/types/contracts/logger' +import type { + PrintModeEvent, + PrintModeJobUpdate, +} from '@codebuff/common/types/print-mode' const createTestContext = () => { let messages: ChatMessage[] = [ @@ -85,6 +91,23 @@ const createTestContext = () => { } } +// Typed event dispatch helper for the job_update/tool_call/tool_result event +// family (RF-2). Validates the payload against `printModeEventSchema` before +// forwarding, so a payload that stops satisfying the discriminated-union +// contract (schema drift) fails the test loudly instead of passing via an +// `as any` escape hatch. Returns the parser-narrowed `PrintModeEvent`. Only +// events in scope for RF-2 go through this helper; the unknown-state forward- +// compat test (RF-1) intentionally bypasses it, since an unlisted state is by +// definition not a valid `PrintModeJobUpdate`. +const dispatchValidEvent = ( + handle: ReturnType, + payload: unknown, +): PrintModeEvent => { + const parsed = printModeEventSchema.parse(payload) + handle(parsed) + return parsed +} + describe('sdk-event-handlers', () => { test('renders provider retry/failover recovery as an ordered resilience timeline', () => { const { ctx, getMessages } = createTestContext() @@ -150,10 +173,21 @@ describe('sdk-event-handlers', () => { expect(getMessages()[0].userError).toBe('Provider failed') }) + test('does not render an error banner for auto-recovering errors', () => { + const { ctx, getMessages } = createTestContext() + createEventHandler(ctx)({ + type: 'error', + message: 'malformed tool call detail\n at x.ts:1:2', + userMessage: 'The model is correcting it automatically.', + autoRecovering: true, + }) + expect(getMessages()[0].userError).toBeUndefined() + }) + test('background agent cards remain running until polling reports settlement', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'subagent_start', agentId: 'child-1', agentType: 'researcher-web', @@ -163,8 +197,8 @@ describe('sdk-event-handlers', () => { spawnIndex: 0, prompt: 'research', onlyChild: true, - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'spawn-bg', toolName: 'spawn_agents', @@ -185,14 +219,14 @@ describe('sdk-event-handlers', () => { ], }, ], - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'agent', status: 'running', backgroundJobId: 'bg-agent-1', }) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'check-bg', toolName: 'check_background_agent', @@ -215,7 +249,7 @@ describe('sdk-event-handlers', () => { }, }, ], - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'agent', status: 'complete', @@ -226,12 +260,12 @@ describe('sdk-event-handlers', () => { test('[ERR-H01] terminal cancellation is immutable when a late result arrives', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'tool-1', toolName: 'read_files', input: { paths: ['a.ts'] }, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'tool' @@ -239,12 +273,12 @@ describe('sdk-event-handlers', () => { : block, ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'tool-1', toolName: 'read_files', output: [{ type: 'json', value: { ok: true } }], - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'tool', lifecycle: 'cancelled', @@ -254,13 +288,13 @@ describe('sdk-event-handlers', () => { test('[COR-H03] any error part makes the terminal tool lifecycle failed', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'tool-2', toolName: 'apply_patch', input: {}, - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'tool-2', toolName: 'apply_patch', @@ -268,19 +302,19 @@ describe('sdk-event-handlers', () => { { type: 'json', value: { applied: true } }, { type: 'json', value: { errorMessage: 'post-commit report failed' } }, ], - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ lifecycle: 'failed' }) }) test('late canonical mutation result replaces cancellation with authoritative state', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'tool-late', toolName: 'write_file', input: { path: 'a.ts' }, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'tool' @@ -288,7 +322,7 @@ describe('sdk-event-handlers', () => { : block, ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'tool-late', toolName: 'write_file', @@ -317,7 +351,7 @@ describe('sdk-event-handlers', () => { }, }, ], - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ lifecycle: 'succeeded', interrupted: true, @@ -338,14 +372,14 @@ describe('sdk-event-handlers', () => { displayName: 'Editor', onlyChild: false, } as any) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'nested-tool-1', toolName: 'edit_transaction', input: { edits: [] }, agentId: 'agent-1', parentAgentId: 'agent-1', - } as any) + }) expect(streaming.has('nested-tool-1')).toBe(true) handleEvent({ type: 'subagent_finish', @@ -385,12 +419,12 @@ describe('sdk-event-handlers', () => { test('root finish fails unresolved foreground tools but preserves live background tools', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'root-running-tool', toolName: 'read_files', input: { paths: ['a.ts'] }, - } as any) + }) handleEvent({ type: 'subagent_start', agentId: 'background-agent', @@ -479,12 +513,12 @@ describe('sdk-event-handlers', () => { const handleEvent = createEventHandler(ctx) // Production path: job id is only known after the SDK starts the process, // so it arrives on tool_result — not on tool_call. No manual mutation. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-bg', toolName: 'run_terminal_command', input: { command: 'npm run dev', process_type: 'BACKGROUND' }, - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'tool', @@ -494,7 +528,7 @@ describe('sdk-event-handlers', () => { (getMessages()[0].blocks?.[0] as any).backgroundJobId, ).toBeUndefined() - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_result', toolCallId: 'term-bg', toolName: 'run_terminal_command', @@ -511,7 +545,7 @@ describe('sdk-event-handlers', () => { }, }, ], - } as any) + }) // Successful BACKGROUND start keeps the card running (not succeeded). expect(getMessages()[0].blocks?.[0]).toMatchObject({ @@ -520,22 +554,22 @@ describe('sdk-event-handlers', () => { lifecycle: 'running', }) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-bg', kind: 'process', state: 'running', sequence: 1, outputDelta: 'listening\n', - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-bg', kind: 'process', state: 'completed', sequence: 2, exitCode: 0, - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'tool', @@ -551,13 +585,102 @@ describe('sdk-event-handlers', () => { // A write queued behind a prior same-path write is emitted with queued:true // and lifecycle 'queued'; the runtime later emits tool_start once the // per-path barrier resolves, which flips the card to running. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'write-queued', toolName: 'write_file', input: { path: 'src/a.ts' }, queued: true, - } as any) + }) + + expect(getMessages()[0].blocks?.[0]).toMatchObject({ + type: 'tool', + queued: true, + lifecycle: 'queued', + }) + + dispatchValidEvent(handleEvent, { + type: 'tool_start', + toolCallId: 'write-queued', + }) + + expect(getMessages()[0].blocks?.[0]).toMatchObject({ + type: 'tool', + queued: false, + lifecycle: 'running', + }) + }) + + test('tool_start flips a queued tool block nested inside an agent block back to running', () => { + const { ctx, getMessages } = createTestContext() + const handleEvent = createEventHandler(ctx) + // Covers the recursive branch of handleToolStart.flipQueued: a queued + // tool_call that lands INSIDE a nested agent block (parentAgentId set) is + // only reachable by recursing into the agent's children. The matching + // tool_start must flip that nested tool back from 'queued' to 'running' + // without disturbing the sibling/root blocks. + dispatchValidEvent(handleEvent, { + type: 'subagent_start', + agentId: 'parent-agent', + agentType: 'editor', + displayName: 'Editor', + onlyChild: true, + }) + dispatchValidEvent(handleEvent, { + type: 'tool_call', + toolCallId: 'nested-write-queued', + toolName: 'write_file', + input: { path: 'src/b.ts' }, + agentId: 'parent-agent', + parentAgentId: 'parent-agent', + queued: true, + }) + + // The queued tool is appended inside the agent block, not at the root. + const agentBlock = getMessages()[0].blocks?.[0] as any + expect(agentBlock).toMatchObject({ type: 'agent', agentId: 'parent-agent' }) + const nestedTool = agentBlock.blocks?.find( + (b: any) => b.type === 'tool' && b.toolCallId === 'nested-write-queued', + ) + expect(nestedTool).toMatchObject({ + type: 'tool', + queued: true, + lifecycle: 'queued', + }) + + dispatchValidEvent(handleEvent, { + type: 'tool_start', + toolCallId: 'nested-write-queued', + }) + + const settledAgent = getMessages()[0].blocks?.[0] as any + const settledNested = settledAgent.blocks?.find( + (b: any) => b.type === 'tool' && b.toolCallId === 'nested-write-queued', + ) + expect(settledNested).toMatchObject({ + type: 'tool', + queued: false, + lifecycle: 'running', + }) + }) + + test('tool_start flips a queued custom/unknown-path tool block back to running', () => { + const { ctx, getMessages } = createTestContext() + const handleEvent = createEventHandler(ctx) + // Pins RF-1: the `queued === true` branch in `executeCustomToolCall` that + // emits `tool_start` for a custom/MCP tool is genuinely reachable, not dead + // defensive code. The CLI handler treats any queued tool_call identically + // regardless of whether it was produced by the native (`executeToolCall`) or + // custom (`executeCustomToolCall`) path, so a custom/unknown-path tool name + // that lands queued must flip from 'queued' to 'running' on tool_start + // exactly like a native write_file. + dispatchValidEvent(handleEvent, { + type: 'tool_call', + toolCallId: 'custom-write-queued', + toolName: 'mcp_server__custom_write', + input: { target: 'custom-resource' }, + queued: true, + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'tool', @@ -565,7 +688,10 @@ describe('sdk-event-handlers', () => { lifecycle: 'queued', }) - handleEvent({ type: 'tool_start', toolCallId: 'write-queued' }) + dispatchValidEvent(handleEvent, { + type: 'tool_start', + toolCallId: 'custom-write-queued', + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'tool', @@ -577,12 +703,12 @@ describe('sdk-event-handlers', () => { test('job_update updates a correlated tool block lifecycle and appends bounded output', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-1', toolName: 'run_terminal_command', input: { command: 'npm test' }, - } as any) + }) // Correlate the run_terminal_command card with a background job id. ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => @@ -592,22 +718,22 @@ describe('sdk-event-handlers', () => { ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-1', kind: 'process', state: 'running', sequence: 1, outputDelta: 'first line\n', - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-1', kind: 'process', state: 'running', sequence: 2, outputDelta: 'second line\n', - } as any) + }) let block = getMessages()[0].blocks?.[0] as any expect(block).toMatchObject({ @@ -616,14 +742,14 @@ describe('sdk-event-handlers', () => { output: 'first line\nsecond line\n', }) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-1', kind: 'process', state: 'completed', sequence: 3, exitCode: 0, - } as any) + }) block = getMessages()[0].blocks?.[0] as any expect(block).toMatchObject({ lifecycle: 'succeeded' }) }) @@ -631,12 +757,12 @@ describe('sdk-event-handlers', () => { test('job_update caps the accumulated tool output at the tail ceiling', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-cap', toolName: 'run_terminal_command', input: { command: 'noisy' }, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'tool' && block.toolCallId === 'term-cap' @@ -645,22 +771,22 @@ describe('sdk-event-handlers', () => { ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-cap', kind: 'process', state: 'running', sequence: 1, outputDelta: 'A'.repeat(60_000), - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-cap', kind: 'process', state: 'running', sequence: 2, outputDelta: 'B'.repeat(5_000), - } as any) + }) const block = getMessages()[0].blocks?.[0] as any expect(block.output.length).toBe(50_000) @@ -671,13 +797,13 @@ describe('sdk-event-handlers', () => { test('job_update updates a correlated agent block status', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'subagent_start', agentId: 'agent-1', agentType: 'researcher-web', displayName: 'Researcher', onlyChild: true, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'agent' && block.agentId === 'agent-1' @@ -686,13 +812,13 @@ describe('sdk-event-handlers', () => { ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-agent', kind: 'agent', state: 'completed', sequence: 1, - } as any) + }) expect(getMessages()[0].blocks?.[0]).toMatchObject({ type: 'agent', @@ -704,35 +830,85 @@ describe('sdk-event-handlers', () => { test('job_update is a no-op when no block correlates to the jobId', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-x', toolName: 'run_terminal_command', input: { command: 'ls' }, - } as any) + }) const before = JSON.stringify(getMessages()) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'unknown-job', kind: 'process', state: 'running', sequence: 1, outputDelta: 'foreign output', - } as any) + }) expect(JSON.stringify(getMessages())).toBe(before) }) + test('job_update maps an unknown state to running (fail-safe) instead of throwing', () => { + // Pins the RF-1 forward-compat contract: the printModeJobUpdateSchema JSDoc + // says consumers should treat unknown variants as no-ops, and handleJobUpdate + // runs in the streaming UI render path. A newer runtime emitting an + // unlisted state must NOT throw and abort the event handler; it should map + // to the least-surprising non-terminal lifecycle ('running') and log a + // warning. An unknown state is by definition not a valid PrintModeJobUpdate, + // so this test bypasses the schema-validating dispatchValidEvent helper and + // casts only the `state` field (not the whole object) to model the scenario + // a future runtime would produce before the schema is widened. + const { ctx, getMessages } = createTestContext() + const warnCalls: Array<{ jobState?: unknown }> = [] + ctx.logger = { + info: () => {}, + warn: (fields?: { jobState?: unknown }) => warnCalls.push(fields ?? {}), + error: () => {}, + debug: () => {}, + } as Logger + const handleEvent = createEventHandler(ctx) + dispatchValidEvent(handleEvent, { + type: 'tool_call', + toolCallId: 'term-unknown', + toolName: 'run_terminal_command', + input: { command: 'some-server' }, + }) + ctx.message.updater.updateAiMessageBlocks((blocks) => + blocks.map((block) => + block.type === 'tool' && block.toolCallId === 'term-unknown' + ? { ...block, backgroundJobId: 'job-unknown' } + : block, + ), + ) + + expect(() => + handleEvent({ + type: 'job_update', + jobId: 'job-unknown', + kind: 'process', + state: 'paused' as PrintModeJobUpdate['state'], + sequence: 1, + }), + ).not.toThrow() + + expect(getMessages()[0].blocks?.[0]).toMatchObject({ + type: 'tool', + lifecycle: 'running', + }) + expect(warnCalls.some((c) => c.jobState === 'paused')).toBe(true) + }) + test('job_update surfaces a failed tool job error in the card output', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-err', toolName: 'run_terminal_command', input: { command: 'boom' }, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'tool' && block.toolCallId === 'term-err' @@ -741,7 +917,7 @@ describe('sdk-event-handlers', () => { ), ) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-err', kind: 'process', @@ -749,7 +925,7 @@ describe('sdk-event-handlers', () => { sequence: 1, outputDelta: 'partial output\n', error: 'command failed with exit code 1', - } as any) + }) const block = getMessages()[0].blocks?.[0] as any expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) @@ -763,30 +939,30 @@ describe('sdk-event-handlers', () => { // Pins the tool-block error dedup that mirrors the agent-block path: an // error/lost job_update delivered more than once without new output must // not append the same error text repeatedly. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-err-dup', toolName: 'run_terminal_command', input: { command: 'npm test' }, backgroundJobId: 'job-err', - } as any) + }) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-err', kind: 'process', state: 'error', sequence: 1, error: 'boom', - } as any) - handleEvent({ + }) + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-err', kind: 'process', state: 'error', sequence: 2, error: 'boom', - } as any) + }) const block = getMessages()[0].blocks?.[0] as any expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) @@ -801,32 +977,106 @@ describe('sdk-event-handlers', () => { // genuinely new error append must NOT be suppressed. The explicit // jobErrorAppended flag (unset until the first error) distinguishes // "already appended this error" from "output coincidentally ends this way". - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'term-coincidental', toolName: 'run_terminal_command', input: { command: 'npm test' }, backgroundJobId: 'job-coincidental', - } as any) + }) // Streamed output that coincidentally ends with the exact error text. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-coincidental', kind: 'process', state: 'running', sequence: 1, outputDelta: 'boom', - } as any) + }) // A genuinely new error carrying the same text; it must still be appended. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-coincidental', kind: 'process', state: 'error', sequence: 2, error: 'boom', - } as any) + }) + + const block = getMessages()[0].blocks?.[0] as any + expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) + // Once from the streamed output, once from the appended error. + expect((block.output.match(/boom/g) ?? []).length).toBe(2) + }) + + test('job_update appends a tool job error wired via tool_result output without duplicating', () => { + const { ctx, getMessages } = createTestContext() + const handleEvent = createEventHandler(ctx) + // Pins error-path parity with the BACKGROUND happy-path test (RF-2): in + // production the runtime emits tool_call WITHOUT backgroundJobId and the + // job id arrives only on tool_result via + // getBackgroundShellJobIdFromToolOutput, then a job_update lands. This + // mirrors that realistic flow (no manual backgroundJobId mutation) with a + // coincidental trailing output equal to the error text, so the + // flag-based dedup still appends a genuinely new error rather than + // suppressing it as a duplicate. + dispatchValidEvent(handleEvent, { + type: 'tool_call', + toolCallId: 'term-bg-err', + toolName: 'run_terminal_command', + input: { command: 'npm test', process_type: 'BACKGROUND' }, + }) + + expect( + (getMessages()[0].blocks?.[0] as any).backgroundJobId, + ).toBeUndefined() + + // tool_result wires backgroundJobId from the BACKGROUND start output; the + // card stays running (a successful BACKGROUND start is not terminal). + dispatchValidEvent(handleEvent, { + type: 'tool_result', + toolCallId: 'term-bg-err', + toolName: 'run_terminal_command', + output: [ + { + type: 'json', + value: { + command: 'npm test', + processId: 4321, + backgroundProcessStatus: 'running', + jobId: 'job-bg-err', + logFile: '/tmp/job-bg-err.log', + startingCwd: '/project', + }, + }, + ], + }) + + expect(getMessages()[0].blocks?.[0]).toMatchObject({ + type: 'tool', + backgroundJobId: 'job-bg-err', + lifecycle: 'running', + }) + + // Live streamed output happens to end with the error text (coincidental). + dispatchValidEvent(handleEvent, { + type: 'job_update', + jobId: 'job-bg-err', + kind: 'process', + state: 'running', + sequence: 1, + outputDelta: 'boom', + }) + // A genuinely new error carrying the same text must still be appended. + dispatchValidEvent(handleEvent, { + type: 'job_update', + jobId: 'job-bg-err', + kind: 'process', + state: 'error', + sequence: 2, + error: 'boom', + }) const block = getMessages()[0].blocks?.[0] as any expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) @@ -834,16 +1084,81 @@ describe('sdk-event-handlers', () => { expect((block.output.match(/boom/g) ?? []).length).toBe(2) }) + test('job_update re-appends a tool job error after a running recovery resets the append flag', () => { + const { ctx, getMessages } = createTestContext() + const handleEvent = createEventHandler(ctx) + // Pins RF-3: after an error append sets `jobErrorAppended`, a non-terminal + // `running` transition must reset the flag so a genuinely new error reported + // after recovery is still surfaced (rather than permanently suppressed by + // the first error). The realistic lifecycle is terminal-once for error/lost, + // but a restart that recovers and then fails again is the documented edge. + dispatchValidEvent(handleEvent, { + type: 'tool_call', + toolCallId: 'term-recover', + toolName: 'run_terminal_command', + input: { command: 'flaky-server' }, + }) + ctx.message.updater.updateAiMessageBlocks((blocks) => + blocks.map((block) => + block.type === 'tool' && block.toolCallId === 'term-recover' + ? { ...block, backgroundJobId: 'job-recover' } + : block, + ), + ) + + // First failure: appends the error and sets jobErrorAppended. + dispatchValidEvent(handleEvent, { + type: 'job_update', + jobId: 'job-recover', + kind: 'process', + state: 'error', + sequence: 1, + error: 'first failure', + }) + let block = getMessages()[0].blocks?.[0] as any + expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) + expect(block.output).toContain('first failure') + + // Recovery back to running (e.g. a restart) resets the append flag. + dispatchValidEvent(handleEvent, { + type: 'job_update', + jobId: 'job-recover', + kind: 'process', + state: 'running', + sequence: 2, + outputDelta: 'recovered\n', + }) + block = getMessages()[0].blocks?.[0] as any + expect(block).toMatchObject({ type: 'tool', lifecycle: 'running' }) + + // A new genuine error after recovery must be appended again. + dispatchValidEvent(handleEvent, { + type: 'job_update', + jobId: 'job-recover', + kind: 'process', + state: 'error', + sequence: 3, + error: 'second failure', + }) + block = getMessages()[0].blocks?.[0] as any + expect(block).toMatchObject({ type: 'tool', lifecycle: 'failed' }) + expect(block.output).toContain('recovered') + expect(block.output).toContain('first failure') + expect(block.output).toContain('second failure') + // The second error text is appended exactly once. + expect((block.output.match(/second failure/g) ?? []).length).toBe(1) + }) + test('job_update appends a single error block to a failed agent job without duplicating', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'subagent_start', agentId: 'agent-err', agentType: 'researcher-web', displayName: 'Researcher', onlyChild: true, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'agent' && block.agentId === 'agent-err' @@ -852,16 +1167,20 @@ describe('sdk-event-handlers', () => { ), ) - const errorEvent = { + // RF-4: dispatch two fresh PrintModeEvent objects rather than reusing one + // reference, so the dedup test stays resilient if the handler ever mutates + // the event in place. Matches the tool-block dedup test, which dispatches + // two distinct events (here the two updates differ in `sequence`). + const errorJobUpdate = (sequence: number): PrintModeEvent => ({ type: 'job_update', jobId: 'job-agent-err', kind: 'agent', state: 'error', - sequence: 1, + sequence, error: 'agent crashed', - } - handleEvent(errorEvent as any) - handleEvent(errorEvent as any) + }) + dispatchValidEvent(handleEvent, errorJobUpdate(1)) + dispatchValidEvent(handleEvent, errorJobUpdate(2)) const agentBlock = getMessages()[0].blocks?.[0] as any expect(agentBlock).toMatchObject({ type: 'agent', status: 'failed' }) @@ -880,13 +1199,13 @@ describe('sdk-event-handlers', () => { // the error text, a genuinely new error must still be appended. The old // string comparison would see the trailing text block match the truncated // error and suppress the append. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'subagent_start', agentId: 'agent-coincidental', agentType: 'researcher-web', displayName: 'Researcher', onlyChild: true, - } as any) + }) ctx.message.updater.updateAiMessageBlocks((blocks) => blocks.map((block) => block.type === 'agent' && block.agentId === 'agent-coincidental' @@ -903,14 +1222,14 @@ describe('sdk-event-handlers', () => { chunk: 'agent crashed', }) // A genuinely new error carrying the same text; it must still be appended. - handleEvent({ + dispatchValidEvent(handleEvent, { type: 'job_update', jobId: 'job-agent-coincidental', kind: 'agent', state: 'error', sequence: 1, error: 'agent crashed', - } as any) + }) const agentBlock = getMessages()[0].blocks?.[0] as any expect(agentBlock).toMatchObject({ type: 'agent', status: 'failed' }) diff --git a/cli/src/utils/sdk-event-handlers.ts b/cli/src/utils/sdk-event-handlers.ts index 8c7e59a780..af00059b0a 100644 --- a/cli/src/utils/sdk-event-handlers.ts +++ b/cli/src/utils/sdk-event-handlers.ts @@ -797,9 +797,16 @@ const JOB_OUTPUT_CHAR_CAP = 50_000 /** * Maps a job-registry lifecycle state to the tool block's lifecycle vocabulary * (`'queued' | 'running' | 'succeeded' | 'failed' | 'cancelled'`). + * + * Forward-compat: the `printModeJobUpdateSchema` JSDoc says consumers should + * treat unknown variants as no-ops. `handleJobUpdate` runs in the streaming + * UI render path, so the `default` branch fails safe by mapping any unknown + * state to `'running'` (the least-surprising non-terminal state) and logging + * a warning, instead of throwing and aborting the whole event handler. */ const jobStateToToolLifecycle = ( state: PrintModeJobUpdate['state'], + logger: Logger, ): ToolContentBlock['lifecycle'] => { switch (state) { case 'queued': @@ -815,12 +822,17 @@ const jobStateToToolLifecycle = ( case 'stopped': case 'cancelled': return 'cancelled' + // Exhaustiveness guard: if `state` is a declared enum value this branch is + // unreachable and `never` keeps the switch exhaustive at compile time. If a + // newer runtime ever emits an unlisted state, this branch fails safe + // (log + `'running'`) rather than throwing on the render path. default: { - // Compile-time exhaustiveness guard (`never`) plus a defensive runtime - // backstop. It changes no emitted event and accepts no new input; no - // declared state value can reach this branch at runtime. const exhaustive: never = state - throw new Error(`Unhandled job state for tool lifecycle: ${String(exhaustive)}`) + logger.warn( + { jobState: String(exhaustive) }, + 'Unknown job state for tool lifecycle; mapping to running (fail-safe)', + ) + return 'running' } } } @@ -828,9 +840,14 @@ const jobStateToToolLifecycle = ( /** * Maps a job-registry lifecycle state to the agent block's status vocabulary * (`'running' | 'complete' | 'failed' | 'cancelled'`). + * + * Forward-compat: see {@link jobStateToToolLifecycle}. The `default` branch + * fails safe by mapping unknown states to `'running'` and logging a warning, + * instead of throwing on the streaming render path. */ const jobStateToAgentStatus = ( state: PrintModeJobUpdate['state'], + logger: Logger, ): AgentContentBlock['status'] => { switch (state) { case 'queued': @@ -845,12 +862,14 @@ const jobStateToAgentStatus = ( case 'stopped': case 'cancelled': return 'cancelled' + // Exhaustiveness guard: see {@link jobStateToToolLifecycle}. default: { - // Compile-time exhaustiveness guard (`never`) plus a defensive runtime - // backstop. It changes no emitted event and accepts no new input; no - // declared state value can reach this branch at runtime. const exhaustive: never = state - throw new Error(`Unhandled job state for agent status: ${String(exhaustive)}`) + logger.warn( + { jobState: String(exhaustive) }, + 'Unknown job state for agent status; mapping to running (fail-safe)', + ) + return 'running' } } } @@ -868,6 +887,15 @@ const handleJobUpdate = ( : undefined const updateBlock = (block: ContentBlock): ContentBlock => { if (block.type === 'tool' && block.backgroundJobId === event.jobId) { + const nextLifecycle = jobStateToToolLifecycle(event.state, state.logger) + // A non-terminal transition (running/queued, e.g. a restart that + // recovers from an earlier error/lost) resets the append flag so a + // genuinely new error reported after recovery is still surfaced — + // otherwise the first error append would permanently suppress all + // later errors for the same job. Terminal error/lost appends keep the + // flag set so a repeated identical error is not duplicated. + const isRecovery = + nextLifecycle === 'queued' || nextLifecycle === 'running' const base = block.output ?? '' const withDelta = event.outputDelta !== undefined ? base + event.outputDelta : base @@ -887,16 +915,18 @@ const handleJobUpdate = ( : block.output return { ...block, - lifecycle: jobStateToToolLifecycle(event.state), + lifecycle: nextLifecycle, ...(nextOutput !== undefined ? { output: nextOutput } : {}), - ...(errorText !== undefined && !errorAlreadyAppended - ? { jobErrorAppended: true } - : {}), + ...(isRecovery + ? { jobErrorAppended: false } + : errorText !== undefined && !errorAlreadyAppended + ? { jobErrorAppended: true } + : {}), } } if (block.type === 'agent') { if (block.backgroundJobId === event.jobId) { - const status = jobStateToAgentStatus(event.state) + const status = jobStateToAgentStatus(event.state, state.logger) if (errorText !== undefined) { const truncatedError = errorText.split('\n').slice(0, 6).join('\n') const existingBlocks = block.blocks ?? [] @@ -921,7 +951,15 @@ const handleJobUpdate = ( ...(alreadyAppended ? {} : { jobErrorAppended: true }), } } - return { ...block, status } + return { + ...block, + status, + // Mirror the tool-block recovery reset: a non-terminal transition + // (recovery back to running) clears the append flag so a genuinely + // new error reported after recovery is still surfaced. The terminal + // error/lost branch above keeps the flag set once it appends. + ...(status === 'running' ? { jobErrorAppended: false } : {}), + } } if (block.blocks) { return { ...block, blocks: block.blocks.map(updateBlock) } @@ -1054,6 +1092,12 @@ const handleRuntimeError = ( event: Extract, ) => { state.logger.error({ event }, 'SDK runtime error event') + // Auto-recoverable model errors (e.g. a malformed tool call the model is + // already correcting) are agent-facing diagnostics, not user-facing errors: + // skip the visible error banner entirely. + if (event.autoRecovering === true) { + return + } const concise = event.userMessage?.trim() if (concise) { state.message.updater.setError(concise) diff --git a/common/src/types/print-mode.ts b/common/src/types/print-mode.ts index b32b1bdc6f..28044152ad 100644 --- a/common/src/types/print-mode.ts +++ b/common/src/types/print-mode.ts @@ -17,6 +17,10 @@ export const printModeErrorSchema = z.object({ // the user instead of the full `message`, which carries detailed recovery // context intended for the agent's message history. userMessage: z.string().optional(), + // True when the runtime is already auto-correcting this error (e.g. a + // malformed tool call the model is retrying). UIs should not surface these + // as user-visible errors; the full `message` still flows to the agent. + autoRecovering: z.boolean().optional(), }) export type PrintModeError = z.infer diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 66a5917ca4..4f3335d1a2 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -141,8 +141,10 @@ export function buildSpawnAgentsHandlerFailureOutput( input: unknown, // Retained for call-site symmetry with the generic failure-output builder // and for logging at the call site; deliberately NOT interpolated into the - // agent-visible errorMessage (see the migration note above). - error: unknown, + // agent-visible errorMessage (see the migration note above). Prefixed with + // `_` so it is explicitly intentionally-unused and lint-safe under + // `noUnusedParameters`. + _error: unknown, ): CodebuffToolOutput<'spawn_agents'> { const inputRecord = input && typeof input === 'object' @@ -1821,6 +1823,7 @@ export async function executeToolCall( message: `${toolCall.error}\n\n${inputLabel}:\n${formattedInput}`, userMessage: `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.`, + autoRecovering: true, }) logger.debug( { toolCall, error: toolCall.error }, @@ -2573,6 +2576,13 @@ export async function executeToolCall( }), ) + // NOTE (spawn-failure MIGRATION NOTE sync, RF-5): the underlying handler + // error MUST be logged here via logger.warn. `buildSpawnAgentsHandlerFailureOutput` + // intentionally does NOT interpolate the raw error into agent-visible output + // (see its MIGRATION NOTE), so this call site is the single logging point for + // that error. If this `.catch` is ever refactored, preserve the + // `logger.warn({ error, toolName, toolCallId }, ...)` contract or the error + // becomes silently lost for spawned agents. const recoverableToolResultPromise = toolResultPromise.catch((error) => { if (isAbortError(error)) throw error logger.warn( @@ -2866,6 +2876,7 @@ export async function executeCustomToolCall( type: 'error', message: `${toolCall.error}\n\n${inputLabel}:\n${formattedInput}`, userMessage: `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.`, + autoRecovering: true, }) logger.debug( { toolCall, error: toolCall.error }, @@ -2919,8 +2930,16 @@ export async function executeCustomToolCall( // `tool_start` transition once the barrier resolves so the CLI can flip the // block from "queued" to "pending". Non-blocking: do NOT await // `previousToolCallFinished` here (the handler still awaits it internally). - // For custom/unknown-path writes `queued` is typically undefined, so this - // is a no-op in practice — guarded for consistency with native writes. + // + // Reachability (RF-1): `queued` is threaded through `ExecuteToolCallParams` + // for any serialized same-path write, and custom/MCP tool paths can be + // queued when a per-path write barrier applies to a custom/unknown-path + // input — so this branch is genuinely reachable, not dead defensive code. + // It is rarer than the native write_file/edit_transaction path because most + // custom tools do not touch the project filesystem and therefore never hit + // the write barrier, but the runtime does not restrict `queued` to native + // tools. The downstream CLI flip is covered by the queued-block tool_start + // tests in sdk-event-handlers.test.ts (including the nested-agent case). if (queued === true) { abortablePreviousToolCallFinished.then( () => {