feat(transport): transport SSOT phase 1 — server-authoritative state

[dimakis]

Summary

• Replace client-side SET_RUNNING with server-authoritative session_state_changed events
• Remove syncRunningState() polling — state is now fully event-driven
• Migrate server-side is_active readers to use state column
• Remove running field from reconnected/session_switched messages

Builds on P0 (PR #431, merged). Net -371 lines — more dead code removed than new code added.

Test plan

• 247 tests pass across protocol-parser, store, and ws-handler-v2
• P0 no-op tests updated to verify P1 action dispatch
• Manual test: start session, verify running state reflects from server events
• Manual test: reconnect, verify no syncRunningState poll

Supersedes #432 (closed when P0 base merged).

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

server/ws-handler-v2.ts

Solid P1 migration from client-derived to server-authoritative session state. The main concern is a running-state gap on session switch: the old immediate running field was removed but no immediate replacement exists — the client relies on periodic sync (5s) or the next live event, which creates a noticeable regression when switching to sessions paused during tool execution.

• 🟡 regressions: handleSwitchSession no longer sends running in session_switched, and the client comment says 'running state restored via replayed session_state_changed events' — but handleSwitchSession does NOT replay events. It only calls connRegistry.watch(), so the client must wait for either the next live query-loop event (immediate if streaming, seconds if in a tool-execution pause) or the ConnectionRegistry's periodic sync (up to 5 seconds). Previously, running was communicated immediately via the session_switched payload. This creates a visible regression: switching to a running-but-paused session will show the send button instead of the stop button for up to 5 seconds. [fixable]

packages/client/src/protocol-parser.ts

Solid P1 migration from client-derived to server-authoritative session state. The main concern is a running-state gap on session switch: the old immediate running field was removed but no immediate replacement exists — the client relies on periodic sync (5s) or the next live event, which creates a noticeable regression when switching to sessions paused during tool execution.

• 🟡 regressions: Removing SET_RUNNING from the subscribed handler means the client no longer derives running state from subscription acknowledgment. Combined with the session_switched removal, there is no immediate-delivery path for running state on session switch — only the deferred periodic sync (5s interval) or a new live event from the query loop. Consider having handleSwitchSession replay the latest session_state_changed event (single event, not full history) to close this gap. [fixable]
• 🔵 unsafe_assumptions: session_state_changed handler checks typeof msg.state === 'string' but does not validate against the ClientSessionState union ('idle' | 'running' | 'requires_action'). An unexpected string value (e.g., from a server version mismatch) would silently map to running=false in the reducer. Consider validating against the known values, or at minimum logging unknown states. This is consistent with the rest of the parser's loose typing but is more consequential now that this event drives primary UI state. [fixable]

packages/client/__tests__/protocol-parser.test.ts

Solid P1 migration from client-derived to server-authoritative session state. The main concern is a running-state gap on session switch: the old immediate running field was removed but no immediate replacement exists — the client relies on periodic sync (5s) or the next live event, which creates a noticeable regression when switching to sessions paused during tool execution.

• 🔵 missing_tests: session_state_changed tests cover 'running' and 'idle' states but omit 'requires_action', the third valid ClientSessionState. Since the reducer maps requires_action to running=false (same as idle), this behavior should be explicitly tested to prevent future regressions if the mapping changes. [fixable]

frontend/src/pages/ChatView.tsx

Solid P1 migration from client-derived to server-authoritative session state. The main concern is a running-state gap on session switch: the old immediate running field was removed but no immediate replacement exists — the client relies on periodic sync (5s) or the next live event, which creates a noticeable regression when switching to sessions paused during tool execution.

• 🔵 style: handleStop dispatches SESSION_STATE_CHANGED('idle') as a client-side optimistic update, which contradicts the PR's stated goal of 'server-authoritative state'. This is likely intentional for UX responsiveness, but worth a brief inline comment noting this is an intentional departure — otherwise a future reader may assume it's a leftover from the old pattern and try to remove it. [fixable]

[dimakis]

Centaur Review

Found 7 issue(s) (3 warning).

server/ws-handler-v2.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🟡 regressions (L410): handleSwitchSession no longer sends running in the session_switched response, and does not replay past events. When a user switches to an already-running session, the client resets to INITIAL_MESSAGES_STATE (running=false) via switchSession() in store.ts:267. The client only learns the session is running when the next live session_state_changed event arrives — but if the session is steadily in ACTIVE state with no transition, no such event fires. The UI will show a send button instead of the stop button until the agent emits a new message_start/block_delta event. Consider either replaying the latest session_state_changed event during switch, or sending a one-shot state snapshot in the session_switched response. [fixable]

packages/client/src/protocol-parser.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🟡 regressions (L357): The session_end handler previously dispatched SET_RUNNING: true immediately when draining a queued message via onSendQueued. Now, running goes false (from the SESSION_END reducer case) and stays false until the server round-trips a session_state_changed: running event. This creates a brief UI flicker where the stop button disappears and the send button appears between the queued send and the server's state event. The old behavior was optimistic — setting running=true at the same time as sending the queued message. [fixable]
• 🔵 style (L268): The typeof msg.state === 'string' check accepts any string, not just valid ClientSessionState values. Since this is a system boundary (server → client), consider validating against the known values ('idle' | 'running' | 'requires_action') to avoid silently accepting malformed server messages that would set running: false for unexpected states. [fixable]

packages/client/src/store.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🟡 regressions (L364): Same issue in the pendingSendTimer callback: the timer fires connection.send(pending) but no longer sets running: true. The UI stays in idle state until the server responds with session_state_changed. Previously SET_RUNNING: true was dispatched synchronously. For a safety-net timer that fires after a 5s delay, the additional round-trip latency compounds the gap. [fixable]

packages/client/src/slices/messages.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🔵 unsafe_assumptions (L578): requires_action maps to running: false since action.state === 'running' is false. While requires_action is currently never emitted from toClientState() (the comment at event-store.ts:24 confirms this), the type contract ClientSessionState = 'idle' | 'running' | 'requires_action' allows it. If a future phase emits it, the stop button would disappear during permission prompts. Consider action.state !== 'idle' to be safe, or document the assumption. [fixable]

packages/client/__tests__/protocol-parser.test.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🔵 missing_tests (L928): Tests cover state: 'running' and state: 'idle' (mapped from ENDED) but there is no test for state: 'requires_action' to verify the expected behavior (currently maps to running=false). Adding a test would document the intended semantics and catch regressions if the mapping changes. [fixable]

packages/client/__tests__/store.test.ts

Clean migration from client-derived to server-authoritative running state. Main concern: switchSession to an already-running session will show running=false until the next live state event, since session_switched no longer carries running and no events are replayed — this is a user-visible regression where the stop button disappears.

• 🔵 style (L1301): The removed syncRunningState tests are replaced with a two-line comment. The foreground recovery section (describe('foreground recovery')) still exists but now only tests message re-fetch — not running state. Consider adding a test that verifies session_state_changed events received after foreground restore correctly update running, to prove the new mechanism works end-to-end. [fixable]

[dimakis]

🟡 regressions: handleSwitchSession no longer sends running in the session_switched response, and does not replay past events. When a user switches to an already-running session, the client resets to INITIAL_MESSAGES_STATE (running=false) via switchSession() in store.ts:267. The client only learns the session is running when the next live session_state_changed event arrives — but if the session is steadily in ACTIVE state with no transition, no such event fires. The UI will show a send button instead of the stop button until the agent emits a new message_start/block_delta event. Consider either replaying the latest session_state_changed event during switch, or sending a one-shot state snapshot in the session_switched response. [fixable]

[dimakis]

🟡 regressions: The session_end handler previously dispatched SET_RUNNING: true immediately when draining a queued message via onSendQueued. Now, running goes false (from the SESSION_END reducer case) and stays false until the server round-trips a session_state_changed: running event. This creates a brief UI flicker where the stop button disappears and the send button appears between the queued send and the server's state event. The old behavior was optimistic — setting running=true at the same time as sending the queued message. [fixable]

[dimakis]

🔵 style: The typeof msg.state === 'string' check accepts any string, not just valid ClientSessionState values. Since this is a system boundary (server → client), consider validating against the known values ('idle' | 'running' | 'requires_action') to avoid silently accepting malformed server messages that would set running: false for unexpected states. [fixable]

[dimakis]

🟡 regressions: Same issue in the pendingSendTimer callback: the timer fires connection.send(pending) but no longer sets running: true. The UI stays in idle state until the server responds with session_state_changed. Previously SET_RUNNING: true was dispatched synchronously. For a safety-net timer that fires after a 5s delay, the additional round-trip latency compounds the gap. [fixable]

[dimakis]

🔵 unsafe_assumptions: requires_action maps to running: false since action.state === 'running' is false. While requires_action is currently never emitted from toClientState() (the comment at event-store.ts:24 confirms this), the type contract ClientSessionState = 'idle' | 'running' | 'requires_action' allows it. If a future phase emits it, the stop button would disappear during permission prompts. Consider action.state !== 'idle' to be safe, or document the assumption. [fixable]

[dimakis]

🔵 missing_tests: Tests cover state: 'running' and state: 'idle' (mapped from ENDED) but there is no test for state: 'requires_action' to verify the expected behavior (currently maps to running=false). Adding a test would document the intended semantics and catch regressions if the mapping changes. [fixable]

[dimakis]

🔵 style: The removed syncRunningState tests are replaced with a two-line comment. The foreground recovery section (describe('foreground recovery')) still exists but now only tests message re-fetch — not running state. Consider adding a test that verifies session_state_changed events received after foreground restore correctly update running, to prove the new mechanism works end-to-end. [fixable]

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

packages/client/src/protocol-parser.ts

Clean migration from client-driven SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. One UX regression worth addressing: pending-send drain no longer sets running=true optimistically, causing a brief stop→send button flicker. Missing test for the critical session_state_changed follow-up after session_switched.

• 🟡 regressions (L356): Removing the optimistic SET_RUNNING true when draining a pending send on session_end creates a brief UI flicker. Flow: SESSION_END sets running=false → pending message sent to server → server eventually emits session_state_changed with running. During that round-trip, the send button replaces the stop button even though a message is in flight. Same issue exists in the store's drainOne timer. The old code bridged this gap with an immediate SET_RUNNING true. Consider dispatching { type: 'SESSION_STATE_CHANGED', state: 'running' } optimistically when draining a queued message, mirroring the optimistic update pattern already used in handleStop. [fixable]

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

Clean migration from client-driven SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. One UX regression worth addressing: pending-send drain no longer sets running=true optimistically, causing a brief stop→send button flicker. Missing test for the critical session_state_changed follow-up after session_switched.

• 🟡 missing_tests (L648): The test 'P1: session_switched does not include running field' only checks transport.sent[0] for the absence of running. It does not verify that a session_state_changed event follows as transport.sent[1], which is the critical new behavior added in handleSwitchSession. Without this assertion, a regression that removes the follow-up event would go undetected. [fixable]

packages/client/__tests__/store.test.ts

Clean migration from client-driven SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. One UX regression worth addressing: pending-send drain no longer sets running=true optimistically, causing a brief stop→send button flicker. Missing test for the critical session_state_changed follow-up after session_switched.

• 🔵 missing_tests (L967): The dispatchMessages test only covers SESSION_STATE_CHANGED with state: 'running' → running: true. There's no corresponding test for state: 'idle' → running: false or state: 'requires_action' → running: false. Since requires_action mapping to running=false is a design decision (permission requests hide the stop button), a test would document this intent and catch accidental regressions if the mapping changes. [fixable]
• 🔵 style (L1299): The comment // P1: syncRunningState tests removed — running state is now server-authoritative is left inside the foreground recovery describe block, which now has only one test. Consider removing the orphaned comment or collapsing the describe block if the remaining test is better grouped elsewhere. [fixable]

server/query-loop.ts

Clean migration from client-driven SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. One UX regression worth addressing: pending-send drain no longer sets running=true optimistically, causing a brief stop→send button flicker. Missing test for the critical session_state_changed follow-up after session_switched.

• 🔵 style (L1396): markSessionInactive() is now dead code — setSessionState('ENDED') atomically syncs is_active via line 633 of event-store.ts. The method definition, its prepared statement, and its tests in event-store.test.ts could be removed to avoid confusion about which path is authoritative. Fine to defer to a cleanup PR. [fixable]

[dimakis]

🟡 regressions: Removing the optimistic SET_RUNNING true when draining a pending send on session_end creates a brief UI flicker. Flow: SESSION_END sets running=false → pending message sent to server → server eventually emits session_state_changed with running. During that round-trip, the send button replaces the stop button even though a message is in flight. Same issue exists in the store's drainOne timer. The old code bridged this gap with an immediate SET_RUNNING true. Consider dispatching { type: 'SESSION_STATE_CHANGED', state: 'running' } optimistically when draining a queued message, mirroring the optimistic update pattern already used in handleStop. [fixable]

[dimakis]

🟡 missing_tests: The test 'P1: session_switched does not include running field' only checks transport.sent[0] for the absence of running. It does not verify that a session_state_changed event follows as transport.sent[1], which is the critical new behavior added in handleSwitchSession. Without this assertion, a regression that removes the follow-up event would go undetected. [fixable]

[dimakis]

🔵 missing_tests: The dispatchMessages test only covers SESSION_STATE_CHANGED with state: 'running' → running: true. There's no corresponding test for state: 'idle' → running: false or state: 'requires_action' → running: false. Since requires_action mapping to running=false is a design decision (permission requests hide the stop button), a test would document this intent and catch accidental regressions if the mapping changes. [fixable]

[dimakis]

🔵 style: The comment // P1: syncRunningState tests removed — running state is now server-authoritative is left inside the foreground recovery describe block, which now has only one test. Consider removing the orphaned comment or collapsing the describe block if the remaining test is better grouped elsewhere. [fixable]

[dimakis]

🔵 style: markSessionInactive() is now dead code — setSessionState('ENDED') atomically syncs is_active via line 633 of event-store.ts. The method definition, its prepared statement, and its tests in event-store.test.ts could be removed to avoid confusion about which path is authoritative. Fine to defer to a cleanup PR. [fixable]

[dimakis]

Centaur Review

Found 6 issue(s) (2 warning).

packages/client/src/protocol-parser.ts

Clean architectural shift from client-inferred to server-authoritative running state, but session_takeover has a regression where the old client never receives the session_state_changed event to clear running state, and the new session_state_changed emission in handleSwitchSession lacks test coverage.

• 🟡 regressions (L132): session_takeover no longer sets running=false. The comment says 'running=false comes from server's session_state_changed event', but the server sends session_takeover to the old transport and then unwatches it (ws-handler-v2.ts:553), so the old client will never receive a subsequent session_state_changed event. The UI will remain in 'running' state with a stop button after takeover, instead of showing the error in an idle state. Consider keeping an inline SESSION_STATE_CHANGED dispatch here, or emitting session_state_changed from the server before unwatching. [fixable]
• 🔵 style (L365): Unnecessary as ClientSessionState cast on a string literal: 'running' as ClientSessionState. The literal 'running' already satisfies the union type. The cast is needed on line 274 (msg.state as ClientSessionState) because it comes from runtime validation, but this one is a compile-time constant. [fixable]
• 🔵 regressions (L423): session_close_ack with accepted=true previously dispatched SET_RUNNING false. Now the comment says 'running=false from server's session_state_changed (CLOSING → idle)'. This is correct only if the server transitions to CLOSING before or concurrently with the ack. Verified: closeSessionByUser calls the agent's closeout flow which should eventually trigger state transitions. However, the 'no active agent' path in closeSessionByUser (chat.ts:1470-1493) calls upsertSession({ isActive: false }) but does NOT call setSessionState('ENDED'), so no session_state_changed event is emitted. This could leave the client in a running state if the agent wasn't active when close was requested. [fixable]
• 🔵 style (L270): The Set of valid states is allocated on every call to parseServerMessage. Since it's a ReadonlySet of constants, hoist it to module scope to avoid repeated allocations in a hot path. [fixable]

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

Clean architectural shift from client-inferred to server-authoritative running state, but session_takeover has a regression where the old client never receives the session_state_changed event to clear running state, and the new session_state_changed emission in handleSwitchSession lacks test coverage.

• 🟡 missing_tests (L621): The test 'P1: session_switched does not include running field' mocks getSessionState('ACTIVE') but only asserts that session_switched lacks a running field. It does not verify that a session_state_changed message is sent as the second transport message — the core new behavior of handleSwitchSession. Add an assertion like: expect(transport.sent[1]).toMatchObject({ type: 'session_state_changed', state: 'running', internalState: 'ACTIVE' }). [fixable]
• 🔵 missing_tests (L621): No test covers handleSwitchSession when getSessionState returns null (session exists in EventStore but has no state yet). The production code guards with if (currentState), but no test verifies that only session_switched is sent (no session_state_changed) in that case. [fixable]

[dimakis]

🟡 regressions: session_takeover no longer sets running=false. The comment says 'running=false comes from server's session_state_changed event', but the server sends session_takeover to the old transport and then unwatches it (ws-handler-v2.ts:553), so the old client will never receive a subsequent session_state_changed event. The UI will remain in 'running' state with a stop button after takeover, instead of showing the error in an idle state. Consider keeping an inline SESSION_STATE_CHANGED dispatch here, or emitting session_state_changed from the server before unwatching. [fixable]

[dimakis]

🔵 style: Unnecessary as ClientSessionState cast on a string literal: 'running' as ClientSessionState. The literal 'running' already satisfies the union type. The cast is needed on line 274 (msg.state as ClientSessionState) because it comes from runtime validation, but this one is a compile-time constant. [fixable]

[dimakis]

🔵 regressions: session_close_ack with accepted=true previously dispatched SET_RUNNING false. Now the comment says 'running=false from server's session_state_changed (CLOSING → idle)'. This is correct only if the server transitions to CLOSING before or concurrently with the ack. Verified: closeSessionByUser calls the agent's closeout flow which should eventually trigger state transitions. However, the 'no active agent' path in closeSessionByUser (chat.ts:1470-1493) calls upsertSession({ isActive: false }) but does NOT call setSessionState('ENDED'), so no session_state_changed event is emitted. This could leave the client in a running state if the agent wasn't active when close was requested. [fixable]

[dimakis]

🔵 style: The Set of valid states is allocated on every call to parseServerMessage. Since it's a ReadonlySet of constants, hoist it to module scope to avoid repeated allocations in a hot path. [fixable]

[dimakis]

🟡 missing_tests: The test 'P1: session_switched does not include running field' mocks getSessionState('ACTIVE') but only asserts that session_switched lacks a running field. It does not verify that a session_state_changed message is sent as the second transport message — the core new behavior of handleSwitchSession. Add an assertion like: expect(transport.sent[1]).toMatchObject({ type: 'session_state_changed', state: 'running', internalState: 'ACTIVE' }). [fixable]

[dimakis]

🔵 missing_tests: No test covers handleSwitchSession when getSessionState returns null (session exists in EventStore but has no state yet). The production code guards with if (currentState), but no test verifies that only session_switched is sent (no session_state_changed) in that case. [fixable]

[dimakis]

Centaur Review

Found 6 issue(s) (1 critical) (3 warning).

server/query-loop.ts

Sound architectural migration from client-inferred to server-authoritative running state, but the is_active column becomes stale for normal query completions due to markSessionInactive removal without syncing is_active in setSessionState — a misleading comment claims otherwise. Two API consumers still read the now-stale field.

• 🔴 bugs (L1399): Comment says 'P1: setSessionState syncs is_active' but setSessionState does NOT update the is_active column — it only updates state, last_state_change, and updated_at (see event-store.ts:190-195). Removing markSessionInactive without either (a) adding is_active sync to setSessionState or (b) migrating ALL is_active readers means the is_active column becomes permanently stale for normal query completions. Two unmigrated readers remain: app.ts:1220 (GET /api/sessions/:id/meta returns isActive: meta.isActive) and chat.ts:1806 (session list filter uses !m.isActive). The meta endpoint is no longer critical since syncRunningState is removed, but the comment is factually wrong and will mislead future maintainers. Either add is_active sync to setSessionState (making the comment true), or fix the comment to say 'is_active is no longer the source of truth; callers use getSessionState() instead'. [fixable]
• 🟡 regressions (L1399): GET /api/sessions/:id/meta (app.ts:1220) still returns isActive from the EventStore's is_active column. After this PR, normally-completed sessions will have is_active=1 forever (markSessionInactive removed) while state='ENDED'. Any consumer of this API (external tools, future features) will see stale isActive=true for completed sessions. Consider either keeping markSessionInactive alongside setSessionState, or having setSessionState also sync is_active when transitioning to ENDED/CLOSING. [fixable]

packages/client/__tests__/protocol-parser.test.ts

Sound architectural migration from client-inferred to server-authoritative running state, but the is_active column becomes stale for normal query completions due to markSessionInactive removal without syncing is_active in setSessionState — a misleading comment claims otherwise. Two API consumers still read the now-stale field.

• 🟡 missing_tests: The session_close_ack handler (protocol-parser.ts:426-435) was changed from SET_RUNNING to SESSION_STATE_CHANGED but has zero test coverage — no test for session_close_ack exists in the protocol parser test suite. This is a safety-net path ('server's no active agent path may not emit session_state_changed') that should be tested. [fixable]
• 🟡 regressions (L623): Five defensive tests for the 'reconnected' handler were deleted: no-ops for undefined currentSessionId, undefined sessions field, non-array sessions, missing required fields, and wrong field types. While the handler no longer processes running state from reconnected, these tests documented runtime validation behavior that guards against malformed server messages. The handler still reads msg.sessions (even if only for replaying events elsewhere), so defensive tests remain valuable. Consider keeping at least the 'no currentSessionId' and 'invalid sessions shape' tests adapted to the new behavior. [fixable]

packages/client/src/protocol-parser.ts

Sound architectural migration from client-inferred to server-authoritative running state, but the is_active column becomes stale for normal query completions due to markSessionInactive removal without syncing is_active in setSessionState — a misleading comment claims otherwise. Two API consumers still read the now-stale field.

• 🔵 style (L128): Temporal 'P1:' prefix comments throughout the code and test names (e.g., 'P1: running state from session_state_changed events', test names like 'P1: no SET_RUNNING') will become meaningless after Phase 1 ships. These should describe the design rationale ('running state derived from session_state_changed events') without referencing the migration phase, since future readers won't know what P1 means. [fixable]

packages/client/src/store.ts

Sound architectural migration from client-inferred to server-authoritative running state, but the is_active column becomes stale for normal query completions due to markSessionInactive removal without syncing is_active in setSessionState — a misleading comment claims otherwise. Two API consumers still read the now-stale field.

• 🔵 unsafe_assumptions (L219): The comment block 'P1: syncRunningState removed — running state is server-authoritative via session_state_changed events. No polling needed.' is reasonable, but there's an implicit assumption that session_state_changed events will always be delivered during foreground recovery. If the WebSocket didn't disconnect during iOS background (just suspended), no reconnect happens, and periodic sync is the only delivery path. Verify that the ConnectionRegistry periodic sync interval is short enough to avoid a stale running indicator on foreground return.

[dimakis]

🔴 bugs: Comment says 'P1: setSessionState syncs is_active' but setSessionState does NOT update the is_active column — it only updates state, last_state_change, and updated_at (see event-store.ts:190-195). Removing markSessionInactive without either (a) adding is_active sync to setSessionState or (b) migrating ALL is_active readers means the is_active column becomes permanently stale for normal query completions. Two unmigrated readers remain: app.ts:1220 (GET /api/sessions/:id/meta returns isActive: meta.isActive) and chat.ts:1806 (session list filter uses !m.isActive). The meta endpoint is no longer critical since syncRunningState is removed, but the comment is factually wrong and will mislead future maintainers. Either add is_active sync to setSessionState (making the comment true), or fix the comment to say 'is_active is no longer the source of truth; callers use getSessionState() instead'. [fixable]

[dimakis]

🟡 regressions: GET /api/sessions/:id/meta (app.ts:1220) still returns isActive from the EventStore's is_active column. After this PR, normally-completed sessions will have is_active=1 forever (markSessionInactive removed) while state='ENDED'. Any consumer of this API (external tools, future features) will see stale isActive=true for completed sessions. Consider either keeping markSessionInactive alongside setSessionState, or having setSessionState also sync is_active when transitioning to ENDED/CLOSING. [fixable]

[dimakis]

🟡 regressions: Five defensive tests for the 'reconnected' handler were deleted: no-ops for undefined currentSessionId, undefined sessions field, non-array sessions, missing required fields, and wrong field types. While the handler no longer processes running state from reconnected, these tests documented runtime validation behavior that guards against malformed server messages. The handler still reads msg.sessions (even if only for replaying events elsewhere), so defensive tests remain valuable. Consider keeping at least the 'no currentSessionId' and 'invalid sessions shape' tests adapted to the new behavior. [fixable]

[dimakis]

🔵 style: Temporal 'P1:' prefix comments throughout the code and test names (e.g., 'P1: running state from session_state_changed events', test names like 'P1: no SET_RUNNING') will become meaningless after Phase 1 ships. These should describe the design rationale ('running state derived from session_state_changed events') without referencing the migration phase, since future readers won't know what P1 means. [fixable]

[dimakis]

🔵 unsafe_assumptions: The comment block 'P1: syncRunningState removed — running state is server-authoritative via session_state_changed events. No polling needed.' is reasonable, but there's an implicit assumption that session_state_changed events will always be delivered during foreground recovery. If the WebSocket didn't disconnect during iOS background (just suspended), no reconnect happens, and periodic sync is the only delivery path. Verify that the ConnectionRegistry periodic sync interval is short enough to avoid a stale running indicator on foreground return.

[dimakis]

Centaur Review

Found 5 issue(s).

packages/client/src/protocol-parser.ts

Clean, well-structured migration from client-managed SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. All SET_RUNNING references properly replaced, test coverage is comprehensive, and the server-side state delivery (handleSwitchSession emitting session_state_changed, handleReconnect using getSessionState for stale cleanup, periodic sync for iOS gaps) is correct. Only minor robustness and style improvements suggested.

• 🔵 unsafe_assumptions (L278): Non-string msg.state (undefined, null, number) is silently dropped without a warning. The else if only catches unknown string values. Adding an else { console.warn(...) } for non-string types would help diagnose serialization bugs or protocol mismatches that could otherwise be invisible. [fixable]
• 🔵 style (L165): Comment says 'Running state restored via replayed session_state_changed events' but handleSwitchSession emits a fresh session_state_changed event immediately after session_switched — it is not a replay from the event store. Consider 'Running state delivered via a follow-up session_state_changed event' for accuracy. [fixable]
• 🔵 unsafe_assumptions (L105): VALID_CLIENT_STATES is defined as a manual Set(['idle', 'running', 'requires_action']) that must stay in sync with the ClientSessionState type. If a new state is added to the type, this set must also be updated manually. Consider deriving the set from a const array that also drives the type, e.g., const CLIENT_STATES = ['idle', 'running', 'requires_action'] as const; type ClientSessionState = typeof CLIENT_STATES[number]; [fixable]

packages/client/__tests__/protocol-parser.test.ts

Clean, well-structured migration from client-managed SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. All SET_RUNNING references properly replaced, test coverage is comprehensive, and the server-side state delivery (handleSwitchSession emitting session_state_changed, handleReconnect using getSessionState for stale cleanup, periodic sync for iOS gaps) is correct. Only minor robustness and style improvements suggested.

• 🔵 missing_tests: The new session_close_ack tests only cover accepted: true and accepted: false. Missing: test for when session_close_ack arrives with accepted: false and a reason field — the current handler silently ignores reason, so the user gets no feedback on why their close request was rejected. [fixable]

frontend/src/pages/ChatView.tsx

Clean, well-structured migration from client-managed SET_RUNNING to server-authoritative SESSION_STATE_CHANGED. All SET_RUNNING references properly replaced, test coverage is comprehensive, and the server-side state delivery (handleSwitchSession emitting session_state_changed, handleReconnect using getSessionState for stale cleanup, periodic sync for iOS gaps) is correct. Only minor robustness and style improvements suggested.

• 🔵 style (L188): The handleStop callback (with its optimistic update comment) is duplicated identically in both ChatView.tsx and DesktopChatView.tsx. This is pre-existing duplication, but since both files are modified in this PR, it may be worth extracting to a shared hook or utility. [fixable]

[dimakis]

🔵 unsafe_assumptions: Non-string msg.state (undefined, null, number) is silently dropped without a warning. The else if only catches unknown string values. Adding an else { console.warn(...) } for non-string types would help diagnose serialization bugs or protocol mismatches that could otherwise be invisible. [fixable]

[dimakis]

🔵 style: Comment says 'Running state restored via replayed session_state_changed events' but handleSwitchSession emits a fresh session_state_changed event immediately after session_switched — it is not a replay from the event store. Consider 'Running state delivered via a follow-up session_state_changed event' for accuracy. [fixable]

[dimakis]

🔵 unsafe_assumptions: VALID_CLIENT_STATES is defined as a manual Set(['idle', 'running', 'requires_action']) that must stay in sync with the ClientSessionState type. If a new state is added to the type, this set must also be updated manually. Consider deriving the set from a const array that also drives the type, e.g., const CLIENT_STATES = ['idle', 'running', 'requires_action'] as const; type ClientSessionState = typeof CLIENT_STATES[number]; [fixable]

[dimakis]

🔵 style: The handleStop callback (with its optimistic update comment) is duplicated identically in both ChatView.tsx and DesktopChatView.tsx. This is pre-existing duplication, but since both files are modified in this PR, it may be worth extracting to a shared hook or utility. [fixable]