From 1fe12d8b9d03c40e2b31ec9e6961a96a957eaf31 Mon Sep 17 00:00:00 2001 From: jsdevninja Date: Tue, 28 Jul 2026 16:32:43 -0500 Subject: [PATCH 1/3] fix(engine): repeated Choose(Opponent)/Choose(Player) picks are independent by default (#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. --- crates/engine/src/database/synthesis.rs | 4 +- crates/engine/src/game/ability_rw.rs | 10 +- crates/engine/src/game/coverage.rs | 2 +- crates/engine/src/game/effects/choose.rs | 116 +++++++--- .../src/game/engine_resolution_choices.rs | 2 +- .../game/triggers_ordering_parity_tests.rs | 2 +- crates/engine/src/parser/oracle.rs | 4 +- crates/engine/src/parser/oracle_effect/mod.rs | 134 +++++++++--- .../parser/oracle_effect/snapshot_tests.rs | 34 ++- .../engine/src/parser/oracle_effect/tests.rs | 11 +- .../engine/src/parser/oracle_replacement.rs | 12 +- crates/engine/src/parser/oracle_vote.rs | 12 +- crates/engine/src/types/ability.rs | 201 +++++++++++++++--- .../integration/baleful_mastery_regression.rs | 2 +- ...lum_scheming_guide_card_predicate_guess.rs | 13 +- .../issue_564_wishclaw_talisman_control.rs | 2 +- ...381_benevolent_offering_repeat_opponent.rs | 156 ++++++++++++++ crates/engine/tests/integration/main.rs | 1 + .../engine/tests/integration/rules/tribute.rs | 2 +- .../the_who_opponent_guess_resolution.rs | 8 +- crates/mtgish-import/src/convert/action.rs | 4 +- .../mtgish-import/src/convert/replacement.rs | 4 +- 22 files changed, 609 insertions(+), 127 deletions(-) create mode 100644 crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs diff --git a/crates/engine/src/database/synthesis.rs b/crates/engine/src/database/synthesis.rs index 10388e1f4b..e18b8c9a9e 100644 --- a/crates/engine/src/database/synthesis.rs +++ b/crates/engine/src/database/synthesis.rs @@ -9712,7 +9712,7 @@ pub fn synthesize_siege_intrinsics(face: &mut CardFace) { protector_replacement.execute = Some(Box::new(AbilityDefinition::new( AbilityKind::Spell, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, selection: crate::types::ability::TargetSelectionMode::Chosen, }, @@ -9841,7 +9841,7 @@ pub fn synthesize_tribute_intrinsics(face: &mut CardFace) { let choose_stage = AbilityDefinition::new( AbilityKind::Spell, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, selection: crate::types::ability::TargetSelectionMode::Chosen, }, diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index ceaab2f1dc..4db0b41672 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -3435,7 +3435,7 @@ fn legacy_guess_subject(subject: &GuessSubject) -> bool { fn legacy_choice_type(choice_type: &crate::types::ability::ChoiceType) -> bool { match choice_type { - crate::types::ability::ChoiceType::Opponent { restriction } => { + crate::types::ability::ChoiceType::Opponent { restriction, .. } => { restriction.as_deref().is_some_and(legacy_player_filter) } crate::types::ability::ChoiceType::CreatureType { .. } @@ -3449,7 +3449,7 @@ fn legacy_choice_type(choice_type: &crate::types::ability::ChoiceType) -> bool { | crate::types::ability::ChoiceType::LandType | crate::types::ability::ChoiceType::CardPredicate { .. } | crate::types::ability::ChoiceType::CardPredicateGuess { .. } - | crate::types::ability::ChoiceType::Player + | crate::types::ability::ChoiceType::Player { .. } | crate::types::ability::ChoiceType::TwoColors | crate::types::ability::ChoiceType::Word | crate::types::ability::ChoiceType::Artist @@ -5728,7 +5728,7 @@ fn rw_guess_subject(subject: &GuessSubject) -> RwProfile { fn rw_choice_type(choice_type: &crate::types::ability::ChoiceType) -> RwProfile { match choice_type { - crate::types::ability::ChoiceType::Opponent { restriction } => match restriction { + crate::types::ability::ChoiceType::Opponent { restriction, .. } => match restriction { Some(filter) => rw_player_filter(filter), None => RwProfile::empty(), }, @@ -5743,7 +5743,7 @@ fn rw_choice_type(choice_type: &crate::types::ability::ChoiceType) -> RwProfile | crate::types::ability::ChoiceType::LandType | crate::types::ability::ChoiceType::CardPredicate { .. } | crate::types::ability::ChoiceType::CardPredicateGuess { .. } - | crate::types::ability::ChoiceType::Player + | crate::types::ability::ChoiceType::Player { .. } | crate::types::ability::ChoiceType::TwoColors | crate::types::ability::ChoiceType::Word | crate::types::ability::ChoiceType::Artist @@ -6824,7 +6824,7 @@ mod tests { #[test] fn b7_choose_persist_member_bound() { let choose = |persist: bool| Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist, selection: TargetSelectionMode::default(), }; diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index e81afd2fea..285a24f6a9 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -2030,7 +2030,7 @@ fn fmt_choice_type(ct: &ChoiceType) -> String { ChoiceType::CardPredicate { .. } => "card predicate", ChoiceType::CardPredicateGuess { .. } => "card predicate guess", ChoiceType::Opponent { .. } => "opponent", - ChoiceType::Player => "player", + ChoiceType::Player { .. } => "player", ChoiceType::TwoColors => "two colors", ChoiceType::Word => "word", ChoiceType::Artist => "artist", diff --git a/crates/engine/src/game/effects/choose.rs b/crates/engine/src/game/effects/choose.rs index 452ea02fdd..e218acba8d 100644 --- a/crates/engine/src/game/effects/choose.rs +++ b/crates/engine/src/game/effects/choose.rs @@ -2,8 +2,8 @@ use rand::Rng; use crate::game::players; use crate::types::ability::{ - ChoiceType, ChoiceValue, ChosenAttribute, Effect, EffectError, EffectKind, ResolvedAbility, - SeatDirection, TargetSelectionMode, + ChoiceType, ChoiceValue, ChosenAttribute, Effect, EffectError, EffectKind, + PlayerChoiceDistinctness, ResolvedAbility, SeatDirection, TargetSelectionMode, }; use crate::types::card_type::CoreType; use crate::types::events::GameEvent; @@ -180,7 +180,7 @@ pub(crate) fn resolve_random_in_chain( // it to the sub via `apply_parent_chain_context`. if matches!( choice_type, - ChoiceType::Player | ChoiceType::Opponent { .. } + ChoiceType::Player { .. } | ChoiceType::Opponent { .. } ) { if let Ok(pid) = chosen.parse::() { let mut updated = ability.chosen_players.clone(); @@ -306,7 +306,7 @@ pub(crate) fn bind_named_choice( | ChoiceType::BasicLandType | ChoiceType::Color { .. } | ChoiceType::Keyword { .. } - | ChoiceType::Player + | ChoiceType::Player { .. } | ChoiceType::Opponent { .. } // CR 613.1: A persisted `Label` gates `ChosenLabelIs` // continuous statics — anchor-word modal permanents @@ -505,12 +505,16 @@ const LAND_TYPES: &[&str] = &[ /// casting or resolution. If an option would be illegal, it can't be chosen. /// /// `already_chosen` is the resolution-scoped list of players picked by earlier -/// `Choose(Player)` instructions in this chain. CR 608.2c + the Gluntch card -/// ruling ("three distinct players") require each successive "choose a player" -/// to exclude players already chosen — `ChoiceType::Player` and -/// `ChoiceType::Opponent` filter them out. When fewer eligible players remain -/// than the card asks for, the options list is empty and the choice (and its -/// dependent effect) does nothing — the standard empty-options path. +/// `Choose(Player)` instructions in this chain. `ChoiceType::Player` and +/// `ChoiceType::Opponent` only consult it when their `distinctness` is +/// `DistinctFromPriorChoices` (CR 608.2c + the Gluntch ordinal-cued "choose a +/// second/third player" ruling, "three distinct players"). The default +/// `Independent` distinctness never filters on it — the "Offering" cycle +/// ruling (Benevolent/Infernal/Intellectual/Sylvan Offering) confirms a +/// repeated "Choose an opponent." may pick the same player again. When +/// `DistinctFromPriorChoices` narrows the eligible set below what the card +/// asks for, the options list is empty and the choice (and its dependent +/// effect) does nothing — the standard empty-options path. fn compute_options( state: &GameState, choice_type: &ChoiceType, @@ -612,15 +616,23 @@ fn compute_options( // (in a free-for-all game, every other player). `players::opponents` // already drops eliminated players (CR 104.3a — a player who loses // leaves the game and is no longer an opponent). - // CR 608.2c: Exclude players already chosen earlier in this resolution. + // CR 608.2c: `DistinctFromPriorChoices` excludes players already chosen + // earlier in this resolution; the default `Independent` does not (the + // "Offering" cycle may repeat the same opponent). // CR 102.3 + CR 608.2d: When a `restriction` is present ("with the most // life among your opponents"), narrow the eligible set to opponents // satisfying that `PlayerFilter` — the controller then picks ONE of the // qualifying opponents (CR 608.2d handles ties), keeping it a single // pick rather than fanning the effect out to every tied opponent. - ChoiceType::Opponent { restriction } => players::opponents(state, controller) + ChoiceType::Opponent { + restriction, + distinctness, + } => players::opponents(state, controller) .iter() - .filter(|id| !already_chosen.contains(id)) + .filter(|id| { + *distinctness != PlayerChoiceDistinctness::DistinctFromPriorChoices + || !already_chosen.contains(id) + }) .filter(|id| { restriction.as_ref().is_none_or(|filter| { super::matches_player_scope(state, **id, filter, controller, source_id) @@ -629,11 +641,16 @@ fn compute_options( .map(|id| id.0.to_string()) .collect(), // CR 102.1: A player is one of the people in the game. - // CR 608.2c: Exclude players already chosen earlier in this resolution. - ChoiceType::Player => state + // CR 608.2c: `DistinctFromPriorChoices` (Gluntch's "choose a + // second/third player") excludes players already chosen earlier in + // this resolution; the default `Independent` does not. + ChoiceType::Player { distinctness } => state .seat_order .iter() - .filter(|id| !already_chosen.contains(id)) + .filter(|id| { + *distinctness != PlayerChoiceDistinctness::DistinctFromPriorChoices + || !already_chosen.contains(id) + }) .map(|id| id.0.to_string()) .collect(), ChoiceType::TwoColors => two_color_options(), @@ -1266,7 +1283,7 @@ mod tests { #[test] fn choose_opponent_lists_opponents() { let mut state = GameState::new_two_player(42); - let ability = make_choose_ability(ChoiceType::Opponent { restriction: None }); + let ability = make_choose_ability(ChoiceType::opponent()); let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); match &state.waiting_for { @@ -1278,10 +1295,56 @@ mod tests { } } + /// Issue #6381 (Benevolent Offering): the "Offering" cycle ruling — + /// "You may choose the same opponent for each of the effects, or you may + /// choose different opponents" — means the default `Independent` + /// distinctness must NOT exclude an opponent chosen by an earlier + /// `Choose(Opponent)` in the same resolution. In a two-player game this is + /// the difference between a legal repeat pick (correct) and an impossible + /// no-op second choice (the reported bug). + #[test] + fn choose_opponent_independent_by_default_allows_repeat_choice() { + let mut state = GameState::new_two_player(42); + let mut ability = make_choose_ability(ChoiceType::opponent()); + ability.chosen_players = vec![PlayerId(1)]; + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::NamedChoice { options, .. } => { + assert_eq!( + options, + &["1"], + "the previously-chosen opponent must remain a legal repeat pick" + ); + } + other => panic!("Expected NamedChoice, got {:?}", other), + } + } + #[test] fn choose_player_lists_all_players() { let mut state = GameState::new_two_player(42); - let ability = make_choose_ability(ChoiceType::Player); + let ability = make_choose_ability(ChoiceType::player()); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::NamedChoice { options, .. } => { + assert_eq!(options.len(), 2); + assert!(options.contains(&"0".to_string())); + assert!(options.contains(&"1".to_string())); + } + other => panic!("Expected NamedChoice, got {:?}", other), + } + } + + #[test] + fn choose_player_independent_by_default_allows_repeat_choice() { + // The default `Independent` distinctness (bare "choose a player") does + // NOT exclude a player already chosen earlier in this resolution — + // only the ordinal-cued `DistinctFromPriorChoices` (Gluntch) does. + let mut state = GameState::new_two_player(42); + let mut ability = make_choose_ability(ChoiceType::player()); + ability.chosen_players = vec![PlayerId(0)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); match &state.waiting_for { @@ -1295,11 +1358,12 @@ mod tests { } #[test] - fn choose_player_excludes_already_chosen_players() { - // CR 608.2c + Gluntch ruling: a successive "choose a player" omits - // players already chosen earlier in the same resolution. + fn choose_player_distinct_from_prior_excludes_already_chosen_players() { + // CR 608.2c + Gluntch ruling ("choose a second/third player"): a + // successive `DistinctFromPriorChoices` pick omits players already + // chosen earlier in the same resolution. let mut state = GameState::new_two_player(42); - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player_distinct_from_prior()); ability.chosen_players = vec![PlayerId(0)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); @@ -1312,7 +1376,7 @@ mod tests { } #[test] - fn choose_player_with_all_players_chosen_resolves_as_no_op() { + fn choose_player_distinct_from_prior_with_all_players_chosen_resolves_as_no_op() { // CR 609.3 (issue #3040): when every eligible player is already chosen, // the engine-enumerated option set is empty — choosing is impossible, so // the choice does nothing and resolution continues. It must NOT raise a @@ -1324,7 +1388,7 @@ mod tests { state.waiting_for = WaitingFor::Priority { player: PlayerId(0), }; - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player_distinct_from_prior()); ability.chosen_players = vec![PlayerId(0), PlayerId(1)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); @@ -1425,7 +1489,7 @@ mod tests { let mut state = GameState::new_two_player(42); let mut ability = ResolvedAbility::new( Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::player(), persist: false, selection: TargetSelectionMode::Random, }, @@ -1454,7 +1518,7 @@ mod tests { // Building-block regression: a Chosen Choose is left to the interactive // `resolve` path (returns false; raises nothing here). let mut state = GameState::new_two_player(42); - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player()); let mut events = Vec::new(); assert!(!resolve_random_in_chain( &mut state, diff --git a/crates/engine/src/game/engine_resolution_choices.rs b/crates/engine/src/game/engine_resolution_choices.rs index e9ca9781b0..edf3f0a465 100644 --- a/crates/engine/src/game/engine_resolution_choices.rs +++ b/crates/engine/src/game/engine_resolution_choices.rs @@ -5633,7 +5633,7 @@ pub(super) fn handle_resolution_choice( // single GameState slot cleared after every drain. if matches!( choice_type, - ChoiceType::Player | ChoiceType::Opponent { .. } + ChoiceType::Player { .. } | ChoiceType::Opponent { .. } ) { if let Ok(pid) = choice.parse::() { if let Some(frame) = state.active_ability_continuation_frame_mut() { diff --git a/crates/engine/src/game/triggers_ordering_parity_tests.rs b/crates/engine/src/game/triggers_ordering_parity_tests.rs index c0c8b70456..eace11120c 100644 --- a/crates/engine/src/game/triggers_ordering_parity_tests.rs +++ b/crates/engine/src/game/triggers_ordering_parity_tests.rs @@ -1906,7 +1906,7 @@ fn choose_opponent_then_draw() -> ResolvedAbility { target: TargetFilter::Controller, }); ra(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::default(), }) diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index dddab1c00f..9f674ea2d5 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -1863,7 +1863,7 @@ fn ability_chain_has_player_choice(def: &AbilityDefinition) -> bool { matches!( def.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, .. } ) || def @@ -1948,7 +1948,7 @@ fn filter_references_source_chosen_player(filter: &TargetFilter) -> bool { /// sub-ability chain) to `persist: true` so its choice is stored durably. fn persist_player_choice_in_ability(def: &mut AbilityDefinition) { if let Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, persist, .. } = def.effect.as_mut() diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 0a9a92d403..e6ca1c1d09 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -104,14 +104,14 @@ use crate::types::ability::{ EffectScope, FilterProp, GameRestriction, GuessSubject, IntensityScope, IterationKindBinding, KeeperConstraint, LibraryPosition, ManaProduction, ManaSpendPermission, MultiTargetSpec, NumberDistinctness, ObjectProperty, ObjectScope, OriginConstraint, PerpetualModification, - PlayPermissionInvalidation, PlayerFilter, PlayerRelation, PlayerScope, PreventionAmount, - PreventionScope, ProhibitedActivity, PtValue, QuantityExpr, QuantityRef, ReplacementCondition, - ReplacementDefinition, RestrictionExpiry, RestrictionPlayerScope, RevealUntilDisposition, - RoundingMode, SharedQuality, SharedQualityRelation, SiblingCondition, SkipScope, - SpellStackToGraveyardReplacement, StaticCondition, StaticDefinition, StepSkipTarget, - SubAbilityLink, TapStateChange, TargetFilter, TargetSelectionMode, ThisWayCause, - TrackedAnaphorSource, TriggerCondition, TriggerDefinition, TypeFilter, TypedFilter, - UnlessPayModifier, UntilCondition, ZoneOwner, + PlayPermissionInvalidation, PlayerChoiceDistinctness, PlayerFilter, PlayerRelation, + PlayerScope, PreventionAmount, PreventionScope, ProhibitedActivity, PtValue, QuantityExpr, + QuantityRef, ReplacementCondition, ReplacementDefinition, RestrictionExpiry, + RestrictionPlayerScope, RevealUntilDisposition, RoundingMode, SharedQuality, + SharedQualityRelation, SiblingCondition, SkipScope, SpellStackToGraveyardReplacement, + StaticCondition, StaticDefinition, StepSkipTarget, SubAbilityLink, TapStateChange, + TargetFilter, TargetSelectionMode, ThisWayCause, TrackedAnaphorSource, TriggerCondition, + TriggerDefinition, TypeFilter, TypedFilter, UnlessPayModifier, UntilCondition, ZoneOwner, }; #[cfg(test)] use crate::types::ability::{AttackScope, AttackSubject}; @@ -529,7 +529,7 @@ fn finalize_committed_guess_choice_types( _ => unreachable!("matched above"), }; *ability.effect = Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }; @@ -7144,8 +7144,15 @@ fn parse_for_each_object_copy_parts( /// `ChosenPlayer` so a *following* sentence ("They put counters on a creature /// they control") binds its "they"/"that player" anaphora to this choice. /// -/// The ordinal word ("second"/"third") is a parse-time consistency hint only — -/// the engine index is derived from chain position, not the ordinal. +/// The ordinal word ("second"/"third") doubles as the CR 608.2c distinctness +/// signal: its presence marks this pick +/// `PlayerChoiceDistinctness::DistinctFromPriorChoices` (Gluntch, the +/// Bestower — each successive choice must exclude players already chosen +/// earlier in this resolution); its absence keeps the default `Independent`, +/// under which a bare repeated "choose a player"/"choose an opponent" may +/// repeat an earlier choice (confirmed by the "Offering" cycle ruling — +/// Benevolent/Infernal/Intellectual/Sylvan Offering — issue #6381). Either +/// way, the engine index is derived from chain position, not the ordinal. /// CR 102.3 + CR 608.2d: Recognize the "with the most life [among /// your opponents]" qualifier on a "choose an opponent" instruction and lower it /// to the equivalent `PlayerFilter::PlayerAttribute` restriction: each candidate @@ -7208,14 +7215,22 @@ fn try_parse_choose_player_to_verb( // and silently dropped opponent-form choose clauses. let player_arm = |i| { let (i, _) = tag::<_, _, OracleError<'_>>(" ").parse(i)?; - let i = super::oracle_util::parse_ordinal(i) - .map(|(_, rest)| rest) - .unwrap_or(i); + let (i, ordinal_present) = match super::oracle_util::parse_ordinal(i) { + Some((_, rest)) => (rest, true), + None => (i, false), + }; let (i, _) = tag::<_, _, OracleError<'_>>("player").parse(i)?; - Ok::<_, nom::Err>>((i, ChoiceType::Player)) + // CR 608.2c: the ordinal ("second"/"third") is the distinctness signal + // — see the doc comment above. + let distinctness = if ordinal_present { + PlayerChoiceDistinctness::DistinctFromPriorChoices + } else { + PlayerChoiceDistinctness::Independent + }; + Ok::<_, nom::Err>>((i, ChoiceType::Player { distinctness })) }; let opponent_arm = value( - ChoiceType::Opponent { restriction: None }, + ChoiceType::opponent(), tag::<_, _, OracleError<'_>>("n opponent"), ); let (after_player, mut choice_type) = @@ -7226,7 +7241,7 @@ fn try_parse_choose_player_to_verb( // restriction to the `Opponent` choice so it stays a single pick (CR 608.2d // resolves ties) rather than fanning out. Consume the qualifier so it is not // left dangling on the verb tail. - let after_player = if let ChoiceType::Opponent { restriction } = &mut choice_type { + let after_player = if let ChoiceType::Opponent { restriction, .. } = &mut choice_type { match parse_opponent_most_life_restriction(after_player) { Ok((rest, filter)) => { *restriction = Some(Box::new(filter)); @@ -7342,7 +7357,7 @@ fn try_parse_an_opponent_to_verb( } let mut clause = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -7388,7 +7403,7 @@ fn try_parse_opponent_guesses_chosen_library_kind( }); let mut choose_opponent = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -14525,13 +14540,25 @@ fn thread_for_each_subject(effect: Effect, original: &str, ctx: &mut ParseContex unless_filter, filter, }, + // CR 119.3 + CR 608.2c (issue #6381): "target player gains N life for + // each X" (issue #1508) needs an actual CR 601.2c target declaration + // (`is_targeted`); "that player gains N life for each X" is instead a + // resolution-scoped anaphor to a player chosen by an earlier "Choose an + // opponent."/"Choose a player." instruction (the "Offering" cycle: + // Benevolent/Infernal/Intellectual/Sylvan Offering) and never sets + // `application.target`. Accept either so both recipient-binding shapes + // rebind away from the no-subject `Controller` default. Effect::GainLife { amount, player: TargetFilter::Controller, - } if is_targeted && target_filter_can_target_player(&target) => Effect::GainLife { - amount, - player: target, - }, + } if target_filter_can_target_player(&target) + && (is_targeted || is_chosen_player_anaphor(&target)) => + { + Effect::GainLife { + amount, + player: target, + } + } // CR 115.1a/c + CR 701.17a + CR 608.2c: "Target opponent/player sacrifices // a [typed] permanent ... for each X" (Urborg Justice, Din of the Fireherd, // Rakdos Riteknife). The for-each interception strips the dynamic count @@ -17837,6 +17864,30 @@ fn try_parse_compound_subject_each( let (consumed_prefix, first_filter, second_filter) = parse_compound_subject_prefix(lower.as_str())?; + // CR 109.4 + CR 608.2c (issue #6381): "that player" in "you and that + // player each ..." is ambiguous prose with two distinct antecedents. The + // grammar's `ScopedPlayer` default is correct for a per-voter/per-opponent + // fan-out body (Master of Ceremonies-style vote bodies), but when a + // PRECEDING "Choose an opponent."/"Choose a player." instruction in the + // same resolution bound `ctx.relative_player_scope` to a + // `ControllerRef::ChosenPlayer { index }` (the "Offering" cycle: + // Benevolent/Infernal/Intellectual/Sylvan Offering), "that player" refers + // to THAT chosen player instead — rebind to match, mirroring + // `rebind_opponent_player_recipient_to_chosen`'s player-only `TargetFilter::Typed` + // encoding. Without this, the second recipient silently defaults to + // `scoped_player_or_controller`'s controller fallback (unset outside a + // fan-out), so every token/effect meant for "that player" was created for + // the caster instead. + let second_filter = match (&second_filter, ctx.relative_player_scope.clone()) { + (TargetFilter::ScopedPlayer, Some(ControllerRef::ChosenPlayer { index })) => { + TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::ChosenPlayer { index }), + ..Default::default() + }) + } + _ => second_filter, + }; + // Slice the original-case body text using the consumed offset. let body_text = text[consumed_prefix..].trim(); if body_text.is_empty() { @@ -20474,6 +20525,24 @@ fn target_filter_can_target_player(filter: &TargetFilter) -> bool { } } +/// CR 608.2c: true when `filter` is the resolution-scoped "that player"/"that +/// opponent" anaphor to a player chosen earlier in the same resolution by a +/// `Choose(Player)`/`Choose(Opponent)` instruction, encoded as the player-only +/// `TargetFilter::Typed` carrying `ControllerRef::ChosenPlayer { index }` +/// (mirrors `retarget_effect_to_chosen_player`'s encoding). Distinct from a +/// CR 601.2c cast-time target declaration ("target player"), which sets +/// `SubjectApplication.target` instead. +fn is_chosen_player_anaphor(filter: &TargetFilter) -> bool { + matches!( + filter, + TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::ChosenPlayer { .. }), + type_filters, + .. + }) if type_filters.is_empty() + ) +} + fn wrap_target_subject_damage( mut clause: ParsedEffectClause, subject: &SubjectPhraseAst, @@ -20716,6 +20785,17 @@ fn inject_subject_target(effect: &mut Effect, subject: &SubjectPhraseAst) { Effect::AdditionalPhase { target, .. } if *target == TargetFilter::Controller => { *target = subject_filter; } + // CR 119.3 (issue #6381): "that player gains N life" / "target player + // gains N life" — the imperative path defaults `player` to the + // no-subject `Controller` (the "you gain life" reading); inject the + // parsed subject when the sentence actually names a different + // recipient. Without this arm, EVERY "[non-you subject] gains N life" + // clause silently credited the ability's controller instead (the + // "Offering" cycle's "that player gains 2 life for each creature they + // control" — Benevolent/Infernal/Intellectual/Sylvan Offering). + Effect::GainLife { player, .. } if *player == TargetFilter::Controller => { + *player = subject_filter; + } // CR 400.7: "shuffle [subject]'s graveyard into their library" — inject // subject target for zone-wide changes and shuffles. Effect::ChangeZoneAll { ref mut target, .. } @@ -23325,7 +23405,7 @@ fn opponent_guess_clause(guesser: ControllerRef, subject: GuessSubject) -> Parse // so the chosen player is threaded to the dependent guess as a // same-resolution `ChosenPlayer`. let mut clause = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -23549,9 +23629,9 @@ pub(crate) fn parse_named_choice_object(rest: &str) -> Option { Some(ChoiceType::LandType) } else if tag::<_, _, E>("an opponent").parse(rest).is_ok() { // CR 800.4a: Choose an opponent from among players in the game. - Some(ChoiceType::Opponent { restriction: None }) + Some(ChoiceType::opponent()) } else if tag::<_, _, E>("a player").parse(rest).is_ok() { - Some(ChoiceType::Player) + Some(ChoiceType::player()) } else if tag::<_, _, E>("two colors").parse(rest).is_ok() { Some(ChoiceType::TwoColors) } else if tag::<_, _, E>("a word").parse(rest).is_ok() { @@ -30264,7 +30344,7 @@ pub(crate) fn parse_effect_chain_ir( if matches!( clause.effect, Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, .. } ) { diff --git a/crates/engine/src/parser/oracle_effect/snapshot_tests.rs b/crates/engine/src/parser/oracle_effect/snapshot_tests.rs index 44dfe8f139..24045f6569 100644 --- a/crates/engine/src/parser/oracle_effect/snapshot_tests.rs +++ b/crates/engine/src/parser/oracle_effect/snapshot_tests.rs @@ -973,7 +973,10 @@ fn strax_choose_a_player_at_random_records_random_selection() { ); match def.effect.as_ref() { Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: + ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::Independent, + }, selection, .. } => assert_eq!(*selection, TargetSelectionMode::Random), @@ -990,16 +993,20 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { AbilityKind::Spell, ); - // Node 0: the first `Choose(Player)`. + // Node 0: the first `Choose(Player)` — no ordinal, so the default + // `Independent` distinctness applies (CR 608.2c; issue #6381 confirms the + // bare "choose a player" must NOT exclude prior choices). assert!( matches!( def.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::Independent + }, .. } ), - "first node must be Choose(Player), got {:?}", + "first node must be Choose(Player) with Independent distinctness, got {:?}", def.effect ); @@ -1017,17 +1024,21 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { "the +1/+1 counters go on a creature the 1st chosen player controls" ); - // Node 2: the second `Choose(Player)`. + // Node 2: the second `Choose(Player)` — "a second player" carries the + // ordinal, so it must be `DistinctFromPriorChoices` (Gluntch's "three + // distinct players" ruling). let node2 = node1.sub_ability.as_ref().expect("2nd Choose node"); assert!( matches!( node2.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices + }, .. } ), - "node 2 must be Choose(Player) — not Unimplemented — got {:?}", + "node 2 must be Choose(Player) with DistinctFromPriorChoices — not Unimplemented — got {:?}", node2.effect ); @@ -1042,17 +1053,20 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { "the 2nd chosen player draws the card" ); - // Node 4: the third `Choose(Player)`. + // Node 4: the third `Choose(Player)` — "a third player" carries the + // ordinal, so it must be `DistinctFromPriorChoices` too. let node4 = node3.sub_ability.as_ref().expect("3rd Choose node"); assert!( matches!( node4.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices + }, .. } ), - "node 4 must be Choose(Player) — not Unimplemented — got {:?}", + "node 4 must be Choose(Player) with DistinctFromPriorChoices — not Unimplemented — got {:?}", node4.effect ); diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 7aecd09b42..2cfbffb6bf 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -24452,7 +24452,10 @@ fn gollum_scheming_guide_guess_sequence_has_no_unimplemented() { && matches!( node.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -24665,7 +24668,10 @@ fn committed_choice_guess_chooses_single_opponent_before_guess() { matches!( choose_opponent.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -39190,6 +39196,7 @@ fn the_master_most_life_villainous_choice() { choice_type: ChoiceType::Opponent { restriction: Some(restriction), + .. }, persist, .. diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index a74fc1f2e6..46e531854b 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -2111,7 +2111,7 @@ fn front_opponent_choice_for_nontargeted_look(reveal: &Effect) -> Option<(Effect // observable outcome. let choose_opponent = Effect::Choose { // CR 608.2d + CR 102.3: the controller chooses one opponent. - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, // Same controller-choice selection mode as the fronted card-name choice. selection: crate::types::ability::TargetSelectionMode::Chosen, @@ -14669,7 +14669,10 @@ mod tests { matches!( &*execute.effect, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: true, .. } @@ -14780,7 +14783,10 @@ mod tests { matches!( &*mid.effect, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: true, .. } diff --git a/crates/engine/src/parser/oracle_vote.rs b/crates/engine/src/parser/oracle_vote.rs index 13fb10298a..45ccb9afc6 100644 --- a/crates/engine/src/parser/oracle_vote.rs +++ b/crates/engine/src/parser/oracle_vote.rs @@ -1210,10 +1210,13 @@ fn parse_vote_for_each_suffix_clause<'a>( opt(tag_no_case("then ")), alt(( value( - ChoiceType::Opponent { restriction: None }, + ChoiceType::opponent(), tag_no_case("choose an opponent at random"), ), - value(ChoiceType::Player, tag_no_case("choose a player at random")), + value( + ChoiceType::player(), + tag_no_case("choose a player at random"), + ), )), tag(". "), )) @@ -2397,7 +2400,10 @@ mod tests { assert_eq!(rest, ""); assert!(matches!( setup, - Some(ChoiceType::Opponent { restriction: None }) + Some(ChoiceType::Opponent { + restriction: None, + .. + }) )); match &*def.effect { Effect::DealDamage { amount, target, .. } => { diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 3a8322bce2..b2e2082ac5 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -525,6 +525,28 @@ pub enum NumberDistinctness { DistinctFromSourceHistory, } +/// CR 608.2c: whether a "choose a player"/"choose an opponent" instruction +/// must exclude players already chosen earlier in the SAME resolution, or is +/// an independent pick that may repeat an earlier choice. Parse-detected; +/// static; serialized only when non-default so existing `Player`/`Opponent` +/// card-data stays byte-stable. Mirrors `NumberDistinctness`'s axis on the +/// sibling `NumberRange` choice. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +pub enum PlayerChoiceDistinctness { + /// The default: repeated "Choose an opponent."/"Choose a player." + /// instructions in one resolution are independent picks — the same + /// player may be chosen more than once. Confirmed by the "Offering" cycle + /// ruling (Benevolent/Infernal/Intellectual/Sylvan Offering): "You may + /// choose the same opponent for each of the effects, or you may choose + /// different opponents." + #[default] + Independent, + /// Ordinal-cued instructions ("choose a second player", "choose a third + /// player" — Gluntch, the Bestower) require each successive choice to + /// exclude every player already chosen earlier in this resolution. + DistinctFromPriorChoices, +} + /// What kind of named choice the player must make at resolution time. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ChoiceType { @@ -590,11 +612,22 @@ pub enum ChoiceType { /// the qualifying opponents (CR 608.2d handles ties) — rather than fanning /// the effect out to every tied opponent. Boxed to avoid inflating the /// `ChoiceType` enum with the recursive `PlayerFilter` payload. + /// + /// `distinctness` (CR 608.2c) governs whether this pick must exclude + /// players already chosen by an earlier `Opponent`/`Player` choice in the + /// same resolution. Defaults to `Independent` — see + /// [`PlayerChoiceDistinctness`]. Opponent { restriction: Option>, + distinctness: PlayerChoiceDistinctness, + }, + /// "Choose a player" — selects any player in the game. `distinctness` + /// (CR 608.2c) governs whether this pick must exclude players already + /// chosen by an earlier `Opponent`/`Player` choice in the same + /// resolution — see [`PlayerChoiceDistinctness`]. + Player { + distinctness: PlayerChoiceDistinctness, }, - /// "Choose a player" — selects any player in the game. - Player, /// "Choose two colors" — selects two distinct mana colors. TwoColors, /// "Choose a word" — names any English word (Un-set and silver-border cards). @@ -702,6 +735,39 @@ impl ChoiceType { Self::CardType { excluded } } + /// Unrestricted "choose an opponent" (CR 102.3), independent of any other + /// choice in the resolution (the "Offering" cycle default). + pub fn opponent() -> Self { + Self::Opponent { + restriction: None, + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// "Choose an opponent [with the most life ...]" (CR 608.2d). + pub fn opponent_with_restriction(restriction: PlayerFilter) -> Self { + Self::Opponent { + restriction: Some(Box::new(restriction)), + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// "Choose a player" (CR 102.1), independent of any other choice in the + /// resolution. + pub fn player() -> Self { + Self::Player { + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// Ordinal-cued "choose a second/third player" (Gluntch, the Bestower): + /// must exclude players already chosen earlier in this resolution. + pub fn player_distinct_from_prior() -> Self { + Self::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices, + } + } + pub fn land_or_nonland_card_predicate_options() -> Vec { vec![CardPredicateChoice::Land, CardPredicateChoice::Nonland] } @@ -840,19 +906,47 @@ impl Serialize for ChoiceType { variant.serialize_field("options", options)?; variant.end() } - // Serialize the unrestricted form as the legacy unit variant - // "Opponent" so existing card-data JSON stays byte-stable; only emit - // the struct form when a restriction is present. - Self::Opponent { restriction } => match restriction { - None => serializer.serialize_unit_variant("ChoiceType", 9, "Opponent"), - Some(restriction) => { - let mut variant = - serializer.serialize_struct_variant("ChoiceType", 9, "Opponent", 1)?; + // Serialize the unrestricted, default-distinctness form as the + // legacy unit variant "Opponent" so existing card-data JSON stays + // byte-stable; only emit the struct form when a restriction + // and/or a non-default `distinctness` is present. + Self::Opponent { + restriction, + distinctness, + } => { + let non_default_distinctness = + *distinctness != PlayerChoiceDistinctness::Independent; + if restriction.is_none() && !non_default_distinctness { + serializer.serialize_unit_variant("ChoiceType", 9, "Opponent") + } else { + let field_count = 1 + non_default_distinctness as usize; + let mut variant = serializer.serialize_struct_variant( + "ChoiceType", + 9, + "Opponent", + field_count, + )?; variant.serialize_field("restriction", restriction)?; + if non_default_distinctness { + variant.serialize_field("distinctness", distinctness)?; + } variant.end() } - }, - Self::Player => serializer.serialize_unit_variant("ChoiceType", 10, "Player"), + } + // Serialize the default-distinctness form as the legacy unit + // variant "Player" so existing card-data JSON stays byte-stable; + // only emit the struct form when `distinctness` is non-default + // (Gluntch, the Bestower's ordinal-cued picks). + Self::Player { distinctness } => { + if *distinctness == PlayerChoiceDistinctness::Independent { + serializer.serialize_unit_variant("ChoiceType", 10, "Player") + } else { + let mut variant = + serializer.serialize_struct_variant("ChoiceType", 10, "Player", 1)?; + variant.serialize_field("distinctness", distinctness)?; + variant.end() + } + } Self::TwoColors => serializer.serialize_unit_variant("ChoiceType", 11, "TwoColors"), Self::Word => serializer.serialize_unit_variant("ChoiceType", 12, "Word"), Self::Artist => serializer.serialize_unit_variant("ChoiceType", 13, "Artist"), @@ -929,6 +1023,12 @@ impl<'de> Deserialize<'de> for ChoiceType { Opponent { #[serde(default)] restriction: Option>, + #[serde(default)] + distinctness: PlayerChoiceDistinctness, + }, + Player { + #[serde(default)] + distinctness: PlayerChoiceDistinctness, }, Keyword { options: Vec, @@ -955,8 +1055,8 @@ impl<'de> Deserialize<'de> for ChoiceType { "CardType" => Ok(Self::card_type()), "CardName" => Ok(Self::CardName), "LandType" => Ok(Self::LandType), - "Opponent" => Ok(Self::Opponent { restriction: None }), - "Player" => Ok(Self::Player), + "Opponent" => Ok(Self::opponent()), + "Player" => Ok(Self::player()), "TwoColors" => Ok(Self::TwoColors), "Word" => Ok(Self::Word), "Artist" => Ok(Self::Artist), @@ -996,7 +1096,14 @@ impl<'de> Deserialize<'de> for ChoiceType { ChoiceTypeData::CardPredicateGuess { options } => { Ok(Self::CardPredicateGuess { options }) } - ChoiceTypeData::Opponent { restriction } => Ok(Self::Opponent { restriction }), + ChoiceTypeData::Opponent { + restriction, + distinctness, + } => Ok(Self::Opponent { + restriction, + distinctness, + }), + ChoiceTypeData::Player { distinctness } => Ok(Self::Player { distinctness }), ChoiceTypeData::Keyword { options, count } => Ok(Self::Keyword { options, count }), ChoiceTypeData::CounterKind { options } => Ok(Self::CounterKind { options }), }, @@ -1379,7 +1486,7 @@ impl ChosenAttribute { distinctness: NumberDistinctness::Repeatable, }, // Player covers both Player and Opponent choice types - Self::Player(_) => ChoiceType::Player, + Self::Player(_) => ChoiceType::player(), Self::TwoColors(_) => ChoiceType::TwoColors, // CR 702.104: Tribute outcome uses a dedicated prompt type rather than // a NamedChoice (two fixed labels: Paid / Declined). Classify under the @@ -1514,7 +1621,7 @@ impl ChoiceValue { } ChoiceType::LandType => Some(Self::LandType(value.to_string())), // CR 800.4a: Parse player ID from string. - ChoiceType::Opponent { .. } | ChoiceType::Player => value + ChoiceType::Opponent { .. } | ChoiceType::Player { .. } => value .parse::() .ok() .map(|id| Self::Player(PlayerId(id))), @@ -24114,14 +24221,14 @@ mod tests { // unrestricted form; it must round-trip to `restriction: None`. let choice_type: ChoiceType = serde_json::from_str("\"Opponent\"").unwrap(); - assert_eq!(choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(choice_type, ChoiceType::opponent()); } #[test] fn choice_type_opponent_unrestricted_serializes_as_legacy_unit() { // The hand-rolled Serialize must keep emitting the bare string for the // unrestricted form so existing card-data.json stays byte-stable. - let json = serde_json::to_string(&ChoiceType::Opponent { restriction: None }).unwrap(); + let json = serde_json::to_string(&ChoiceType::opponent()).unwrap(); assert_eq!(json, "\"Opponent\""); } @@ -24182,22 +24289,20 @@ mod tests { // The Master, Gallifrey's End: "choose an opponent with the most life". // The hand-rolled Serialize/Deserialize for the restricted struct form // must be symmetric or card-data.json load corrupts silently. - let original = ChoiceType::Opponent { - restriction: Some(Box::new(PlayerFilter::PlayerAttribute { - relation: PlayerRelation::Opponent, - attr: Box::new(QuantityRef::LifeTotal { - player: PlayerScope::ScopedPlayer, - }), - comparator: Comparator::GE, - value: Box::new(QuantityExpr::Ref { - qty: QuantityRef::LifeTotal { - player: PlayerScope::Opponent { - aggregate: AggregateFunction::Max, - }, + let original = ChoiceType::opponent_with_restriction(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::Opponent, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GE, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Opponent { + aggregate: AggregateFunction::Max, }, - }), - })), - }; + }, + }), + }); let json = serde_json::to_string(&original).unwrap(); // Restricted form must use the externally-tagged struct variant so it is @@ -24211,6 +24316,34 @@ mod tests { assert_eq!(round_tripped, original); } + #[test] + fn choice_type_player_unrestricted_serializes_as_legacy_unit() { + // The hand-rolled Serialize must keep emitting the bare "Player" string + // for the default-distinctness form so existing card-data.json (Strax, + // Sontaran Nurse) stays byte-stable. + let json = serde_json::to_string(&ChoiceType::player()).unwrap(); + assert_eq!(json, "\"Player\""); + + let round_tripped: ChoiceType = serde_json::from_str(&json).unwrap(); + assert_eq!(round_tripped, ChoiceType::player()); + } + + #[test] + fn choice_type_player_distinct_from_prior_serde_round_trips() { + // Gluntch, the Bestower's ordinal-cued "choose a second/third player" + // must serialize to the struct form carrying the non-default + // `distinctness`, and round-trip back losslessly. + let original = ChoiceType::player_distinct_from_prior(); + let json = serde_json::to_string(&original).unwrap(); + assert!( + json.starts_with(r#"{"Player":"#), + "non-default distinctness should serialize as a struct variant, got: {json}" + ); + + let round_tripped: ChoiceType = serde_json::from_str(&json).unwrap(); + assert_eq!(round_tripped, original); + } + #[test] fn restricted_color_choice_value_rejects_excluded_color() { assert_eq!( diff --git a/crates/engine/tests/integration/baleful_mastery_regression.rs b/crates/engine/tests/integration/baleful_mastery_regression.rs index 89bfdc70a3..1f8ff20dc9 100644 --- a/crates/engine/tests/integration/baleful_mastery_regression.rs +++ b/crates/engine/tests/integration/baleful_mastery_regression.rs @@ -170,7 +170,7 @@ fn baleful_mastery_alternative_cost_makes_chosen_opponent_draw() { options, .. } => { - assert_eq!(choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(choice_type, ChoiceType::opponent()); assert_eq!(options, vec![P1.0.to_string()]); runner .act(GameAction::ChooseOption { diff --git a/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs b/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs index cb3cceb7b3..70c612bf94 100644 --- a/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs +++ b/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs @@ -63,7 +63,10 @@ fn gollum_attack_trigger_parser_promotes_choices_to_card_predicates() { assert!(matches!( choose_opponent.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -278,7 +281,13 @@ fn choose_opponent( }; assert_eq!(player, expected_chooser, "wrong player choosing opponent"); assert!( - matches!(choice_type, ChoiceType::Opponent { restriction: None }), + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), "expected opponent choice, got {choice_type:?}" ); let choice = opponent.0.to_string(); diff --git a/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs b/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs index 7f3a001bc7..f2004f5bf7 100644 --- a/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs +++ b/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs @@ -29,7 +29,7 @@ fn choose_opponent(runner: &mut GameRunner, opponent: engine::types::PlayerId) { options, .. } => { - assert_eq!(*choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(*choice_type, ChoiceType::opponent()); assert!( options.contains(&opponent.0.to_string()), "opponent must be legal; options={options:?}" diff --git a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs new file mode 100644 index 0000000000..09a61593f8 --- /dev/null +++ b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs @@ -0,0 +1,156 @@ +//! Regression (issue #6381): Benevolent Offering's two independent "Choose an +//! opponent." instructions must each accept the SAME opponent in a two-player +//! game. Official ruling: "You may choose the same opponent for each of the +//! effects, or you may choose different opponents." (Confirmed identically +//! for the "Offering" cycle: Infernal/Intellectual/Sylvan Offering.) +//! +//! Before the fix, `ChoiceType::Opponent`/`ChoiceType::Player` unconditionally +//! excluded players already chosen earlier in the same resolution (correct +//! only for Gluntch, the Bestower's ordinal-cued "choose a second/third +//! player"). In a two-player game that made the SECOND "Choose an opponent." +//! impossible — CR 609.3 turned it into a no-op, so "that player" never got +//! bound for the life-gain clause and the chosen opponent gained 0 life +//! instead of 2 life per creature they control. + +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::ChoiceType; +use engine::types::game_state::WaitingFor; +use engine::types::mana::{ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; + +const BENEVOLENT_OFFERING: &str = "Choose an opponent. You and that player each create three 1/1 white Spirit \ + creature tokens with flying.\nChoose an opponent. You gain 2 life for each creature you control and that \ + player gains 2 life for each creature they control."; + +fn floating_mana(color: ManaType, n: usize) -> Vec { + (0..n) + .map(|_| { + ManaUnit::new( + color, + engine::types::identifiers::ObjectId(0), + false, + vec![], + ) + }) + .collect() +} + +fn player_life(runner: &GameRunner, player: PlayerId) -> i32 { + runner + .state() + .players + .iter() + .find(|p| p.id == player) + .unwrap() + .life +} + +/// Assert a `NamedChoice(Opponent)` prompt is showing, that `opponent` is +/// among the legal (non-excluded) options, then answer it. +fn choose_opponent(runner: &mut GameRunner, opponent: PlayerId) { + match &runner.state().waiting_for { + WaitingFor::NamedChoice { + choice_type, + options, + .. + } => { + assert!( + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), + "expected an unrestricted opponent choice, got {choice_type:?}" + ); + assert!( + options.contains(&opponent.0.to_string()), + "opponent P{} must remain a legal pick (Offering cycle ruling allows \ + repeating an earlier choice); options={options:?}", + opponent.0 + ); + } + other => panic!("expected NamedChoice(Opponent), got {other:?}"), + } + runner + .act(engine::types::actions::GameAction::ChooseOption { + choice: opponent.0.to_string(), + }) + .expect("ChooseOption(opponent) must succeed"); +} + +#[test] +fn benevolent_offering_allows_choosing_the_same_opponent_twice() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool( + P0, + [ + floating_mana(ManaType::Colorless, 3), + floating_mana(ManaType::White, 1), + ] + .concat(), + ); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Benevolent Offering", true, BENEVOLENT_OFFERING) + .id(); + + let mut runner = scenario.build(); + let life_before_p0 = player_life(&runner, P0); + let life_before_p1 = player_life(&runner, P1); + + runner.cast(spell).resolve(); + + // First "Choose an opponent." (fronting the twin token creation). + choose_opponent(&mut runner, P1); + // Second "Choose an opponent." — must offer P1 again, not exclude it. + choose_opponent(&mut runner, P1); + + for _ in 0..8 { + if matches!(runner.state().waiting_for, WaitingFor::Priority { .. }) + && runner.state().stack.is_empty() + { + break; + } + runner + .act(engine::types::actions::GameAction::PassPriority) + .ok(); + } + + // CR 111.7: each player controls exactly three of the created Spirit tokens. + let p0_spirits = runner + .state() + .objects + .values() + .filter(|o| o.controller == P0 && o.name == "Spirit") + .count(); + let p1_spirits = runner + .state() + .objects + .values() + .filter(|o| o.controller == P1 && o.name == "Spirit") + .count(); + assert_eq!(p0_spirits, 3, "the caster must control three Spirit tokens"); + assert_eq!( + p1_spirits, 3, + "the chosen opponent must control three Spirit tokens" + ); + + // CR 119.3: each player gains 2 life per creature they control (their own + // three Spirit tokens). Under the pre-fix bug, P1's gain was 0 because the + // second Choose(Opponent) resolved as an impossible no-op. + assert_eq!( + player_life(&runner, P0) - life_before_p0, + 6, + "the caster must gain 2 life per creature controlled (3 Spirits)" + ); + assert_eq!( + player_life(&runner, P1) - life_before_p1, + 6, + "the chosen opponent must gain 2 life per creature controlled (3 Spirits) \ + — this is the reported defect: it read 0 before the fix" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 6011431b36..5048e7fd8a 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -613,6 +613,7 @@ mod issue_6092_ability_block_reason; mod issue_6102_ragavan_exile_cast; mod issue_6157_gold_token_auto_mana_payment; mod issue_629_fractured_sanity_cycling; +mod issue_6381_benevolent_offering_repeat_opponent; mod issue_6403_moonmist_mass_transform; mod issue_6416_extra_turn_resume_order; mod issue_6431_lava_dart_flashback_control_turn; diff --git a/crates/engine/tests/integration/rules/tribute.rs b/crates/engine/tests/integration/rules/tribute.rs index 1c12efc64a..2c30e4628f 100644 --- a/crates/engine/tests/integration/rules/tribute.rs +++ b/crates/engine/tests/integration/rules/tribute.rs @@ -64,7 +64,7 @@ fn cast_tribute_creature(count: u32, paid: bool) -> GameRunner { .. } => { assert_eq!(*player, P0, "controller should be choosing the opponent"); - assert_eq!(*choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(*choice_type, ChoiceType::opponent()); assert!( options.contains(&P1.0.to_string()), "P1 must be a valid opponent choice, got {options:?}" diff --git a/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs b/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs index 2fed2bc254..f4560960ad 100644 --- a/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs +++ b/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs @@ -126,7 +126,13 @@ fn choose_guessing_opponent(runner: &mut GameRunner, opponent: PlayerId) { "the controller chooses which opponent makes the guess" ); assert!( - matches!(choice_type, ChoiceType::Opponent { restriction: None }), + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), "expected opponent choice before the guess, got {choice_type:?}" ); assert!( diff --git a/crates/mtgish-import/src/convert/action.rs b/crates/mtgish-import/src/convert/action.rs index d4cdb8f421..edec390cce 100644 --- a/crates/mtgish-import/src/convert/action.rs +++ b/crates/mtgish-import/src/convert/action.rs @@ -4441,8 +4441,8 @@ pub fn convert(a: &Action) -> ConvResult { // existing `players_to_controller` bridge for opponent detection. Action::ChooseAPlayer(players) => { let choice_type = match filter_mod::players_to_controller(players.as_ref()) { - Ok(ControllerRef::Opponent) => ChoiceType::Opponent { restriction: None }, - _ => ChoiceType::Player, + Ok(ControllerRef::Opponent) => ChoiceType::opponent(), + _ => ChoiceType::player(), }; Effect::Choose { choice_type, diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index 1b9a2f6319..c3803c2fdf 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -1871,8 +1871,8 @@ fn build_replacement_exec( A::ChooseAPlayer(players) => { let choice_type = match crate::convert::filter::players_to_controller(players.as_ref()) { - Ok(ControllerRef::Opponent) => ChoiceType::Opponent { restriction: None }, - _ => ChoiceType::Player, + Ok(ControllerRef::Opponent) => ChoiceType::opponent(), + _ => ChoiceType::player(), }; Effect::Choose { choice_type, From ecf9daa468a0b0e393e8831c9e1b1d156e568af2 Mon Sep 17 00:00:00 2001 From: jsdevninja Date: Wed, 29 Jul 2026 01:29:30 -0500 Subject: [PATCH 2/3] fix(engine): gate GainLife subject-injection on a genuine player filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the parse-diff artifact on PR #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 ." 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. --- crates/engine/src/parser/oracle_effect/mod.rs | 22 +++- ...381_benevolent_offering_repeat_opponent.rs | 113 +++++++++++++++++- .../mtgish-import/src/convert/replacement.rs | 4 +- 3 files changed, 130 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index e6ca1c1d09..95786e59d7 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -20788,12 +20788,22 @@ fn inject_subject_target(effect: &mut Effect, subject: &SubjectPhraseAst) { // CR 119.3 (issue #6381): "that player gains N life" / "target player // gains N life" — the imperative path defaults `player` to the // no-subject `Controller` (the "you gain life" reading); inject the - // parsed subject when the sentence actually names a different - // recipient. Without this arm, EVERY "[non-you subject] gains N life" - // clause silently credited the ability's controller instead (the - // "Offering" cycle's "that player gains 2 life for each creature they - // control" — Benevolent/Infernal/Intellectual/Sylvan Offering). - Effect::GainLife { player, .. } if *player == TargetFilter::Controller => { + // parsed subject when the sentence actually names a different, + // genuinely player-denoting recipient. Without this arm, EVERY + // "[non-you subject] gains N life" clause silently credited the + // ability's controller instead (the "Offering" cycle's "that player + // gains 2 life for each creature they control" — Benevolent/Infernal/ + // Intellectual/Sylvan Offering). Gated on `target_filter_can_target_player` + // (mirrors the `thread_for_each_subject` GainLife arm) so an + // unresolved compound subject ("you and that player each gain that + // much life" — Angel of Destiny, where the "each"-bodied split + // doesn't cover `GainLife` and falls back to a non-player `Any` + // subject) leaves the safer `Controller` default alone instead of + // rebinding to a nonsensical recipient. + Effect::GainLife { player, .. } + if *player == TargetFilter::Controller + && target_filter_can_target_player(&subject_filter) => + { *player = subject_filter; } // CR 400.7: "shuffle [subject]'s graveyard into their library" — inject diff --git a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs index 09a61593f8..fe055aba90 100644 --- a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs +++ b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs @@ -13,11 +13,12 @@ //! instead of 2 life per creature they control. use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; -use engine::types::ability::ChoiceType; +use engine::types::ability::{ChoiceType, ControllerRef, Effect, TargetFilter, TypedFilter}; use engine::types::game_state::WaitingFor; use engine::types::mana::{ManaType, ManaUnit}; use engine::types::phase::Phase; use engine::types::player::PlayerId; +use engine::types::triggers::TriggerMode; const BENEVOLENT_OFFERING: &str = "Choose an opponent. You and that player each create three 1/1 white Spirit \ creature tokens with flying.\nChoose an opponent. You gain 2 life for each creature you control and that \ @@ -154,3 +155,113 @@ fn benevolent_offering_allows_choosing_the_same_opponent_twice() { — this is the reported defect: it read 0 before the fix" ); } + +/// Intellectual Offering shares Benevolent Offering's "Choose an opponent. +/// You and that player each ." shape, so it exercises the SAME +/// `try_parse_compound_subject_each` fix: "that player" must rebind to the +/// resolution-scoped chosen player (`ChosenPlayer { index }`), not the +/// unrelated vote/fan-out `ScopedPlayer` axis. Locks in that the whole +/// "Offering" cycle — not just Benevolent Offering — benefits. +#[test] +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 + ); +} + +/// Regression guard for the fix above: `inject_subject_target`'s new +/// `GainLife` arm must NOT rebind the recipient when the detected subject +/// isn't a genuine player reference. Angel of Destiny's "you and that player +/// each gain that much life" is a compound subject that `GainLife` doesn't +/// support in `rewrite_recipient_on_link` (Token/Draw/Discard/Mill/Pump/ +/// GenericEffect only), so it falls through to a non-player-denoting subject +/// filter; the safe no-subject `Controller` default must survive rather than +/// being corrupted into an incoherent recipient. This is a pre-existing gap +/// (the damaged player still doesn't gain life) — not fixed here — but the +/// `player` field must stay a well-defined `Controller`, not silently swap to +/// something meaningless. +#[test] +fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let creature = scenario + .add_creature_from_oracle( + P0, + "Angel of Destiny", + 3, + 4, + "Flying, double strike\nWhenever a creature you control deals combat damage to a player, you and that player each gain that much life.\nAt the beginning of your end step, if you have at least 15 life more than your starting life total, each player this creature attacked this turn loses the game.", + ) + .id(); + let runner = scenario.build(); + let obj = runner.state().objects.get(&creature).unwrap(); + let damage_trigger = obj + .trigger_definitions + .iter_unchecked() + .find(|entry| matches!(entry.definition.mode, TriggerMode::DamageDone)) + .expect("Angel of Destiny must have a DamageDone trigger"); + let execute = damage_trigger + .definition + .execute + .as_ref() + .expect("DamageDone trigger must have an execute body"); + assert!( + matches!( + execute.effect.as_ref(), + Effect::GainLife { + player: TargetFilter::Controller, + .. + } + ), + "GainLife.player must stay the well-defined Controller default, not an \ + unresolved compound-subject filter like Any, got {:?}", + execute.effect + ); +} diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index c3803c2fdf..a11778a778 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -1864,8 +1864,8 @@ fn build_replacement_exec( persist: true, selection: engine::types::ability::TargetSelectionMode::Chosen, }, - // CR 800.4a: opponent-scoped player choice when the schema - // filter narrows to opponents; broader player choice + // CR 102.1-102.3 + CR 608.2d: opponent-scoped player choice when the + // schema filter narrows to opponents; broader player choice // otherwise. Re-uses the existing `players_to_controller` // bridge for opponent detection. A::ChooseAPlayer(players) => { From 23d568e715c7d7a4507b3942497e8484e9b31531 Mon Sep 17 00:00:00 2001 From: jsdevninja Date: Wed, 29 Jul 2026 06:41:22 -0500 Subject: [PATCH 3/3] test(engine): add runtime draw-count proof for Intellectual Offering 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. --- ...381_benevolent_offering_repeat_opponent.rs | 87 +++++++++++++++++-- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs index fe055aba90..35cb715289 100644 --- a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs +++ b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs @@ -156,23 +156,98 @@ fn benevolent_offering_allows_choosing_the_same_opponent_twice() { ); } +const INTELLECTUAL_OFFERING: &str = "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."; + +fn hand_count(runner: &GameRunner, player: PlayerId) -> usize { + runner + .state() + .objects + .values() + .filter(|o| o.owner == player && o.zone == engine::types::zones::Zone::Hand) + .count() +} + +/// Runtime counterpart to `intellectual_offering_second_draw_binds_to_chosen_opponent` +/// below: drives the real cast/resolution pipeline (not just the parsed AST) +/// so the fix is proven all the way through `game/effects/draw.rs`'s +/// `ChosenPlayer` resolution (`game/effects/mod.rs`'s `resolve_player_for_context_ref`), +/// not just the parser. An AST-only assertion would stay green even if the +/// resolver drew for the wrong player. +#[test] +fn intellectual_offering_draws_three_for_caster_and_chosen_opponent() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool( + P0, + [ + floating_mana(ManaType::Colorless, 4), + floating_mana(ManaType::Blue, 1), + ] + .concat(), + ); + // Seed both libraries well past the three cards each side draws. + scenario.with_library_top(P0, &["Forest", "Forest", "Forest", "Forest", "Forest"]); + scenario.with_library_top(P1, &["Island", "Island", "Island", "Island", "Island"]); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Intellectual Offering", true, INTELLECTUAL_OFFERING) + .id(); + + let mut runner = scenario.build(); + // The spell itself occupies P0's hand until it resolves off the stack; + // measure the draw delta from the post-cast baseline, not pre-cast. + let hand_before_p1 = hand_count(&runner, P1); + + runner.cast(spell).resolve(); + let hand_before_p0 = hand_count(&runner, P0); + + // First "Choose an opponent." (fronting the twin three-card draw). + choose_opponent(&mut runner, P1); + // Second "Choose an opponent." — must offer P1 again, not exclude it. + choose_opponent(&mut runner, P1); + + for _ in 0..8 { + if matches!(runner.state().waiting_for, WaitingFor::Priority { .. }) + && runner.state().stack.is_empty() + { + break; + } + runner + .act(engine::types::actions::GameAction::PassPriority) + .ok(); + } + + assert_eq!( + hand_count(&runner, P0) - hand_before_p0, + 3, + "the caster must draw three cards" + ); + assert_eq!( + hand_count(&runner, P1) - hand_before_p1, + 3, + "the chosen opponent must draw three cards — this is the runtime proof \ + that ChosenPlayer{{index: 0}} (not the unrelated ScopedPlayer default) \ + resolves the second Draw's recipient" + ); +} + /// Intellectual Offering shares Benevolent Offering's "Choose an opponent. /// You and that player each ." shape, so it exercises the SAME /// `try_parse_compound_subject_each` fix: "that player" must rebind to the /// resolution-scoped chosen player (`ChosenPlayer { index }`), not the /// unrelated vote/fan-out `ScopedPlayer` axis. Locks in that the whole /// "Offering" cycle — not just Benevolent Offering — benefits. +/// +/// AST-shape companion to `intellectual_offering_draws_three_for_caster_and_chosen_opponent` +/// above; kept as a SHAPE test (see the `card-test` skill) because it pins +/// the exact parser output distinct from the runtime draw-count proof. #[test] 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.", - ) + .add_spell_to_hand_from_oracle(P0, "Intellectual Offering", true, INTELLECTUAL_OFFERING) .id(); let runner = scenario.build(); let ability = &runner.state().objects.get(&spell).unwrap().abilities[0];