feat(transport): transport SSOT phase 0 foundation - #431
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 8 issue(s) (3 warning).
packages/protocol/src/event-store.ts
Solid P0 foundation — the SSE default flip, crash recovery, and state event plumbing are correct in the happy path. Main concerns: CLOSING state omitted from crash recovery (potential zombie sessions), inline db.prepare() on every state transition (perf), and no tests for the three new behaviors (recoverStaleSessions, toClientState, event emission).
- 🟡 bugs (L664): recoverStaleSessions() omits CLOSING state. After a crash, sessions left in CLOSING will never reach ENDED — the process performing the graceful shutdown is gone. These sessions become zombies with is_active=0 but state≠ENDED, which may confuse downstream code that checks state vs. is_active.
[fixable] - 🟡 unsafe_assumptions (L630): The is_active sync in setSessionState() creates a new prepared statement via this.db!.prepare() on every call instead of using a pre-compiled statement (like the existing stmts.markInactive). On hot paths with frequent state transitions this re-parses the SQL each time. Should be added to the stmts object in init().
[fixable] - 🔵 style (L23): toClientState() never returns 'requires_action', making it a dead value in the ClientSessionState union. The function is module-private so callers can't extend it. Consider either removing 'requires_action' from the type until Phase 1 adds it, or adding a comment to the function noting the intentional gap — otherwise readers will wonder if it's a bug.
[fixable] - 🟡 missing_tests (L663): recoverStaleSessions() has no test coverage. The existing event-store.test.ts has comprehensive state machine tests — this method should be tested there: verify it transitions ACTIVE/STARTING/DETACHED/SUSPENDED to ENDED, returns the correct count, emits session_state_changed events, and is idempotent when no stale sessions exist.
[fixable] - 🔵 missing_tests (L23): toClientState() has no unit tests. It maps all 7 internal states to 3 client states — this mapping is a key contract for the protocol and should be verified exhaustively (one assertion per SessionState value).
[fixable] - 🔵 missing_tests (L634): The session_state_changed event emission inside setSessionState() is untested. Existing state machine tests don't verify that append() is called with the correct event payload. At minimum, verify the event is stored and contains the expected clientState mapping.
[fixable] - 🔵 regressions (L628): The is_active sync sets DETACHED and SUSPENDED sessions to is_active=1, but the previous code path (markSessionInactive called from query-loop.ts) may have set is_active=0 for detached sessions. If any callers still use markSessionInactive() independently of setSessionState(), the two can get out of sync — is_active becomes a dual-writer field. Verify that all state changes go through setSessionState() now.
packages/client/src/protocol-parser.ts
Solid P0 foundation — the SSE default flip, crash recovery, and state event plumbing are correct in the happy path. Main concerns: CLOSING state omitted from crash recovery (potential zombie sessions), inline db.prepare() on every state transition (perf), and no tests for the three new behaviors (recoverStaleSessions, toClientState, event emission).
- 🔵 style (L295): The session_state_changed case is a silent no-op with a comment referencing 'Phase 1'. Consider logging via console.debug or a callbacks hook so the event is observable during development, and to validate the message is actually received.
[fixable]
| * Returns the number of sessions recovered. | ||
| */ | ||
| recoverStaleSessions(): number { | ||
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED']; |
There was a problem hiding this comment.
🟡 bugs: recoverStaleSessions() omits CLOSING state. After a crash, sessions left in CLOSING will never reach ENDED — the process performing the graceful shutdown is gone. These sessions become zombies with is_active=0 but state≠ENDED, which may confuse downstream code that checks state vs. is_active. [fixable]
|
|
||
| // Backwards-compatible: sync is_active from state (P0) | ||
| const isActive = newState !== 'ENDED' && newState !== 'CLOSING' ? 1 : 0; | ||
| this.db!.prepare( |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The is_active sync in setSessionState() creates a new prepared statement via this.db!.prepare() on every call instead of using a pre-compiled statement (like the existing stmts.markInactive). On hot paths with frequent state transitions this re-parses the SQL each time. Should be added to the stmts object in init(). [fixable]
| }; | ||
|
|
||
| /** Map internal 7-state lifecycle to client-facing 3-state. */ | ||
| function toClientState(state: SessionState): ClientSessionState { |
There was a problem hiding this comment.
🔵 style: toClientState() never returns 'requires_action', making it a dead value in the ClientSessionState union. The function is module-private so callers can't extend it. Consider either removing 'requires_action' from the type until Phase 1 adds it, or adding a comment to the function noting the intentional gap — otherwise readers will wonder if it's a bug. [fixable]
| * Any session in ACTIVE, STARTING, DETACHED, or SUSPENDED is transitioned to ENDED. | ||
| * Returns the number of sessions recovered. | ||
| */ | ||
| recoverStaleSessions(): number { |
There was a problem hiding this comment.
🟡 missing_tests: recoverStaleSessions() has no test coverage. The existing event-store.test.ts has comprehensive state machine tests — this method should be tested there: verify it transitions ACTIVE/STARTING/DETACHED/SUSPENDED to ENDED, returns the correct count, emits session_state_changed events, and is idempotent when no stale sessions exist. [fixable]
| }; | ||
|
|
||
| /** Map internal 7-state lifecycle to client-facing 3-state. */ | ||
| function toClientState(state: SessionState): ClientSessionState { |
There was a problem hiding this comment.
🔵 missing_tests: toClientState() has no unit tests. It maps all 7 internal states to 3 client states — this mapping is a key contract for the protocol and should be verified exhaustively (one assertion per SessionState value). [fixable]
| "UPDATE sessions SET is_active = ?, updated_at = unixepoch('now', 'subsec') * 1000 WHERE session_id = ?", | ||
| ).run(isActive, sessionId); | ||
|
|
||
| // Emit session_state_changed event for client consumption (P0) |
There was a problem hiding this comment.
🔵 missing_tests: The session_state_changed event emission inside setSessionState() is untested. Existing state machine tests don't verify that append() is called with the correct event payload. At minimum, verify the event is stored and contains the expected clientState mapping. [fixable]
|
|
||
| this.stmts.setSessionState.run(newState, now, sessionId); | ||
|
|
||
| // Backwards-compatible: sync is_active from state (P0) |
There was a problem hiding this comment.
🔵 regressions: The is_active sync sets DETACHED and SUSPENDED sessions to is_active=1, but the previous code path (markSessionInactive called from query-loop.ts) may have set is_active=0 for detached sessions. If any callers still use markSessionInactive() independently of setSessionState(), the two can get out of sync — is_active becomes a dual-writer field. Verify that all state changes go through setSessionState() now.
| callbacks.onSessionRenamed?.(msg.name as string); | ||
| break; | ||
|
|
||
| case 'session_state_changed': |
There was a problem hiding this comment.
🔵 style: The session_state_changed case is a silent no-op with a comment referencing 'Phase 1'. Consider logging via console.debug or a callbacks hook so the event is observable during development, and to validate the message is actually received. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
server/index.ts
Solid P0 foundation: state machine, crash recovery, and SSE default are well-structured with good test coverage on the EventStore side. Main concerns are the SSE default flip being a silent behavioral change for existing clients, missing protocol-parser test for the new event type, and minor style nits (duplicate logging, unnecessary console guard).
- 🔵 style (L1030): Duplicate log:
recoverStaleSessions()already logs 'recovered stale sessions on startup' with session IDs (event-store.ts:682), and each individual transition is also logged bysetSessionState(event-store.ts:646). Thelog.infoat this call site adds a third, redundant log line. Consider removing it or at minimum removing the EventStore-internal one, since callers have better context.[fixable]
packages/client/src/protocol-parser.ts
Solid P0 foundation: state machine, crash recovery, and SSE default are well-structured with good test coverage on the EventStore side. Main concerns are the SSE default flip being a silent behavioral change for existing clients, missing protocol-parser test for the new event type, and minor style nits (duplicate logging, unnecessary console guard).
- 🟡 missing_tests (L295): The new
session_state_changedcase inparseServerMessagehas no test coverage in the protocol-parser test suite. Even though it's currently console-only, a test verifying it doesn't throw (and doesn't produce spurious actions) would prevent regressions when Phase 1 adds real behavior.[fixable] - 🔵 style (L297): The
if (typeof console !== 'undefined')guard is unnecessary —consoleis defined in every environment where this code runs (browsers, Node, React Native). All otherconsole.warncalls in this file (e.g., lines 168, 338, 468) are unguarded.[fixable]
packages/protocol/src/event-store.ts
Solid P0 foundation: state machine, crash recovery, and SSE default are well-structured with good test coverage on the EventStore side. Main concerns are the SSE default flip being a silent behavioral change for existing clients, missing protocol-parser test for the new event type, and minor style nits (duplicate logging, unnecessary console guard).
- 🔵 unsafe_assumptions (L670): Minor:
recoverStaleSessionscreates a new prepared statement on every call viathis.db!.prepare(...). Since this only runs once at startup it's not a performance concern, but it's inconsistent with the rest of the file which pre-prepares all statements in the constructor. If this method is ever called more frequently (e.g., periodic health checks), cache the statement.[fixable] - 🔵 style (L27):
toClientStateis a module-private function with no direct unit tests. It's covered indirectly throughsetSessionStateintegration tests, which is acceptable for P0, but consider exporting it (or a test-only alias) so the mapping table can be tested in isolation — especially once Phase 1 addsrequires_action.[fixable]
frontend/src/client-store.ts
Solid P0 foundation: state machine, crash recovery, and SSE default are well-structured with good test coverage on the EventStore side. Main concerns are the SSE default flip being a silent behavioral change for existing clients, missing protocol-parser test for the new event type, and minor style nits (duplicate logging, unnecessary console guard).
- 🟡 regressions (L26): Changing the default transport from WS to SSE is a user-visible behavioral change. Any client with no
mitzo:transportlocalStorage key will silently switch from WebSocket to SSE on upgrade. If SSE has gaps (e.g., the parser only logssession_state_changedrather than acting on it), this could regress existing users. Ensure the SSE path has been validated end-to-end before merging, or gate behind a more explicit opt-in.
| // Recover sessions left in incomplete states after crash/restart (Transport SSOT P0) | ||
| const recovered = eventStore.recoverStaleSessions(); | ||
| if (recovered > 0) { | ||
| log.info(`recovered ${recovered} stale session(s) on startup`); |
There was a problem hiding this comment.
🔵 style: Duplicate log: recoverStaleSessions() already logs 'recovered stale sessions on startup' with session IDs (event-store.ts:682), and each individual transition is also logged by setSessionState (event-store.ts:646). The log.info at this call site adds a third, redundant log line. Consider removing it or at minimum removing the EventStore-internal one, since callers have better context. [fixable]
| callbacks.onSessionRenamed?.(msg.name as string); | ||
| break; | ||
|
|
||
| case 'session_state_changed': |
There was a problem hiding this comment.
🟡 missing_tests: The new session_state_changed case in parseServerMessage has no test coverage in the protocol-parser test suite. Even though it's currently console-only, a test verifying it doesn't throw (and doesn't produce spurious actions) would prevent regressions when Phase 1 adds real behavior. [fixable]
|
|
||
| case 'session_state_changed': | ||
| // P0: log for observability, no UI action yet (Phase 1 will bind to running state) | ||
| if (typeof console !== 'undefined') { |
There was a problem hiding this comment.
🔵 style: The if (typeof console !== 'undefined') guard is unnecessary — console is defined in every environment where this code runs (browsers, Node, React Native). All other console.warn calls in this file (e.g., lines 168, 338, 468) are unguarded. [fixable]
| recoverStaleSessions(): number { | ||
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED', 'CLOSING']; | ||
| const placeholders = staleStates.map(() => '?').join(', '); | ||
| const rows = this.db!.prepare( |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Minor: recoverStaleSessions creates a new prepared statement on every call via this.db!.prepare(...). Since this only runs once at startup it's not a performance concern, but it's inconsistent with the rest of the file which pre-prepares all statements in the constructor. If this method is ever called more frequently (e.g., periodic health checks), cache the statement. [fixable]
| * Note: 'requires_action' is never returned here — it is emitted separately | ||
| * by the permission_request handler (Phase 1), not from lifecycle transitions. | ||
| */ | ||
| function toClientState(state: SessionState): ClientSessionState { |
There was a problem hiding this comment.
🔵 style: toClientState is a module-private function with no direct unit tests. It's covered indirectly through setSessionState integration tests, which is acceptable for P0, but consider exporting it (or a test-only alias) so the mapping table can be tested in isolation — especially once Phase 1 adds requires_action. [fixable]
| * Revert SSE: localStorage.removeItem('mitzo:transport'); location.reload(); | ||
| */ | ||
| const useSSE = typeof window !== 'undefined' && localStorage.getItem('mitzo:transport') === 'sse'; | ||
| const useSSE = typeof window !== 'undefined' && localStorage.getItem('mitzo:transport') !== 'ws'; |
There was a problem hiding this comment.
🟡 regressions: Changing the default transport from WS to SSE is a user-visible behavioral change. Any client with no mitzo:transport localStorage key will silently switch from WebSocket to SSE on upgrade. If SSE has gaps (e.g., the parser only logs session_state_changed rather than acting on it), this could regress existing users. Ensure the SSE path has been validated end-to-end before merging, or gate behind a more explicit opt-in.
Centaur ReviewFound 6 issue(s) (1 critical) (2 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
frontend/src/types/ws-messages.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. Main concern is DETACHED mapping to 'idle' while is_active=true, creating a semantic mismatch that should be documented or resolved in P1.
- 🟡 regressions: SessionStateEvent (or a local SessionStateChangedMsg interface) is not added to the ServerMessage union. The protocol parser handles it, so it won't crash, and this matches the existing pattern (e.g., boot_context is also missing from the union). However, if any consumer ever narrows on ServerMessage['type'], 'session_state_changed' won't be a valid discriminant. Worth adding to the union for Phase 1 when UI binds to this event.
[fixable]
packages/protocol/src/event-store.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. Main concern is DETACHED mapping to 'idle' while is_active=true, creating a semantic mismatch that should be documented or resolved in P1.
- 🟡 bugs: toClientState maps DETACHED and SUSPENDED to 'idle', but the comment says 'preserve last emitted state' and acknowledges the EventStore can't do this. In practice this means a session that is DETACHED (agent still alive, just disconnected from WS) will emit state='idle' to the client — potentially misleading since the agent is still running. The is_active sync correctly keeps is_active=1 for DETACHED, creating a mismatch: is_active=true but client state='idle'. Consider returning 'running' for DETACHED to match the semantic that the generation is still alive, or document this as a known P0 limitation that P1 resolves.
[fixable] - 🔵 unsafe_assumptions: recoverStaleSessions() uses an inline db.prepare() with string interpolation of placeholders. This is safe because the placeholders are '?' joined from a fixed-length array, but a future maintainer adding a state with special chars to staleStates could be surprised. The comment 'runs once at startup, not worth caching' is a good callout — no action needed, just noting for awareness.
- 🔵 style: toClientState is a module-level function but SessionStateEvent (defined in types.ts) is not imported or referenced by the function signature — it returns ClientSessionState. The function's return type is inferred. Adding an explicit return type annotation
function toClientState(state: SessionState): ClientSessionStatewould make the contract clearer. (It already has this — just confirming, no issue.)
packages/protocol/__tests__/event-store.test.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. Main concern is DETACHED mapping to 'idle' while is_active=true, creating a semantic mismatch that should be documented or resolved in P1.
- 🔵 missing_tests: recoverStaleSessions tests don't verify that is_active is synced to 0 for recovered sessions. The setSessionState path does set is_active=0 for ENDED, but an explicit assertion like
expect(store.getSession('active-1')!.isActive).toBe(false)after recovery would document this guarantee.[fixable]
frontend/src/client-store.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. Main concern is DETACHED mapping to 'idle' while is_active=true, creating a semantic mismatch that should be documented or resolved in P1.
- 🔵 missing_tests: The SSE-default flip (localStorage !== 'ws' instead of === 'sse') is a behavioral change affecting all clients. There are no client-side tests for the transport selector logic. If a user had previously set localStorage to 'sse' explicitly, that value is now ignored (they'd get SSE anyway, which is fine) — but there's no test documenting the new default or the fallback behavior.
[fixable]
Centaur ReviewFound 6 issue(s) (2 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (1 warning).
packages/protocol/src/event-store.ts
Well-structured P0 foundation. No bugs or critical issues — the state machine, crash recovery, SSE default flip, and event emission are all correctly implemented with thorough test coverage. Findings are minor style/documentation improvements.
- 🔵 style (L36): Comment says 'DETACHED/SUSPENDED: preserve last emitted state' but the code actually always returns 'idle' — it does not preserve anything. The comment describes aspirational P1 behavior, not what the code does. Suggest rewording to 'DETACHED/SUSPENDED: return idle as a safe default. Phase 1 will preserve last emitted state.'
[fixable] - 🟡 regressions (L634): is_active is now set to 1 for CREATED state (via the
newState !== 'ENDED' && newState !== 'CLOSING'check). This is consistent with the schema default (DEFAULT 1) so it's not a regression today. However, CREATED means the session hasn't started the query loop — if any consumer relies on is_active=true meaning 'the agent is running', this could be misleading. The existing markSessionInactive() in query-loop.ts (line 1401) is called right before setSessionState(ENDED) at line 1402, causing a redundant double-write. The PR comment acknowledges this P1 migration, which is good. - 🔵 unsafe_assumptions (L671): recoverStaleSessions() calls this.db!.prepare() with a non-null assertion. If recoverStaleSessions() were ever called after close(), db would be null and this would throw. Currently safe because it's only called at startup in server/index.ts, but a guard (or asserting this.db in the method) would be more defensive.
[fixable]
packages/protocol/__tests__/event-store.test.ts
Well-structured P0 foundation. No bugs or critical issues — the state machine, crash recovery, SSE default flip, and event emission are all correctly implemented with thorough test coverage. Findings are minor style/documentation improvements.
- 🔵 missing_tests: No test verifies that setSessionState on a non-existent session handles gracefully (getSession returns null, fromState is null). The code handles it via the
if (fromState && !opts?.force)guard, but an explicit test would document the contract for crash-recovery scenarios where the session row might not exist.[fixable]
packages/protocol/src/types.ts
Well-structured P0 foundation. No bugs or critical issues — the state machine, crash recovery, SSE default flip, and event emission are all correctly implemented with thorough test coverage. Findings are minor style/documentation improvements.
- 🔵 style (L217): SessionStateEvent interface is defined but never imported or used anywhere in the codebase. If it's scaffolding for Phase 1, consider adding a brief comment (e.g., '// Used by Phase 1 UI binding') or deferring the definition to the PR that consumes it, to avoid dead code.
[fixable]
| case 'CLOSING': | ||
| case 'ENDED': | ||
| return 'idle'; | ||
| // DETACHED/SUSPENDED: preserve last emitted state. |
There was a problem hiding this comment.
🔵 style: Comment says 'DETACHED/SUSPENDED: preserve last emitted state' but the code actually always returns 'idle' — it does not preserve anything. The comment describes aspirational P1 behavior, not what the code does. Suggest rewording to 'DETACHED/SUSPENDED: return idle as a safe default. Phase 1 will preserve last emitted state.' [fixable]
| // Sync is_active from state in the same UPDATE (backwards-compatible, P0). | ||
| // DETACHED/SUSPENDED keep is_active=1 because the generation is still alive server-side. | ||
| // markSessionInactive() callers should migrate to setSessionState(ENDED) in P1. | ||
| const isActive = newState !== 'ENDED' && newState !== 'CLOSING' ? 1 : 0; |
There was a problem hiding this comment.
🟡 regressions: is_active is now set to 1 for CREATED state (via the newState !== 'ENDED' && newState !== 'CLOSING' check). This is consistent with the schema default (DEFAULT 1) so it's not a regression today. However, CREATED means the session hasn't started the query loop — if any consumer relies on is_active=true meaning 'the agent is running', this could be misleading. The existing markSessionInactive() in query-loop.ts (line 1401) is called right before setSessionState(ENDED) at line 1402, causing a redundant double-write. The PR comment acknowledges this P1 migration, which is good.
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED', 'CLOSING']; | ||
| const placeholders = staleStates.map(() => '?').join(', '); | ||
| // Inline prepare is intentional — this runs once at startup, not worth caching. | ||
| const rows = this.db!.prepare( |
There was a problem hiding this comment.
🔵 unsafe_assumptions: recoverStaleSessions() calls this.db!.prepare() with a non-null assertion. If recoverStaleSessions() were ever called after close(), db would be null and this would throw. Currently safe because it's only called at startup in server/index.ts, but a guard (or asserting this.db in the method) would be more defensive. [fixable]
| */ | ||
| export type ClientSessionState = 'idle' | 'running' | 'requires_action'; | ||
|
|
||
| /** Server-authoritative state event emitted on every lifecycle transition. */ |
There was a problem hiding this comment.
🔵 style: SessionStateEvent interface is defined but never imported or used anywhere in the codebase. If it's scaffolding for Phase 1, consider adding a brief comment (e.g., '// Used by Phase 1 UI binding') or deferring the definition to the PR that consumes it, to avoid dead code. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
frontend/src/client-store.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. The main concern is the default transport flip from WS to SSE, which is a silent behavioral change for all existing clients.
- 🟡 regressions (L27): Flipping the default transport from WS to SSE is a behavioral change that affects all clients on upgrade. Users with
mitzo:transportunset in localStorage (the common case) will silently switch from WebSocket to SSE. If the SSE transport has any feature gaps or bugs, this becomes a production regression with no server-side rollback — each client must manually setlocalStorage('mitzo:transport', 'ws'). Consider whether the SSE path has been validated at parity with WS before making it the default, or gate this behind a more visible opt-in (e.g., server-sent feature flag).
packages/protocol/src/event-store.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. The main concern is the default transport flip from WS to SSE, which is a silent behavioral change for all existing clients.
- 🟡 bugs (L634): The
is_activederivation treatsCREATEDas active (is_active=1), butrecoverStaleSessionsdoes NOT recoverCREATEDsessions. This is internally consistent (CREATED is not stale), but creates a subtle invariant: a session in CREATED state will appear active forever ifstartChatnever runs. If this is intentional (CREATED is a transient state that doesn't persist long), it's fine — but worth documenting why CREATED is excluded from recovery yet keptis_active=1.[fixable] - 🔵 style (L671):
this.db!non-null assertion inrecoverStaleSessions()— ifclose()has been called before startup completes (unlikely but possible in tests or error paths), this will throw an unhandled error. Other methods in this class use the cachedthis.stmtswhich would also fail, so this is consistent with the existing pattern, but a guard-and-return-0 would be more defensive for a startup-only method.[fixable] - 🔵 style (L668): The
staleStatesarray duplicates the set of states that map tois_active=1minusCREATED. Consider deriving it from the same source as theisActivelogic (line 634) to keep them in sync, or add a comment cross-referencing the two. If a new state is added toSessionStatein the future, both locations need updating independently.[fixable]
packages/protocol/__tests__/event-store.test.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. The main concern is the default transport flip from WS to SSE, which is a silent behavioral change for all existing clients.
- 🔵 missing_tests: The
setSessionState syncs is_activetests cover ACTIVE, DETACHED, SUSPENDED, ENDED, and CLOSING — but not CREATED and STARTING. Adding assertions for these two states would complete the matrix and guard against regressions in theisActivederivation logic.[fixable]
server/index.ts
Solid P0 foundation — state machine, crash recovery, and event emission are well-tested and correctly implemented. The main concern is the default transport flip from WS to SSE, which is a silent behavioral change for all existing clients.
- 🔵 unsafe_assumptions (L1028):
recoverStaleSessions()runs synchronously in theserver.listencallback, which is fine for SQLite. But it runs beforereconcileSessionsBackground()— if reconciliation also touches session state, the ordering dependency should be documented (recovery must happen first so reconciliation sees ENDED states).[fixable]
| * Revert SSE: localStorage.removeItem('mitzo:transport'); location.reload(); | ||
| */ | ||
| const useSSE = typeof window !== 'undefined' && localStorage.getItem('mitzo:transport') === 'sse'; | ||
| const useSSE = typeof window !== 'undefined' && localStorage.getItem('mitzo:transport') !== 'ws'; |
There was a problem hiding this comment.
🟡 regressions: Flipping the default transport from WS to SSE is a behavioral change that affects all clients on upgrade. Users with mitzo:transport unset in localStorage (the common case) will silently switch from WebSocket to SSE. If the SSE transport has any feature gaps or bugs, this becomes a production regression with no server-side rollback — each client must manually set localStorage('mitzo:transport', 'ws'). Consider whether the SSE path has been validated at parity with WS before making it the default, or gate this behind a more visible opt-in (e.g., server-sent feature flag).
| // Sync is_active from state in the same UPDATE (backwards-compatible, P0). | ||
| // DETACHED/SUSPENDED keep is_active=1 because the generation is still alive server-side. | ||
| // markSessionInactive() callers should migrate to setSessionState(ENDED) in P1. | ||
| const isActive = newState !== 'ENDED' && newState !== 'CLOSING' ? 1 : 0; |
There was a problem hiding this comment.
🟡 bugs: The is_active derivation treats CREATED as active (is_active=1), but recoverStaleSessions does NOT recover CREATED sessions. This is internally consistent (CREATED is not stale), but creates a subtle invariant: a session in CREATED state will appear active forever if startChat never runs. If this is intentional (CREATED is a transient state that doesn't persist long), it's fine — but worth documenting why CREATED is excluded from recovery yet kept is_active=1. [fixable]
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED', 'CLOSING']; | ||
| const placeholders = staleStates.map(() => '?').join(', '); | ||
| // Inline prepare is intentional — this runs once at startup, not worth caching. | ||
| const rows = this.db!.prepare( |
There was a problem hiding this comment.
🔵 style: this.db! non-null assertion in recoverStaleSessions() — if close() has been called before startup completes (unlikely but possible in tests or error paths), this will throw an unhandled error. Other methods in this class use the cached this.stmts which would also fail, so this is consistent with the existing pattern, but a guard-and-return-0 would be more defensive for a startup-only method. [fixable]
| * Returns the number of sessions recovered. | ||
| */ | ||
| recoverStaleSessions(): number { | ||
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED', 'CLOSING']; |
There was a problem hiding this comment.
🔵 style: The staleStates array duplicates the set of states that map to is_active=1 minus CREATED. Consider deriving it from the same source as the isActive logic (line 634) to keep them in sync, or add a comment cross-referencing the two. If a new state is added to SessionState in the future, both locations need updating independently. [fixable]
| connRegistry.startPeriodicSync(); | ||
|
|
||
| // Recover sessions left in incomplete states after crash/restart (Transport SSOT P0). | ||
| // recoverStaleSessions() logs internally — no need to log here. |
There was a problem hiding this comment.
🔵 unsafe_assumptions: recoverStaleSessions() runs synchronously in the server.listen callback, which is fine for SQLite. But it runs before reconcileSessionsBackground() — if reconciliation also touches session state, the ordering dependency should be documented (recovery must happen first so reconciliation sees ENDED states). [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
packages/protocol/src/event-store.ts
Solid P0 foundation — correct state machine mapping, good test coverage, clean crash recovery. Main concern is the missing exhaustiveness guard in toClientState which could cause a silent undefined return if a new state is added.
- 🔵 style (L36): Comment says "preserve last emitted state" but the code returns a hardcoded 'idle'. The comment describes aspirational P1 behavior, not what the code does. Consider rewriting to: "DETACHED/SUSPENDED: return 'idle' as a safe default. The server layer can override if needed." (drop the misleading "preserve last emitted" line).
[fixable] - 🟡 regressions (L18): ClientSessionState is re-exported from event-store.ts (the server-side entry point '@mitzo/protocol/event-store') but not from the main package index (packages/protocol/src/index.ts). Frontend consumers importing from '@mitzo/protocol' won't have access to this type. This will become a problem when Phase 1 needs the type in frontend code.
[fixable] - 🟡 bugs (L27): toClientState lacks a default case, so TypeScript won't catch a missing branch if a new SessionState variant is added. The return type is ClientSessionState but a new unmatched variant would return undefined at runtime. Add
default: { const _exhaustive: never = state; return _exhaustive; }for compile-time exhaustiveness checking.[fixable] - 🔵 style (L631): The 6-line comment block above
const isActive = ...is heavy for what is a straightforward boolean derivation. The migration note about markSessionInactive() is useful but could be a single line; the CREATED explanation is better placed near recoverStaleSessions where it actually matters.[fixable]
packages/protocol/__tests__/event-store.test.ts
Solid P0 foundation — correct state machine mapping, good test coverage, clean crash recovery. Main concern is the missing exhaustiveness guard in toClientState which could cause a silent undefined return if a new state is added.
- 🔵 missing_tests: No test for the toClientState exhaustiveness guarantee. If a new SessionState variant is added to the union, the switch in toClientState will silently return undefined (no default case). Consider adding a test that asserts toClientState covers all SessionState values, or adding an exhaustive check (e.g.,
default: state satisfies never).[fixable]
packages/protocol/src/types.ts
Solid P0 foundation — correct state machine mapping, good test coverage, clean crash recovery. Main concern is the missing exhaustiveness guard in toClientState which could cause a silent undefined return if a new state is added.
- 🔵 style (L218): SessionStateEvent interface is defined and exported from types.ts but never imported or used anywhere in the codebase. If it's meant to be a documented contract for Phase 1 consumers, consider exporting it from the package index. Otherwise it's dead code.
[fixable]
| case 'CLOSING': | ||
| case 'ENDED': | ||
| return 'idle'; | ||
| // DETACHED/SUSPENDED: preserve last emitted state. |
There was a problem hiding this comment.
🔵 style: Comment says "preserve last emitted state" but the code returns a hardcoded 'idle'. The comment describes aspirational P1 behavior, not what the code does. Consider rewriting to: "DETACHED/SUSPENDED: return 'idle' as a safe default. The server layer can override if needed." (drop the misleading "preserve last emitted" line). [fixable]
| SessionMeta, | ||
| SessionSearchResult, | ||
| SessionState, | ||
| ClientSessionState, |
There was a problem hiding this comment.
🟡 regressions: ClientSessionState is re-exported from event-store.ts (the server-side entry point '@mitzo/protocol/event-store') but not from the main package index (packages/protocol/src/index.ts). Frontend consumers importing from '@mitzo/protocol' won't have access to this type. This will become a problem when Phase 1 needs the type in frontend code. [fixable]
| * Note: 'requires_action' is never returned here — it is emitted separately | ||
| * by the permission_request handler (Phase 1), not from lifecycle transitions. | ||
| */ | ||
| function toClientState(state: SessionState): ClientSessionState { |
There was a problem hiding this comment.
🟡 bugs: toClientState lacks a default case, so TypeScript won't catch a missing branch if a new SessionState variant is added. The return type is ClientSessionState but a new unmatched variant would return undefined at runtime. Add default: { const _exhaustive: never = state; return _exhaustive; } for compile-time exhaustiveness checking. [fixable]
| } | ||
|
|
||
| this.stmts.setSessionState.run(newState, now, sessionId); | ||
| // Sync is_active from state in the same UPDATE (backwards-compatible, P0). |
There was a problem hiding this comment.
🔵 style: The 6-line comment block above const isActive = ... is heavy for what is a straightforward boolean derivation. The migration note about markSessionInactive() is useful but could be a single line; the CREATED explanation is better placed near recoverStaleSessions where it actually matters. [fixable]
| export type ClientSessionState = 'idle' | 'running' | 'requires_action'; | ||
|
|
||
| /** Server-authoritative state event emitted on every lifecycle transition. */ | ||
| export interface SessionStateEvent { |
There was a problem hiding this comment.
🔵 style: SessionStateEvent interface is defined and exported from types.ts but never imported or used anywhere in the codebase. If it's meant to be a documented contract for Phase 1 consumers, consider exporting it from the package index. Otherwise it's dead code. [fixable]
Centaur ReviewFound 5 issue(s) (1 warning).
|
Centaur ReviewFound 5 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (2 warning).
packages/protocol/src/event-store.ts
Solid P0 foundation — the core state machine, crash recovery, and event emission are well-tested. Main concern is the dual is_active mutation path (markSessionInactive vs setSessionState) which should be consolidated before Phase 1 to make state the true SSOT.
- 🟡 regressions (L636):
markSessionInactive()(line 574) still directly setsis_active=0without updatingstateor emittingsession_state_changed. Now thatsetSessionState()derivesis_activefrom state, there are two divergent mutation paths. Inquery-loop.ts:1401both are called sequentially (redundant but safe), but any future caller ofmarkSessionInactivealone will leavestateandis_activeinconsistent — e.g.,state=ACTIVEwithis_active=0. Consider deprecatingmarkSessionInactivein favor ofsetSessionState('ENDED')to make state the single source of truth.[fixable] - 🟡 bugs (L670):
recoverStaleSessions()excludes CREATED, butsetSessionStatesetsis_active=1for CREATED (line 636). If the server crashes betweensetSessionState('CREATED')(chat.ts:870) andsetSessionState('STARTING'), the session is stuck as CREATED+active forever — no recovery path cleans it up. The window is small but real. Consider including CREATED in recovery, or at least adding a comment explaining why an orphaned CREATED is acceptable.[fixable] - 🔵 style (L37): The DETACHED/SUSPENDED cases (lines 36-40) are separated from the CREATED/CLOSING/ENDED cases (lines 32-35) with a comment, but they all return the same value ('idle'). Combining them into a single fall-through block would be clearer:
case 'CREATED': case 'CLOSING': case 'ENDED': case 'DETACHED': case 'SUSPENDED': return 'idle';— the comment about server override can go above the combined block.[fixable] - 🔵 unsafe_assumptions (L675):
this.db!non-null assertion inrecoverStaleSessions()will throw if called afterclose(). All other methods usethis.stmtswhich shares the same assumption, so this is consistent — but sincerecoverStaleSessionsis only called at startup, the risk is negligible.
packages/protocol/__tests__/event-store.test.ts
Solid P0 foundation — the core state machine, crash recovery, and event emission are well-tested. Main concern is the dual is_active mutation path (markSessionInactive vs setSessionState) which should be consolidated before Phase 1 to make state the true SSOT.
- 🔵 missing_tests: No test verifies the interaction between
markSessionInactive()andsetSessionState(). Given that both now modifyis_active, a test confirming that callingmarkSessionInactivefollowed bysetSessionState('ACTIVE', { force: true })re-enablesis_active(or vice versa) would catch future inconsistencies.[fixable] - 🔵 missing_tests: No test covers
recoverStaleSessions()emitting the correct event count when multiple sessions are recovered simultaneously. The existing test ('emits session_state_changed events for recovered sessions') only checks a single session. A test recovering 3+ sessions and verifying each got exactly one newsession_state_changedevent would guard against event duplication.[fixable]
packages/client/src/protocol-parser.ts
Solid P0 foundation — the core state machine, crash recovery, and event emission are well-tested. Main concern is the dual is_active mutation path (markSessionInactive vs setSessionState) which should be consolidated before Phase 1 to make state the true SSOT.
- 🔵 style (L295): The
session_state_changedhandler usesconsole.debugfor production observability. If the project already has a structured logging pattern for the client package (e.g., via a logger utility), consider using that instead for consistency. If not,console.debugis fine for P0.[fixable]
| this.stmts.setSessionState.run(newState, now, sessionId); | ||
| // Sync is_active from state (backwards-compatible, P0). | ||
| // Only ENDED/CLOSING are inactive. CREATED is transient — see recoverStaleSessions(). | ||
| const isActive = newState !== 'ENDED' && newState !== 'CLOSING' ? 1 : 0; |
There was a problem hiding this comment.
🟡 regressions: markSessionInactive() (line 574) still directly sets is_active=0 without updating state or emitting session_state_changed. Now that setSessionState() derives is_active from state, there are two divergent mutation paths. In query-loop.ts:1401 both are called sequentially (redundant but safe), but any future caller of markSessionInactive alone will leave state and is_active inconsistent — e.g., state=ACTIVE with is_active=0. Consider deprecating markSessionInactive in favor of setSessionState('ENDED') to make state the single source of truth. [fixable]
| * Returns the number of sessions recovered. | ||
| */ | ||
| recoverStaleSessions(): number { | ||
| // CREATED is excluded: it's transient and moves to STARTING immediately (see isActive comment |
There was a problem hiding this comment.
🟡 bugs: recoverStaleSessions() excludes CREATED, but setSessionState sets is_active=1 for CREATED (line 636). If the server crashes between setSessionState('CREATED') (chat.ts:870) and setSessionState('STARTING'), the session is stuck as CREATED+active forever — no recovery path cleans it up. The window is small but real. Consider including CREATED in recovery, or at least adding a comment explaining why an orphaned CREATED is acceptable. [fixable]
| case 'ENDED': | ||
| return 'idle'; | ||
| // DETACHED/SUSPENDED: return 'idle' as safe default. The server layer | ||
| // can override if needed (e.g. to reflect last-known running state). |
There was a problem hiding this comment.
🔵 style: The DETACHED/SUSPENDED cases (lines 36-40) are separated from the CREATED/CLOSING/ENDED cases (lines 32-35) with a comment, but they all return the same value ('idle'). Combining them into a single fall-through block would be clearer: case 'CREATED': case 'CLOSING': case 'ENDED': case 'DETACHED': case 'SUSPENDED': return 'idle'; — the comment about server override can go above the combined block. [fixable]
| const staleStates = ['ACTIVE', 'STARTING', 'DETACHED', 'SUSPENDED', 'CLOSING']; | ||
| const placeholders = staleStates.map(() => '?').join(', '); | ||
| // Inline prepare is intentional — this runs once at startup, not worth caching. | ||
| const rows = this.db!.prepare( |
There was a problem hiding this comment.
🔵 unsafe_assumptions: this.db! non-null assertion in recoverStaleSessions() will throw if called after close(). All other methods use this.stmts which shares the same assumption, so this is consistent — but since recoverStaleSessions is only called at startup, the risk is negligible.
| callbacks.onSessionRenamed?.(msg.name as string); | ||
| break; | ||
|
|
||
| case 'session_state_changed': |
There was a problem hiding this comment.
🔵 style: The session_state_changed handler uses console.debug for production observability. If the project already has a structured logging pattern for the client package (e.g., via a logger utility), consider using that instead for consistency. If not, console.debug is fine for P0. [fixable]
…y, SSE default Transport SSOT Phase 0: server-authoritative session state foundation. - Add ClientSessionState type (idle/running/requires_action) with exhaustive toClientState() mapping from 7-state SessionState - Emit session_state_changed events on every setSessionState() call - Sync is_active column from state in the same UPDATE (backwards-compatible) - Add recoverStaleSessions() for crash recovery (ACTIVE/STARTING/ DETACHED/SUSPENDED/CLOSING → ENDED) - Re-export ClientSessionState and SessionStateEvent from package index - Flip default transport from WS to SSE (Transport SSOT P0) - Add protocol-parser handler for session_state_changed (P0: observability) - 151 tests covering state mapping, is_active sync, crash recovery, and protocol-parser event handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e830856 to
9e2fcbc
Compare
…with session_state_changed Transport SSOT Phase 1: client reads running state from server events only. Server: - handleReconnect uses getSessionState() instead of getSession().isActive - Remove redundant markSessionInactive() in query-loop (setSessionState syncs is_active) - Remove running field from reconnected and session_switched messages - connection-registry isSessionActive uses state instead of is_active column Client: - Activate session_state_changed handler to dispatch SESSION_STATE_CHANGED action - Replace SET_RUNNING action with SESSION_STATE_CHANGED (derives running from state) - Remove syncRunningState() REST polling and foreground call site - Remove running field handling from reconnected/session_switched/takeover/subscribed parsers - handleStop uses SESSION_STATE_CHANGED instead of SET_RUNNING Dead code removed: SET_RUNNING action type, syncRunningState(), runningSyncInFlight, running field in reconnected summaries and session_switched messages. Design doc: docs/design/transport-ssot.md (on design/transport-ssot branch) Depends on: PR #431 (P0 foundation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…with session_state_changed Transport SSOT Phase 1: client reads running state from server events only. Server: - handleReconnect uses getSessionState() instead of getSession().isActive - Remove redundant markSessionInactive() in query-loop (setSessionState syncs is_active) - Remove running field from reconnected and session_switched messages - connection-registry isSessionActive uses state instead of is_active column Client: - Activate session_state_changed handler to dispatch SESSION_STATE_CHANGED action - Replace SET_RUNNING action with SESSION_STATE_CHANGED (derives running from state) - Remove syncRunningState() REST polling and foreground call site - Remove running field handling from reconnected/session_switched/takeover/subscribed parsers - handleStop uses SESSION_STATE_CHANGED instead of SET_RUNNING Dead code removed: SET_RUNNING action type, syncRunningState(), runningSyncInFlight, running field in reconnected summaries and session_switched messages. Design doc: docs/design/transport-ssot.md (on design/transport-ssot branch) Depends on: PR #431 (P0 foundation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…with session_state_changed Transport SSOT Phase 1: client reads running state from server events only. Server: - handleReconnect uses getSessionState() instead of getSession().isActive - Remove redundant markSessionInactive() in query-loop (setSessionState syncs is_active) - Remove running field from reconnected and session_switched messages - connection-registry isSessionActive uses state instead of is_active column Client: - Activate session_state_changed handler to dispatch SESSION_STATE_CHANGED action - Replace SET_RUNNING action with SESSION_STATE_CHANGED (derives running from state) - Remove syncRunningState() REST polling and foreground call site - Remove running field handling from reconnected/session_switched/takeover/subscribed parsers - handleStop uses SESSION_STATE_CHANGED instead of SET_RUNNING Dead code removed: SET_RUNNING action type, syncRunningState(), runningSyncInFlight, running field in reconnected summaries and session_switched messages. Design doc: docs/design/transport-ssot.md (on design/transport-ssot branch) Depends on: PR #431 (P0 foundation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…433) * feat(transport): p1 server-authoritative state — replace SET_RUNNING with session_state_changed Transport SSOT Phase 1: client reads running state from server events only. Server: - handleReconnect uses getSessionState() instead of getSession().isActive - Remove redundant markSessionInactive() in query-loop (setSessionState syncs is_active) - Remove running field from reconnected and session_switched messages - connection-registry isSessionActive uses state instead of is_active column Client: - Activate session_state_changed handler to dispatch SESSION_STATE_CHANGED action - Replace SET_RUNNING action with SESSION_STATE_CHANGED (derives running from state) - Remove syncRunningState() REST polling and foreground call site - Remove running field handling from reconnected/session_switched/takeover/subscribed parsers - handleStop uses SESSION_STATE_CHANGED instead of SET_RUNNING Dead code removed: SET_RUNNING action type, syncRunningState(), runningSyncInFlight, running field in reconnected summaries and session_switched messages. Design doc: docs/design/transport-ssot.md (on design/transport-ssot branch) Depends on: PR #431 (P0 foundation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(transport): address Centaur review findings on transport SSOT p1 - Emit session_state_changed on session switch to close running-state gap - Validate msg.state against ClientSessionState union before dispatch - Add requires_action + unknown state tests - Add optimistic dispatch comments in handleStop - Export toClientState for server-side state mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(transport): address second Centaur review findings on SSOT p1 - Fix requires_action mapping: action.state !== 'idle' (keeps stop button during permission prompts) - Add optimistic running=true on pending send drain (session_end + timer) - Add session_state_changed e2e test in store foreground recovery - Fix prettier formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(transport): address third Centaur review findings on SSOT p1 - Fix session_takeover regression: inline idle dispatch (server unwatches old client) - Fix session_close_ack: inline idle dispatch for 'no active agent' path - Hoist VALID_CLIENT_STATES Set to module scope - Remove unnecessary 'as ClientSessionState' cast on string literal - Add test: session_state_changed emission on session switch - Add test: no emission when getSessionState returns null Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(transport): address fourth Centaur review findings on SSOT p1 - Fix session_takeover regression: inline idle dispatch (server unwatches old client) - Fix session_close_ack: inline idle dispatch for 'no active agent' path - Add session_close_ack test coverage - Remove temporal 'P1:' comment prefixes — describe rationale, not phase - Note periodic sync as iOS foreground gap fallback Critical finding (is_active stale) was a false positive — setSessionState already syncs is_active at event-store.ts:631-634. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
session_state_changedevents fromsetSessionState()with 7-state → 3-state mapping (idle/running/requires_action)is_activefromstateon every transition (backwards-compatible for P4 removal)recoverStaleSessions()— marks ACTIVE/STARTING/DETACHED/SUSPENDED → ENDED on server startuplocalStorage.setItem('mitzo:transport', 'ws'))stateto/api/sessions/:id/metaresponsesession_state_changed(no-op, ready for P1)Context
Design doc: PR #430 (
design/transport-ssotbranch)Telos: 97361f814c5f54df
Test plan
tsc -b)🤖 Generated with Claude Code