lanspread/CALL_TO_PLAY_REVIEW_FABLE_5_XHIGH.md

# Call to Play review — Fable 5 (xhigh)

## Verdict

The plan is faithfully implemented — all five commits match the planned sequence, scope, and invariants, and the full acceptance suite passes on my machine: workspace tests (189), frontend tests (26), clippy, fmt, `git diff --check`, frontend `tsc`, and the docker scenarios S48 and S49. The few deviations from the plan's letter are genuine improvements. I found no correctness bugs. There is one architectural edge case worth knowing about (self-healing, arguably by design) and one real UX friction point.

## a) Plan fidelity

Each plan bullet traced to code:

- **Atomic merge** — `merge_batch_at` (`call_to_play.rs:83`) validates the whole batch, dedups, detects ID conflicts, evaluates tombstones/rootedness against retained-plus-batch, compacts exactly once, and only then commits. Store-unchanged-on-error is tested for both invalid and conflicting batches. `Create + AddTime` revival is tested in both input orders.
- **Acknowledged delivery** — `PROTOCOL_VERSION` 7, `CallToPlayAck` with all six planned outcomes, request/response in `send_call_to_play_events`, and the resync matrix in `deliver_to_peer`/`delivery_resync_reason` exactly matches the plan: transport failure/malformed/NeedHandshake/NeedHistory → one Hello resync; Rejected → logged, non-retriable; Applied/Duplicate/Obsolete → done. The IP check is gone; identity is roster membership + envelope==actor, and ARCHITECTURE.md now states plainly that this is not hostile-peer authentication.
- **Terminal retention** — 15-minute full-history window, then tombstone-for-session, separate 5-minute recovery window for unresolved calls, frontend `running`/`cancelled` states with `terminalAt`, badge exclusion, sort-last, disabled composer/controls, raw-event pruning, and doc updates. S49 proves late-joiner reconstruction of a terminal call.
- **Deadline extension** — `extendDeadline = max(now, deadline) + 5min`, card passes `nomination.deadline`, tested for both the early-ready and overdue cases.
- **Startup message** — the Tauri command returns `Ok(false)` only for uninitialized peer core and `Err(store reason)` otherwise, and the hook maps these to the four distinct messages without marking transport unavailable for store errors. The connecting message self-clears once the 2-second snapshot poll succeeds.

**Deviations, all justified:**

1. The plan said "rebuild event IDs from retained events." The implementation went further and **deleted the separate ID set entirely** — dedup scans retained history directly. This makes the "IDs correspond only to retained events" invariant structurally impossible to violate rather than merely maintained. Better than the plan.
2. "Always permit Start and Cancel at the cap" is implemented as a generalization: the 4,096 cap counts only *unresolved* events (`unresolved_event_count`), so a terminal action inherently passes because it resolves the call, and settled histories/tombstones never consume active capacity. Cleaner than special-casing two action types, and both behaviors are tested.
3. A nice detail beyond the plan: a pre-terminal chat message arriving *after* the call went terminal still merges into the read-only display during the 15-minute window (the obsolete check compares against the terminal event's order key, not mere terminal existence). That's consistent with "every visible call has its entire history."

## b) Architecture

The design holds up well. `merge_batch` is now the single choke point for every mutation path — local publish, live delivery, and handshake all flow through one atomic validate → dedup → apply → compact operation. That is exactly the seam the findings pointed at, and collapsing findings 2, 3, and 5 into it was the right call. Convergence comes from a grow-only deduplicated event set plus deterministic compaction, with no consensus machinery — appropriate for a trusted LAN.

Two observations, neither blocking:

- **Rootless tombstones don't propagate.** The "missing-root actions are not retained alone" invariant applies to Start/Cancel too, so a peer that joins *after* a call's 15-minute window never stores the creator's tombstone (its handshake merge returns `NeedHistory`, which in the handshake path only logs). If a third peer that slept through the finish later hands that fresh peer the stale active history, the finished call briefly resurrects on the fresh peer until its next handshake with any tombstone-holder roots the call and applies the tombstone. It self-heals and requires an unusual sequence (long-deadline scheduled call + offline peer + fresh joiner), so I think the trade-off is fine — but be aware of it, and note the secondary symptom: the fresh peer logs a "handshake omitted roots" warning on every handshake with a tombstone-holder for the rest of the session. If that log noise bothers you, downgrading that specific case to debug would be cheap.
- **The backend `HistoryIndex` and the frontend reducer are parallel implementations** of the same semantics (creator authority, `(at, id)` ordering, earliest-terminal-wins, latest-extension-wins). I checked them against each other and they agree today, including the subtle cases (forged terminal by non-creator, extension ordering, pre-create actions). This duplication is inherent to having a Rust store and a TS presentation reducer, but it's the seam most likely to drift — any future rule change must land in both `call_to_play.rs` and `callToPlay.ts`.

Minor: `merge_batch` clones the full store per call, so a live event costs O(n) — irrelevant under the 4,096 cap, just don't raise the cap by 100× without revisiting.

## c) User perspective

The lifecycle is now genuinely intuitive. "Time's up" being unresolved-but-recoverable, and "Running" being an explicit success receipt that only the creator's Start can produce, is a real conceptual improvement — deadline passage never silently claims a game happened. The ticker ordering supports this: Time's up ranks *first* (it needs the creator's attention), terminal receipts sink to the bottom in muted colors and don't inflate the badge. "Add 5 more minutes" finally does what it says. The startup message no longer sends users hunting for a game-folder problem that doesn't exist.

One real friction point: **when a call starts, participants get no launch affordance.** The creator's "Start now" auto-launches locally, but everyone else's card flips to a read-only "X is running." note — at precisely the moment they all need to launch the game, they must close the overlay and find it in the library. The plan specified terminal cards as read-only, so this is faithful — but a "Launch" button on the Running card (for participants who have the game installed) would remove the most awkward step in the happy path. Worth a follow-up commit if you agree.

Two nits: `design/launcher/SPEC.md` still describes the ticker sort as "ready → starting-soon → the rest (TICKER_RANK = ready 0, soon 1…)" while the code ranks `expired` first — that mismatch predates this branch, but since the spec section was touched anyway it could have been corrected. And the ticker's "waiting to start" line for Ready calls doesn't say *who* everyone is waiting for, while the card note does name the creator — a tiny inconsistency, fine as is.