stop resume-cursor guessing that a session is claude
B7. `msg.model || DEFAULT_MODEL` declared every session without an explicit model to be claude-code, and the parity doc recorded only the visible half of what that cost. The durable false cut-off is real: endTurnIfAgentIsGone asked the claude sidecar about a key it had never held, was told false, and wrote "the agent went away" into a turn that was running fine. It survives reload, because surviving reload is what that row is for. The same default also handed the session to adoptOrphanedSession as a claude one, which subscribes it to that sidecar bus and pins session.model — so an opencode turn output never arrived, and stopping it called killClaude on a key that sidecar never had. A stop button that silently does nothing. decideResume makes both rules explicit: the server record beats the client claim, and an unknown harness stays unknown — no adoption, no cut-off check, just the replay. Silence is the safe failure when the wrong answer is written durably. DEFAULT_MODEL stays in handleAttach and is now commented as to why: that path reached its sessionId by asking the claude sidecar to resolve a claudeSessionId, so only claude could have answered. First test in api/chat, which had none. websocket.ts has no seam to drive the handler through, so the decision is extracted and tested; the wiring around it is not covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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');
|
||||
});
|
||||
});
|
||||
@@ -614,11 +614,22 @@ async function handleResumeCursor(
|
||||
msg: { sessionId: string; cursor: number; model?: string; cwd?: string; generating?: boolean },
|
||||
): Promise<void> {
|
||||
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<WSData>, 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, '');
|
||||
|
||||
|
||||
Reference in New Issue
Block a user