Fix Wheel and Deal#6571
Conversation
📝 WalkthroughWalkthroughThis change binds implicit continuations after multi-target player clauses to selected targets, consumes declared players correctly during multi-target casts, and separates per-target resolution from detached sequential tails. Parser, unit, and integration regressions cover opponent fan-out, pauses, explicit controller subjects, and empty selections. ChangesMulti-target continuation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpellCast
participant TargetSelection
participant MultiTargetResolver
participant PlayerChoice
participant SequentialTail
SpellCast->>TargetSelection: Consume declared multi-target players
TargetSelection->>MultiTargetResolver: Provide selected opponents
MultiTargetResolver->>PlayerChoice: Resolve each opponent instruction
PlayerChoice-->>MultiTargetResolver: Resume after choices
MultiTargetResolver->>SequentialTail: Resolve detached tail once
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/scenario.rs (1)
2894-2906: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
AbilityActivationand resolution-timeTargetSelectionstill reuse multi-target player declarations.Both sites drive the identical
WaitingFor::TargetSelectionshape thatSpellCast::try_commitnow feedsremaining_multi_target_playersinto, but they hard-codeNone, falling back to the old reusable-declared_playersbehavior. A future test scenario driving an activated ability (or a resolution-time target selection) withmulti_target: Some(...)and two distinct declared players would silently reuse the first declared player for every slot instead of consuming them in order — the same bug class this PR fixes for cast-timeSpellCast.Also applies to: 3141-3159
🤖 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/engine/src/game/scenario.rs` around lines 2894 - 2906, Update the `WaitingFor::TargetSelection` branches for both `AbilityActivation` and resolution-time target selection to maintain and pass a mutable `remaining_multi_target_players` collection into `pick_slot_target`, matching the `SpellCast::try_commit` behavior. Replace the hard-coded `None` while preserving single-target behavior and consuming declared multi-target players in order across slots.
🧹 Nitpick comments (1)
crates/engine/src/game/effects/mod.rs (1)
3583-3599: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared root cause: multi-target-player-subject classification is duplicated between parser and runtime. Both sites independently re-implement the same
TargetFiltershape test that decides whether a head is an "any number of target players/opponents" subject; keeping them as one predicate removes the risk of the parser's continuation rewrite and the runtime's fan-out gate silently drifting apart when a new Oracle lowering is added.
crates/engine/src/game/effects/mod.rs#L3583-L3599: extract thePlayer | Opponent | Typed{opponent-only}match into a shared helper onTargetFilter(e.g. incrate::types::ability) that both this file and the parser can call.crates/engine/src/parser/oracle_effect/assembly.rs#L82-L98: replaceis_multi_target_player_subject_definition's inline match with a call to the same shared helper.🤖 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/engine/src/game/effects/mod.rs` around lines 3583 - 3599, The multi-target player classification is duplicated between runtime and parser code. Extract the `TargetFilter::Player | Opponent | Typed` opponent-only shape check from `is_multi_target_player_filter` into a shared `TargetFilter` helper, then update `crates/engine/src/game/effects/mod.rs` lines 3583-3599 and `crates/engine/src/parser/oracle_effect/assembly.rs` lines 82-98 so both `is_multi_target_player_filter` and `is_multi_target_player_subject_definition` call it.
🤖 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/engine/src/parser/oracle_effect/tests.rs`:
- Around line 11691-11710: Update
multi_target_player_subject_does_not_capture_explicit_controller_draw to first
assert def.multi_target is present and verify the multi-target head targets
players, then retain the existing controller-only draw assertion for the
continuation. Use the parsed head symbols already exposed by the definition
rather than relying solely on sub_ability.
In `@crates/engine/tests/integration/wheel_and_deal.rs`:
- Around line 114-115: Update the comment above this test to cite CR 115.6 as
the rule permitting a targeted spell to have zero targets, while retaining CR
608.2c for the independent trailing draw instruction; remove the incorrect CR
601.2c reference.
- Around line 85-112: Strengthen the integration test around the multi-player
resolution flow after the existing hand assertions so it verifies ordering, not
just final graveyard sizes: assert that both P1 and p2 discard their hands
before either player receives the seven-card draw. Use the production pipeline’s
observable state or event sequence, while preserving the existing terminal-zone
assertions.
---
Outside diff comments:
In `@crates/engine/src/game/scenario.rs`:
- Around line 2894-2906: Update the `WaitingFor::TargetSelection` branches for
both `AbilityActivation` and resolution-time target selection to maintain and
pass a mutable `remaining_multi_target_players` collection into
`pick_slot_target`, matching the `SpellCast::try_commit` behavior. Replace the
hard-coded `None` while preserving single-target behavior and consuming declared
multi-target players in order across slots.
---
Nitpick comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 3583-3599: The multi-target player classification is duplicated
between runtime and parser code. Extract the `TargetFilter::Player | Opponent |
Typed` opponent-only shape check from `is_multi_target_player_filter` into a
shared `TargetFilter` helper, then update
`crates/engine/src/game/effects/mod.rs` lines 3583-3599 and
`crates/engine/src/parser/oracle_effect/assembly.rs` lines 82-98 so both
`is_multi_target_player_filter` and `is_multi_target_player_subject_definition`
call it.
🪄 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: 6334928f-8a77-4242-a20c-dab37ddd6871
📒 Files selected for processing (8)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/scenario.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/wheel_and_deal.rsdocs/parser-misparse-backlog.md
💤 Files with no reviewable changes (1)
- docs/parser-misparse-backlog.md
| /// CR 608.2c: the lexical conjugation is load-bearing. A fresh imperative | ||
| /// subject (`you`) after the same multi-target head must remain controller-only. | ||
| #[test] | ||
| fn multi_target_player_subject_does_not_capture_explicit_controller_draw() { | ||
| let def = parse_effect_chain( | ||
| "Any number of target opponents each discard their hands, then you draw a card.", | ||
| AbilityKind::Spell, | ||
| ); | ||
| let controller_draw = def | ||
| .sub_ability | ||
| .as_ref() | ||
| .expect("the explicit controller draw must remain a continuation"); | ||
| assert!(matches!( | ||
| &*controller_draw.effect, | ||
| Effect::Draw { | ||
| count: QuantityExpr::Fixed { value: 1 }, | ||
| target: TargetFilter::Controller, | ||
| } | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prove the multi-target parser path was reached.
This negative assertion passes if the multi-target head fails to parse but an unrelated continuation still yields you draw. Assert def.multi_target and that the head targets players before asserting the controller-only tail.
🤖 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/engine/src/parser/oracle_effect/tests.rs` around lines 11691 - 11710,
Update multi_target_player_subject_does_not_capture_explicit_controller_draw to
first assert def.multi_target is present and verify the multi-target head
targets players, then retain the existing controller-only draw assertion for the
continuation. Use the parsed head symbols already exposed by the definition
rather than relying solely on sub_ability.
Source: Path instructions
| outcome.assert_hand_drawn(P0, 1); | ||
| outcome.assert_hand_drawn(P1, 5); | ||
| outcome.assert_hand_drawn(p2, 5); | ||
| assert_eq!( | ||
| outcome | ||
| .state() | ||
| .players | ||
| .iter() | ||
| .find(|player| player.id == P1) | ||
| .expect("P1 exists") | ||
| .graveyard | ||
| .len(), | ||
| 2, | ||
| "P1's whole hand must be discarded before drawing seven" | ||
| ); | ||
| assert_eq!( | ||
| outcome | ||
| .state() | ||
| .players | ||
| .iter() | ||
| .find(|player| player.id == p2) | ||
| .expect("P2 exists") | ||
| .graveyard | ||
| .len(), | ||
| 2, | ||
| "P2's whole hand must be discarded before drawing seven" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover simultaneous multi-player actions.
These terminal-zone assertions also pass if resolution processes P1’s discard/draw before p2’s. CR 608.2e requires each multi-player clause to be processed simultaneously, so add a production-pipeline assertion that proves all discards occur before any seven-card draw. (media.wizards.com)
🤖 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/engine/tests/integration/wheel_and_deal.rs` around lines 85 - 112,
Strengthen the integration test around the multi-player resolution flow after
the existing hand assertions so it verifies ordering, not just final graveyard
sizes: assert that both P1 and p2 discard their hands before either player
receives the seven-card draw. Use the production pipeline’s observable state or
event sequence, while preserving the existing terminal-zone assertions.
Source: Path instructions
| /// CR 601.2c + CR 608.2c: "Any number" permits no targets. That skips the | ||
| /// opponent-qualified instruction while preserving the independent final draw. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the CR rule that permits zero targets.
CR 115.6—not CR 601.2c—establishes that a targeted spell may allow zero targets. Keep CR 608.2c for the independent trailing instruction, but cite CR 115.6 for this claim. (media.wizards.com)
🤖 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/engine/tests/integration/wheel_and_deal.rs` around lines 114 - 115,
Update the comment above this test to cite CR 115.6 as the rule permitting a
targeted spell to have zero targets, while retaining CR 608.2c for the
independent trailing draw instruction; remove the incorrect CR 601.2c reference.
Source: Path instructions
github-actions
Bot
added
the
needs-maintainer
|
This is a well-built fix — the split between the per-target chain and the controller-only tail is the right decomposition, and the tests discriminate on exactly the property that matters. Nothing blocking. Three items, one of which is a class-level question I'd like answered before it lands. Reviewed at head 🟡 Non-blocking1. The fan-out iterates target-declaration order, not APNAP — and that matters for the class, not for this card. This is not hypothetical: #6545 is fixing precisely this bug in the per-owner library-order fan-out, and landed on 2. 3. Heads-up on #6572, which touches the same class. #6572 adds a post-lowering pass attaching ✅ Clean
Recommendation: answer the APNAP question in #1 and fix the CR 121 reference; I'm happy to approve on either "here's why declaration order is right" or a switch to |
|
Addendum to my review above — a second pass found one blocker I missed, and corrected one of my own CR references. 🔴 Blocker — vacuous negative test
That passes for the wrong reason. If the Add the same guard: assert!(def.multi_target.is_some());
assert!(def.effect.target_filter().is_some_and(target_filter_can_target_player));@coderabbitai raised this as 🟠 Major and it's unaddressed — that one was right and I should have confirmed it in my first pass rather than leaving it open. Correcting my own CR referenceIn my comment above I cited CR 101.4 for the ordering concern. The sharper rule is CR 608.2e ( Two things follow. This is pre-existing fan-out architecture, not a regression you introduced — your change improves granularity and doesn't make it worse, so I'm downgrading it from "please answer this" to a follow-up item. And separately, my zero-target citation was wrong: CR 115.6 ( Two more non-blocking
Reinforcing what's goodThe parse-diff is an exact match: 1 card, 1 signature, Wheel and Deal, And the sentence-boundary reasoning is stronger than I gave it credit for: Revised recommendation: add the reach-guard to the negative test, fix the CR 115.6 citation, and answer the Turtle Tracks question. The APNAP/CR 608.2e item is a follow-up, not yours to fix here. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline crates/engine/src/parser/oracle_effect/tests.rs \
--items all --type function --match 'multi_target_player_subject_does_not_capture_explicit_controller_draw|wheel_and_deal_implicit_draw_inherits_each_targeted_opponent'
ast-grep outline crates/engine/src/parser/oracle_effect/assembly.rs \
--items all --type function --match 'is_multi_target_player_subject_definition|assemble_effect_chain'
rg -n -C 12 \
'multi_target_player_subject_does_not_capture_explicit_controller_draw|wheel_and_deal_implicit_draw_inherits_each_targeted_opponent|is_multi_target_player_subject_definition|Effect::Draw|Turtle Tracks|CR 601\.2c|CR 115\.6' \
crates/engine/src/parser/oracle_effect/tests.rs \
crates/engine/src/parser/oracle_effect/assembly.rs \
crates/engine/src/game/effects/mod.rs \
crates/engine/tests/integration/wheel_and_deal.rs \
docs/parser-misparse-backlog.md
rg -n -C 5 \
'fn is_multi_target_player_filter|TargetFilter::Opponent|is_multi_target_player_subject_definition' \
crates/engine/src/game/effects/mod.rs \
crates/engine/src/parser/oracle_effect/assembly.rs \
crates/engine/src/parser/oracle_effect/mod.rsLength of output: 201631
Also confirmed that the rewrite in Thank you for the CR corrections as well: CR 115.6 is the relevant zero-target permission, and the CR 608.2e/APNAP behavior is pre-existing fan-out architecture rather than a regression introduced here. The duplicated multi-target-player predicate is a worthwhile cleanup, but can remain non-blocking. 🐇🔎 ✏️ Learnings added
|
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
|
Approving. The seam is right, the fix generalizes further than the title suggests, and the tests are discriminating. Reviewed at head Verified against the card, not the descriptionPulled the Oracle text from Scryfall rather than trusting the PR body: It matches the fixture at I pushed hard on "one card or a class?" — and the measurement answered itA single-card title is the usual smell, so I checked both halves. The runtime half is already class-shaped. The parser half looks card-shaped ( Parse-diff sticky confirms it independently: 1 card, 1 signature, The integration test genuinely flips on revert: restoring 🟡 Non-blocking — worth a follow-up, not another roundThe exactly-one-target path is production-reachable and has no fixture.
[LOW] The three-way player-filter predicate is duplicated across the parser/runtime boundary — [LOW] The PR body cites "CR 120 — draw". CR 120 is Damage; drawing is CR 121 ( 📋 Checked and clean
Approving and enqueueing. The two MED items are follow-up material; I'd particularly like the fail-open guard inverted before anyone extends this pattern, but it shouldn't hold up a correct fix. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — see the review comment above. Findings are non-blocking follow-ups.
2 participants
Summary
Fixes Wheel and Deal so its selected opponents discard their hands and draw seven cards, while the controller performs the final draw exactly once. Removes the card from the parser misparse backlog.
Files changed
CR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: GPT-5 Codex
Thinking: high
Tier: Frontier
Verification
cargo fmt --allgit diff --check./scripts/check-parser-combinators.sh— PASSGate A
Gate A PASS head=483bd981a65b3555950b3c210afe7977a11609b9 base=21a53d50cbcb8c21e6d250ae2c1f914fd6b0de15
Final review-impl
PASS: fresh review of
21a53d50c..483bd981a65b3555950b3c210afe7977a11609b9found no remaining issues.Claimed parse impact
Validation failures
cargo clippy-strictcould not complete because this environment lacks the system OpenSSL development package required byopenssl-sys; engine-focused clippy was additionally blocked behind the shared Cargo target-directory lock.CI failures
None observed locally.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation