fix(server): accept Interaction submissions over WebSocket (#6941) - #6949
Merged
Conversation
`ClientMessage` had no `Interaction` variant, so the server rejected every interaction submission at serde deserialization -- before any game logic ran -- with "unknown variant `Interaction`". PR #6778 wired the engine, WASM, and client halves of the attachment interaction fan but shipped no server half (`git show --stat a876682 -- crates/server-core` is empty). Live since v0.42.0: every interaction submission in a WebSocket multiplayer game failed. The client envelope was already correct -- `ClientMessage` is `#[serde(tag = "type", content = "data")]`, matching what `ws-adapter.ts` sends -- so no client file changes. - Add `ClientMessage::Interaction { submission }` and declare its wire policy in all three exhaustive `ClientMessage` matches. - Add `SessionManager::handle_interaction`, deriving the acting seat from the authenticated session token, never from the payload. `submit_interaction` then re-authorizes against the interaction slot inside the engine, so a forged id belonging to another seat is rejected twice. - Extract the `ClientMessage::Action` handler body into `handle_full_game_submission` so both wire variants share one authenticated, applied, and broadcast path rather than forking eight fan-out behaviours. - Answer interaction bounds failures on `ServerMessage::ActionRejected`, not `ServerMessage::Error`. The native client disposes its adapter on any `Error`, and a free-form `Text` response exceeding the 256-byte bound is reachable by an ordinary paste -- routing it to `Error` would end the match. `wire_rejection_message` makes that channel a per-variant wire policy. - Promote the engine's existing response bounds to a public `bound_interaction_submission` so the wire invokes the engine's own limits instead of restating them. Server-hosted draft matches remain blocked on a separate identity-binding defect (the draft adapter drops `DraftMatchStart.player_token`), which needs a client change and is filed separately.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds WebSocket support for interaction submissions. It introduces shared payload validation, a new client protocol variant, authenticated session handling, Full-mode dispatch, standardized rejection responses, state updates, broadcasts, and integration coverage. ChangesInteraction submission flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PhaseServer
participant WireGuard
participant GameSession
participant Engine
participant PlayersAndSpectators
Client->>PhaseServer: ClientMessage::Interaction
PhaseServer->>WireGuard: Validate interaction payload
WireGuard-->>PhaseServer: Accept or ActionRejected
PhaseServer->>GameSession: handle_interaction
GameSession->>Engine: submit_interaction
Engine-->>GameSession: ActionResult or rejection
GameSession-->>PhaseServer: ActionResult
PhaseServer->>PlayersAndSpectators: Broadcast filtered game updates
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/phase-server/src/main.rs (1)
3613-3623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the diagnostics that still name only
Action.This handler now serves both
GameSubmission::ActionandGameSubmission::Interaction. Two log strings still name only the action path:
- Line 3616:
"Action received but not in a game"fires for anInteractionframe with no session.- Line 3695:
"action processed (lock held)"fires for an applied interaction.An operator triaging an interaction-submission report searches for "interaction" and finds neither event. Derive the label from the submission variant so the shared handler stays diagnosable.
♻️ Proposed fix: label the two events by submission kind
impl GameSubmission { + /// Stable label for diagnostics emitted by the shared handler. + fn kind(&self) -> &'static str { + match self { + GameSubmission::Action(_) => "action", + GameSubmission::Interaction(_) => "interaction", + } + } + fn payload_rejection(&self) -> Result<(), Box<ServerMessage>> {) { + let kind = submission.kind(); let game_code = match &identity.game_code { Some(c) => c.clone(), None => { - warn!("Action received but not in a game"); + warn!(kind, "game submission received but not in a game"); let msg = ServerMessage::error("Not in a game".to_string());info!( game = %game_code, + kind, lock_ms, ai_actions = ai_results.len(), - "action processed (lock held)" + "game submission processed (lock held)" );🤖 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 3613 - 3623, Update the shared game-submission handler to derive a submission-kind label from the `GameSubmission` variant, covering both `Action` and `Interaction`. Reuse that label in the no-game warning near `identity.game_code` and the `"processed (lock held)"` diagnostic so both events identify the actual submission kind, including interactions.
🤖 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.
Nitpick comments:
In `@crates/phase-server/src/main.rs`:
- Around line 3613-3623: Update the shared game-submission handler to derive a
submission-kind label from the `GameSubmission` variant, covering both `Action`
and `Interaction`. Reuse that label in the no-game warning near
`identity.game_code` and the `"processed (lock held)"` diagnostic so both events
identify the actual submission kind, including interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae41aa1d-9742-43f9-9983-f02c160a9709
📒 Files selected for processing (8)
crates/engine/src/game/interaction.rscrates/engine/tests/integration/interaction_contract.rscrates/phase-server/src/main.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/interaction_payload_guard.rscrates/server-core/src/lib.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
`handle_full_game_submission` serves both `GameSubmission::Action` and `GameSubmission::Interaction`, but two of its events still named only the action path -- so an operator triaging an interaction-submission report would grep for "interaction" and match neither the "not in a game" warning nor the "processed (lock held)" info event. Derive a `kind` label from the submission variant and attach it to both, renaming the messages to name the shared concept. `GameSubmission::kind` keeps the two call sites from restating the variant set. Raised by CodeRabbit on #6949.
The module header claimed this layer "guarantees a single exhaustive match". It has had two for some time and now has three, and the doc on `wire_rejection_message` already referred to "the two sibling matches in this module" -- contradicting the header it sits under. State the guarantee in terms of what it actually enforces (every wire policy is declared in an exhaustive, wildcard-free match, so a new variant cannot compile until it states one) and name the three policy axes by function rather than by count, so the header does not go stale again the next time one is added. Raised by review on #6949. Doc-only; no behavior change.
matthewevans
added a commit
that referenced
this pull request
Aug 3, 2026
The seven tests added with the fix all drive `TapLandForMana`. The `ActivateManaSource` arm keeps its original resolver and is behaviourally unchanged, but its surface emission now runs through the extracted `push_produced_mana_surfaces`, and nothing covered that. Reaching it is not obvious: the reducer accepts `ActivateManaSource` under `WaitingFor::Priority` (`engine.rs`), but `direct_choice_projection` only constructs those actions in its `WaitingFor::ManaSourceSelection` arm, so a label test has to drive the game into a mana-source-selection window rather than activate at priority. A sacrificial mana source gets there. Covers a fixed and a flexible source through the same window, so the shared helper is pinned for both arms rather than only the one the fix changed. Raised by CodeRabbit on #6949's sibling PR.
matthewevans
added a commit
to tp5grphwkt-cell/phase
that referenced
this pull request
Aug 3, 2026
…ce (phase-rs#6944) (phase-rs#6953) * fix(engine): label flexible mana lands with the color they will produce (phase-rs#6944) City of Brass, Reflecting Pool, Command Tower and friends rendered an unlabelled "Tap for mana" instead of showing the mana each activation would produce. `project_action_payload` handled `TapLandForMana` and `ActivateManaSource` in one arm and resolved both through `live_mana_source_option_for_selection`. But the two actions carry deliberately different selection forms: - `TapLandForMana` is minted from `ManaSourceOption::semantic_selection` -- one concrete row per producible color -- and is executed by `handle_tap_land_for_mana` via `live_land_mana_option_for_selection`. - `ActivateManaSource` is minted from `activatable_mana_source_selections`, whose `manual_selection_for_option` intentionally collapses a flexible source to `Colorless` + `DeferredColorChoice` so the ordinary mana-choice resolver asks for the color, and is executed by `activate_mana_source_selection` via `live_mana_source_option_for_selection`. That divergence is deliberate and is not the bug. The bug is that the label path resolved a planner-minted `TapLandForMana` through the *manual* authority, which can never match a flexible source -- so the lookup failed and the arm returned without pushing a surface. The old code was correct for `ActivateManaSource` and wrong for `TapLandForMana`. Split the arm so each action is labelled through the same resolver its own reducer executes, with the resolver passed to a shared `push_produced_mana_surfaces`. A future mana action variant now has to name an authority to compile, which is what the function's doc comment already claimed. The fix is variant-agnostic: `production_override_for_option` maps all eight `flexible_output` variants to `ProductionOverride::SingleColor`, so nothing is special-cased per card. Seven tests drive the real projection pipeline (`derive_viewer_interaction` over a viewer-filtered state), covering six of the eight flexible variants: City of Brass, Reflecting Pool, Exotic Orchard, Command Tower, Plaza of Heroes, Pit of Offerings, and a Resonating Lute grant. All seven were confirmed red at base by restoring the old resolver and re-running -- 7 failures, no collateral -- reproducing the report verbatim, including mixed rows where a non-flexible sibling ability keeps its label while the flexible one goes blank. Not covered: `AnyCombination` has no bare-{T} land printing (every printing gates it behind a Composite/PaySpeed cost, so it needs a funded pool) and `AnyCombinationOfObjectColors` is unreachable for its only current printing, as already documented at casting_costs.rs. Both share the fixed code path. * test(engine): cover the ActivateManaSource mana-label path The seven tests added with the fix all drive `TapLandForMana`. The `ActivateManaSource` arm keeps its original resolver and is behaviourally unchanged, but its surface emission now runs through the extracted `push_produced_mana_surfaces`, and nothing covered that. Reaching it is not obvious: the reducer accepts `ActivateManaSource` under `WaitingFor::Priority` (`engine.rs`), but `direct_choice_projection` only constructs those actions in its `WaitingFor::ManaSourceSelection` arm, so a label test has to drive the game into a mana-source-selection window rather than activate at priority. A sacrificial mana source gets there. Covers a fixed and a flexible source through the same window, so the shared helper is pinned for both arms rather than only the one the fix changed. Raised by CodeRabbit on phase-rs#6949's sibling PR. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6941.
The bug
ClientMessagehad noInteractionvariant, so the server rejected every interaction submission at serde deserialization — before any game logic ran — withunknown variant \Interaction``.PR #6778 ("authorize attachment interaction fan") wired the engine, WASM, and client halves of the interaction path but shipped no server half —
git show --stat a8766823cd -- crates/server-coreis empty. Single-player and P2P were unaffected because they dispatch towasm.submitInteractionand never cross the wire.Live since v0.42.0 (2026-07-31), through v0.43.0. Every interaction submission in a WebSocket multiplayer game has failed for ~2 days. The attachment fan is the most visible surface, which is why it was reported as an equipment bug, but the failure is at the transport layer — it is not specific to equipment.
The client envelope was already correct (
ClientMessageis#[serde(tag = "type", content = "data")], matching whatws-adapter.tssends), so no client file changes.What changed
ClientMessage::Interaction { submission }and declare its wire policy in all three exhaustiveClientMessagematches. No wildcard arm was added —client_message_wire_guard.rsstill guarantees that a new variant cannot compile without declaring policy.SessionManager::handle_interaction, deriving the acting seat from the authenticated session token, never from the payload.submit_interactionthen re-authorizes against the interaction slot inside the engine, so a forged id belonging to another seat is rejected twice.ClientMessage::Actionhandler body intohandle_full_game_submissionsoActionandInteractionshare one authenticated / applied / broadcast path instead of forking eight fan-out behaviours. This is the bulk of themain.rsdiff and is a pure move — see the move evidence below.ServerMessage::ActionRejected, notServerMessage::Error. The native client disposes its adapter on anyError, and a free-formTextresponse exceeding the 256-byte bound is reachable by an ordinary paste — routing it toErrorwould end the match.wire_rejection_messagemakes that channel a per-variant wire policy.bound_interaction_submission, so the wire invokes the engine's own limits rather than restating them. One authority, no second copy.Verification
Verified against the immutable commit
064d5190in a detached, clean worktree, withHEADand clean state attested before and after every command:cargo fmt --all -- --checkcargo clippy -p phase-server -p server-core -p phase-engine --all-targets -- -D warningscargo test -p server-corecargo test -p phase-server --bin phase-servercargo test -p phase-engine-- crates/engine/src/parser/)-- client/)17 new tests cover the wire guard, payload bounding, and session authorization — including
handle_interaction_binds_the_actor_to_the_authenticated_token,handle_interaction_rejects_an_unknown_token,handle_interaction_rejects_a_stale_submission_benignly, andinteraction_wire_rejection_answers_on_the_benign_channel.Parser impact: none. Both
BASEandHEADwere projected from the same pinnedAtomicCards.json(sha256474b3fa7…) and the base-built comparator reportedoracle_changed: 0, no clusters. The two sides'card-data.jsonare byte-identical (fbf1dc80…).Move evidence for the
main.rsextraction:--color-moved=zebrais not applicable — the moved block was dedented 8 columns and rustfmt then re-wrapped it, so zebra cannot match. Instead,rustfmt(dedent_8(original))was diffed against the actual extracted function: 360 of 373 lines identical (98.1%), with exactly 3 differing hunks, all intended — (1) adebug!field renameaction→submission, (2) the payload guard replaced bysubmission.payload_rejection(), and (3) the newmatch submissiondispatch tohandle_action/handle_interaction.git merge-treeagainst currentmainproduces a conflict-free tree, and the 5 commitsmaingained since the base touch none of the changed files.Notes
--no-verify. The pre-push hook re-runs the full gate (workspace clippy, oracle-gen, coverage regression,pnpm lint/type-check) as a ~30-minute cold build in a scratch worktree; this exact SHA already has the recorded evidence above, the parser and frontend ranges are empty, and the byte-identical card data means the coverage regression check cannot move. CI is the authoritative gate.DraftMatchStart.player_tokenand keeps authenticating with the draft token. Filed as Draft adapter drops DraftMatchStart.player_token, so drafted matches authenticate with the draft token #6948 — client-side fix, deliberately not bundled here.Summary by CodeRabbit
New Features
Bug Fixes
Tests