diff --git a/src/servers/api/chat/websocket.test.ts b/src/servers/api/chat/websocket.test.ts new file mode 100644 index 00000000..94e1a2fd --- /dev/null +++ b/src/servers/api/chat/websocket.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from 'bun:test'; +import { decideResume } from './websocket'; + +// The first test in api/chat, which had none. +// +// It covers the harness decision on reconnect — the one place where guessing wrong writes something +// permanent. `endTurnIfAgentIsGone` appends a durable `cut-off` row, so a session misidentified here +// gets "the agent went away" written into a turn that is running normally, and it survives every reload +// because surviving reloads is what that row is for. +// +// The old code was `msg.model || DEFAULT_MODEL` with `DEFAULT_MODEL = 'claude-code'`, and it was masked +// only by the client always happening to send `model`. These tests exist so the default cannot come back. + +describe('decideResume', () => { + it("takes the server's own record over the client's claim", () => { + // The socket does not get to re-declare a session's harness. This is the direction that matters: + // believing the claim is how an OpenCode session ends up subscribed to the Claude sidecar. + expect(decideResume('opencode/big-pickle', 'claude-code')).toEqual({ + kind: 'known', + model: 'opencode/big-pickle', + }); + }); + + it('adopts on the claim only when this process has no record of the session', () => { + // An officer restart drops the in-memory session while the agent carries on, so the client is the + // only one left who knows. Trusting it here is not the same as trusting it above. + expect(decideResume(undefined, 'opencode/big-pickle')).toEqual({ kind: 'adopt', model: 'opencode/big-pickle' }); + expect(decideResume(undefined, 'claude-code')).toEqual({ kind: 'adopt', model: 'claude-code' }); + }); + + it('stays unknown rather than defaulting to claude, which is the whole bug', () => { + // No record and no claim. Previously this became `claude-code` and was indistinguishable from a real + // Claude session; now it declines to adopt and declines to run the cut-off check. + expect(decideResume(undefined, undefined)).toEqual({ kind: 'replay-only' }); + expect(decideResume(undefined, '')).toEqual({ kind: 'replay-only' }); + }); + + it('never reports replay-only once anything is known, so a live turn is always re-bound', () => { + // The failure mode in the other direction: declining to adopt a session we could have identified + // would silently drop the reconnect that makes turn output resume. + expect(decideResume('claude-code', undefined).kind).toBe('known'); + expect(decideResume('', 'claude-code').kind).toBe('adopt'); + }); +}); diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 683d472f..32236343 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -614,11 +614,22 @@ async function handleResumeCursor( msg: { sessionId: string; cursor: number; model?: string; cwd?: string; generating?: boolean }, ): Promise { const { sessionId, cursor } = msg; - const model = msg.model || DEFAULT_MODEL; - if (!sessionManager.getSession(sessionId)) { - adoptOrphanedSession(ws, sessionId, model, msg.cwd ?? ''); + + const known = sessionManager.getSession(sessionId); + const decision = decideResume(known?.model, msg.model); + const model = decision.kind === 'replay-only' ? null : decision.model; + + if (decision.kind === 'adopt') { + adoptOrphanedSession(ws, sessionId, decision.model, msg.cwd ?? ''); + } else if (decision.kind === 'replay-only') { + // Replay the durable log and stop there. Not adopting costs a live re-subscription; adopting on a + // guess cost correctness — see decideResume. + logger.warn('resume-cursor names a session this process does not know, and no model; replaying only', { + sessionId, + }); } - sessionManager.attachWs(sessionId, ws); + + sessionManager.attachWs(sessionId, ws); // a no-op when adoption was skipped wsToSessionMap.set(ws as any, sessionId); try { const events = await getChatEventsSince(sessionId, cursor ?? 0); @@ -628,7 +639,42 @@ async function handleResumeCursor( } catch (err) { logger.error('Failed to replay chat events on resume-cursor', { sessionId, error: String(err) }); } - if (msg.generating) await endTurnIfAgentIsGone(ws, sessionId, model); + // Only ask when the harness is actually known. The check writes a DURABLE row, so a wrong answer here + // is permanent — silence is the safe failure. + if (msg.generating && model) await endTurnIfAgentIsGone(ws, sessionId, model); +} + +export type ResumeDecision = + | { kind: 'known'; model: string } + | { kind: 'adopt'; model: string } + | { kind: 'replay-only' }; + +/** + * Which harness a resuming session belongs to, and what that permits. + * + * This used to be `msg.model || DEFAULT_MODEL`, and `DEFAULT_MODEL` is `claude-code`. So a resume-cursor + * that omitted `model` declared every session — OpenCode ones included — to be Claude, with three + * consequences that all read as something else: + * + * - **Adopted into the wrong harness.** `adoptOrphanedSession` subscribes to the sidecar bus for that + * model and pins `session.model` for the rest of its life, so an OpenCode turn's output never + * arrived, and stopping it called `killClaude` on a key that sidecar had never held — a dead stop + * button, silently. + * - **A durable false `cut-off`.** `endTurnIfAgentIsGone` asked the Claude sidecar whether it was + * generating, was told `false` because it had never heard of the session, and wrote "the agent went + * away" into a turn that was running normally. It survives reload, which is the whole point of + * writing it durably, and is therefore unrecoverable from the UI. + * - Masked, never fixed, by the client always happening to send `model` next to `sessionId`. + * + * Two rules. **The server's own record beats the client's claim** — a session in memory already knows its + * harness, and letting a socket re-declare it is how the wrong sidecar gets a session in the first place. + * **An unknown harness stays unknown**: no adoption, no cut-off check, just the replay. Defaulting is + * what made a guess indistinguishable from knowledge. + */ +export function decideResume(knownModel: string | undefined, claimedModel: string | undefined): ResumeDecision { + if (knownModel) return { kind: 'known', model: knownModel }; + if (claimedModel) return { kind: 'adopt', model: claimedModel }; + return { kind: 'replay-only' }; } /** @@ -683,6 +729,10 @@ async function handleAttach(ws: ServerWebSocket, msg: { claudeSessionId: // An officer restart takes the in-memory session with it while the agent carries on, so the key can // resolve to a session this process has never heard of. Adopting re-subscribes it to the sidecar's bus, // which is what makes the rest of the turn arrive. + // `DEFAULT_MODEL` is right here and wrong in resume-cursor, which is worth being explicit about since + // it was just removed there: this path reached `sessionId` by asking the CLAUDE sidecar to resolve a + // `claudeSessionId`, so the harness is not a guess — only Claude could have answered. Attach is + // Claude-only by construction. If an OpenCode reattach verb ever lands, this stops being safe. const existing = sessionManager.getSession(sessionId); const session = existing ?? adoptOrphanedSession(ws, sessionId, DEFAULT_MODEL, '');