diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 147e0f55..b3842bc0 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -6,7 +6,7 @@ import { query, type Query, type SpawnOptions, type SpawnedProcess } from '@anth import type { ChatEvent, PromptImage } from '../../api/chat/types'; import type { ClaudeSpawnParams, ClaudeSpawnStreamingParams, ClaudeCodeResult, LiveClaudeSession } from '../protocol'; import { setClaudeSession, clearClaudeSession, getClaudeSession } from './state'; -import { diagnoseExit, stderrReason } from './exit-diagnosis'; +import { countsAsOutput, diagnoseExit, stderrReason } from './exit-diagnosis'; import { createParseState, processMessage } from './stream-parser'; import { spawnClaudeAsMember } from './spawn-as-member'; import { claudeBinIn } from '@@/os-user-claude'; @@ -620,13 +620,12 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // Any event at all is a sign of life, so push the stall deadline back. Placed after the branch // above so a terminal event disarms rather than re-arms it. armStall(session); + // See `countsAsOutput`: an error is the one event that does not prove the harness was working. + if (countsAsOutput(event.type)) session.sawOutput = true; session.emit(event); }; try { for await (const msg of q as AsyncGenerator>) { - // One message — any message — means the child got far enough to talk. That distinguishes a resume - // that could not even load from a turn that broke halfway, which want opposite handling. - session.sawOutput = true; processMessage(msg, state, { onEvent: emit, onSessionId: (id: string) => setClaudeSession(sessionKey, id, params.userId), diff --git a/src/servers/sidecar/claude/exit-diagnosis.test.ts b/src/servers/sidecar/claude/exit-diagnosis.test.ts index 14760e77..6714bd90 100644 --- a/src/servers/sidecar/claude/exit-diagnosis.test.ts +++ b/src/servers/sidecar/claude/exit-diagnosis.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { diagnoseExit, stderrReason } from './exit-diagnosis'; +import { countsAsOutput, diagnoseExit, stderrReason } from './exit-diagnosis'; // The bug these encode: a member's chat answered "Claude Code process exited with code 1" four times in a // row and could never recover, because every message retried the same `--resume` id and nothing ever @@ -55,3 +55,39 @@ describe('stderrReason', () => { expect(stderrReason('x'.repeat(5000)).length).toBeLessThanOrEqual(300); }); }); + +// ── What counts as the harness having worked ── +// +// These exist because the first version of this rule was wrong in the only case it was written for, and +// nothing failed: a broken resume emits a terminal error result, that was counted as output, and the +// recovery it was supposed to trigger could never fire. + +describe('countsAsOutput', () => { + test('an error does NOT count — it is what a failed resume emits', () => { + expect(countsAsOutput('error')).toBe(false); + }); + + test('real work counts', () => { + for (const type of ['assistant:text', 'assistant:delta', 'tool:start', 'tool:result', 'result']) { + expect(countsAsOutput(type)).toBe(true); + } + }); + + test('a stopped turn counts — it was running, so the conversation loaded', () => { + // Clearing someone's transcript because they pressed stop quickly would be its own bug. + expect(countsAsOutput('stopped')).toBe(true); + }); + + // The end-to-end shape of the reported failure: no real output, an error result, a resuming turn. + test('the reported failure now resolves to "give up on the resume"', () => { + const events = ['error']; + const sawOutput = events.some(countsAsOutput); + expect(diagnoseExit({ sawOutput, resumeId: '49527b33', stderrTail: '' }).clearResume).toBe(true); + }); + + test('and a turn that spoke before failing does not', () => { + const events = ['assistant:text', 'error']; + const sawOutput = events.some(countsAsOutput); + expect(diagnoseExit({ sawOutput, resumeId: '49527b33', stderrTail: '' }).clearResume).toBe(false); + }); +}); diff --git a/src/servers/sidecar/claude/exit-diagnosis.ts b/src/servers/sidecar/claude/exit-diagnosis.ts index d30df2d1..29726f63 100644 --- a/src/servers/sidecar/claude/exit-diagnosis.ts +++ b/src/servers/sidecar/claude/exit-diagnosis.ts @@ -17,6 +17,24 @@ * turn that broke later. Clearing the resume on the second would trade one bad turn for a lost * conversation, which is the wrong direction. */ +/** + * Whether an event proves the harness was actually working. + * + * This is the flag's definition and the first version got it backwards in exactly the case it exists for. + * It counted ANY SDK message as output — but a turn that cannot load its conversation still emits a + * terminal `result` with `is_error` and no text, which is what reaches the user as "Claude Code returned an + * error". So the very event that proves the resume failed was being read as proof it succeeded, and the + * recovery could never fire. Caught by tracing the two error strings in a screenshot to their sources + * rather than by testing, which is luck; hence this being a named function with tests rather than an + * inline `!==`. + * + * `stopped` DOES count. A turn someone interrupted was running, which means the conversation loaded — and + * discarding the transcript because somebody pressed stop quickly would be its own bug. + */ +export function countsAsOutput(eventType: string): boolean { + return eventType !== 'error'; +} + export type ExitDiagnosis = { /** Forget the stored transcript id, so the next message starts a new session instead of failing again. */ clearResume: boolean;