From 9203472be8a1614656bc04f741c5a946a72e4b49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 20:41:47 +0000 Subject: [PATCH] =?UTF-8?q?the=20recovery=20could=20never=20have=20fired?= =?UTF-8?q?=20=E2=80=94=20fix=20its=20condition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shipped e6422052 an hour ago claiming a chat that cannot be resumed would recover on the next message. It would not have. Caught before it was pulled, by tracing the two error strings in a screenshot back to their sources instead of trusting the change. `sawOutput` was set on ANY SDK message. But a turn that cannot load its conversation still emits a terminal `result` with `is_error` and no text — that is exactly what reaches the user as "Claude Code returned an error", one of the two lines in the report. So the single event that proves the resume failed was being counted as proof it had worked, and `clearResume` could never be true in the one situation it exists for. The flag now ignores `error` and counts everything else, as `countsAsOutput` — a named function with tests rather than an inline `!==`, because this is the second time this rule has been wrong and both times it was wrong silently. `stopped` deliberately counts: an interrupted turn was running, so its conversation loaded, and discarding a transcript because somebody pressed stop quickly would be a new bug of the same shape. Five more tests, including the reported failure end to end (`['error']` -> give up on the resume) and its opposite (`['assistant:text', 'error']` -> keep it). Still does not explain WHY the resume fails. Also worth recording: my evidence that "resume works by hand" may be worth nothing — `claude --resume` with an id it cannot find may simply start a fresh session and exit 0, which would look identical to success in both tests I ran. The new spawn log is what will settle it. tsgo clean, 950 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 --- src/servers/sidecar/claude/claude-manager.ts | 7 ++-- .../sidecar/claude/exit-diagnosis.test.ts | 38 ++++++++++++++++++- src/servers/sidecar/claude/exit-diagnosis.ts | 18 +++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) 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;