feat(protocol): Symposium Phase 1 — types, event-store, tests

[dimakis]

Summary

• New types: SessionType, TurnMode, InterceptMode, SeatConfig, TurnRules, SymposiumConfig
• Event-store migration: session_type + symposium_config columns
• upsertSession wiring for symposium config
• 14 comprehensive tests covering type shapes and config composition

Test Plan

• All protocol package tests pass (257/257)
• Types importable from @mitzo/protocol
• Migration runs cleanly on existing databases
• Symposium config serializes/deserializes correctly

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 7 issue(s) (4 warning).

packages/client/src/sse-connection.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🟡 unsafe_assumptions (L219): When the reconnect POST fails, _pendingReconnectSessions retains the sessions for retry on the next welcome. However, checkAndReconnect() (line 172) does NOT clear _pendingReconnectSessions — it only sets _connected = false and creates a new EventSource. This means getReconnectSessions() will use the stale pending list on the next welcome, which is the intended design. But if an onerror fires (EventSource auto-reconnect, line 267), _connected is set to false but _pendingReconnectSessions is also preserved. This is correct for the retry intent, but if the server-side session has been cleaned up between failures, the client will keep retrying a reconnect POST for a session that no longer exists — there's no TTL or attempt limit on _pendingReconnectSessions. Consider adding a retry cap or TTL to prevent infinite reconnect POST attempts for dead sessions. [fixable]
• 🟡 regressions (L226): The old doReconnectPost had a staleness guard that checked BOTH this.es !== welcomeEs (EventSource instance identity) AND this._connectionId !== welcomeConnectionId. The new inline handler only checks this._connectionId !== postConnectionId. This drops the ES-identity guard. If checkAndReconnect() is called while a reconnect POST is in-flight, it closes the old ES and creates a new one, but _connectionId doesn't change until the new welcome arrives. During that window, if the stale POST resolves, the connectionId check alone won't catch it — _connectionId is still the old value because no new welcome has arrived yet. The old code would have bailed via the es !== welcomeEs check. This could cause _connected = true and flushPendingSends() to fire on a stale EventSource. [fixable]

packages/client/src/__tests__/sse-connection.test.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🟡 missing_tests: The PR removes tests for several important edge cases that were previously covered: (1) bails out if disconnect() called during in-flight reconnect POST — the new code clears _pendingReconnectSessions on disconnect, but there's no test that the in-flight POST's .then() callback is a no-op after disconnect. (2) ignores stale reconnect POST when a newer welcome arrives — the new code uses connectionId-based staleness, but the only test for this (marks connected after reconnect POST succeeds) doesn't test the race with a second welcome. (3) dispatches SSE events to listener while reconnect POST is in-flight — removed entirely with no replacement. SSE event delivery during the reconnect window is a critical path. [fixable]

packages/harness/src/connection-registry.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🟡 regressions: Removing periodic sync entirely means there is no server-side mechanism to retry delivery of events that failed during broadcast(). The old code would retry via periodic sync every 5s. The new design relies entirely on client-initiated reconnect (EventSource auto-reconnect → welcome → replay from EventStore). This is a deliberate architectural change documented in comments, but it means that a transient broadcast() failure (e.g., WebSocket buffer full) on a still-connected client will result in a permanently missed event until the client disconnects and reconnects. The comment at line 133 says 'reconnect replay will cover the gap' but that only happens on reconnect, not on transient send failures where the connection stays alive.

packages/protocol/__tests__/event-store.test.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🔵 style (L897): The test 'events table supports seat_id in payload' is misleading — seatId is just a JSON field inside the payload TEXT column, not a database column. The events table stores payload as a JSON string, so any key can be stored. This test doesn't validate any schema change; it only confirms that JSON round-trips correctly, which is already covered by existing event append/read tests. Consider removing or renaming to clarify it tests payload serialization of symposium-specific fields. [fixable]

server/ws-handler-v2.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🔵 style (L585): The 11-line TODO comment block (lines 585–595) explaining the cursor/seq-dedup trade-off is excessive inline documentation. The key insight ('client-side seq dedup handles duplicates between watch and reconnect POST') could be a 2-line comment. The detailed analysis of event loop ordering and HTTP request interleaving belongs in a design doc or commit message, not in the source code where it will rot. [fixable]

packages/protocol/__tests__/types.test.ts

Solid Phase 1 PR combining symposium types with a significant transport simplification (periodic sync removal, reconnect POST refactoring). The symposium additions are clean and complete. The main risk is the SSE reconnect refactoring: the staleness guard lost the ES-identity check (regression), and several edge-case tests were removed without replacement.

• 🔵 bugs (L215): The symposium type tests (lines 215–319) only verify TypeScript type assignability at compile time — they don't validate any runtime behavior, Zod schemas, or serialization. For example, SessionType[] = ['chat', 'symposium'] proves the type accepts these literals, but doesn't verify exhaustiveness (a third value added to the union wouldn't fail this test). These tests provide marginal value over the TypeScript compiler itself. Consider adding Zod schema validation tests if runtime validation is planned.

[dimakis]

🟡 unsafe_assumptions: When the reconnect POST fails, _pendingReconnectSessions retains the sessions for retry on the next welcome. However, checkAndReconnect() (line 172) does NOT clear _pendingReconnectSessions — it only sets _connected = false and creates a new EventSource. This means getReconnectSessions() will use the stale pending list on the next welcome, which is the intended design. But if an onerror fires (EventSource auto-reconnect, line 267), _connected is set to false but _pendingReconnectSessions is also preserved. This is correct for the retry intent, but if the server-side session has been cleaned up between failures, the client will keep retrying a reconnect POST for a session that no longer exists — there's no TTL or attempt limit on _pendingReconnectSessions. Consider adding a retry cap or TTL to prevent infinite reconnect POST attempts for dead sessions. [fixable]

[dimakis]

🟡 regressions: The old doReconnectPost had a staleness guard that checked BOTH this.es !== welcomeEs (EventSource instance identity) AND this._connectionId !== welcomeConnectionId. The new inline handler only checks this._connectionId !== postConnectionId. This drops the ES-identity guard. If checkAndReconnect() is called while a reconnect POST is in-flight, it closes the old ES and creates a new one, but _connectionId doesn't change until the new welcome arrives. During that window, if the stale POST resolves, the connectionId check alone won't catch it — _connectionId is still the old value because no new welcome has arrived yet. The old code would have bailed via the es !== welcomeEs check. This could cause _connected = true and flushPendingSends() to fire on a stale EventSource. [fixable]

[dimakis]

🔵 style: The test 'events table supports seat_id in payload' is misleading — seatId is just a JSON field inside the payload TEXT column, not a database column. The events table stores payload as a JSON string, so any key can be stored. This test doesn't validate any schema change; it only confirms that JSON round-trips correctly, which is already covered by existing event append/read tests. Consider removing or renaming to clarify it tests payload serialization of symposium-specific fields. [fixable]

[dimakis]

🔵 style: The 11-line TODO comment block (lines 585–595) explaining the cursor/seq-dedup trade-off is excessive inline documentation. The key insight ('client-side seq dedup handles duplicates between watch and reconnect POST') could be a 2-line comment. The detailed analysis of event loop ordering and HTTP request interleaving belongs in a design doc or commit message, not in the source code where it will rot. [fixable]

[dimakis]

🔵 bugs: The symposium type tests (lines 215–319) only verify TypeScript type assignability at compile time — they don't validate any runtime behavior, Zod schemas, or serialization. For example, SessionType[] = ['chat', 'symposium'] proves the type accepts these literals, but doesn't verify exhaustiveness (a third value added to the union wouldn't fail this test). These tests provide marginal value over the TypeScript compiler itself. Consider adding Zod schema validation tests if runtime validation is planned.

[dimakis]

Centaur Review

Found 6 issue(s) (3 warning).

packages/protocol/src/event-store.ts

Well-structured PR that cleanly adds Symposium Phase 1 types/migration and simplifies reconnect by removing periodic sync and deferring ownership to send/interrupt handlers. The main concern is an unsafe cast of session_type from the database; the reconnect simplification is sound with proper stale-callback guards.

• 🟡 unsafe_assumptions (L812): Unsafe cast of row.session_type to SessionMeta['sessionType'] without validation. If the database contains a value other than 'chat' or 'symposium' (e.g. from a manual edit or future migration rollback), the cast silently passes through an invalid string. Consider validating the value: row.session_type === 'symposium' ? 'symposium' : 'chat'. [fixable]

server/chat.ts

Well-structured PR that cleanly adds Symposium Phase 1 types/migration and simplifies reconnect by removing periodic sync and deferring ownership to send/interrupt handlers. The main concern is an unsafe cast of session_type from the database; the reconnect simplification is sound with proper stale-callback guards.

• 🔵 missing_tests (L1273): The new try/catch around queryInstance.interrupt() has no dedicated test. Adding a test where interrupt() rejects (e.g. ProcessTransport 'not ready for writing') would verify the function continues to queue the message and returns true despite the error. [fixable]

server/ws-handler-v2.ts

Well-structured PR that cleanly adds Symposium Phase 1 types/migration and simplifies reconnect by removing periodic sync and deferring ownership to send/interrupt handlers. The main concern is an unsafe cast of session_type from the database; the reconnect simplification is sound with proper stale-callback guards.

• 🟡 unsafe_assumptions (L584): After watch() on handleSendV2's running-session path, cursor starts at 0 (default) until the reconnect POST arrives and calls resetCursor. The inline comment acknowledges this relies on client-side seq dedup for correctness. While the TODO(P4) notes this, the window is real: if the server broadcasts a high-seq event between watch() and resetCursor(), the client receives it (fine), but a concurrent reconnect replay could also deliver the same events. The comment says 'bandwidth trade-off, not correctness' but duplicate events could confuse non-idempotent client logic (e.g. incrementing counters). Verify that all client message handlers are truly idempotent under seq dedup.
• 🟡 regressions (L266): Reconnect now reattaches detached sessions (refreshing transport) but no longer rekeys the clientId. The inline comment (lines 259-265) documents this is safe because 'event delivery uses the transport (refreshed)'. However, between reattach and the first send, getOwnerConnection() returns the stale connectionId. If another connection attempts a send/interrupt for the same session during this window, the ownership check sees the stale owner as 'gone' (not in connRegistry) and performs a takeover — which is correct behavior but may trigger spurious takeover logs. Not a bug, but worth being aware of in production log analysis.

packages/client/src/sse-connection.ts

Well-structured PR that cleanly adds Symposium Phase 1 types/migration and simplifies reconnect by removing periodic sync and deferring ownership to send/interrupt handlers. The main concern is an unsafe cast of session_type from the database; the reconnect simplification is sound with proper stale-callback guards.

• 🔵 style (L226): The reconnect POST .then(successCb, failureCb) form is harder to read than async/await with try/catch, especially since the success handler is 5 lines and failure handler is 4 lines. However, since the welcome event handler is synchronous and the POST is intentionally fire-and-forget, the .then() form avoids making the handler async. This is acceptable — just noting the trade-off.

packages/protocol/__tests__/event-store.test.ts

Well-structured PR that cleanly adds Symposium Phase 1 types/migration and simplifies reconnect by removing periodic sync and deferring ownership to send/interrupt handlers. The main concern is an unsafe cast of session_type from the database; the reconnect simplification is sound with proper stale-callback guards.

• 🔵 missing_tests (L901): The test 'events table supports seat_id in payload' only verifies that seatId round-trips through the JSON payload column. It doesn't test that the session associated with 'symp-4' was auto-created (upsertSession wasn't called). This works because append() auto-creates session rows, but makes the test implicitly dependent on that behavior. Consider calling store.upsertSession({ sessionId: 'symp-4', sessionType: 'symposium' }) first for clarity. [fixable]

[dimakis]

🟡 unsafe_assumptions: Unsafe cast of row.session_type to SessionMeta['sessionType'] without validation. If the database contains a value other than 'chat' or 'symposium' (e.g. from a manual edit or future migration rollback), the cast silently passes through an invalid string. Consider validating the value: row.session_type === 'symposium' ? 'symposium' : 'chat'. [fixable]

[dimakis]

🔵 missing_tests: The new try/catch around queryInstance.interrupt() has no dedicated test. Adding a test where interrupt() rejects (e.g. ProcessTransport 'not ready for writing') would verify the function continues to queue the message and returns true despite the error. [fixable]

[dimakis]

🟡 unsafe_assumptions: After watch() on handleSendV2's running-session path, cursor starts at 0 (default) until the reconnect POST arrives and calls resetCursor. The inline comment acknowledges this relies on client-side seq dedup for correctness. While the TODO(P4) notes this, the window is real: if the server broadcasts a high-seq event between watch() and resetCursor(), the client receives it (fine), but a concurrent reconnect replay could also deliver the same events. The comment says 'bandwidth trade-off, not correctness' but duplicate events could confuse non-idempotent client logic (e.g. incrementing counters). Verify that all client message handlers are truly idempotent under seq dedup.

[dimakis]

🟡 regressions: Reconnect now reattaches detached sessions (refreshing transport) but no longer rekeys the clientId. The inline comment (lines 259-265) documents this is safe because 'event delivery uses the transport (refreshed)'. However, between reattach and the first send, getOwnerConnection() returns the stale connectionId. If another connection attempts a send/interrupt for the same session during this window, the ownership check sees the stale owner as 'gone' (not in connRegistry) and performs a takeover — which is correct behavior but may trigger spurious takeover logs. Not a bug, but worth being aware of in production log analysis.

[dimakis]

🔵 style: The reconnect POST .then(successCb, failureCb) form is harder to read than async/await with try/catch, especially since the success handler is 5 lines and failure handler is 4 lines. However, since the welcome event handler is synchronous and the POST is intentionally fire-and-forget, the .then() form avoids making the handler async. This is acceptable — just noting the trade-off.

[dimakis]

🔵 missing_tests: The test 'events table supports seat_id in payload' only verifies that seatId round-trips through the JSON payload column. It doesn't test that the session associated with 'symp-4' was auto-created (upsertSession wasn't called). This works because append() auto-creates session rows, but makes the test implicitly dependent on that behavior. Consider calling store.upsertSession({ sessionId: 'symp-4', sessionType: 'symposium' }) first for clarity. [fixable]