fix(native): route resolve all decisions - #7215
Conversation
📝 WalkthroughWalkthroughThis change adds native Resolve All protocol support. The server resolves batches of AI-controlled turns, publishes compact state updates, and returns progress acknowledgements. The WebSocket adapter tracks pending requests and failures. The board supports Resolve All for native-AI games. ChangesResolve All protocol and wire contract
Server batch resolution
Client integration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant ActionButton
participant WebSocketAdapter
participant phase_server as phase-server
participant SessionManager
participant Players
Player->>ActionButton: Click Resolve All
ActionButton->>WebSocketAdapter: resolveAll(requester, aiSeats, maxResolutions)
WebSocketAdapter->>phase_server: ClientMessage.ResolveAll
phase_server->>SessionManager: resolve_all_for_player
SessionManager-->>phase_server: Resolution summary and state result
phase_server-->>Players: Compact state update
phase_server-->>WebSocketAdapter: ResolveAllResult
WebSocketAdapter-->>ActionButton: BatchResolveResult
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/components/board/ActionButton.tsx (1)
429-435: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMove native AI seat authority out of
ActionButton.These lines derive native AI player IDs and difficulties from local state and preferences. The native
ResolveAllframe does not send this data, andWebSocketAdapter.resolveAllignores it. A nonempty fabricated list only enables the batch path.Expose an engine-owned Resolve All capability or typed resolution scope. Use that capability for native sessions. Keep AI-seat data only where the local worker requires it.
As per path instructions,
client/src/must not infer GAME data; expose it from the engine instead.🤖 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/components/board/ActionButton.tsx` around lines 429 - 435, Update the Resolve All logic in ActionButton so native-ai sessions use an engine-owned Resolve All capability or typed resolution scope instead of deriving player IDs and difficulties from gs and aiSeats. Keep the fabricated AI-seat list only for local worker AI sessions, and ensure client/src does not infer game data for native sessions.Source: Path instructions
🤖 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/ws-adapter.ts`:
- Around line 1482-1484: Update the Resolve All failure handling in
client/src/adapter/ws-adapter.ts at lines 1482-1484 and 1723-1726 so
ActionRejected and Error responses are correlated by request_id before rejecting
pendingResolveAll; ignore uncorrelated failures from earlier requests, and add
coverage for an earlier rejected request followed by Resolve All.
In `@crates/phase-server/src/main.rs`:
- Around line 498-502: Update the Resolve All broadcast builder and
handle_resolve_all flow so batch.log_entries are not discarded. Publish a
payload-bounded tail of the engine-authored entries, or a single summary entry
identifying the number resolved, while preserving guard_state_snapshot_broadcast
limits and acknowledgement progress metadata.
- Around line 4350-4373: Extend the terminal-game teardown in the batch-ending
path after state.remove_game to remove the game_code entries from connections
and game_spectators, matching the cleanup performed by
ClientMessage::AbandonGame. Verify whether started games retain a lobby
registration; if they do, thread the lobby handle into handle_resolve_all and
unregister the game there as well, otherwise leave lobby handling unchanged.
---
Outside diff comments:
In `@client/src/components/board/ActionButton.tsx`:
- Around line 429-435: Update the Resolve All logic in ActionButton so native-ai
sessions use an engine-owned Resolve All capability or typed resolution scope
instead of deriving player IDs and difficulties from gs and aiSeats. Keep the
fabricated AI-seat list only for local worker AI sessions, and ensure client/src
does not infer game data for native sessions.
🪄 Autofix
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: 6ed18cb8-96c1-4712-8dd2-ad737508499c
📒 Files selected for processing (11)
client/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/ws-adapter.tsclient/src/components/board/ActionButton.tsxclient/src/components/board/__tests__/ActionButton.test.tsxclient/src/game/dispatch.tscrates/lobby-broker/src/protocol.rscrates/phase-server/src/main.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rsscripts/check-protocol-version.mjs
| } else if (this.pendingResolveAll) { | ||
| this.pendingResolveAll.reject(actionRejectionError(data.reason)); | ||
| this.pendingResolveAll = null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Correlate Resolve All failures with request_id.
ActionRejected and Error carry no request ID. A rejection from an earlier fire-and-forget request can arrive after resolveAll() installs pendingResolveAll. The adapter then rejects Resolve All even though the server still processes it.
Add a correlated Resolve All failure response. Reject only when its request_id matches the pending request. Add coverage for an earlier rejected request followed by Resolve All.
client/src/adapter/ws-adapter.ts#L1482-L1484: do not assign an uncorrelatedActionRejectedtopendingResolveAll.client/src/adapter/ws-adapter.ts#L1723-L1726: do not assign an uncorrelatedErrortopendingResolveAll.
📍 Affects 1 file
client/src/adapter/ws-adapter.ts#L1482-L1484(this comment)client/src/adapter/ws-adapter.ts#L1723-L1726
🤖 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/ws-adapter.ts` around lines 1482 - 1484, Update the
Resolve All failure handling in client/src/adapter/ws-adapter.ts at lines
1482-1484 and 1723-1726 so ActionRejected and Error responses are correlated by
request_id before rejecting pendingResolveAll; ignore uncorrelated failures from
earlier requests, and add coverage for an earlier rejected request followed by
Resolve All.
| /// Resolve All can collect thousands of engine events. They are needed for | ||
| /// authoritative transition bookkeeping, but replaying them in one wire frame | ||
| /// defeats the batch path and breaches the snapshot payload guard. Publish the | ||
| /// final, fully-derived state only; the requester acknowledgement carries | ||
| /// progress metadata separately. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve All discards the batch's game-log entries entirely.
The builder sets log_entries: Vec::new() for every recipient, and handle_resolve_all binds the session's log entries to _logs at Line 4269. The engine produces batch.log_entries for every resolved stack item, and SessionManager::resolve_all_for_player returns them, but no wire frame carries them.
The consequence is a permanent hole in user-visible game history: after one Resolve All, no player and no spectator ever sees what resolved. Log entries are not part of the GameState snapshot on this path, so a reconnect does not recover them either.
The doc comment justifies dropping events to respect guard_state_snapshot_broadcast. That argument bounds the payload; it does not require zero log entries. Consider publishing a bounded tail of batch.log_entries, or one engine-authored summary entry naming the count resolved, so the log stays continuous.
Also applies to: 540-540
🤖 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 498 - 502, Update the Resolve
All broadcast builder and handle_resolve_all flow so batch.log_entries are not
discarded. Publish a payload-bounded tail of the engine-authored entries, or a
single summary entry identifying the number resolved, while preserving
guard_state_snapshot_broadcast limits and acknowledgement progress metadata.
| if !terminal_deliveries.is_empty() { | ||
| let conns = connections.lock().await; | ||
| if let Some(players) = conns.get(&game_code) { | ||
| for (player, delivery) in &terminal_deliveries { | ||
| if *player == requester { | ||
| let _ = tx.send(ServerMessage::TerminalResult { | ||
| delivery: Some(delivery.clone()), | ||
| }); | ||
| } else if let Some(sender) = players.get(player) { | ||
| let _ = sender.send(ServerMessage::TerminalResult { | ||
| delivery: Some(delivery.clone()), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| drop(conns); | ||
| report_draft_game_over( | ||
| draft_state, | ||
| connections, | ||
| &game_code, | ||
| game_over_winner.flatten(), | ||
| ) | ||
| .await; | ||
| state.lock().await.remove_game(&game_code); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The terminal path removes the session but leaves connections, game_spectators, and the lobby entry behind.
When the batch ends the game, this block sends TerminalResult and calls state.lock().await.remove_game(&game_code). It never removes the connections entry for game_code, never removes the game_spectators entry, and never unregisters the lobby entry.
Compare ClientMessage::AbandonGame at Lines 6281-6283, which performs all three cleanups after remove_game.
The spectator entry is the clearest leak. Lines 4340-4346 only remove the game_spectators key when the sender list becomes empty. Any live spectator keeps a map entry that points at a game code with no session, and nothing on this path reclaims it.
🧹 Proposed teardown to match the abandon path
drop(conns);
report_draft_game_over(
draft_state,
connections,
&game_code,
game_over_winner.flatten(),
)
.await;
state.lock().await.remove_game(&game_code);
+ connections.lock().await.remove(&game_code);
+ game_spectators.lock().await.remove(&game_code);
}
}Note that lobby.unregister_game also runs on the abandon path. handle_resolve_all does not receive the lobby handle, so confirm whether a started game still holds a lobby entry at this point before adding that call.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !terminal_deliveries.is_empty() { | |
| let conns = connections.lock().await; | |
| if let Some(players) = conns.get(&game_code) { | |
| for (player, delivery) in &terminal_deliveries { | |
| if *player == requester { | |
| let _ = tx.send(ServerMessage::TerminalResult { | |
| delivery: Some(delivery.clone()), | |
| }); | |
| } else if let Some(sender) = players.get(player) { | |
| let _ = sender.send(ServerMessage::TerminalResult { | |
| delivery: Some(delivery.clone()), | |
| }); | |
| } | |
| } | |
| } | |
| drop(conns); | |
| report_draft_game_over( | |
| draft_state, | |
| connections, | |
| &game_code, | |
| game_over_winner.flatten(), | |
| ) | |
| .await; | |
| state.lock().await.remove_game(&game_code); | |
| if !terminal_deliveries.is_empty() { | |
| let conns = connections.lock().await; | |
| if let Some(players) = conns.get(&game_code) { | |
| for (player, delivery) in &terminal_deliveries { | |
| if *player == requester { | |
| let _ = tx.send(ServerMessage::TerminalResult { | |
| delivery: Some(delivery.clone()), | |
| }); | |
| } else if let Some(sender) = players.get(player) { | |
| let _ = sender.send(ServerMessage::TerminalResult { | |
| delivery: Some(delivery.clone()), | |
| }); | |
| } | |
| } | |
| } | |
| drop(conns); | |
| report_draft_game_over( | |
| draft_state, | |
| connections, | |
| &game_code, | |
| game_over_winner.flatten(), | |
| ) | |
| .await; | |
| state.lock().await.remove_game(&game_code); | |
| connections.lock().await.remove(&game_code); | |
| game_spectators.lock().await.remove(&game_code); |
🤖 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 4350 - 4373, Extend the
terminal-game teardown in the batch-ending path after state.remove_game to
remove the game_code entries from connections and game_spectators, matching the
cleanup performed by ClientMessage::AbandonGame. Verify whether started games
retain a lobby registration; if they do, thread the lobby handle into
handle_resolve_all and unregister the game there as well, otherwise leave lobby
handling unchanged.
Summary by CodeRabbit
New Features
Bug Fixes