lanspread/CALL_TO_PLAY_REVIEW_KIMI_K3.md

Review: Call to Play fix series (872692e..2c204ac)

a) Faithfulness to the plan — high, with only minor test-plan gaps

Commit mapping is 1:1 with the planned sequence, same titles, and every bullet lands:

Plan	Implementation	Verdict
1. Atomic merge	merge_batch validates + dedups the whole batch, conflicts reject without mutation (self.events only assigned at the end), one post-merge compaction, IDs derived from retained events, orphan actions → missing_call_ids, Create+AddTime revives in any order (tested both orders), applied = only what survives compaction, cap counts only unresolved events so Start/Cancel always settle	✓ Faithful
2. Acknowledged replication	PROTOCOL_VERSION 6→7, CallToPlayAck with exactly the six planned outcomes, IP check removed, roster + actor-matches-envelope checks, NeedHandshake/NeedHistory/transport/malformed → one Hello/HelloAck resync, Applied/Duplicate delivered, Obsolete finished, Rejected logged non-retriable, publication spawns deliveries into task_tracker and returns after local merge (verified: join_all is inside the spawned task, so an offline peer can't block the UI), ARCHITECTURE.md documents the trust model honestly	✓ Faithful
3. Terminal retention	15-min full terminal histories in snapshots, then tombstone-only for the session; unresolved Time's-up evicted as a unit after 5 min; running/cancelled states with terminalAt; ticker/overlay rendering, sorted last, excluded from badge; unknown-game degraded rendering kept; frontend raw-map pruning; SPEC.md + ARCHITECTURE.md updated incl. clock-skew assumption; S49 added	✓ Faithful
4. Extend from current deadline	max(now, deadline) + 5min, overdue case preserved, terminal calls non-extendable (controls unrendered)	✓ Faithful
5. Startup message	Exact planned wording, store errors no longer mark transport unavailable, four distinct messages (startup / obsolete / full / unexpected), connecting message auto-clears once the peer is ready	✓ Faithful

All six Fable-5 findings are addressed, and every invariant in the plan's list verifiably holds in the final code. The "merge histories atomically" fix correctly treats findings 2+5 as one problem, as the findings demanded.

Test-plan gaps (minor):

1. No end-to-end test of the triggered heal. The plan's "NeedHistory, NeedHandshake, and lost acknowledgements trigger idempotent handshake healing" is only covered at the delivery_resync_reason decision level. S48/S49 prove handshake-based reconstruction, but nothing drives an orphan-AddTime → NeedHistory → resync → revival sequence. Understandable (needs 5-min waits or clock mocking), but it's a real gap against the plan's own list.
2. "Terminal controls and chat composer are disabled" has no automated test (frontend tests are lib-level only; there is no component-test infrastructure). Verified by inspection instead.
3. "Fix snapshot waiting via the direct reply path" turned out to be a no-op — the CLI's call_to_play_events already polls through the reply channel. Justified deviation.

Nits: known_peer_id_accepts_live_events_without_transport_ip_matching is somewhat vacuous (the function no longer takes remote_addr, so IP mismatch can't even be expressed) — harmless as intent documentation. callToPlayPublishErrorMessage substring-matches backend error strings — a brittle coupling, though the tests pin the current wording.

b) Architecture — sound choices throughout

• Derived event IDs + tombstones is the elegant part: the ID set is bounded by construction, revival works, resurrection is blocked, and finding 2's "retained IDs correspond to retained events and deliberate terminal tombstones" is literally realized.
• Fail-closed batch rejection (invalid/conflict → store untouched) is the right default under the trusted-LAN model. The trade-off — one bad event voids an entire handshake heal — is practically unreachable: IDs are UUIDs and validation is deterministic and identical sender-side.
• Retention symmetry is the quiet win: backend compaction and frontend derivation use the same constants (5/15 min) keyed off event timestamps, not receipt times. All peers converge on identical visibility with zero extra protocol, and the S49 late-joiner case falls out naturally.
• Ack semantics map 1:1 onto merge outcomes, and the heal is idempotent (lost ack → resync → duplicate). Not rebroadcasting live events keeps it loop-free. The identity story is now honest: roster + actor match, documented as not-authentication.
• Capacity on unresolved history only fixes the "Start at cap" deadlock and keeps settled calls from pressuring new ones. Tombstones accumulate one small event per finished call per session — negligible and deliberate.
• The handshake receiver logging missing roots rather than re-requesting is correct — the handshake is itself the heal, and re-requesting would loop.

c) User experience — a genuine improvement; lifecycle finally coherent

The old flow had two genuinely weird behaviors: a started call vanished after 3 seconds, and a cancelled call vanished instantly — mid-conversation, for everyone. The new flow (Open/Ready → Time's up, recoverable → Running/Cancelled receipts for 15 min → retired) matches how a LAN party actually works: "who's playing what right now?" is answerable at a glance, receipts sort last, stay out of the badge, and chat remains readable. "Time's up" vs "Running" being distinct states (unresolved vs final receipt) is the right call — deadline passage never implies the game started. Add-time now does what its label says, the startup message no longer blames the wrong cause, and error messages are specific and actionable ("Start or cancel an active call, then try again").

Residual friction, in decreasing order of importance:

1. First-run users see "still connecting… try again in a moment" forever. The peer only starts via update_game_directory, so without a game folder there is no "moment" after which it connects. Finding 6 explicitly wanted folder guidance reserved for a known missing-folder condition — the plan narrowed that to just the connecting message and the implementation follows the plan, so this is faithful, but the finding's full intent isn't realized. The frontend already has hasGameDirectory in useGameDirectory; wiring it into the Call to Play error path would close this cheaply. This is the one place where deviating from the plan would have been justified.
2. Terminal ticker rows render MiniBubbles with live "ready in Xm" countdown tags for participants whose readyAt hadn't elapsed — a Running receipt with ticking countdowns looks slightly alive when it's meant to be a receipt.
3. Terminal cards render empty roster slots up to maxPlayers — on a Cancelled receipt, empty slots can read as "seats still open".
4. Pre-existing, out of scope: the badge counts Time's-up calls for everyone, though only the creator can act on them.

Verdict

Approve. The plan is implemented faithfully and, where it matters (derived IDs, atomic merge, ack-driven healing, retention symmetry), the execution is as good as or better than the plan described. Architecture and UX are coherent. Before merging I'd only consider: (1) the missing-folder special case, since the finding called for it and the data is already available in the frontend, (2) the two cosmetic terminal-receipt nits, and (3) noting the untested heal loop as a known coverage gap — none of which are blockers.