From f608eaa6b160e49676034fc0c374fa73d038ff8a Mon Sep 17 00:00:00 2001 From: ddidderr Date: Fri, 24 Jul 2026 07:44:08 +0200 Subject: [PATCH] arrrrr... doch pub/private keys... --- FINDINGS_AUSWERTUNG_SOL.md | 129 +++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 FINDINGS_AUSWERTUNG_SOL.md diff --git a/FINDINGS_AUSWERTUNG_SOL.md b/FINDINGS_AUSWERTUNG_SOL.md new file mode 100644 index 0000000..43b464c --- /dev/null +++ b/FINDINGS_AUSWERTUNG_SOL.md @@ -0,0 +1,129 @@ +# Call to Play review findings evaluation + +The implementation is fundamentally sound, but I would make four focused +follow-ups before calling it finished. Two address real behavioral gaps; two +complete the new lifecycle cleanly. + +## Worth fixing before finish + +### 1. Propagate rootless terminal tombstones + +This is the most important finding. + +After 15 minutes, a peer retains only the `Start` or `Cancel` event. A fresh +peer receiving that tombstone during handshake currently rejects it as missing +its `Create` root, and the handshake path merely logs the missing root. + +That means the fresh peer has no protection against a stale peer later +resurrecting the finished call. It only self-heals if a tombstone-holder is +still online afterward. + +I would: + +- Add an explicit handshake merge policy that permits rootless `Start` and + `Cancel` events as non-visible tombstones. +- Keep local and live orphan actions returning `NeedHistory`. +- Never expose a rootless tombstone as a nomination in the UI. +- Verify that a later stale complete history is classified obsolete. +- Add store and handshake tests for this sequence. + +This restores the intended "tombstone prevents resurrection for the rest of the +session" invariant. I would fix the semantics, not merely downgrade the +repeated warning. + +### 2. Distinguish missing game directory from peer startup + +Kimi is correct here. The peer is only started after a valid directory reaches +`update_game_directory`. Without one, "still connecting -- try again" never +becomes true. + +I would: + +- Model directory readiness as `checking | missing | ready`, rather than + passing only a boolean that conflates hydration with a known missing + directory. +- Pass that prerequisite state into `useCallToPlay`. +- Show folder guidance only for the confirmed `missing` state. +- Preserve the current connecting message for `checking` or + `ready-but-peer-starting`. +- Test both states and the transition after a valid directory is selected. + +That finishes the original finding's full intent without returning to +misleading folder advice during normal startup. + +### 3. Add a local Launch action to Running receipts + +Fable's UX point is persuasive. Participants currently reach the key moment +and see only that the game is running. + +I would add a local-only Launch button when: + +- The call is `running`. +- The game is installed and launchable locally. +- No conflicting operation prevents launch. + +This would not violate the read-only terminal invariant: launching the local +game does not mutate the replicated call. I would use a dedicated play +callback rather than the generic primary action, so a button labelled "Launch" +cannot unexpectedly initiate an install or update. + +### 4. Make terminal receipts visually static and correct the spec + +The terminal receipt details are small but real: + +- `MiniBubbles` continue advancing pending-ready countdowns after a call is + Running or Cancelled. +- Terminal cards still draw empty seats. +- The specification still says "one row per active call" and documents the old + ticker rank, despite terminal rows and expired-first ordering. + +I would: + +- Freeze participant readiness at `terminalAt`, or render terminal + participants without countdown tags. +- Suppress empty roster slots on terminal cards. +- Change Ready ticker text to name the creator. +- Update the ticker specification to match the actual visible-call and ranking + behavior. + +## Coverage to add alongside those fixes + +I would close the acknowledgement-heal test gap, but without introducing a full +React test stack or waiting five real minutes: + +- Exercise `NeedHistory` from a live orphan action. +- Apply the full history through the handshake merge path. +- Assert the receiver is revived exactly once. +- Cover the new rootless-tombstone handshake followed by stale-history + rejection. +- Rename the IP test so its name honestly describes what it proves; the current + helper no longer accepts a source address. + +## Findings not worth pursuing now + +- **Backend/frontend semantic duplication:** a real maintenance risk, but + eliminating it would require an architectural rewrite. Shared conformance + fixtures could be a later improvement. +- **Full-store cloning per merge:** acceptable under the 4,096 unresolved-event + cap. +- **Substring-matched frontend errors:** brittle, but currently pinned by + tests; a proper fix requires a typed peer-to-Tauri error contract and is + disproportionate for finishing this branch. +- **Missing component-test infrastructure:** inspection plus pure reducer tests + is adequate here; I would not add a UI test framework solely for these + controls. +- **Time's-up calls in the badge:** intentional. These calls remain unresolved + and visible, even for non-creators. +- **The direct-reply observation:** there is nothing to fix; the CLI already + uses the reply channel. + +## Recommended finish scope + +The recommended finish scope is: + +1. Tombstone propagation. +2. Accurate directory/startup diagnosis. +3. A Running-card Launch action. +4. Terminal-receipt polish and specification corrections. +5. Targeted replication tests. +