fix(engine): repeated Choose(Opponent)/Choose(Player) picks are independent by default (#6381) - #6747
Conversation
…endent by default (phase-rs#6381) Benevolent Offering's two "Choose an opponent." instructions unconditionally excluded a player already chosen earlier in the resolution, which is only correct for Gluntch, the Bestower's ordinal-cued "choose a second/third player." Per the "Offering" cycle ruling ("You may choose the same opponent for each of the effects, or you may choose different opponents"), a bare repeated choose must allow repeating an earlier pick. In a two-player game the old behavior made the second choice impossible, silently dropping the chosen opponent's life gain. Adds `PlayerChoiceDistinctness` (mirroring `NumberRange`'s `distinctness` axis) so `ChoiceType::Player`/`ChoiceType::Opponent` only exclude prior choices when explicitly ordinal-cued. The parser sets it from the "second"/ "third" ordinal it already scans for. Also fixes two related recipient-binding gaps this card exposed: the "you and that player each ..." compound-subject splitter bound "that player" to the unrelated vote-fanout `ScopedPlayer` axis instead of the resolution-scoped chosen player, and `GainLife`'s subject-injection pass never rebound "that player gains N life" (for-each and plain) away from its no-subject `Controller` default.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds distinctness metadata to player and opponent choices, updates parsing and resolution behavior, preserves legacy serialization, and adds regression coverage for repeated independent opponent choices and chosen-player target binding. ChangesPlayer choice distinctness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant OracleParser
participant ChoiceType
participant ChooseResolver
participant GameState
OracleText->>OracleParser: Parse player or opponent choice
OracleParser->>ChoiceType: Set distinctness from ordinal
ChoiceType->>ChooseResolver: Provide structured choice type
ChooseResolver->>GameState: Filter prior choices or allow repeats
GameState-->>ChooseResolver: Return legal options
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
![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
🧹 Nitpick comments (1)
crates/engine/src/types/ability.rs (1)
738-770: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a symmetric
opponent_distinct_from_prior()builder.
ChoiceType::Opponent.distinctnessis documented to participate in the same "exclude players already chosen by an earlierOpponent/Playerchoice" semantics asPlayer.distinctness(see the doc comment on theOpponentvariant, lines 615-619), but onlyPlayergets aplayer_distinct_from_prior()builder + round-trip test. If another layer (parser/synthesis) ever needs an ordinal-cued distinct opponent pick, it will have to hand-construct the struct literal instead of using a documented builder, and this leaf stays untested here.♻️ Suggested addition
pub fn player_distinct_from_prior() -> Self { Self::Player { distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices, } } + + /// Ordinal-cued "choose a second/third opponent": must exclude players + /// already chosen earlier in this resolution. + pub fn opponent_distinct_from_prior() -> Self { + Self::Opponent { + restriction: None, + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices, + } + }🤖 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/types/ability.rs` around lines 738 - 770, Add an opponent_distinct_from_prior() builder alongside player_distinct_from_prior(), returning ChoiceType::Opponent with no restriction and PlayerChoiceDistinctness::DistinctFromPriorChoices. Add the corresponding round-trip test using the existing test pattern for player_distinct_from_prior().
🤖 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/mtgish-import/src/convert/replacement.rs`:
- Around line 1871-1875: Update the surrounding documentation for the
ChooseAPlayer conversion near players_to_controller and ChoiceType to replace
the incorrect CR 800.4a citation with the verified player/opponent choice
references: CR 102.1, CR 102.2, and CR 608.2d.
---
Nitpick comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 738-770: Add an opponent_distinct_from_prior() builder alongside
player_distinct_from_prior(), returning ChoiceType::Opponent with no restriction
and PlayerChoiceDistinctness::DistinctFromPriorChoices. Add the corresponding
round-trip test using the existing test pattern for
player_distinct_from_prior().
🪄 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: f0291d07-526a-4d11-aefb-7313b4adbcaf
📒 Files selected for processing (22)
crates/engine/src/database/synthesis.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/choose.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/triggers_ordering_parity_tests.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/snapshot_tests.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_vote.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/baleful_mastery_regression.rscrates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rscrates/engine/tests/integration/issue_564_wishclaw_talisman_control.rscrates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/rules/tribute.rscrates/engine/tests/integration/the_who_opponent_guess_resolution.rscrates/mtgish-import/src/convert/action.rscrates/mtgish-import/src/convert/replacement.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Reconcile the full current-head parse-diff with the claimed scope. Evidence: CI run 30401472552, artifact 8705074336, reports two modified signatures: Intellectual Offering's Draw.target changed from scoped player to chosen player, and Angel of Destiny's GainLife.player changed from ∅ to any target; the PR description does not name either card or give an exact matching count/card set. Why it matters: these are behavior-affecting parser changes outside the stated Benevolent Offering/choice-distinctness narrative, so the review cannot distinguish intended class coverage from unintended parser blast radius. Suggested fix: explicitly name and justify both card-level changes as intended (including why they belong to this PR), or narrow the implementation so the artifact matches the stated scope.
[MED] Correct the CR annotation on ChooseAPlayer and resolve the existing CodeRabbit thread. Evidence: crates/mtgish-import/src/convert/replacement.rs:1867 annotates the player/opponent choice conversion with CR 800.4a; the locally verified Comprehensive Rules text defines 800.4a as the procedure when a player leaves the game. CR 102.1–102.3 define players, opponents, and teammates, while CR 608.2d governs choices made while resolving an effect. Why it matters: an incorrect CR reference gives false rules provenance to engine conversion logic and leaves a confirmed external review finding unresolved. Suggested fix: replace the annotation with the verified, scope-appropriate references (for example, CR 102.1–102.3 + CR 608.2d) or another directly applicable verified citation, then resolve CodeRabbit discussion r3669445472.
The required CI checks and the parse-diff technical gate are green on this head, but I cannot approve while these two substantive review items remain open.
Review of the parse-diff artifact on PR phase-rs#6747 found two card-level signature changes beyond the stated scope: - Intellectual Offering's second Draw now binds to ChosenPlayer{index: 0} instead of ScopedPlayer — intended: it shares Benevolent Offering's "Choose an opponent. You and that player each <body>." shape, so it exercises the same try_parse_compound_subject_each fix. Locked in with a new test. - Angel of Destiny's GainLife.player changed from the no-subject Controller default to TargetFilter::Any — a real regression. "You and that player each gain that much life" is a compound subject that GainLife isn't wired into in rewrite_recipient_on_link (Token/Draw/ Discard/Mill/Pump/GenericEffect only), so it falls through to a non-player-denoting subject filter; the new inject_subject_target arm blindly accepted it. Gated the arm on target_filter_can_target_player (mirroring the existing thread_for_each_subject GainLife arm) so an unresolved compound subject leaves the safer Controller default alone. Locked in with a regression test. Also corrects a pre-existing wrong CR citation (CR 800.4a, which governs a player leaving a multiplayer game) on the ChooseAPlayer replacement conversion touched by this PR, to CR 102.1-102.3 + CR 608.2d — the player/opponent + in-resolution-choice rules that actually apply. Resolves CodeRabbit thread r3669445472.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs`:
- Around line 166-217: Replace the AST-only assertions in
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:166-217
with a production-pipeline integration test that casts and resolves Intellectual
Offering, chooses an opponent, and verifies that opponent draws exactly three
cards; update the Angel of Destiny regression at
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:231-267
to deal combat damage and assert both the Angel’s controller and damaged player
gain that amount. Ensure both tests exercise runtime resolution rather than
inspecting parsed Effect trees.
🪄 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: 8fc4ac2e-b6c6-45da-adb8-a05d333571a1
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rscrates/mtgish-import/src/convert/replacement.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/mtgish-import/src/convert/replacement.rs
- crates/engine/src/parser/oracle_effect/mod.rs
| fn intellectual_offering_second_draw_binds_to_chosen_opponent() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let spell = scenario | ||
| .add_spell_to_hand_from_oracle( | ||
| P0, | ||
| "Intellectual Offering", | ||
| true, | ||
| "Choose an opponent. You and that player each draw three cards.\nChoose an opponent. Untap all nonland permanents you control and all nonland permanents that player controls.", | ||
| ) | ||
| .id(); | ||
| let runner = scenario.build(); | ||
| let ability = &runner.state().objects.get(&spell).unwrap().abilities[0]; | ||
| assert!( | ||
| matches!( | ||
| ability.effect.as_ref(), | ||
| Effect::Choose { | ||
| choice_type: ChoiceType::Opponent { .. }, | ||
| .. | ||
| } | ||
| ), | ||
| "head must be Choose(Opponent), got {:?}", | ||
| ability.effect | ||
| ); | ||
| let first_draw = ability.sub_ability.as_ref().expect("first Draw node"); | ||
| assert!( | ||
| matches!( | ||
| first_draw.effect.as_ref(), | ||
| Effect::Draw { | ||
| target: TargetFilter::OriginalController, | ||
| .. | ||
| } | ||
| ), | ||
| "the caster's draw must target OriginalController, got {:?}", | ||
| first_draw.effect | ||
| ); | ||
| let second_draw = first_draw.sub_ability.as_ref().expect("second Draw node"); | ||
| assert!( | ||
| matches!( | ||
| second_draw.effect.as_ref(), | ||
| Effect::Draw { | ||
| target: TargetFilter::Typed(TypedFilter { | ||
| controller: Some(ControllerRef::ChosenPlayer { index: 0 }), | ||
| .. | ||
| }), | ||
| .. | ||
| } | ||
| ), | ||
| "the chosen opponent's draw must bind to ChosenPlayer{{index: 0}}, not ScopedPlayer, got {:?}", | ||
| second_draw.effect | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Drive these recipient-binding regressions through resolution. Both tests only inspect parsed Effect trees, so they can pass while runtime target evaluation ignores ChosenPlayer or while Angel of Destiny still grants life solely to its controller—the latter is explicitly accepted by this test despite the card requiring both players to gain life.
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs#L166-L217: cast Intellectual Offering, choose an opponent, resolve it, and assert that opponent draws exactly three cards.crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs#L231-L267: produce combat damage to a player and assert both the Angel’s controller and the damaged player gain the damage amount.
As per path instructions, parser AST tests do not establish runtime semantics; integration regressions must use the production resolution pipeline.
📍 Affects 1 file
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs#L166-L217(this comment)crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs#L231-L267
🤖 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/issue_6381_benevolent_offering_repeat_opponent.rs`
around lines 166 - 217, Replace the AST-only assertions in
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:166-217
with a production-pipeline integration test that casts and resolves Intellectual
Offering, chooses an opponent, and verifies that opponent draws exactly three
cards; update the Angel of Destiny regression at
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:231-267
to deal combat damage and assert both the Angel’s controller and damaged player
gain that amount. Ensure both tests exercise runtime resolution rather than
inspecting parsed Effect trees.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
1 remaining blocker on the current head.
🔴 Blocker
[MED] Add a production-pipeline regression for Intellectual Offering’s newly changed draw recipient. Evidence: crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:166-217 only inspects the parsed Effect::Draw shape, while the behavior enters game/effects/draw.rs:139-194 and resolves ChosenPlayer at game/effects/mod.rs:6143-6154. The existing Benevolent Offering scenario proves repeated choice plus token/life handling, but it does not execute the Draw resolver. Why it matters: this PR’s current parse-diff intentionally changes Intellectual Offering from ScopedPlayer to ChosenPlayer; an AST-only assertion would remain green if runtime resolution drew for the wrong player. Suggested fix: cast and resolve the real Intellectual Offering text with seeded libraries, choose the opponent, and assert that opponent draws three cards.
✅ Resolved
The current parse-diff now contains only Intellectual Offering, matching the updated scope. The previous Angel of Destiny signature is gone, and CR 102.1-102.3 + CR 608.2d is verified against the local Comprehensive Rules; the prior CR 800.4a citation was unrelated and has been corrected.
Recommendation: request changes for the focused Intellectual Offering runtime regression; no Angel of Destiny behavior expansion is requested in this PR.
The existing intellectual_offering_second_draw_binds_to_chosen_opponent test only inspects the parsed Effect::Draw shape; it stays green even if runtime resolution drew for the wrong player, since it never executes game/effects/draw.rs's ChosenPlayer resolution. Adds intellectual_offering_draws_three_for_caster_and_chosen_opponent, which casts the real Oracle text with seeded libraries and mana, drives both Choose(Opponent) prompts to the same opponent (proving the repeated- choice fix along the way), and asserts both the caster and the chosen opponent actually draw three cards through the production cast/resolve pipeline. Keeps the original AST-shape test as a companion SHAPE assertion per the card-test skill.
|
Added a runtime regression in 23d568e: `intellectual_offering_draws_three_for_caster_and_chosen_opponent` casts the real Oracle text with seeded libraries (`{4}{U}`, 5 library cards per side), drives both `Choose(Opponent)` prompts to the same opponent through the production cast/resolve pipeline, and asserts both the caster and the chosen opponent actually draw three cards — exercising `game/effects/draw.rs`'s `ChosenPlayer` resolution end to end, not just the parsed AST. The prior AST-only test is kept alongside it as a SHAPE assertion (per the `card-test` skill) pinning the exact parser output. |
Review update — hold on current headThe requested Intellectual Offering regression is now satisfied on The remaining unresolved CodeRabbit request to expand Angel of Destiny is not a blocker for this PR: the current parse-diff contains only Intellectual Offering's Holding rather than approving while the current-head Rust gate is still running and the branch is behind |
Maintainer update — current-head holdThe branch was cleanly updated through current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head ce62f4e. The choice-distinctness model and targeted Intellectual Offering coverage are sound; the current-head CI and CodeRabbit checks are successful. Approving and enabling auto-merge.
2 participants
Summary
Fixes #6381. Benevolent Offering's two independent "Choose an opponent." instructions unconditionally excluded a player already chosen earlier in the same resolution — correct for Gluntch, the Bestower's ordinal-cued "choose a second/third player", but wrong here. The official ruling for the whole "Offering" cycle (Benevolent/Infernal/Intellectual/Sylvan Offering) is explicit:
In a two-player game the old behavior made the second "Choose an opponent." impossible (CR 609.3 no-op), silently dropping the chosen opponent's life gain — the observable "wrong game result" behind the bug report.
PlayerChoiceDistinctness(mirrors the existingNumberRange.distinctnessaxis) toChoiceType::Player/ChoiceType::Opponent. DefaultIndependentnever excludes prior choices;DistinctFromPriorChoicespreserves Gluntch's "three distinct players" behavior. The parser sets the latter from the "second"/"third" ordinal it already scans for (previously discarded as a "consistency hint only").game/effects/choose.rs::compute_optionsonly consultschosen_players(the exclusion set) whendistinctness == DistinctFromPriorChoices.While building a runtime regression test for the actual card, found and fixed two related recipient-binding gaps that also made Benevolent Offering resolve incorrectly (both are class-level fixes, not single-card patches):
ScopedPlayeraxis instead of the resolution-scoped chosen player from a precedingChoose(Opponent), so both Spirit-token creation effects silently went to the caster.GainLife's subject-injection pass never rebound "that player gains N life" (plain or "for each") away from its no-subjectControllerdefault, so the second life-gain clause also credited the caster instead of the chosen opponent.Parse-diff: two other cards' signatures change, both accounted for
CI's parse-diff artifact (run 30401472552 / artifact 8705074336) flags two more cards whose parsed shape changes. Both are direct, intended consequences of the two class-level fixes above — not incidental blast radius:
Choose an opponent. You and that player each draw three cards. / Choose an opponent. Untap all nonland permanents you control and all nonland permanents that player controls.Same "Choose an opponent. You and that player each ..." shape as Benevolent Offering's first clause, so it goes through the sametry_parse_compound_subject_eachfix: the secondDraw.targetnow binds toChosenPlayer{index: 0}instead ofScopedPlayer. Intended — it's the same "Offering" cycle bug, on the same code path. Covered by the new testintellectual_offering_second_draw_binds_to_chosen_opponent.Whenever a creature you control deals combat damage to a player, you and that player each gain that much life.This also hits the compound-subject splitter, butGainLifeisn't wired intorewrite_recipient_on_link, so the split fails and the clause falls through to a singleGainLifenode with an unresolved "you and that player" subject. The first version of this PR'sinject_subject_targetGainLifearm blindly accepted that fallback subject, corruptingGainLife.playerfrom the no-subjectControllerdefault toTargetFilter::Any— a real regression, not an intended change. Fixed: the arm is now gated ontarget_filter_can_target_player(mirroring the existingthread_for_each_subjectGainLife arm), so an unresolvable compound subject leaves the safeControllerdefault alone. The damaged-player life gain is a separate, pre-existing gap (unrelated to Benevolent offering allows a player to select themselves as the opponent. — [[Benevolent offering]] #6381) — not fixed here, just no longer made worse. Covered by the new regression testangel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient.No other cards changed shape.
Test plan
game/effects/choose.rscoveringIndependent(repeat allowed) vsDistinctFromPriorChoices(Gluntch-style exclusion) for bothChoiceType::PlayerandChoiceType::Opponent.gluntch_choose_player_chain_parses_with_chosen_player_scopessnapshot test to assert the ordinal-cued picks parse asDistinctFromPriorChoicesand the plain first pick asIndependent.issue_6381_benevolent_offering_repeat_opponent.rs: casts the real Oracle text, drives bothChoose(Opponent)prompts to the same opponent, and asserts the token split (3/3) and life gain (+6/+6) are both correct.intellectual_offering_second_draw_binds_to_chosen_opponent(names and locks in the intended parse-diff card) andangel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient(regression guard for the fixed-in-reviewGainLife/Anybug).cargo fmt --all,cargo clippy -p engine --tests -- -D warningsclean.cargo test -p engine --lib(17918 passed) andcargo test -p engine --test integration(4166 passed) green.cargo check -p mtgish-import -p phase-ai --testsclean (both crates constructChoiceType::Opponent/ChoiceType::Playervia the new helper constructors);ChooseAPlayerCR citation corrected (CR 800.4a → CR 102.1-102.3 + CR 608.2d) and CodeRabbit thread r3669445472 resolved.Summary by CodeRabbit
New Features
Bug Fixes
Tests