fix: state-based session routing to eliminate double-message bug

[dimakis]

Summary

• Root cause: handleSwitchSession didn't call watch(), so events from the query loop never reached the reconnected client. Combined with a staleInMemory check that could incorrectly kill a running query loop when EventStore and SessionRegistry diverged, the first message after navigating back to a session was silently swallowed.
• Replace the staleInMemory boolean check with Phase 3 state-based routing using getSessionState() as the single source of truth in both handleSendV2 and handleInterruptV2
• Add watch() call to handleSwitchSession + return running state to client
• Use stopChat() (abort signal) instead of registry.remove() for zombie sessions

Test plan

• 125/125 ws-handler-v2 tests pass (verified)
• Navigate away from active session and back — first message should get a response immediately
• Repeat with session that was idle >48h (ENDED state) — should resume cleanly
• Verify interrupt works after session switch without double-send

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

server/ws-handler-v2.ts

Solid fix for the double-message bug via state-based routing. The main concern is that handleReconnect still uses the old isActive boolean pattern, leaving a consistency gap. Missing test coverage for the new running field in handleSwitchSession.

• 🟡 regressions (L215): handleReconnect still uses the old storeMeta.isActive boolean + sessionRegistry.remove() pattern (lines 215-223) while handleSendV2/handleInterruptV2 have been migrated to the state-based storeState + stopChat() approach. This inconsistency means the same stale-state divergence that caused the double-message bug in send/interrupt could still manifest during reconnect. Consider migrating handleReconnect to the same state-based routing for consistency. [fixable]
• 🔵 style (L482): Case 1 comment lists states "ACTIVE, DETACHED, SUSPENDED, STARTING, CREATED" but the condition storeState !== 'ENDED' also matches CLOSING. The comment should include CLOSING for accuracy, since CLOSING sessions are routable (they accept messages during graceful closeout). [fixable]

server/__tests__/ws-handler-v2.test.ts

Solid fix for the double-message bug via state-based routing. The main concern is that handleReconnect still uses the old isActive boolean pattern, leaving a consistency gap. Missing test coverage for the new running field in handleSwitchSession.

• 🟡 missing_tests (L362): handleSwitchSession tests don't assert the new running field in the session_switched response. The existing test uses expect.objectContaining which silently ignores unasserted fields. Should add test cases for: (1) running=true when session is active in registry, (2) running=false when session is not in registry. [fixable]

server/chat.ts

Solid fix for the double-message bug via state-based routing. The main concern is that handleReconnect still uses the old isActive boolean pattern, leaving a consistency gap. Missing test coverage for the new running field in handleSwitchSession.

• 🔵 style (L898): Double cast msg as unknown as Record<string, unknown> is unnecessarily permissive. BootContextMessage is an object type with string keys, so a single as Record<string, unknown> would suffice and preserve more type safety. The as unknown intermediate effectively turns off the type checker. [fixable]
• 🔵 unsafe_assumptions (L142): localBootContextFallback hardcodes a list of 5 source paths (lines 142-148) with the comment "coupled to SOURCE_PATHS in scripts/build_boot_context.py". If the Python script's source list changes, these will silently diverge. Consider extracting to a shared constant or reading the script output's actual source list. [fixable]

[dimakis]

Centaur Review

Found 7 issue(s) (3 warning).

server/ws-handler-v2.ts

Solid fix for the double-message bug with clean state-machine routing, but the CLOSING state falls through to the active path unintentionally, and the new handleSwitchSession changes (running field, early watch) lack dedicated test coverage.

• 🟡 bugs (L510): Comment lists states routed to Case 1 as "ACTIVE, DETACHED, SUSPENDED, STARTING, CREATED" but the condition storeState !== 'ENDED' && storeState !== null also admits CLOSING. A session in CLOSING state is winding down (CLOSING can only transition to ENDED per VALID_TRANSITIONS). Routing a new message via sendToChat to a closing session may race with the closeout — the agent could receive a new prompt while it's trying to commit/summarize. Either add CLOSING to the exclusion list alongside ENDED (treating it as a zombie) or document in the comment that CLOSING is intentionally treated as active. [fixable]
• 🟡 bugs (L672): Same CLOSING-state gap in handleInterruptV2: the comment says "ACTIVE, DETACHED, SUSPENDED, STARTING, CREATED" but the condition also admits CLOSING. Should be consistent with whatever decision is made for handleSendV2. [fixable]
• 🔵 style (L497): The block comments explaining the state-based routing (lines 497-511, 563-565, 576, and similarly in handleInterruptV2) are quite verbose for this codebase's conventions. CLAUDE.md says "default to writing no comments" and "only add one when the WHY is non-obvious." The initial Phase 3 header comment is useful context, but the per-case comments restate what the code already expresses through the condition structure. Consider trimming to one-line comments per case. [fixable]
• 🔵 unsafe_assumptions (L377): handleSwitchSession determines running via ctx.sessionRegistry.isActive(found.clientId) — the in-memory registry. But the state-based routing in handleSendV2/handleInterruptV2 explicitly avoids trusting the in-memory registry alone (the whole point of this PR). A zombie session would show running: true here even though the EventStore state is ENDED. Consider cross-referencing eventStore.getSessionState() for consistency, similar to how handleReconnect already does zombie detection (test at line 896). [fixable]

server/__tests__/ws-handler-v2.test.ts

Solid fix for the double-message bug with clean state-machine routing, but the CLOSING state falls through to the active path unintentionally, and the new handleSwitchSession changes (running field, early watch) lack dedicated test coverage.

• 🟡 missing_tests: No test verifies that the session_switched response includes the running field. The existing handleSwitchSession tests (lines 362-426) assert on mode/cwd/branch/wtId/tokens but not running. A test with an active session (isActive → true) should assert running: true, and a test with an ended session should assert running: false. [fixable]
• 🔵 missing_tests: No test covers the early watch() call added in handleSwitchSession (line 368). The intent — preventing a blind spot between switch and first send — is important enough to verify that connRegistry.watch is called before the response is sent. A spy-ordering test or at minimum an assertion that watch was called with the correct args would prevent a regression. [fixable]
• 🔵 missing_tests: No test covers the CLOSING state for either handleSendV2 or handleInterruptV2. Given the ambiguity noted above, a test that sets getSessionState to 'CLOSING' would document the intended behavior. [fixable]

[dimakis]

🟡 bugs: Comment lists states routed to Case 1 as "ACTIVE, DETACHED, SUSPENDED, STARTING, CREATED" but the condition storeState !== 'ENDED' && storeState !== null also admits CLOSING. A session in CLOSING state is winding down (CLOSING can only transition to ENDED per VALID_TRANSITIONS). Routing a new message via sendToChat to a closing session may race with the closeout — the agent could receive a new prompt while it's trying to commit/summarize. Either add CLOSING to the exclusion list alongside ENDED (treating it as a zombie) or document in the comment that CLOSING is intentionally treated as active. [fixable]

[dimakis]

🟡 bugs: Same CLOSING-state gap in handleInterruptV2: the comment says "ACTIVE, DETACHED, SUSPENDED, STARTING, CREATED" but the condition also admits CLOSING. Should be consistent with whatever decision is made for handleSendV2. [fixable]

[dimakis]

🔵 style: The block comments explaining the state-based routing (lines 497-511, 563-565, 576, and similarly in handleInterruptV2) are quite verbose for this codebase's conventions. CLAUDE.md says "default to writing no comments" and "only add one when the WHY is non-obvious." The initial Phase 3 header comment is useful context, but the per-case comments restate what the code already expresses through the condition structure. Consider trimming to one-line comments per case. [fixable]

[dimakis]

🔵 unsafe_assumptions: handleSwitchSession determines running via ctx.sessionRegistry.isActive(found.clientId) — the in-memory registry. But the state-based routing in handleSendV2/handleInterruptV2 explicitly avoids trusting the in-memory registry alone (the whole point of this PR). A zombie session would show running: true here even though the EventStore state is ENDED. Consider cross-referencing eventStore.getSessionState() for consistency, similar to how handleReconnect already does zombie detection (test at line 896). [fixable]

[dimakis]

Centaur Review (R2)

Clean fix following the Phase 3 design doc. State-based routing logic is sound, CLOSING exclusion is correct, zombie abort via stopChat() prevents leaked query loops.

No blocking issues.

Minor notes for follow-up:

• handleReconnect still uses old getSession().isActive pattern (different code path, not a regression)
• No protocol-parser test for SET_RUNNING dispatch (low risk)

130/130 tests passing. Approved.