docs(design): transport SSOT architecture

[dimakis]

Summary

• Design doc for server-authoritative transport architecture to permanently fix the recurring "send twice to wake agent" bug
• Based on industry analysis of Anthropic SDK, OpenAI, LibreChat, LobeChat, and Vercel AI SDK transport patterns
• 5-phase migration: P0 foundation (state events, crash recovery, SSE default), P1 server-authoritative state, P2 always-send (remove client routing), P3 clean reconnect (cursor-based, multi-client fan-out), P4 cleanup (remove WS)
• Identifies 6 distinct root causes across 45+ fix attempts and 3 transport architectures

Resolved Decisions

1. SSE-first: Flip SSE to default in P0, remove WS in P4
2. Multi-client fan-out: Designed in from the start — any client can send, all clients receive events, no primary/secondary
3. EventStore migration: Backwards-compatible — add state alongside is_active, migrate readers, drop is_active in P4
4. Heartbeat: Default 20s, configurable via MITZO_HEARTBEAT_INTERVAL_MS

Test plan

• Review design doc for completeness
• Validate assumptions against codebase (done — code audit confirms all assumptions)
• Resolve open questions (done — all 4 resolved)
• Centaur review (2 rounds completed)
• Begin P0 implementation

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

docs/design/transport-ssot.md

Well-researched design doc with accurate diagnosis of the transport bug taxonomy. Two file path inaccuracies in the Files Involved table (connection-registry.ts is in packages/harness/, not server/), and the is_active column removal in Phase 1 conflicts with its continued use in handleReconnect which isn't removed until Phase 3.

• 🟡 bugs (L292): Files Involved table lists server/connection-registry.ts, but the actual file is packages/harness/src/connection-registry.ts. The harness package owns this module. Using the wrong path will mislead implementers. [fixable]
• 🔵 style (L7): "Supersedes: Parts of session-state-machine.md" — that doc is still Status: Proposed (never implemented). Consider saying "Incorporates and replaces" rather than "Supersedes" since there's nothing live to supersede. The crash recovery concept (recoverStaleSessions) originates from that doc's Phase 4 and is pulled forward here to Phase 0, which is worth calling out explicitly. [fixable]
• 🟡 unsafe_assumptions (L253): Phase 1 step 5 says "Remove is_active boolean column from EventStore" — but is_active is used in 4+ SQL queries in event-store.ts (session listing, meta endpoint, reconnect checks) and exposed via the /api/sessions/:id/meta response (isActive field). Removing it in Phase 1 while handleReconnect still exists (removed in Phase 3) creates a gap. Either the migration plan needs a compatibility shim in Phase 1, or is_active removal should move to Phase 3 alongside handleReconnect removal. [fixable]
• 🔵 unsafe_assumptions (L272): Phase 2 step 4 proposes "retry with exponential backoff on POST failure (max 3 attempts)" but doesn't address idempotency. If the POST succeeds server-side but the response is lost (network error), the client would retry and send the message again. The seq number in the 202 response could serve as an idempotency key, but the design doesn't specify this. Worth noting in the design or in Open Questions. [fixable]
• 🔵 style (L288): The Files Involved tables list server-side files as server/ paths but connection-registry.ts and session-registry.ts actually live in packages/harness/src/. The event-store.ts path is correctly listed as packages/protocol/src/event-store.ts. For consistency, harness files should use their actual package paths (packages/harness/src/connection-registry.ts). [fixable]

[dimakis]

🟡 bugs: Files Involved table lists server/connection-registry.ts, but the actual file is packages/harness/src/connection-registry.ts. The harness package owns this module. Using the wrong path will mislead implementers. [fixable]

[dimakis]

🔵 style: "Supersedes: Parts of session-state-machine.md" — that doc is still Status: Proposed (never implemented). Consider saying "Incorporates and replaces" rather than "Supersedes" since there's nothing live to supersede. The crash recovery concept (recoverStaleSessions) originates from that doc's Phase 4 and is pulled forward here to Phase 0, which is worth calling out explicitly. [fixable]

[dimakis]

🟡 unsafe_assumptions: Phase 1 step 5 says "Remove is_active boolean column from EventStore" — but is_active is used in 4+ SQL queries in event-store.ts (session listing, meta endpoint, reconnect checks) and exposed via the /api/sessions/:id/meta response (isActive field). Removing it in Phase 1 while handleReconnect still exists (removed in Phase 3) creates a gap. Either the migration plan needs a compatibility shim in Phase 1, or is_active removal should move to Phase 3 alongside handleReconnect removal. [fixable]

[dimakis]

🔵 unsafe_assumptions: Phase 2 step 4 proposes "retry with exponential backoff on POST failure (max 3 attempts)" but doesn't address idempotency. If the POST succeeds server-side but the response is lost (network error), the client would retry and send the message again. The seq number in the 202 response could serve as an idempotency key, but the design doesn't specify this. Worth noting in the design or in Open Questions. [fixable]

[dimakis]

🔵 style: The Files Involved tables list server-side files as server/ paths but connection-registry.ts and session-registry.ts actually live in packages/harness/src/. The event-store.ts path is correctly listed as packages/protocol/src/event-store.ts. For consistency, harness files should use their actual package paths (packages/harness/src/connection-registry.ts). [fixable]

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

docs/design/transport-ssot.md

Well-researched design doc with accurate codebase references. All major claims verified against real code. A few naming inaccuracies (shouldSync doesn't exist) and the requires_action state needs a clearer emission path since it's orthogonal to the SessionState machine.

• 🟡 bugs (L270): shouldSync() is referenced as dead code to remove from connection-registry.ts, but no function by that name exists. The actual methods are startPeriodicSync() and stopPeriodicSync(). Same issue on line 326. Update to reference the real function names to avoid confusion during implementation. [fixable]
• 🟡 unsafe_assumptions (L139): The SessionStateEvent maps permission_request -> 'requires_action', but permission_request is a v2 protocol event type, not a SessionState enum value. The 7-state SessionState has no requires_action equivalent — it's a cross-cutting concern (permissions can arrive in ACTIVE state). The doc should clarify that requires_action is derived from permission events, not from setSessionState(), which contradicts the "Emit session_state_changed on every setSessionState() call" instruction in Section 3. This needs a second emission path (from permission_request events) that isn't mentioned in the implementation plan. [fixable]
• 🔵 style (L81): The architecture diagram shows POST /send but Section 4 (Write Acknowledgment) references POST /api/chat/send. The existing codebase uses the /api/chat/send path (via chat-rest-handler.ts). The diagram should use the full path for consistency and to avoid ambiguity during implementation. [fixable]
• 🔵 unsafe_assumptions (L249): Phase 2 references clientMsgId as an idempotency key and claims "server already has idempotency checking (#386)". This should be verified during implementation — if the idempotency check is keyed differently or has been removed/refactored since #386, retried POSTs could create duplicate messages. Consider adding a Phase 2 prerequisite to verify the idempotency mechanism exists and is keyed on clientMsgId.
• 🔵 style (L176): Phase 0 proposes recoverStaleSessions() to transition ACTIVE/STARTING/DETACHED sessions to ENDED on startup, but doesn't mention SUSPENDED. The SessionState type includes SUSPENDED as a state where the session is not cleanly ended. If the server crashes while a session is SUSPENDED, it should also be recovered. Consider adding SUSPENDED to the recovery set, or document why it's excluded. [fixable]

[dimakis]

🟡 bugs: shouldSync() is referenced as dead code to remove from connection-registry.ts, but no function by that name exists. The actual methods are startPeriodicSync() and stopPeriodicSync(). Same issue on line 326. Update to reference the real function names to avoid confusion during implementation. [fixable]

[dimakis]

🟡 unsafe_assumptions: The SessionStateEvent maps permission_request -> 'requires_action', but permission_request is a v2 protocol event type, not a SessionState enum value. The 7-state SessionState has no requires_action equivalent — it's a cross-cutting concern (permissions can arrive in ACTIVE state). The doc should clarify that requires_action is derived from permission events, not from setSessionState(), which contradicts the "Emit session_state_changed on every setSessionState() call" instruction in Section 3. This needs a second emission path (from permission_request events) that isn't mentioned in the implementation plan. [fixable]

[dimakis]

🔵 style: The architecture diagram shows POST /send but Section 4 (Write Acknowledgment) references POST /api/chat/send. The existing codebase uses the /api/chat/send path (via chat-rest-handler.ts). The diagram should use the full path for consistency and to avoid ambiguity during implementation. [fixable]

[dimakis]

🔵 unsafe_assumptions: Phase 2 references clientMsgId as an idempotency key and claims "server already has idempotency checking (#386)". This should be verified during implementation — if the idempotency check is keyed differently or has been removed/refactored since #386, retried POSTs could create duplicate messages. Consider adding a Phase 2 prerequisite to verify the idempotency mechanism exists and is keyed on clientMsgId.

[dimakis]

🔵 style: Phase 0 proposes recoverStaleSessions() to transition ACTIVE/STARTING/DETACHED sessions to ENDED on startup, but doesn't mention SUSPENDED. The SessionState type includes SUSPENDED as a state where the session is not cleanly ended. If the server crashes while a session is SUSPENDED, it should also be recovered. Consider adding SUSPENDED to the recovery set, or document why it's excluded. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

docs/design/transport-ssot.md

Well-grounded design doc — all referenced code artifacts verified as existing. One inaccurate file reference for the clientMsgId idempotency guard (it's in chat.ts, not chat-rest-handler.ts) and minor clarification opportunities.

• 🟡 unsafe_assumptions (L305): Phase 2 prerequisite says to verify clientMsgId idempotency guard is "present and keyed correctly in chat-rest-handler.ts". The guard actually lives in server/chat.ts (storeAndEchoIfNew() at line ~1162, which calls eventStore.hasUserMessage()). chat-rest-handler.ts passes clientMsgId through to handleSendV2() which calls sendToChat() in chat.ts, but the dedup logic itself is not in chat-rest-handler.ts. The prerequisite should reference chat.ts (or both files) so an implementer doesn't look in the wrong place and conclude the guard is missing. [fixable]
• 🔵 style (L8): streaming-input-session-control.md status is listed implicitly as never implemented (via the "Incorporates and replaces" line grouping it with session-state-machine.md). However, streaming-input-session-control.md has Status: Implemented (v0.1.0) and was shipped with protocol v2 (PR #33). The relationship is different — this doc replaces a shipped design, not a proposed one. Consider clarifying the distinction (e.g., "replaces session-state-machine.md (never implemented) and supersedes streaming-input-session-control.md (implemented, but scope expanded here)"). [fixable]
• 🔵 unsafe_assumptions (L349): The Files Involved table lists frontend/src/client-store.ts for the SSE default transport flip. This file does exist and contains the transport selector logic (localStorage.getItem('mitzo:transport') === 'sse'). However, if the SSE flip also involves changes to packages/client/src/store.ts (where the MitzoConnection is instantiated), that file should be listed in the table as well. Currently the Server table lists the store file for removing wasRunning but the Phase 0 SSE flip may span both locations. [fixable]
• 🔵 style (L181): The SessionStateEvent type comment says setSessionState() maps DETACHED/SUSPENDED -> 'preserve last emitted (server buffers)'. This is an important behavioral detail but it's buried in a code comment. The buffering mechanism (how the server remembers and replays the last emitted client-facing state after a transient internal state) should be called out explicitly in the design text, as implementers need to build this state tracking. [fixable]

[dimakis]

🟡 unsafe_assumptions: Phase 2 prerequisite says to verify clientMsgId idempotency guard is "present and keyed correctly in chat-rest-handler.ts". The guard actually lives in server/chat.ts (storeAndEchoIfNew() at line ~1162, which calls eventStore.hasUserMessage()). chat-rest-handler.ts passes clientMsgId through to handleSendV2() which calls sendToChat() in chat.ts, but the dedup logic itself is not in chat-rest-handler.ts. The prerequisite should reference chat.ts (or both files) so an implementer doesn't look in the wrong place and conclude the guard is missing. [fixable]

[dimakis]

🔵 style: streaming-input-session-control.md status is listed implicitly as never implemented (via the "Incorporates and replaces" line grouping it with session-state-machine.md). However, streaming-input-session-control.md has Status: Implemented (v0.1.0) and was shipped with protocol v2 (PR #33). The relationship is different — this doc replaces a shipped design, not a proposed one. Consider clarifying the distinction (e.g., "replaces session-state-machine.md (never implemented) and supersedes streaming-input-session-control.md (implemented, but scope expanded here)"). [fixable]

[dimakis]

🔵 unsafe_assumptions: The Files Involved table lists frontend/src/client-store.ts for the SSE default transport flip. This file does exist and contains the transport selector logic (localStorage.getItem('mitzo:transport') === 'sse'). However, if the SSE flip also involves changes to packages/client/src/store.ts (where the MitzoConnection is instantiated), that file should be listed in the table as well. Currently the Server table lists the store file for removing wasRunning but the Phase 0 SSE flip may span both locations. [fixable]

[dimakis]

🔵 style: The SessionStateEvent type comment says setSessionState() maps DETACHED/SUSPENDED -> 'preserve last emitted (server buffers)'. This is an important behavioral detail but it's buried in a code comment. The buffering mechanism (how the server remembers and replays the last emitted client-facing state after a transient internal state) should be called out explicitly in the design text, as implementers need to build this state tracking. [fixable]

[dimakis]

Centaur Review

Found 7 issue(s) (4 warning).

docs/design/transport-ssot.md

Well-researched design doc with accurate problem analysis. Main issues: Phase 0 describes already-implemented work (state column migration) without noting it, the is_active/state synchronization gap during transition needs explicit risk callout, and the UX impact of moving from optimistic to acknowledged sends should be addressed.

• 🟡 style (L284): Phase 0 item 7 ('Add state column to EventStore sessions table alongside is_active') is already implemented — the migration exists in packages/protocol/src/event-store.ts:282-293 and setSessionState() already writes to it. This should be marked as 'Done' or noted as pre-existing to avoid confusion during implementation. Similarly, the setSessionState method and VALID_TRANSITIONS table already exist. [fixable]
• 🔵 style (L109): The doc says the wasRunning branch is '~50 lines' to remove. The actual wasRunning branch in packages/client/src/store.ts:377-398 is 22 lines. The ~50 figure may include related code scattered elsewhere (pendingSend declarations, timer helpers, drain logic), but as written it overstates the size of the single branch. Consider saying '~50 lines across store and parser' for accuracy. [fixable]
• 🟡 unsafe_assumptions (L280): Phase 0 item 8 says 'Migrate setSessionState() to write both state and is_active'. Currently setSessionState() (event-store.ts:190-196) only writes state and last_state_change — it does NOT write is_active. Meanwhile markSessionInactive() only writes is_active. This means during the transition period, a session ended via setSessionState(ENDED) won't flip is_active = 0, causing stale is_active = 1 for ended sessions. The doc correctly identifies this gap but should explicitly call out the risk: until this migration step is done, any code path that uses setSessionState() to end a session without also calling markSessionInactive() will leave a stale is_active. [fixable]
• 🟡 unsafe_assumptions (L247): The Multi-Client Fan-Out section says 'Concurrent sends are serialized. If two clients POST simultaneously, AsyncQueue serializes them. The SDK processes one turn at a time.' This assumes the SDK handles interleaved user messages gracefully. If the SDK's query() doesn't support multiple sequential user messages pushed mid-generation (i.e., a second message arriving while the first is still generating), this could produce unexpected behavior. The doc should note whether this is a tested SDK capability or an assumption that needs verification. [fixable]
• 🔵 style (L215): The SessionStateEvent type comment says 'CREATED/ENDED -> idle', 'STARTING/ACTIVE -> running'. The DETACHED/SUSPENDED mapping says 'preserve last emitted (server buffers)'. However, CLOSING state is not mentioned in the mapping. Given the 7-state server model includes CLOSING (referenced by the 'Why 3 states, not 7' paragraph), the mapping should specify CLOSING's client-facing state — likely 'running' (still generating) or 'idle' (winding down). [fixable]
• 🟡 regressions (L316): Phase 2 removes sendMessage()'s wasRunning guard and makes all sends go via HTTP POST with acknowledgment. But the current sendMessage() fallback path (line 400-403 in store.ts) handles connection.send() returning false with a sendError message. Phase 2 step 3 says 'Client awaits POST response before marking message as sent' — this changes the UX from immediate-optimistic (message appears instantly, error shown later) to acknowledged (message appears only after 202). The doc should explicitly note this UX change and whether the USER_SEND reducer action (which adds the user bubble immediately at line 364) should be deferred until acknowledgment or kept as-is with a 'pending' state. [fixable]
• 🔵 style (L336): Phase 3 item 6 says 'ConnectionRegistry fan-out: track set of connections per session, not single owner'. But the current ConnectionRegistry (packages/harness/src/connection-registry.ts) already tracks per-connection per-session cursors via a two-level Map, and connections have watchedSessions: Set<string>. The inverse mapping (session -> connections) may need addition, but the doc should clarify what specifically needs to change vs what's already there. [fixable]

[dimakis]

🟡 style: Phase 0 item 7 ('Add state column to EventStore sessions table alongside is_active') is already implemented — the migration exists in packages/protocol/src/event-store.ts:282-293 and setSessionState() already writes to it. This should be marked as 'Done' or noted as pre-existing to avoid confusion during implementation. Similarly, the setSessionState method and VALID_TRANSITIONS table already exist. [fixable]

[dimakis]

🔵 style: The doc says the wasRunning branch is '~50 lines' to remove. The actual wasRunning branch in packages/client/src/store.ts:377-398 is 22 lines. The ~50 figure may include related code scattered elsewhere (pendingSend declarations, timer helpers, drain logic), but as written it overstates the size of the single branch. Consider saying '~50 lines across store and parser' for accuracy. [fixable]

[dimakis]

🟡 unsafe_assumptions: Phase 0 item 8 says 'Migrate setSessionState() to write both state and is_active'. Currently setSessionState() (event-store.ts:190-196) only writes state and last_state_change — it does NOT write is_active. Meanwhile markSessionInactive() only writes is_active. This means during the transition period, a session ended via setSessionState(ENDED) won't flip is_active = 0, causing stale is_active = 1 for ended sessions. The doc correctly identifies this gap but should explicitly call out the risk: until this migration step is done, any code path that uses setSessionState() to end a session without also calling markSessionInactive() will leave a stale is_active. [fixable]

[dimakis]

🟡 unsafe_assumptions: The Multi-Client Fan-Out section says 'Concurrent sends are serialized. If two clients POST simultaneously, AsyncQueue serializes them. The SDK processes one turn at a time.' This assumes the SDK handles interleaved user messages gracefully. If the SDK's query() doesn't support multiple sequential user messages pushed mid-generation (i.e., a second message arriving while the first is still generating), this could produce unexpected behavior. The doc should note whether this is a tested SDK capability or an assumption that needs verification. [fixable]

[dimakis]

🔵 style: The SessionStateEvent type comment says 'CREATED/ENDED -> idle', 'STARTING/ACTIVE -> running'. The DETACHED/SUSPENDED mapping says 'preserve last emitted (server buffers)'. However, CLOSING state is not mentioned in the mapping. Given the 7-state server model includes CLOSING (referenced by the 'Why 3 states, not 7' paragraph), the mapping should specify CLOSING's client-facing state — likely 'running' (still generating) or 'idle' (winding down). [fixable]

[dimakis]

🟡 regressions: Phase 2 removes sendMessage()'s wasRunning guard and makes all sends go via HTTP POST with acknowledgment. But the current sendMessage() fallback path (line 400-403 in store.ts) handles connection.send() returning false with a sendError message. Phase 2 step 3 says 'Client awaits POST response before marking message as sent' — this changes the UX from immediate-optimistic (message appears instantly, error shown later) to acknowledged (message appears only after 202). The doc should explicitly note this UX change and whether the USER_SEND reducer action (which adds the user bubble immediately at line 364) should be deferred until acknowledgment or kept as-is with a 'pending' state. [fixable]

[dimakis]

🔵 style: Phase 3 item 6 says 'ConnectionRegistry fan-out: track set of connections per session, not single owner'. But the current ConnectionRegistry (packages/harness/src/connection-registry.ts) already tracks per-connection per-session cursors via a two-level Map, and connections have watchedSessions: Set<string>. The inverse mapping (session -> connections) may need addition, but the doc should clarify what specifically needs to change vs what's already there. [fixable]

[dimakis]

Centaur Review

Found 6 issue(s) (4 warning).

docs/design/transport-ssot.md

Well-structured design doc with strong problem analysis and phased migration plan. Key concern: the ConnectionRegistry "Current" vs "Target" comparison misrepresents the existing data model (it's already connection→sessions, not session→connection), which could mislead implementers about the scope of multi-client fan-out changes.

• 🟡 bugs (L251): The "Current" pseudo-code sessions: Map<sessionId, { connectionId, cursor }> misrepresents ConnectionRegistry's actual structure. In reality, the primary map is connections: Map<connectionId, Connection> where each Connection has watchedSessions: Set<string>, and cursors are tracked separately in cursors: Map<connectionId, Map<sessionId, number>>. The relationship is inverted (connection→sessions, not session→connection). The current code already supports multiple connections watching the same session. This means the "Target" change may be smaller than described, and implementers working from this doc will start from the wrong mental model. [fixable]
• 🟡 bugs (L254): The target type Set<{ connectionId: string; cursor: number }> won't work correctly in TypeScript/JavaScript — Set uses reference equality for objects, making lookup and deletion by value impossible. A Map<string, number> (connectionId → cursor) would be the correct data structure here. Given that the existing ConnectionRegistry already uses per-connection cursor maps, the target should be described in terms of the actual existing structure rather than an invented one. [fixable]
• 🟡 unsafe_assumptions (L111): The doc says the wasRunning branch is "~50 lines" but the actual implementation spans lines 339-404 in packages/client/src/store.ts (66 lines). Minor inaccuracy, but an implementer using this doc to scope removal work would underestimate. [fixable]
• 🟡 unsafe_assumptions (L318): Phase 2 prerequisite says idempotency guard storeAndEchoIfNew() lives in server/chat.ts and calls eventStore.hasUserMessage(). This is accurate today, but the doc warns it may have been "refactored or removed" since PR #386 — this safety net is good. However, the Phase 2 plan adds retry with exponential backoff (up to 3 attempts) relying on this dedup, but doesn't specify whether the HTTP POST handler (chat-rest-handler.ts) also needs dedup at the HTTP layer (e.g., returning 202 for already-accepted messages instead of processing them again up to the dedup check). If the dedup is only in storeAndEchoIfNew(), the agent will still receive the message twice via AsyncQueue before dedup fires.
• 🔵 style (L148): The SessionStateEvent type comment says DETACHED/SUSPENDED → 'preserve last emitted (server buffers)', but the 3-state type 'idle' | 'running' | 'requires_action' doesn't include a 'no change' sentinel. The prose below explains lastEmittedClientState buffering, but the inline type comment could mislead — someone reading just the type definition might expect a fourth state value. Consider clarifying inline that these server states suppress emission entirely (no event sent) rather than emit a special value. [fixable]
• 🔵 style (L360): The Files Involved table lists frontend/src/client-store.ts for "Flip SSE to default transport (P0)" but the actual SSE/WS transport selection logic lives in packages/client/src/store.ts (where MitzoConnection is configured). frontend/src/client-store.ts re-exports from the package. If the flip is just a config default change, the doc should reference the actual file where the transport choice is made. [fixable]

[dimakis]

🟡 bugs: The "Current" pseudo-code sessions: Map<sessionId, { connectionId, cursor }> misrepresents ConnectionRegistry's actual structure. In reality, the primary map is connections: Map<connectionId, Connection> where each Connection has watchedSessions: Set<string>, and cursors are tracked separately in cursors: Map<connectionId, Map<sessionId, number>>. The relationship is inverted (connection→sessions, not session→connection). The current code already supports multiple connections watching the same session. This means the "Target" change may be smaller than described, and implementers working from this doc will start from the wrong mental model. [fixable]

[dimakis]

🟡 bugs: The target type Set<{ connectionId: string; cursor: number }> won't work correctly in TypeScript/JavaScript — Set uses reference equality for objects, making lookup and deletion by value impossible. A Map<string, number> (connectionId → cursor) would be the correct data structure here. Given that the existing ConnectionRegistry already uses per-connection cursor maps, the target should be described in terms of the actual existing structure rather than an invented one. [fixable]

[dimakis]

🟡 unsafe_assumptions: The doc says the wasRunning branch is "~50 lines" but the actual implementation spans lines 339-404 in packages/client/src/store.ts (66 lines). Minor inaccuracy, but an implementer using this doc to scope removal work would underestimate. [fixable]

[dimakis]

🟡 unsafe_assumptions: Phase 2 prerequisite says idempotency guard storeAndEchoIfNew() lives in server/chat.ts and calls eventStore.hasUserMessage(). This is accurate today, but the doc warns it may have been "refactored or removed" since PR #386 — this safety net is good. However, the Phase 2 plan adds retry with exponential backoff (up to 3 attempts) relying on this dedup, but doesn't specify whether the HTTP POST handler (chat-rest-handler.ts) also needs dedup at the HTTP layer (e.g., returning 202 for already-accepted messages instead of processing them again up to the dedup check). If the dedup is only in storeAndEchoIfNew(), the agent will still receive the message twice via AsyncQueue before dedup fires.

[dimakis]

🔵 style: The SessionStateEvent type comment says DETACHED/SUSPENDED → 'preserve last emitted (server buffers)', but the 3-state type 'idle' | 'running' | 'requires_action' doesn't include a 'no change' sentinel. The prose below explains lastEmittedClientState buffering, but the inline type comment could mislead — someone reading just the type definition might expect a fourth state value. Consider clarifying inline that these server states suppress emission entirely (no event sent) rather than emit a special value. [fixable]

[dimakis]

🔵 style: The Files Involved table lists frontend/src/client-store.ts for "Flip SSE to default transport (P0)" but the actual SSE/WS transport selection logic lives in packages/client/src/store.ts (where MitzoConnection is configured). frontend/src/client-store.ts re-exports from the package. If the flip is just a config default change, the doc should reference the actual file where the transport choice is made. [fixable]

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

docs/design/transport-ssot.md

Well-researched design doc with accurate codebase references. Main concern: Phase 0 items are already implemented but presented as future work, and the DETACHED/SUSPENDED buffering design doesn't match the current implementation — either the doc or the code needs updating to close the gap.

• 🟡 unsafe_assumptions (L141): The design specifies DETACHED/SUSPENDED should 'preserve last emitted' client state via a lastEmittedClientState field per session and suppress emission. However, the already-implemented toClientState() in packages/protocol/src/event-store.ts:35-37 returns 'idle' as a safe default for DETACHED/SUSPENDED with a comment noting it can't track last-emitted state at the EventStore level, and emission is not suppressed. The design should either acknowledge this gap as a Phase 1+ follow-up or be updated to match the current behavior. [fixable]
• 🔵 style (L284): Phase 0 items 1-2 (session_state_changed event, emission from setSessionState), item 5 (recoverStaleSessions), items 7-8 (state column, setSessionState writing both columns) are already implemented in the codebase. The migration plan reads as future work but these are completed. Consider marking them as done (e.g., with checkboxes or a 'Status: Complete' annotation) to avoid confusion about what remains. [fixable]
• 🟡 unsafe_assumptions (L238): The ConnectionRegistry multi-client fan-out section shows the target structure as Map<sessionId, Set<{ connectionId, cursor }>>, but object literals in a Set won't deduplicate by value — Set uses reference equality. This would need to be a Map<sessionId, Map<connectionId, { cursor }>> or similar structure in practice. Since this is a design doc (not code), this is minor, but implementers may copy the pattern verbatim. [fixable]
• 🔵 unsafe_assumptions (L185): The HTTP POST send endpoint returns 202 with { accepted, sessionId, seq }. The design doesn't specify behavior when the client POSTs to a session that exists in EventStore but has no live process in SessionRegistry (e.g., session in ENDED or DETACHED state without a running agent). Should this return 404, 409, or attempt to restart? This edge case is worth documenting since it's the exact failure mode the design is trying to eliminate. [fixable]
• 🔵 style (L375): The Files Involved table lists frontend/src/client-store.ts for 'Flip SSE to default transport (P0)', but the table header column says 'File' with paths like server/query-loop.ts (no leading slash). The client-store entry is the only frontend file in the Server table — consider moving it to the Client table or adding a note that this is a cross-cutting change. [fixable]

[dimakis]

🟡 unsafe_assumptions: The design specifies DETACHED/SUSPENDED should 'preserve last emitted' client state via a lastEmittedClientState field per session and suppress emission. However, the already-implemented toClientState() in packages/protocol/src/event-store.ts:35-37 returns 'idle' as a safe default for DETACHED/SUSPENDED with a comment noting it can't track last-emitted state at the EventStore level, and emission is not suppressed. The design should either acknowledge this gap as a Phase 1+ follow-up or be updated to match the current behavior. [fixable]

[dimakis]

🔵 style: Phase 0 items 1-2 (session_state_changed event, emission from setSessionState), item 5 (recoverStaleSessions), items 7-8 (state column, setSessionState writing both columns) are already implemented in the codebase. The migration plan reads as future work but these are completed. Consider marking them as done (e.g., with checkboxes or a 'Status: Complete' annotation) to avoid confusion about what remains. [fixable]

[dimakis]

🟡 unsafe_assumptions: The ConnectionRegistry multi-client fan-out section shows the target structure as Map<sessionId, Set<{ connectionId, cursor }>>, but object literals in a Set won't deduplicate by value — Set uses reference equality. This would need to be a Map<sessionId, Map<connectionId, { cursor }>> or similar structure in practice. Since this is a design doc (not code), this is minor, but implementers may copy the pattern verbatim. [fixable]

[dimakis]

🔵 unsafe_assumptions: The HTTP POST send endpoint returns 202 with { accepted, sessionId, seq }. The design doesn't specify behavior when the client POSTs to a session that exists in EventStore but has no live process in SessionRegistry (e.g., session in ENDED or DETACHED state without a running agent). Should this return 404, 409, or attempt to restart? This edge case is worth documenting since it's the exact failure mode the design is trying to eliminate. [fixable]

[dimakis]

🔵 style: The Files Involved table lists frontend/src/client-store.ts for 'Flip SSE to default transport (P0)', but the table header column says 'File' with paths like server/query-loop.ts (no leading slash). The client-store entry is the only frontend file in the Server table — consider moving it to the Client table or adding a note that this is a cross-cutting change. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

docs/design/transport-ssot.md

Well-structured design doc with clear problem taxonomy and phased migration. Main concern: the DETACHED/SUSPENDED buffering spec is not implemented — current toClientState() contradicts the design by mapping those states to 'idle' instead of preserving the last emitted state. This will matter at Phase 3 (multi-client fan-out). Three minor suggestions on pseudocode accuracy and phase tracking.

• 🟡 unsafe_assumptions (L129): DETACHED/SUSPENDED buffering described here (preserve last emitted (server buffers) with lastEmittedClientState) is not implemented. The current toClientState() in packages/protocol/src/event-store.ts:35-36 maps both DETACHED and SUSPENDED to 'idle' and always emits — no suppression, no buffering. During multi-client fan-out (Phase 3), other clients watching the same session would incorrectly see idle when one client's transport drops while the agent is still running. The doc should note this as an open implementation gap, or the mapping should be fixed before Phase 3. [fixable]
• 🔵 style (L125): The SessionStateEvent comment block maps CREATED/ENDED -> 'idle' and STARTING/ACTIVE -> 'running' but omits CLOSING. The actual toClientState() implementation maps CLOSING to 'idle'. Add it to the CREATED/ENDED line for completeness: CREATED/CLOSING/ENDED -> 'idle'. [fixable]
• 🔵 style (L244): The ConnectionRegistry pseudocode shows sessions: Map<sessionId, { connectionId, cursor }> as the current structure, but the actual implementation is connection-centric: Map<connectionId, Connection> where each Connection has watchedSessions: Set<string>, with cursors tracked in a separate Map<connectionId, Map<sessionId, number>>. The 'Target' pseudocode should be revised to match the actual data model being extended, not an idealized version that doesn't correspond to the code. [fixable]
• 🔵 style (L281): Phase 0 items (state column, recoverStaleSessions, SSE default, session_state_changed emission) and Phase 1 items (syncRunningState removal, SET_RUNNING removal) are already implemented in the codebase but the doc reads as if they are future work. Consider adding completion markers (e.g. checkboxes or a status column) to each phase so readers know the current progress. [fixable]

[dimakis]

🟡 unsafe_assumptions: DETACHED/SUSPENDED buffering described here (preserve last emitted (server buffers) with lastEmittedClientState) is not implemented. The current toClientState() in packages/protocol/src/event-store.ts:35-36 maps both DETACHED and SUSPENDED to 'idle' and always emits — no suppression, no buffering. During multi-client fan-out (Phase 3), other clients watching the same session would incorrectly see idle when one client's transport drops while the agent is still running. The doc should note this as an open implementation gap, or the mapping should be fixed before Phase 3. [fixable]

[dimakis]

🔵 style: The SessionStateEvent comment block maps CREATED/ENDED -> 'idle' and STARTING/ACTIVE -> 'running' but omits CLOSING. The actual toClientState() implementation maps CLOSING to 'idle'. Add it to the CREATED/ENDED line for completeness: CREATED/CLOSING/ENDED -> 'idle'. [fixable]

[dimakis]

🔵 style: The ConnectionRegistry pseudocode shows sessions: Map<sessionId, { connectionId, cursor }> as the current structure, but the actual implementation is connection-centric: Map<connectionId, Connection> where each Connection has watchedSessions: Set<string>, with cursors tracked in a separate Map<connectionId, Map<sessionId, number>>. The 'Target' pseudocode should be revised to match the actual data model being extended, not an idealized version that doesn't correspond to the code. [fixable]

[dimakis]

🔵 style: Phase 0 items (state column, recoverStaleSessions, SSE default, session_state_changed emission) and Phase 1 items (syncRunningState removal, SET_RUNNING removal) are already implemented in the codebase but the doc reads as if they are future work. Consider adding completion markers (e.g. checkboxes or a status column) to each phase so readers know the current progress. [fixable]

[dimakis]

Centaur Review

Found 6 issue(s) (4 warning).

docs/design/transport-ssot.md

Design doc is well-structured and the architecture is sound, but several claims about the current codebase are stale — key Phase 0/1 items (SSE default, SESSION_STATE_CHANGED action type, state column) appear already implemented, and the ConnectionRegistry data model description is inaccurate. These should be corrected before implementation begins to avoid confusion.

• 🟡 bugs (L131): Section 2 claims the client uses SET_RUNNING actions and lists removing SET_RUNNING as Phase 1 dead code. However, the codebase already uses SESSION_STATE_CHANGED action type (in packages/client/src/slices/messages.ts:160) with the exact 3-state model (idle | running | requires_action) this doc proposes. The protocol-parser already dispatches { type: 'SESSION_STATE_CHANGED', state } in multiple places and the reducer derives running from it. Phase 1 steps 1-2 appear to be already implemented — the doc describes the current state as the target state, which will confuse implementors. [fixable]
• 🟡 bugs (L258): Phase 0 step 6 ('Flip SSE to default transport') is already done. frontend/src/client-store.ts lines 19-26 explicitly document SSE + HTTP POST as the default transport with a comment: 'This is an intentional flip from WS-default per the transport-ssot design doc.' The migration plan should note which steps are already complete vs. remaining. [fixable]
• 🟡 bugs (L225): The ConnectionRegistry current-state description ('maps one connection per session (owner model)') is inaccurate. The actual model in packages/harness/src/connection-registry.ts is one connection per browser tab/client, where each connection watches multiple sessions and has one active session. The ConnectionRegistry uses connections: Map<string, Connection> (keyed by connectionId) with watchedSessions: Set<string> per connection — not a session→connection map. The proposed 'Target' change to Map<sessionId, Set<...>> is actually inverting the current key structure, which is a larger change than described. [fixable]
• 🟡 unsafe_assumptions (L259): Phase 0 step 7 ('Add state column to EventStore sessions table alongside is_active') may already be partially done. The EventStore in packages/protocol/src/event-store.ts already has a state property in SessionMeta (line 80) and a setSessionState() method (line 609). The doc should clarify what's already present vs. what needs to be added to avoid redundant migration work. [fixable]
• 🔵 style (L364): The 'Files Involved' table lists frontend/src/client-store.ts for Phase 0 ('Flip SSE to default transport'), but this is already implemented per that file's current contents. Consider adding a 'Status' column to the files table or marking completed items, so implementors can distinguish completed from remaining work. [fixable]
• 🔵 style (L373): The 'Dead Code Candidates' section lists SET_RUNNING action type for Phase 1 removal, but this action type doesn't exist in the codebase (only SESSION_STATE_CHANGED does). This entry should be removed or corrected to reflect the actual dead code that needs cleaning up. [fixable]

[dimakis]

🟡 bugs: Section 2 claims the client uses SET_RUNNING actions and lists removing SET_RUNNING as Phase 1 dead code. However, the codebase already uses SESSION_STATE_CHANGED action type (in packages/client/src/slices/messages.ts:160) with the exact 3-state model (idle | running | requires_action) this doc proposes. The protocol-parser already dispatches { type: 'SESSION_STATE_CHANGED', state } in multiple places and the reducer derives running from it. Phase 1 steps 1-2 appear to be already implemented — the doc describes the current state as the target state, which will confuse implementors. [fixable]

[dimakis]

🟡 bugs: Phase 0 step 6 ('Flip SSE to default transport') is already done. frontend/src/client-store.ts lines 19-26 explicitly document SSE + HTTP POST as the default transport with a comment: 'This is an intentional flip from WS-default per the transport-ssot design doc.' The migration plan should note which steps are already complete vs. remaining. [fixable]

[dimakis]

🟡 bugs: The ConnectionRegistry current-state description ('maps one connection per session (owner model)') is inaccurate. The actual model in packages/harness/src/connection-registry.ts is one connection per browser tab/client, where each connection watches multiple sessions and has one active session. The ConnectionRegistry uses connections: Map<string, Connection> (keyed by connectionId) with watchedSessions: Set<string> per connection — not a session→connection map. The proposed 'Target' change to Map<sessionId, Set<...>> is actually inverting the current key structure, which is a larger change than described. [fixable]

[dimakis]

🟡 unsafe_assumptions: Phase 0 step 7 ('Add state column to EventStore sessions table alongside is_active') may already be partially done. The EventStore in packages/protocol/src/event-store.ts already has a state property in SessionMeta (line 80) and a setSessionState() method (line 609). The doc should clarify what's already present vs. what needs to be added to avoid redundant migration work. [fixable]

[dimakis]

🔵 style: The 'Files Involved' table lists frontend/src/client-store.ts for Phase 0 ('Flip SSE to default transport'), but this is already implemented per that file's current contents. Consider adding a 'Status' column to the files table or marking completed items, so implementors can distinguish completed from remaining work. [fixable]

[dimakis]

🔵 style: The 'Dead Code Candidates' section lists SET_RUNNING action type for Phase 1 removal, but this action type doesn't exist in the codebase (only SESSION_STATE_CHANGED does). This entry should be removed or corrected to reflect the actual dead code that needs cleaning up. [fixable]