feat: add scoped BO3 concession controls - #6817
Conversation
📝 WalkthroughWalkthroughThis PR adds full-session identity fencing, durable terminal-result delivery, bound match concessions, and authorization-backed Bo3 intergame flows across client, server, persistence, and engine layers. It updates protocols, resume handling, concession UI, localized copy, and integration coverage. ChangesMultiplayer authority and terminal delivery
Draft settlement and Bo3 intergame flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
client/src/adapter/p2p-adapter.ts (1)
2246-2320: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReconnect never re-delivers the retained terminal result.
commitTerminalIfCompletefansterminal_resultout only to sessions present at commit time (Line 1796). A guest that was disconnected at that moment (or drops before the frame flushes) reconnects, getsreconnect_ackwith the current revision, and then receives nothing further — the host holdsthis.terminalResultbut never replays it, so that guest is stuck in a live-game UI on a finished game.gameRunState === "terminal"also means their next action is rejected withGame terminalrather than a settled result.Since the terminal statement is recipient-bound (recipient +
finalStateCommitment+ revision), the reconnect path needs to mint and send a fresh recipient-scoped result after the ack, using the revision it just stamped.🔧 Sketch
} else { const snapshot = await this.wasm.getViewerSnapshot(pid as PlayerId); void this.send(session, { type: "reconnect_ack", ... }); } + if (this.terminalResult) { + await this.redeliverTerminalResult(session, pid as PlayerId); + } })();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-adapter.ts` around lines 2246 - 2320, Update handleReconnect so that after sending reconnect_ack, it checks whether terminalResult is retained and the game is terminal, then mints and sends a fresh recipient-scoped terminal_result for the reconnecting player using the same revision stamped in the ack. Preserve the existing native and WASM snapshot handling, and reuse the established terminal-result construction and send flow rather than replaying a previously recipient-bound message.crates/server-core/src/protocol.rs (1)
837-846: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThese test match arms are now non-exhaustive struct patterns.
GameCreatedandSessionAttachedgainedfull_key, and the construction sites were updated (lines 833, 1110) but the destructuring arms were not —ServerMessage::GameCreated { game_code, player_token }no longer covers all fields, socargo testfails to compile.🐛 Proposed fix
ServerMessage::GameCreated { game_code, player_token, + .. } => {ServerMessage::SessionAttached { game_code, player_id, player_token, + .. } => {Also applies to: 1114-1126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server-core/src/protocol.rs` around lines 837 - 846, Update the test match arms for ServerMessage::GameCreated and ServerMessage::SessionAttached to destructure the newly added full_key field, either by binding it or explicitly ignoring it. Preserve the existing assertions for game_code and player_token while making both struct patterns exhaustive.crates/phase-server/src/main.rs (1)
2582-2631: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftSynchronous SQLite work runs while the global
SessionManagermutex is held.
create_full_session_keyandinitialize_full_runtimeboth perform blockingrusqlitetransactions (including the single-user activation write) inside thestate.lock().awaitcritical section, on the async executor thread. Every other game's action handling in this process stalls behind that disk commit. Everywhere else in this file persistence goes throughtokio::task::spawn_blocking(persist_full_session_async,prepare_full_terminal,retire_unstarted_session_async).Allocate the key and run the activation/first save outside the lock, then install
full_runtimeunder a short second lock (rolling the game back if the session vanished meanwhile).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/main.rs` around lines 2582 - 2631, Refactor the game-creation flow around create_full_session_key and initialize_full_runtime so all blocking SQLite key-generation and activation/initial-save work runs via spawn_blocking before holding the SessionManager lock. After completion, reacquire a short lock to install full_runtime, and remove/rollback the game if the session no longer exists or the blocking operation fails; preserve the existing returned values and error handling.client/src/network/draftProtocol.ts (1)
97-134: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBackfill
DraftMatchLaunch.bindingon restorematchLaunchesare persisted without any compatibility gate, but resumed Bo3 flows readlaunch.binding.matchAuthoritySeatandloadDraftSettlementOutbox(launch.binding). A snapshot created before this field existed can break recovered matches after reload. Rehydrate the binding fromsession.matchBindingsor drop incompatible launches during restore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/network/draftProtocol.ts` around lines 97 - 134, Update the restore logic for persisted matchLaunches to validate that each launch has a compatible binding before resumed Bo3 flows access launch.binding. Rehydrate missing binding data from session.matchBindings when possible; otherwise discard the incompatible launch, ensuring restored entries always support launch.binding.matchAuthoritySeat and loadDraftSettlementOutbox(launch.binding).client/src/services/gamePersistence.ts (1)
82-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop legacy P2P host records without
sessionKey.loadP2PHostSession()currently returns raw IDB data, so older saved sessions still reach the host-resume path withsessionKey === undefinedand get fed intoloadP2PTerminalResult(...). Add a guard here to reject or migrate stale records before resume.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/services/gamePersistence.ts` around lines 82 - 87, Update loadP2PHostSession() to validate persisted records before returning them, rejecting or migrating legacy entries that lack sessionKey. Ensure only records containing a valid sessionKey reach the host-resume path and loadP2PTerminalResult(...).
🟡 Minor comments (4)
client/src/adapter/p2p-draft-host.ts-1010-1016 (1)
1010-1016: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA conflicting receipt ID is dropped with neither an ack nor an error.
When
settlementReceiptsalready holds a differentreceiptId, the method returns silently. The match-authority seat that regenerated a receipt ID (e.g. it never saw the first ack and its local record was rebuilt) then waits forever: nodraft_match_settlement_ack, nodraft_error. Every other rejection path on this method reports back — this one should too.🛡️ Proposed fix
const receipt = this.settlementReceipts.get(binding.matchId); if (receipt) { if (receipt.receiptId === settlement.receiptId) { void this.sendSettlementAck(submittingSeat, binding.matchId, receipt); + } else { + this.guestSessions.get(submittingSeat)?.send({ + type: "draft_error", + reason: "Match already settled", + }); } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-draft-host.ts` around lines 1010 - 1016, Update the settlement receipt handling around settlementReceipts and sendSettlementAck so a receiptId mismatch sends the submitting seat an appropriate draft_error before returning. Preserve the existing acknowledgement for matching receipt IDs and ensure every conflicting receipt is reported instead of being silently dropped.crates/phase-server/src/main.rs-5480-5487 (1)
5480-5487: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrefer a rejection over
.expecton the join path.A missing
full_runtimehere panics the socket task mid-join, leaving the joiner seated inSessionManagerwith no reply. Since the surrounding block already evaluates toResult<JoinOutcome, String>, return an error string instead so the existingErr(e)arm reports it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/main.rs` around lines 5480 - 5487, Replace the `.expect` on `session.full_runtime.as_ref()` in the `JoinOutcome::Waiting` construction with error propagation that returns a descriptive `Err(String)` when `full_runtime` is missing. Preserve the existing `Ok(JoinOutcome::Waiting { ... })` path when the runtime and key are available so the surrounding `Err(e)` handling reports the rejection instead of panicking.crates/server-core/src/session.rs-1432-1443 (1)
1432-1443: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winForward the concession into
log_entries.
apply_trusted_match_forfeitonly producesGameEvent::GameOver, while the normal action path forwardsresult.log_entries; this branch hardcodesVec::new(), so the concession never reaches the game log. Return the forfeit log entry here or have the engine emit it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server-core/src/session.rs` around lines 1432 - 1443, Update the return tuple in apply_trusted_match_forfeit so the log_entries position forwards the concession/forfeit log entry instead of hardcoding Vec::new(). Ensure the entry is produced by the engine or returned alongside GameEvent::GameOver, matching the normal action path’s result.log_entries behavior.client/src/pages/GamePage.tsx-436-438 (1)
436-438: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
terminalUnavailableshows untranslated, frontend-authored copy. The event carries an opaquemessageminted in the adapter/provider layer, wheret()isn't available, andGamePagetoasts it verbatim. Unlikedisplay.reason(server-authored, correctly raw) andrequestRejected.reason(server-authored), strings like "Failed to retain terminal delivery" and "Terminal result is unavailable" are frontend-authored user-facing text and must route throught(). Carrying a typed reason on the event and localizing at the presentation boundary fixes all sites at once.
client/src/pages/GamePage.tsx#L436-L438: switch on a typedevent.reasonand callt()with amultiplayerkey instead ofshowToast(event.message).client/src/pages/GamePage.tsx#L664-L666: same change for the P2PterminalUnavailablecase.client/src/adapter/ws-adapter.ts#L1451-L1478: replace the authoredmessage: "Failed to retain terminal delivery"with a typed reason (e.g.reason: "retention-failed" | "transport"), keeping any rawError.messageas a separate diagnostic field rather than the displayed text.client/src/providers/GameProvider.tsx#L1149-L1157: same for the threeterminalUnavailableemits here ("Terminal result is unavailable", "Failed to retain terminal delivery" at Line 1185) — emit the typed reason and letGamePagelocalize it.As per path instructions, "frontend-authored user-facing text (titles, labels, buttons, tooltips, placeholders, log templates) must route through
t(); engine/card pass-through … must NOT be wrapped int()". Based on learnings, engine/server-provided strings stay raw — which is whydisplay.reasonis correct as-is and only these authored failure strings need keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/GamePage.tsx` around lines 436 - 438, Replace frontend-authored terminal-unavailable messages with a typed reason and localize them at the presentation boundary: in client/src/pages/GamePage.tsx lines 436-438 and 664-666, switch on event.reason and pass the corresponding multiplayer translation key to t() instead of displaying event.message. In client/src/adapter/ws-adapter.ts lines 1451-1478 and client/src/providers/GameProvider.tsx lines 1149-1157, emit typed reasons for retention and transport failures, keeping raw Error.message separate as diagnostic data; leave server- or engine-authored reasons raw.Sources: Path instructions, Learnings
🧹 Nitpick comments (13)
client/src/adapter/p2p-adapter.ts (1)
3039-3063: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead re-checks in
acceptsHostAuthority.Line 3046 already returns when
this.authority === null, so thethis.authority &&at Line 3049 and thethis.authority !== null &&at Line 3060 can never be false. Drop them so the intent (per-message-type incarnation policy) reads unambiguously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-adapter.ts` around lines 3039 - 3063, Update acceptsHostAuthority to remove the redundant this.authority && guard in the reconnect_ack session-key comparison and the redundant this.authority !== null check in the final authority comparison, while preserving the existing per-message-type authority validation behavior.client/src/services/draftPersistence.ts (1)
59-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
bo3Statere-declares the host'sBo3MatchStateshape inline.
p2p-draft-host.tsalready ownsBo3MatchState(and maps it into/out of this array field), so the deck/score/seat shape now exists in two places and will drift the next time Bo3 state gains a field — with no compile error, since the persisted array is structurally typed. Export the host-side state type (or aPersistedBo3MatchState = { matchId: string } & Bo3MatchState) and reference it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/services/draftPersistence.ts` around lines 59 - 79, Replace the inline element type of DraftPersistence.bo3State with the exported Bo3MatchState-based persisted type from p2p-draft-host.ts, preserving the matchId field as required. Update the host-side type export and import/reference it here so Bo3 state fields have a single source of truth.client/src/services/intergameCommandLedger.ts (1)
96-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
hold()trusts the caller-suppliedlaunchDigest.
payloadDigestis derived, butlaunchDigestis taken verbatim whilelaunchPayloadis deep-cloned right beside it — nothing guarantees the two agree, and a mismatched pair yields a command no acknowledgement can ever satisfy. Derive it:launchDigest: draftIntergameDigest(command.launchPayload), and drop it from theholdparameter type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/services/intergameCommandLedger.ts` around lines 96 - 105, Update IntergameCommandLedger.hold to derive launchDigest from command.launchPayload using draftIntergameDigest, matching the existing payloadDigest derivation. Remove launchDigest from the hold parameter type so callers cannot supply an inconsistent value, while preserving the immutable launchPayload handling and pending status.client/src/services/__tests__/intergameCommandLedger.test.ts (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPermit single-use is untested.
consumeIntergamePermitdeletes the WeakMap entry, which is the actual one-shot gate; the suite only asserts the happy path once. Addexpect(consumeIntergamePermit(permit!, acknowledgement)).toBe(false)on a second call, plus a case where the permit is consumed with a different ack (cross-matchlaunchDigest) — that's the reusable primitive worth covering rather than only the single sequence.Test reusable primitives and parameterized handlers rather than only a single case — as per coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/services/__tests__/intergameCommandLedger.test.ts` around lines 25 - 33, Add coverage in the intergame permit tests around consumeIntergamePermit: assert a second consumption with the same acknowledgement returns false, and add a cross-match case using a different acknowledgement whose launchDigest does not match. Keep the existing happy-path sequence and test the reusable permit primitive directly rather than only through the controller flow.Source: Coding guidelines
client/src/stores/multiplayerDraftStore.ts (1)
947-947: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate launch→engine-seat mapping.
seatForLaunchGamePlayeralready encodes the launch-type→seat correspondence; this line hand-inlines its inverse. ExtractgamePlayerForLaunch(launch)next to it so the two directions can't drift (e.g. if a launch variant is added).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/stores/multiplayerDraftStore.ts` at line 947, Replace the inline actor assignment near seatForLaunchGamePlayer with a shared gamePlayerForLaunch(launch) helper defined alongside it, and use that helper to derive the engine seat. Preserve the existing HumanGuest-to-1 and other-launch-types-to-0 behavior while centralizing the bidirectional mapping.client/src/services/p2pTerminalResult.ts (1)
110-121: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCommitment hashes a non-canonical encoding.
JSON.stringify(state)is key-insertion-order dependent, so the host (hashing its wasm/native-produced object) and the recipient (hashing an object rebuilt byJSON.parseof a possibly differently-ordered frame) can disagree and produce a spurious "did not match the final state" rejection inacceptTerminalResult. The repo already has a canonical serializer —canonicalizeinservices/intergameCommandLedger.ts— reuse it here instead of relying on stringify ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/services/p2pTerminalResult.ts` around lines 110 - 121, Update p2pFinalStateCommitment to serialize state with the repository’s canonicalize helper from intergameCommandLedger.ts instead of JSON.stringify, while preserving the existing UTF-8 encoding, SHA-256 digest, and sha256-prefixed hex result.client/src/stores/__tests__/multiplayerDraftStore.test.ts (1)
286-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the binding is forwarded, not just
winnerSeat.
objectContaining({ winnerSeat })passes even if the store forwards a wrong, stale, or emptybinding/receiptId— which is the actual contract this PR introduces (the host rejects anything that failssameBinding). Tighten to includebinding: matchPairing.bindingand a non-emptyreceiptId.💚 Proposed tightening
- expect(mockHostAdapter.submitMatchSettlement).toHaveBeenCalledWith(expect.objectContaining({ - winnerSeat: 4, - })); + expect(mockHostAdapter.submitMatchSettlement).toHaveBeenCalledWith(expect.objectContaining({ + winnerSeat: 4, + receiptId: expect.any(String), + binding: expect.objectContaining({ matchId: "match-1", sessionKey: "session-1", nonce: "nonce-1" }), + }));Also applies to: 325-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/stores/__tests__/multiplayerDraftStore.test.ts` around lines 286 - 288, Strengthen the submitMatchSettlement assertions in the multiplayer draft store tests to verify the forwarded settlement binding, not only winnerSeat. In each affected expectation, include binding: matchPairing.binding and assert that receiptId is non-empty while preserving the existing winnerSeat assertion.client/src/adapter/p2p-draft-host.ts (1)
966-983: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPersist amplification per pairing dispatch.
persistSession()now serialises match bindings, the settlement outbox/receipts, the ledger snapshot, bo3 state, launch digests and every storedDraftMatchLaunch— anddispatchMatchLaunchtriggers it three times per pairing (matchBindingFor, plussendMatchLaunchfor each seat), each one awaiting a fulladapter.exportSession()throughpersistQueue. For an 8-seat pod that is ~12 full-session exports per round. Consider persisting once at the end ofdispatchMatchLaunch/generatePairingsinstead of inside these helpers.Also applies to: 1136-1152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-draft-host.ts` around lines 966 - 983, Reduce redundant session persistence during pairing dispatch: remove the persistSession call from matchBindingFor and the corresponding per-seat persistence in sendMatchLaunch, then invoke persistSession once after dispatchMatchLaunch or generatePairings completes. Preserve binding creation and launch behavior while ensuring the final state is persisted after all pairings and launches are processed.crates/phase-server/src/persistence.rs (1)
906-952: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a uniqueness constraint backing this idempotency check.
The exactly-once guarantee rests entirely on the read-then-write inside one transaction. A
UNIQUE (game_code, player_key)index onranked_match_historywould make a double application impossible at the storage layer (and would let you detect the conflict instead of silently double-crediting if the read path is ever refactored). It would also give theWHERE game_code = ?1lookup an index — today it's a full scan of the history table on every ranked game.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/persistence.rs` around lines 906 - 952, The idempotency check in save_ranked_result_idempotent lacks storage-level uniqueness and an indexed lookup. Add a UNIQUE constraint or unique index on ranked_match_history(game_code, player_key), ensure the insert path propagates its conflict as an error rather than applying a second result, and retain the existing same-game validation and receipt behavior.crates/phase-server/src/main.rs (2)
3595-3627: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThese three handlers call blocking SQLite directly on the socket task.
bootstrap_terminal_delivery/read_terminal_result/ack_terminal_deliveryare synchronousrusqlitecalls awaited inline, unlike the rest of the terminal layer which wraps persistence inspawn_blocking. Under contention on the shared connection mutex this parks an executor thread. Wrapping them keeps the pattern uniform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/main.rs` around lines 3595 - 3627, Update the ClientMessage handlers for BootstrapTerminalDelivery, ReadTerminalResult, and AckTerminalDelivery to execute their synchronous game_db calls inside spawn_blocking, awaiting each task before constructing the existing ServerMessage responses. Preserve the current success and error mappings while ensuring the socket task never invokes bootstrap_terminal_delivery, read_terminal_result, or ack_terminal_delivery directly.
1143-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-serializing the already-deserialized snapshot is a needless round trip.
load_active_full_sessionsreturns a typedFullPersistSnapshot; here it is re-serialized to JSON purely sorestore_persisted_sessioncan parse it back. Consider exposing arestore_persisted_session_from(persisted: PersistedSession, db)variant and keeping the string path for the legacy loader.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/main.rs` around lines 1143 - 1158, The persisted session restoration loop should avoid serializing and reparsing the already-typed snapshot data. Add a typed restoration variant such as restore_persisted_session_from that accepts the snapshot’s PersistedSession value and db, use it from the persisted_games loop, and retain restore_persisted_session’s JSON-based path for the legacy loader.crates/server-core/src/session.rs (1)
1663-1682: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
handle_match_concedehas no test in this module.The snapshot round-trip is covered, but the new authenticated concession path — wrong token rejected, pending-takeback rejected,
match_forfeit_result/waiting_forreachingGameOverwith the right winner — is untested. This file already has the two-player fixture (setup_two_player_game) needed for it. Want me to draft those cases?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server-core/src/session.rs` around lines 1663 - 1682, Extend the session tests with coverage for handle_match_concede using the existing setup_two_player_game fixture: verify an incorrect token is rejected, a concession during pending takeback is rejected, and match_forfeit_result/waiting_for transition the match to GameOver with the correct winner. Keep the assertions focused on the authenticated concession path and resulting game state.client/src/pages/__tests__/GamePage.bracketViolation.test.tsx (1)
649-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the paired Bo1 negative, and drop the duplicate mock.
Two nits in the new suite:
FakeWebSocketAdapterdeclaressendMatchConcede = vi.fn()(Line 651) and the test then overwrites it with a secondvi.fn()(Line 659-661). The class field is dead — keep one.- There's no negative case.
matchActionis gated onsupportsMatchConcede(adapter) && isBestOfThree, so a Bo1 fixture assertingmatchAction === undefinedis what proves the gate isn't simply always-on — the same positive/negative pairing every other suite in this file uses ("withholds takeback when the adapter cannot send it").♻️ Drop the duplicate mock and add the Bo1 negative
describe("GamePage — bound whole-match concession", () => { class FakeWebSocketAdapter extends WebSocketAdapter { sendMatchConcede = vi.fn(); constructor() { super("ws://test/ws", "host", { main_deck: [], sideboard: [] }); } } + function seedBo3State(matchType: "Bo1" | "Bo3") { + const waiting = { + type: "BetweenGamesChoosePlayDraw", + data: { player: 0, game_number: 2, score: { p0_wins: 1, p1_wins: 0, draws: 0 } }, + }; + storeOverrides.gameState = { + match_config: { match_type: matchType }, + waiting_for: waiting, + players: [], + objects: {}, + battlefield: [], + stack: [], + exile: [], + }; + storeOverrides.waitingFor = waiting; + } + it("offers and invokes the WebSocket whole-match capability for a Bo3", () => { - const sendMatchConcede = vi.fn(); const adapter = new FakeWebSocketAdapter(); - adapter.sendMatchConcede = sendMatchConcede; storeOverrides.adapter = adapter; - storeOverrides.gameState = { /* … */ }; - storeOverrides.waitingFor = { /* … */ }; + seedBo3State("Bo3"); renderGamePage("/game/test-game-123?mode=host"); act(() => (capturedGameMenuProps?.onConcede as () => void)()); const matchAction = capturedConcedeDialogProps?.matchAction as | { onConfirm: () => void } | undefined; expect(matchAction?.onConfirm).toBeTypeOf("function"); act(() => matchAction?.onConfirm()); - expect(sendMatchConcede).toHaveBeenCalledOnce(); + expect(adapter.sendMatchConcede).toHaveBeenCalledOnce(); }); + + it("withholds the whole-match capability outside a Bo3", () => { + storeOverrides.adapter = new FakeWebSocketAdapter(); + seedBo3State("Bo1"); + + renderGamePage("/game/test-game-123?mode=host"); + act(() => (capturedGameMenuProps?.onConcede as () => void)()); + + // Reach guard: the dialog really rendered, so the absent action is the gate. + expect(capturedConcedeDialogProps?.gameAction).toBeDefined(); + expect(capturedConcedeDialogProps?.matchAction).toBeUndefined(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/__tests__/GamePage.bracketViolation.test.tsx` around lines 649 - 698, In the “GamePage — bound whole-match concession” suite, keep only one mock for FakeWebSocketAdapter.sendMatchConcede by removing either the class-field mock or the test-level reassignment. Add a paired Bo1 test using the same setup, assert capturedConcedeDialogProps.matchAction is undefined after opening the concede flow, and retain the existing Bo3 assertion that the action is offered and invokes sendMatchConcede.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-server/src/main.rs`:
- Around line 3903-3917: Update the terminal-artifact handling in
handle_client_message, including the game-over, concede, and concede-match
branches, so terminal_artifact failures are propagated through the surrounding
Result or match flow rather than using ? in a function returning (). Preserve
the existing terminal behavior for successful artifact creation and handle each
error through the established handler flow.
---
Outside diff comments:
In `@client/src/adapter/p2p-adapter.ts`:
- Around line 2246-2320: Update handleReconnect so that after sending
reconnect_ack, it checks whether terminalResult is retained and the game is
terminal, then mints and sends a fresh recipient-scoped terminal_result for the
reconnecting player using the same revision stamped in the ack. Preserve the
existing native and WASM snapshot handling, and reuse the established
terminal-result construction and send flow rather than replaying a previously
recipient-bound message.
In `@client/src/network/draftProtocol.ts`:
- Around line 97-134: Update the restore logic for persisted matchLaunches to
validate that each launch has a compatible binding before resumed Bo3 flows
access launch.binding. Rehydrate missing binding data from session.matchBindings
when possible; otherwise discard the incompatible launch, ensuring restored
entries always support launch.binding.matchAuthoritySeat and
loadDraftSettlementOutbox(launch.binding).
In `@client/src/services/gamePersistence.ts`:
- Around line 82-87: Update loadP2PHostSession() to validate persisted records
before returning them, rejecting or migrating legacy entries that lack
sessionKey. Ensure only records containing a valid sessionKey reach the
host-resume path and loadP2PTerminalResult(...).
In `@crates/phase-server/src/main.rs`:
- Around line 2582-2631: Refactor the game-creation flow around
create_full_session_key and initialize_full_runtime so all blocking SQLite
key-generation and activation/initial-save work runs via spawn_blocking before
holding the SessionManager lock. After completion, reacquire a short lock to
install full_runtime, and remove/rollback the game if the session no longer
exists or the blocking operation fails; preserve the existing returned values
and error handling.
In `@crates/server-core/src/protocol.rs`:
- Around line 837-846: Update the test match arms for ServerMessage::GameCreated
and ServerMessage::SessionAttached to destructure the newly added full_key
field, either by binding it or explicitly ignoring it. Preserve the existing
assertions for game_code and player_token while making both struct patterns
exhaustive.
---
Major comments:
In `@client/src/adapter/draftPodGuestAdapter.ts`:
- Around line 347-365: Update sendMatchSettlement, handleMatchBetweenGames,
submitAuthorized, and acknowledgeAuthorized to reject when this.guest is
unavailable instead of silently using optional calls. Match the existing
submitPick/submitDeck behavior by throwing an appropriate error before sending,
while preserving normal transport calls when guest is present.
In `@client/src/adapter/p2p-draft-host.ts`:
- Around line 1455-1490: The SubmitSideboard branch in receiptIntergameCommand
must validate the receipted main and sideboard against the registered launch
deck in matchDecks before updating state.decks. Require the submitted 75 cards
to match the launch deck’s card-name multiset, and only then assign deck.main
and deck.sideboard; reject or ignore invalid payloads without mutating the
authority-owned default.
- Around line 1246-1251: Keep the rethrow in reportMatchResult at
client/src/adapter/p2p-draft-host.ts:1246-1251. At
client/src/adapter/p2p-draft-host.ts:1018-1028, wrap the await reportMatchResult
call in try/catch; on failure send draft_error to submittingSeat and leave the
outbox record without setting its receipt. At
client/src/adapter/p2p-draft-host.ts:1882-1900, catch failures per settlement so
an unreplayable record is skipped without aborting restoreFromPersisted.
- Around line 1492-1514: Update autoSubmitSideboards so it processes each
unsubmitted seat independently: submit the default sideboard for seats with a
registered deck, while emitting the existing error for each seat whose deck is
missing. Remove the aggregate defaults.some early return, and retain the
existing submitDefaultIntergameCommand behavior for valid decks.
In `@client/src/adapter/ws-adapter.ts`:
- Around line 1139-1146: Make full-session-key validation strict and consistent
across the GameCreated, SessionAttached, and GameStarted handlers: call
acceptFullSessionKey with data.full_key without short-circuiting when it is
absent, and stop processing when validation fails. Preserve the existing
acceptFullSessionKey error and rejection behavior so missing or changed
identities cannot continue without reconnect credentials.
- Around line 53-81: Update terminalSocketRequest to enforce a response timeout
after the socket opens, rejecting and closing the socket when no response
arrives before the deadline. Centralize settlement so success, parse failure,
socket errors, close events, and timeout all clear the timer, detach websocket
handlers, and settle the promise only once; preserve the existing response
parsing and error messages where applicable.
In `@client/src/components/multiplayer/ConcedeDialog.tsx`:
- Around line 59-75: Update both concession action buttons in ConcedeDialog,
identified by gameAction.onConfirm and matchAction.onConfirm, to include the
min-h-11 class while preserving their existing styling and behavior.
In `@client/src/providers/GameProvider.tsx`:
- Around line 1125-1159: Update the terminalDelivery branch around
readFullTerminalResult so refresh errors fall back to the retained
terminalDelivery and continue displaying and acknowledging it, matching the
existing refreshed ?? terminalDelivery behavior. Keep retention failures from
replaceFullTerminalDelivery as the only errors that enter the
terminalUnavailable path, while preserving cancellation and cleanup handling.
- Around line 1161-1198: Update the reconnect flow around
bootstrapFullTerminalDelivery so probe exceptions are treated as no terminal
delivery and fall through to WebSocketAdapter construction, preserving normal
reconnect/backoff behavior. Remove the disconnected status update,
terminalUnavailable event, and early return from that catch path; retain the
existing short-circuit handling when a bootstrap delivery is successfully
returned and commitFullTerminalDelivery fails.
In `@client/src/services/fullTerminalResult.ts`:
- Around line 58-79: Update commitFullTerminalDelivery to compare
terminal_revision when an existing record is found: return true for deliveries
at the same or newer revision, allowing the newer delivery to replace the cached
record, while returning false for stale revisions. Preserve idempotence for
matching delivery_id and credential and use the existing validated
terminal_revision field.
In `@client/src/services/intergameCommandLedger.ts`:
- Around line 43-52: The 32-bit FNV implementation in draftIntergameDigest is
insufficient for untrusted cross-peer authorization bindings. Replace it with
the existing SHA-256-over-canonical-JSON approach from p2pTerminalResult.ts,
preserving the digest’s string-returning contract and updating the
function/callers as needed for the asynchronous crypto primitive; reuse existing
helpers rather than introducing a new hashing implementation.
- Around line 165-175: Update canonicalize to sort object keys with
deterministic code-unit ordering instead of localeCompare. Replace the
locale-dependent comparator in the Object.entries sorting step while preserving
recursive canonicalization and the existing array/value handling.
In `@client/src/services/p2pSession.ts`:
- Around line 90-108: Update ownsP2PHostLease and releaseP2PHostLease so
releasing a lease writes a session-keyed tombstone to localStorage instead of
removing the key, preserving cross-tab fencing while still making the lease
appear unowned. Ensure readHostLease treats the tombstone as null and prevent
the in-memory fallback from reviving a superseded incarnation after a storage
stamp has been observed.
In `@client/src/services/p2pTerminalResult.ts`:
- Around line 75-90: The get-then-set sequence in commitP2PTerminalResult is not
atomic and can allow competing results to overwrite the first committed value.
Replace it with idb-keyval.update or another single-transaction write path that
preserves the existing first-valid-result and same-result idempotency behavior,
and update the related test to assert update rather than set.
In `@client/src/stores/multiplayerDraftStore.ts`:
- Around line 289-295: Remove the duplicate winnerSeatForGameResult or
winnerSeatForLaunch helper, retain a single shared winner-seat function, and
update both existing call sites to use the retained function without changing
null handling or seatForLaunchGamePlayer behavior.
- Around line 905-909: Update submitIntergameCommand to select gameNumber based
on payload.type: use playDrawPrompt for bo3ChoosePlayDraw commands and
sideboardPrompt for sideboard commands, rather than relying on nullish prompt
precedence. Preserve the existing early return when the selected prompt or other
required match state is unavailable.
- Around line 942-951: Persist the ledger snapshot immediately after
controller.begin succeeds and before adapter.submitAction is invoked, so the
Authorized-to-Executing state is durable across reloads. Update the flow around
submitAuthorized, begin, and saveDraftIntergameCommands while retaining the
existing receipt-time persistence for the completed command.
- Around line 926-948: Update submitAuthorized to re-read current state after
all awaited work, especially before invoking the adapter, and use the latest
matchAdapter rather than the stale initial snapshot. Before crossing the sink,
require the command’s seat to equal the current local seatIndex; reject
mismatches, and derive the actor from the validated current state/launch while
preserving the existing permit checks.
In `@crates/engine/src/types/game_state.rs`:
- Around line 12211-12214: Update the existing match_config import in the game
state module to include MatchForfeitResult, bringing the type used by the
match_forfeit_result field into scope for Card data and WASM compilation.
In `@crates/engine/src/types/mod.rs`:
- Around line 55-56: Fix the re-export list in the types module by removing the
unresolved DelayedTriggerToken entry or re-exporting it from the module where it
is actually defined; do not import it from identifiers, and preserve the
existing exports for CardId, ObjectId, and the other valid symbols.
In `@crates/phase-server/src/main.rs`:
- Around line 4208-4228: The draft-game teardown currently depends on
terminal_deliveries being non-empty, leaving completed games resident when no
recipients exist. In the GameOver handling flow, keep delivery sending
conditional on terminal_deliveries, but run report_draft_game_over and
state.lock().await.remove_game(&game_code) whenever the game-over marker
(game_over_winner.is_some() or terminal.is_some()) confirms the game reached
GameOver.
- Around line 2736-2761: Update crates/phase-server/src/main.rs:2736-2761 in
prepare_full_terminal so db.prepare_full_terminal remains the commit gate, but
unresolved recipient deliveries are skipped with a warning and the successfully
resolved deliveries are returned instead of failing the entire result; retain
task-join and preparation errors. Update
crates/phase-server/src/main.rs:3975-3988 in the caller to log preparation
failures and continue through the state broadcast and remove_game teardown path
rather than returning early, ensuring finished games are always removed after
preparation.
- Around line 1276-1321: Update the terminal candidate construction to handle
terminal_artifact errors explicitly and log the game code and error instead of
silently discarding them. Change the removal filter in the expired-session
cleanup to remove every started expired session regardless of whether its game
code exists in prepared, while retaining the existing handling for unstarted
sessions.
- Around line 495-498: Update build_spectator_game_started_message so its
spectator projection always sets full_key to None instead of copying
session.full_runtime.key; retain the full session key only in
authenticated/player message projections.
In `@crates/phase-server/src/persistence.rs`:
- Around line 703-772: Update bootstrap_terminal_delivery and the
terminal_bootstrap_requests schema so request_id idempotency is scoped per
game_code, generation, and player_id, allowing the same client id in different
matches without InvalidQuery while preserving conflict detection within one
recipient. Extend delete_stale to remove terminal_bootstrap_requests entries
associated with stale terminal deliveries, using created_at or the referenced
terminal-row lifecycle so the ledger is pruned and cannot grow indefinitely.
- Around line 244-246: Replace the domain-level rusqlite::Error::InvalidQuery
returns at all seven persistence rejection sites with a new FullPersistError
enum containing the specified domain variants plus Sqlite(rusqlite::Error).
Update the affected persistence methods to return FullPersistError, map genuine
SQLite failures to Sqlite, and adjust callers such as main.rs to distinguish
domain refusals from database errors.
- Around line 995-1002: Update the migration logic around the game_sessions
PRAGMA inspection and create_full_game_session_schema so it treats the table as
current only when generation, mutation_revision, activation_epoch, and retired
are all present; otherwise continue the migration path that adds any missing
columns.
In `@crates/server-core/src/client_message_wire_guard.rs`:
- Around line 74-97: Add the existing MAX_TOKEN_LEN-style length validation to
terminal payload fields in the ClientMessage guard: request.player_token,
request.request_id, credential.0, and delivery_id.0. Preserve the current
empty-field errors while rejecting values exceeding the established bound,
reusing the existing token validation or length-checking helper rather than
introducing new limits.
In `@crates/server-core/src/protocol.rs`:
- Around line 101-109: Replace the derived Debug implementation for the
capability type TerminalCredential with a manual redacting implementation that
formats as TerminalCredential(<redacted>) without exposing its inner String.
Keep its existing derives and serde behavior unchanged, and leave
TerminalDeliveryId’s Debug implementation unaffected.
- Around line 160-166: Make the Reconnect protocol change backward-compatible by
either adding serde-default handling for an optional full_key and explicitly
rejecting None in the reconnect guard with a clear reason, or, preferably for
the required field, increment PROTOCOL_VERSION from 23 so older clients are
rejected during ClientHello instead of failing deserialization.
In `@crates/server-core/src/session.rs`:
- Around line 1406-1428: Add a game-start guard in handle_match_concede after
session lookup and authentication, returning an error when session.game_started
is false before calling apply_trusted_match_forfeit. Preserve the existing
pending_takeback validation and normal concession flow for started games,
allowing unstarted sessions to use retire_unstarted_full_session instead.
- Around line 293-305: The persistence snapshot currently reuses state_revision,
so pre-start mutations can share a revision and be rejected by the SQL fence. In
crates/server-core/src/session.rs lines 293-305, add a session-local monotonic
persistence counter, increment it for every persistable mutation, and use it for
FullPersistSnapshot.mutation_revision instead of state_revision. In
crates/phase-server/src/persistence.rs lines 303-364, retain the strict
greater-than fence once revisions are monotonic; only use greater-than-or-equal
if the aliasing remains.
---
Minor comments:
In `@client/src/adapter/p2p-draft-host.ts`:
- Around line 1010-1016: Update the settlement receipt handling around
settlementReceipts and sendSettlementAck so a receiptId mismatch sends the
submitting seat an appropriate draft_error before returning. Preserve the
existing acknowledgement for matching receipt IDs and ensure every conflicting
receipt is reported instead of being silently dropped.
In `@client/src/pages/GamePage.tsx`:
- Around line 436-438: Replace frontend-authored terminal-unavailable messages
with a typed reason and localize them at the presentation boundary: in
client/src/pages/GamePage.tsx lines 436-438 and 664-666, switch on event.reason
and pass the corresponding multiplayer translation key to t() instead of
displaying event.message. In client/src/adapter/ws-adapter.ts lines 1451-1478
and client/src/providers/GameProvider.tsx lines 1149-1157, emit typed reasons
for retention and transport failures, keeping raw Error.message separate as
diagnostic data; leave server- or engine-authored reasons raw.
In `@crates/phase-server/src/main.rs`:
- Around line 5480-5487: Replace the `.expect` on
`session.full_runtime.as_ref()` in the `JoinOutcome::Waiting` construction with
error propagation that returns a descriptive `Err(String)` when `full_runtime`
is missing. Preserve the existing `Ok(JoinOutcome::Waiting { ... })` path when
the runtime and key are available so the surrounding `Err(e)` handling reports
the rejection instead of panicking.
In `@crates/server-core/src/session.rs`:
- Around line 1432-1443: Update the return tuple in apply_trusted_match_forfeit
so the log_entries position forwards the concession/forfeit log entry instead of
hardcoding Vec::new(). Ensure the entry is produced by the engine or returned
alongside GameEvent::GameOver, matching the normal action path’s
result.log_entries behavior.
---
Nitpick comments:
In `@client/src/adapter/p2p-adapter.ts`:
- Around line 3039-3063: Update acceptsHostAuthority to remove the redundant
this.authority && guard in the reconnect_ack session-key comparison and the
redundant this.authority !== null check in the final authority comparison, while
preserving the existing per-message-type authority validation behavior.
In `@client/src/adapter/p2p-draft-host.ts`:
- Around line 966-983: Reduce redundant session persistence during pairing
dispatch: remove the persistSession call from matchBindingFor and the
corresponding per-seat persistence in sendMatchLaunch, then invoke
persistSession once after dispatchMatchLaunch or generatePairings completes.
Preserve binding creation and launch behavior while ensuring the final state is
persisted after all pairings and launches are processed.
In `@client/src/pages/__tests__/GamePage.bracketViolation.test.tsx`:
- Around line 649-698: In the “GamePage — bound whole-match concession” suite,
keep only one mock for FakeWebSocketAdapter.sendMatchConcede by removing either
the class-field mock or the test-level reassignment. Add a paired Bo1 test using
the same setup, assert capturedConcedeDialogProps.matchAction is undefined after
opening the concede flow, and retain the existing Bo3 assertion that the action
is offered and invokes sendMatchConcede.
In `@client/src/services/__tests__/intergameCommandLedger.test.ts`:
- Around line 25-33: Add coverage in the intergame permit tests around
consumeIntergamePermit: assert a second consumption with the same
acknowledgement returns false, and add a cross-match case using a different
acknowledgement whose launchDigest does not match. Keep the existing happy-path
sequence and test the reusable permit primitive directly rather than only
through the controller flow.
In `@client/src/services/draftPersistence.ts`:
- Around line 59-79: Replace the inline element type of
DraftPersistence.bo3State with the exported Bo3MatchState-based persisted type
from p2p-draft-host.ts, preserving the matchId field as required. Update the
host-side type export and import/reference it here so Bo3 state fields have a
single source of truth.
In `@client/src/services/intergameCommandLedger.ts`:
- Around line 96-105: Update IntergameCommandLedger.hold to derive launchDigest
from command.launchPayload using draftIntergameDigest, matching the existing
payloadDigest derivation. Remove launchDigest from the hold parameter type so
callers cannot supply an inconsistent value, while preserving the immutable
launchPayload handling and pending status.
In `@client/src/services/p2pTerminalResult.ts`:
- Around line 110-121: Update p2pFinalStateCommitment to serialize state with
the repository’s canonicalize helper from intergameCommandLedger.ts instead of
JSON.stringify, while preserving the existing UTF-8 encoding, SHA-256 digest,
and sha256-prefixed hex result.
In `@client/src/stores/__tests__/multiplayerDraftStore.test.ts`:
- Around line 286-288: Strengthen the submitMatchSettlement assertions in the
multiplayer draft store tests to verify the forwarded settlement binding, not
only winnerSeat. In each affected expectation, include binding:
matchPairing.binding and assert that receiptId is non-empty while preserving the
existing winnerSeat assertion.
In `@client/src/stores/multiplayerDraftStore.ts`:
- Line 947: Replace the inline actor assignment near seatForLaunchGamePlayer
with a shared gamePlayerForLaunch(launch) helper defined alongside it, and use
that helper to derive the engine seat. Preserve the existing HumanGuest-to-1 and
other-launch-types-to-0 behavior while centralizing the bidirectional mapping.
In `@crates/phase-server/src/main.rs`:
- Around line 3595-3627: Update the ClientMessage handlers for
BootstrapTerminalDelivery, ReadTerminalResult, and AckTerminalDelivery to
execute their synchronous game_db calls inside spawn_blocking, awaiting each
task before constructing the existing ServerMessage responses. Preserve the
current success and error mappings while ensuring the socket task never invokes
bootstrap_terminal_delivery, read_terminal_result, or ack_terminal_delivery
directly.
- Around line 1143-1158: The persisted session restoration loop should avoid
serializing and reparsing the already-typed snapshot data. Add a typed
restoration variant such as restore_persisted_session_from that accepts the
snapshot’s PersistedSession value and db, use it from the persisted_games loop,
and retain restore_persisted_session’s JSON-based path for the legacy loader.
In `@crates/phase-server/src/persistence.rs`:
- Around line 906-952: The idempotency check in save_ranked_result_idempotent
lacks storage-level uniqueness and an indexed lookup. Add a UNIQUE constraint or
unique index on ranked_match_history(game_code, player_key), ensure the insert
path propagates its conflict as an error rather than applying a second result,
and retain the existing same-game validation and receipt behavior.
In `@crates/server-core/src/session.rs`:
- Around line 1663-1682: Extend the session tests with coverage for
handle_match_concede using the existing setup_two_player_game fixture: verify an
incorrect token is rejected, a concession during pending takeback is rejected,
and match_forfeit_result/waiting_for transition the match to GameOver with the
correct winner. Keep the assertions focused on the authenticated concession path
and resulting game state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 222d507d-964e-420c-b235-fab1f0e415eb
⛔ Files ignored due to path filters (1)
client/src/wasm/engine_wasm.d.tsis excluded by!client/src/wasm/**,!**/*.d.ts
📒 Files selected for processing (59)
client/src/adapter/__tests__/p2p-adapter-multiplayer.test.tsclient/src/adapter/__tests__/p2pDraftHostBo3.test.tsclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/draftPodGuestAdapter.tsclient/src/adapter/draftPodHostAdapter.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/p2p-draft-guest.tsclient/src/adapter/p2p-draft-host.tsclient/src/adapter/types.tsclient/src/adapter/ws-adapter.tsclient/src/components/multiplayer/ConcedeDialog.tsxclient/src/components/multiplayer/__tests__/ConcedeDialog.test.tsxclient/src/hooks/__tests__/useConcedeHandler.test.tsxclient/src/hooks/useConcedeHandler.tsclient/src/i18n/locales/de/multiplayer.jsonclient/src/i18n/locales/en/multiplayer.jsonclient/src/i18n/locales/es/multiplayer.jsonclient/src/i18n/locales/fr/multiplayer.jsonclient/src/i18n/locales/it/multiplayer.jsonclient/src/i18n/locales/pl/multiplayer.jsonclient/src/i18n/locales/pt/multiplayer.jsonclient/src/network/__tests__/draftProtocol.test.tsclient/src/network/__tests__/protocol.test.tsclient/src/network/draftProtocol.tsclient/src/network/protocol.tsclient/src/pages/DraftPodPage.tsxclient/src/pages/GamePage.tsxclient/src/pages/__tests__/DraftPodPage.betweenGames.test.tsxclient/src/pages/__tests__/GamePage.bracketViolation.test.tsxclient/src/providers/GameProvider.tsxclient/src/providers/__tests__/GameProvider.nativeEngine.test.tsxclient/src/services/__tests__/draftPersistence.test.tsclient/src/services/__tests__/fullTerminalResult.test.tsclient/src/services/__tests__/gamePersistence.test.tsclient/src/services/__tests__/intergameCommandLedger.test.tsclient/src/services/__tests__/multiplayerSession.test.tsclient/src/services/__tests__/p2pSession.test.tsclient/src/services/__tests__/p2pTerminalResult.test.tsclient/src/services/draftPersistence.tsclient/src/services/fullTerminalResult.tsclient/src/services/gamePersistence.tsclient/src/services/intergameCommandLedger.tsclient/src/services/multiplayerSession.tsclient/src/services/p2pSession.tsclient/src/services/p2pTerminalResult.tsclient/src/stores/__tests__/multiplayerDraftStore.test.tsclient/src/stores/__tests__/multiplayerStore.test.tsclient/src/stores/multiplayerDraftStore.tsclient/src/stores/multiplayerStore.tscrates/engine/src/game/match_flow.rscrates/engine/src/types/game_state.rscrates/engine/src/types/match_config.rscrates/engine/src/types/mod.rscrates/phase-server/src/main.rscrates/phase-server/src/persistence.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/lib.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
…BO3-concession-controls
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/adapter/p2p-adapter.ts (1)
2374-2438: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
concedePlayerdoesn't short-circuit an already-eliminated seat.
action(line 2047) andinteraction(line 2105) both explicitly checkthis.eliminatedSeats.has(pid)and reject without a WASM/native round-trip — the comment at line 2044 explains this is deliberate ("Reject at the adapter so the wire log is clear and the WASM round-trip is skipped").concedePlayerhas no equivalent guard, so a repeatedconcedewire message (line 2174), or the host's ownsendConcede()(line 1897) called twice, re-submits aConcedeengine action for a seat that's already eliminated — an unnecessary round-trip and, depending on the engine's guard behavior for double-elimination, a possible spurious secondbroadcastStateUpdate/player_concededbroadcast to remaining guests.Guard once at the shared call site rather than duplicating the check at every caller (
concedewire case,sendConcede,kickPlayer,concedeDisconnected):🐛 Proposed fix
private async concedePlayer( pid: PlayerId, reason: string, origin: "kick" | "conceded", ): Promise<void> { if (!this.ownsAuthority()) return; + // Same short-circuit as action/interaction: an already-eliminated seat + // has nothing left to concede, and every caller (guest `concede`, + // `sendConcede`, `kickPlayer`, `concedeDisconnected`) shares this guard. + if (this.eliminatedSeats.has(pid)) return; // Cancel any active grace timer for this seat. `timer` may be null if the // host already called `holdForReconnect`. const grace = this.disconnectedSeats.get(pid);Also applies to: 2171-2186, 1895-1901
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/adapter/p2p-adapter.ts` around lines 2374 - 2438, Update the shared concedePlayer method to return immediately when eliminatedSeats already contains pid, before performing cleanup or submitting an engine action. Preserve the existing behavior for seats not yet eliminated, and keep the guard centralized rather than adding duplicate checks to concede wire handling, sendConcede, kickPlayer, or concedeDisconnected.crates/phase-server/src/main.rs (1)
3903-3991: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
prepare_full_terminalfailure leaves the game permanently stuck with zero broadcast, across all three termination paths.In
Action,Concede, andConcedeMatch, the state lock is used to mutatesession.stateintoWaitingFor::GameOver(or apply the match concede), and only afterward — outside the lock — isprepare_full_terminal(...).awaitcalled to durably finalize the result. None of the three branches persist the pre-terminal mutation (persist_full_session_asyncis skipped on the terminal path), and none of the threeErrarms perform any compensating action: no rollback of the in-memoryGameOvertransition, no retry, no removal fromstate, and critically no broadcast to any connected player, including the actor. Sincewaiting_for == GameOverblocks further actions, the affected game becomes permanently unplayable in-memory until a server restart — which then silently resurrects the last durably-persisted (pre-terminal) snapshot, rolling players back.This is reachable via a real concurrency window, not just a storage fault:
prepare_full_terminalreturnsErr(InvalidQuery)(not idempotentAlreadyPrepared) whenever a different concurrent terminal artifact for the same(game_code, generation)was already recorded with a different digest — e.g. the reconnect-grace-expiry sweep racing a live action/concede on the same game.
crates/phase-server/src/main.rs#L3903-L3991: onprepare_full_terminalErr, broadcast the already-computedStateUpdate/events to all players (so the game's actual outcome is visible) before erroring, and/or retry finalization instead of leaving the in-memory session stuck inGameOverwith no persisted trace.crates/phase-server/src/main.rs#L5738-L5826: apply the same fix — broadcast the concede'sStateUpdate/Concededresult to all players even when terminal finalization fails, and add a recovery path instead of an unconditional earlyreturn.crates/phase-server/src/main.rs#L5828-L5911: apply the same fix for match-concede — ensure players are notified of the outcome and the session isn't left permanently wedged whenprepare_full_terminalfails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/main.rs` around lines 3903 - 3991, Update the terminal handling in crates/phase-server/src/main.rs:3903-3991, 5738-5826, and 5828-5911 so prepare_full_terminal failures do not return while the session remains stuck in GameOver. In the Action path, broadcast the computed StateUpdate/events before reporting the error and add recovery or retry finalization; apply the same outcome broadcast and recovery behavior to the Concede and ConcedeMatch paths, including their Conceded results. Ensure each path leaves a durable or recoverable session state instead of an unpersisted terminal mutation.crates/phase-server/src/persistence.rs (1)
178-204: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAdd a retention policy for terminal bookkeeping rows.
delete_staleonly trims non-retired sessions/drafts;full_generation_high_water,terminal_match_results,terminal_match_delivery, andterminal_bootstrap_requestsstill have no GC path and will grow with every match on a long-lived server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-server/src/persistence.rs` around lines 178 - 204, Extend delete_stale to also remove expired terminal bookkeeping rows from full_generation_high_water, terminal_match_results, terminal_match_delivery, and terminal_bootstrap_requests using the same cutoff policy, transaction, and deleted-row count. Keep the existing cleanup for sessions, drafts, and P2P backups unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/adapter/p2p-adapter.ts`:
- Around line 1804-1825: Update terminalResultForRecipient to populate revision
from the immutable terminal.revision captured in this.terminalResult, rather
than the mutable authoritativeRevision. Apply the same correction to the
corresponding terminal-result construction sites, and add a regression assertion
in the guest reconnect redelivery test that verifies result.revision matches the
originally committed terminal revision.
---
Outside diff comments:
In `@client/src/adapter/p2p-adapter.ts`:
- Around line 2374-2438: Update the shared concedePlayer method to return
immediately when eliminatedSeats already contains pid, before performing cleanup
or submitting an engine action. Preserve the existing behavior for seats not yet
eliminated, and keep the guard centralized rather than adding duplicate checks
to concede wire handling, sendConcede, kickPlayer, or concedeDisconnected.
In `@crates/phase-server/src/main.rs`:
- Around line 3903-3991: Update the terminal handling in
crates/phase-server/src/main.rs:3903-3991, 5738-5826, and 5828-5911 so
prepare_full_terminal failures do not return while the session remains stuck in
GameOver. In the Action path, broadcast the computed StateUpdate/events before
reporting the error and add recovery or retry finalization; apply the same
outcome broadcast and recovery behavior to the Concede and ConcedeMatch paths,
including their Conceded results. Ensure each path leaves a durable or
recoverable session state instead of an unpersisted terminal mutation.
In `@crates/phase-server/src/persistence.rs`:
- Around line 178-204: Extend delete_stale to also remove expired terminal
bookkeeping rows from full_generation_high_water, terminal_match_results,
terminal_match_delivery, and terminal_bootstrap_requests using the same cutoff
policy, transaction, and deleted-row count. Keep the existing cleanup for
sessions, drafts, and P2P backups unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6d1fc89-3560-4eaa-b6cb-2b604dee1082
📒 Files selected for processing (10)
client/src/adapter/__tests__/p2p-adapter-multiplayer.test.tsclient/src/adapter/__tests__/p2pDraftHostBo3.test.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/p2p-draft-host.tsclient/src/services/__tests__/gamePersistence.test.tsclient/src/services/gamePersistence.tscrates/phase-server/src/main.rscrates/phase-server/src/persistence.rscrates/server-core/src/protocol.rscrates/server-core/tests/lobby_wire_contract.rs
| /** | ||
| * A terminal statement is recipient-bound because its commitment covers the | ||
| * recipient's filtered final state. Reconnects therefore need a newly | ||
| * committed statement rather than replaying the host's retained result. | ||
| */ | ||
| private async terminalResultForRecipient( | ||
| recipient: PlayerId, | ||
| terminalState: GameState, | ||
| ): Promise<P2PTerminalResult> { | ||
| const terminal = this.terminalResult; | ||
| if (terminal === null) throw new Error("No terminal result to deliver"); | ||
| return { | ||
| key: this.sessionKey, | ||
| lease: this.authority, | ||
| recipient, | ||
| revision: this.authoritativeRevision, | ||
| terminalId: crypto.randomUUID(), | ||
| finalStateCommitment: await p2pFinalStateCommitment(terminalState), | ||
| display: terminal.display, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Redelivered terminal result can carry a stale/inconsistent revision.
terminalResultForRecipient stamps revision: this.authoritativeRevision instead of the immutable terminal.revision that was captured when the terminal statement was first committed. Two ways this can drift after the reconnect flow starts:
- In
handleReconnect,reconnect_ack.revisionis read fromthis.authoritativeRevisionand sent (awaited network I/O), thenterminalResultForRecipientre-readsthis.authoritativeRevisiona moment later — a TOCTOU window where an interleaving update could make the two values disagree. - In
handleNativeRevision,this.authoritativeRevision = revision;executes unconditionally, before the early-return insidecommitTerminalIfComplete— so any native revision event arriving after the game is already terminal keeps advancingauthoritativeRevisionpast the value the originalterminal_resultwas committed with.
Since the guest's terminal acceptance validates the delivered revision against its cached value (per the referenced acceptTerminalResult logic), this drift could cause a legitimate redelivered terminal result to be rejected on reconnect. Use the immutable terminal.revision instead, which is guaranteed consistent with the originally committed statement.
🐛 Proposed fix
private async terminalResultForRecipient(
recipient: PlayerId,
terminalState: GameState,
): Promise<P2PTerminalResult> {
const terminal = this.terminalResult;
if (terminal === null) throw new Error("No terminal result to deliver");
return {
key: this.sessionKey,
lease: this.authority,
recipient,
- revision: this.authoritativeRevision,
+ revision: terminal.revision,
terminalId: crypto.randomUUID(),
finalStateCommitment: await p2pFinalStateCommitment(terminalState),
display: terminal.display,
};
}Worth adding a regression assertion on result.revision in the existing "redelivers a recipient-bound terminal result after a guest reconnects" test, since the current assertions don't cover this.
Also applies to: 2335-2343, 1711-1719
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/adapter/p2p-adapter.ts` around lines 1804 - 1825, Update
terminalResultForRecipient to populate revision from the immutable
terminal.revision captured in this.terminalResult, rather than the mutable
authoritativeRevision. Apply the same correction to the corresponding
terminal-result construction sites, and add a regression assertion in the guest
reconnect redelivery test that verifies result.revision matches the originally
committed terminal revision.
github-actions
Bot
added
the
needs-maintainer
1 participant
Summary
Adds separate current-game and whole-match concession flows for two-seat best-of-three sessions. The engine owns the trusted match-forfeit result; WebSocket, P2P, draft, persistence, and UI layers bind the actor, preserve terminal state, and present localized scoped choices.
Files changed
client/src/adapter/__tests__/p2p-adapter-multiplayer.test.tsclient/src/adapter/__tests__/p2pDraftHostBo3.test.tsclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/draftPodGuestAdapter.tsclient/src/adapter/draftPodHostAdapter.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/p2p-draft-guest.tsclient/src/adapter/p2p-draft-host.tsclient/src/adapter/types.tsclient/src/adapter/ws-adapter.tsclient/src/components/multiplayer/ConcedeDialog.tsxclient/src/components/multiplayer/__tests__/ConcedeDialog.test.tsxclient/src/hooks/__tests__/useConcedeHandler.test.tsxclient/src/hooks/useConcedeHandler.tsclient/src/i18n/locales/de/multiplayer.jsonclient/src/i18n/locales/en/multiplayer.jsonclient/src/i18n/locales/es/multiplayer.jsonclient/src/i18n/locales/fr/multiplayer.jsonclient/src/i18n/locales/it/multiplayer.jsonclient/src/i18n/locales/pl/multiplayer.jsonclient/src/i18n/locales/pt/multiplayer.jsonclient/src/network/__tests__/draftProtocol.test.tsclient/src/network/__tests__/protocol.test.tsclient/src/network/draftProtocol.tsclient/src/network/protocol.tsclient/src/pages/DraftPodPage.tsxclient/src/pages/GamePage.tsxclient/src/pages/__tests__/DraftPodPage.betweenGames.test.tsxclient/src/pages/__tests__/GamePage.bracketViolation.test.tsxclient/src/providers/GameProvider.tsxclient/src/providers/__tests__/GameProvider.nativeEngine.test.tsxclient/src/services/__tests__/draftPersistence.test.tsclient/src/services/__tests__/fullTerminalResult.test.tsclient/src/services/__tests__/gamePersistence.test.tsclient/src/services/__tests__/intergameCommandLedger.test.tsclient/src/services/__tests__/multiplayerSession.test.tsclient/src/services/__tests__/p2pSession.test.tsclient/src/services/__tests__/p2pTerminalResult.test.tsclient/src/services/draftPersistence.tsclient/src/services/fullTerminalResult.tsclient/src/services/gamePersistence.tsclient/src/services/intergameCommandLedger.tsclient/src/services/multiplayerSession.tsclient/src/services/p2pSession.tsclient/src/services/p2pTerminalResult.tsclient/src/stores/__tests__/multiplayerDraftStore.test.tsclient/src/stores/__tests__/multiplayerStore.test.tsclient/src/stores/multiplayerDraftStore.tsclient/src/stores/multiplayerStore.tsclient/src/wasm/engine_wasm.d.tscrates/engine/tests/fixtures/cr733/authority_matrix.json.gzcrates/engine/src/game/match_flow.rscrates/engine/src/types/game_state.rscrates/engine/src/types/match_config.rscrates/engine/src/types/mod.rscrates/phase-server/src/main.rscrates/phase-server/src/persistence.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/lib.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rsTrack
Developer
LLM
Model: gpt-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— passgit diff --check— pass./scripts/check-prelowered-ratchet.sh— Gate P PASS./scripts/check-parser-combinators.sh origin/main— Gate G and Gate A PASSimplementation review — CLEAN after the scoped import and terminal-wiring repairs; the current head completes all Full-session protocol, error-propagation, and fixture call sites
hosted CI — rerunning after the CR733 authority-matrix fixture was updated for the new non-reachable match_forfeit_result field
Gate A
Gate A PASS head=cb918947132528c350e4802ca4fec5c47b8ff640 base=6c79e41a5fee318f33cd701674856d00d226a2a7
Anchored on
Final review-impl
Final review-impl PASS head=cb918947132528c350e4802ca4fec5c47b8ff640
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
Resolved: Rust tests (shard 1/2) found the CR733 census entry missing for
match_forfeit_result;cb91894713adds the audited out-of-reachable-closure matrix record. The rerun is in progress.Summary by CodeRabbit
New Features
Bug Fixes