diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 8ae1ad4e59..0c5b7943de 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -3326,26 +3326,92 @@ fn is_player_scope_local_continuation( return true; } - // CR 608.2c + CR 701.24a: "Each player shuffles the cards from their hand - // into their library" is one per-player move/shuffle instruction only for - // an all-player scope. Keep the terminal shuffle with its immediately - // preceding ChangeZoneAll; a following Draw remains the detached - // all-player instruction. - matches!(scope, PlayerFilter::All) - && matches!( - (parent, child), - ( - Effect::ChangeZoneAll { - origin: Some(Zone::Hand), - destination: Zone::Library, - target: TargetFilter::ScopedPlayer, - .. - }, - Effect::Shuffle { - target: TargetFilter::ScopedPlayer, - } - ) + // CR 608.2c + CR 701.24a: " shuffles the cards from their hand + // into their library" is ONE per-player move/shuffle instruction. Keep the + // terminal shuffle with its immediately preceding ChangeZoneAll; a following + // Draw remains the detached post-loop instruction. + let is_scoped_whole_hand_shuffle = matches!( + (parent, child), + ( + Effect::ChangeZoneAll { + origin: Some(Zone::Hand), + destination: Zone::Library, + target: TargetFilter::ScopedPlayer, + .. + }, + Effect::Shuffle { + target: TargetFilter::ScopedPlayer, + } ) + ); + is_scoped_whole_hand_shuffle && scope_keeps_scoped_whole_hand_shuffle_local(scope) +} + +/// CR 115.10 + CR 608.2c + CR 701.24a: Does this `player_scope` filter keep the +/// scoped whole-hand-move/shuffle pair inside its own per-player iteration? +/// +/// EXHAUSTIVE BY DESIGN — no `_` arm, no `matches!`, and it must stay that way. +/// The predecessor form was `matches!(scope, PlayerFilter::All)`: a +/// hand-maintained allowlist that silently answered "detach" for every filter it +/// did not name (#6957). That answer is structural, not inert — the caller +/// (`detach_after_player_scope_local_chain`) uses it to pull the shuffle out of +/// the loop and run it ONCE as an unscoped tail, where `ScopedPlayer` has no +/// iteration player to bind to. Compiler exhaustiveness cannot see inside a +/// `matches!`, so a new SCOPE-position filter joined the wrong side for free. +/// This form makes the compiler demand an answer, exactly as +/// `player_filter_references_tracked_set` does for the publication allowlist. +/// +/// The answer is the same for every filter, and that uniformity is the finding: +/// the discriminator is the `TargetFilter::ScopedPlayer` recipient on BOTH halves +/// of the pair, never the identity of the player set. Whichever players the +/// filter selects, each one moves their OWN hand (CR 115.10 — the iteration +/// player is bound as the effect resolves) and must therefore shuffle their OWN +/// library as part of that same instruction (CR 701.24a). A filter that selects +/// no player at all (CR 104.5 `HasLostTheGame`, CR 506.2 + CR 508.6 +/// `OpponentOfTriggeringPlayerNotAttacked` — both count-position-only, matching +/// no live recipient) runs the pair zero times, which is also correct; detaching +/// there would instead run one unbound shuffle after an empty loop. +/// +/// Adding a variant to a `false` group must therefore be a deliberate decision +/// backed by a rule, not the accident this predicate used to make. +fn scope_keeps_scoped_whole_hand_shuffle_local(scope: &PlayerFilter) -> bool { + match scope { + // Plain relations and whole-table scopes. + PlayerFilter::Controller + | PlayerFilter::Opponent + | PlayerFilter::All + // Event-context and trigger anchors. + | PlayerFilter::DefendingPlayer + | PlayerFilter::TriggeringPlayer + | PlayerFilter::OpponentOtherThanTriggering + | PlayerFilter::OpponentOfTriggeringPlayer + | PlayerFilter::OpponentOfTriggeringPlayerNotAttacked + | PlayerFilter::ParentObjectTargetController + | PlayerFilter::ParentObjectTargetOwner + | PlayerFilter::ChosenPlayer { .. } + // Turn/combat ledgers. + | PlayerFilter::OpponentLostLife + | PlayerFilter::OpponentGainedLife + | PlayerFilter::HasLostTheGame + | PlayerFilter::OpponentDealtDamage { .. } + | PlayerFilter::OpponentAttacked { .. } + | PlayerFilter::OpponentAttackingEnchantedPlayer + | PlayerFilter::HighestSpeed + // Resolution-local ledgers ("… this way") and linked-exile piles. + | PlayerFilter::ZoneChangedThisWay + | PlayerFilter::PerformedActionThisWay { .. } + | PlayerFilter::TrackedSetPossessor { .. } + | PlayerFilter::VotedFor { .. } + | PlayerFilter::OwnersOfCardsExiledBySource + // Per-candidate board / scalar comparisons. + | PlayerFilter::ControlsCount { .. } + | PlayerFilter::PlayerAttribute { .. } => true, + // The negation wrapper inherits its inner filter's decision, matching + // `player_filter_references_tracked_set`'s treatment of the same variant. + // Behaviour-identical while every arm above is `true`; recursing keeps it + // from silently disagreeing with its anchor the moment one is not. + PlayerFilter::AllExcept { exclude } => scope_keeps_scoped_whole_hand_shuffle_local(exclude), + } } /// CR 109.5 + CR 115.10 + CR 119.3: Detect that an effect's recipient is bound @@ -6867,24 +6933,168 @@ fn extract_event_context_filter(effect: &Effect) -> Option<&TargetFilter> { } } +/// CR 608.2c: What a chain step reports into `state.last_effect_amount` for a +/// following "that many" / "that much" clause. +/// +/// SINGLE AUTHORITY. Both the summing rule and the zero-result rule are derived +/// from ONE match over `Effect` (`amount_channel`), so a new arm cannot join one +/// and miss the other. The predecessor shipped a `_ => 0` summing arm paired to +/// a separate `_ => false` zero-policy arm — a twin registry, and adding a +/// summing arm without the matching zero arm would have silently restored #6956 +/// with no compile error. That is the same hand-maintained-registry mechanism +/// #6957 exists to remove, so it is not reintroduced here. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct AmountChannel { + events: AmountEvents, + zero: ZeroSemantics, +} + +/// Which event channel an effect's scalar result is summed from. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AmountEvents { + /// CR 120.1: `DamageDealt.amount`, every recipient. + DamageDealt, + /// CR 120.10: `DamageDealt.excess`, scoped to the fought creature. + FightExcessOnFoughtCreature, + /// CR 119.3: the negative side of `LifeChanged`. + LifeLost, + /// CR 119.3: the positive side of `LifeChanged`. + LifeGained, + /// CR 122.1: `CounterRemoved.count`. + CountersRemoved, +} + +/// What a ZERO total on that channel means for the effect that produced it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ZeroSemantics { + /// The effect owns this channel outright, so a zero total is a REAL zero + /// result once the instruction completed and must overwrite the slot. + MeasuredResult, + /// A zero total on this channel cannot be told apart from "this effect + /// produced nothing here", so it is pinned at the predecessor behaviour and + /// leaves the slot alone. Every use names why below. + Indistinguishable, +} + +/// CR 608.2c: The one classifier. `None` means the effect has no scalar channel +/// at all — the predecessor's `_ => 0` summing arm, which always filtered back +/// out to `None`, so this is exactly equivalent. +fn amount_channel(effect: &Effect) -> Option { + let (events, zero) = match effect { + // CR 120.8 + CR 614.7a: "If a source would deal 0 damage, it does not + // deal damage at all" — and CR 615.1 prevention likewise leaves no + // damage event behind. In both cases the damage DEALT this way is zero, + // which is what a following "that much" back-references. Reading an + // earlier step's amount instead is never correct. + Effect::DealDamage { .. } | Effect::DamageAll { .. } | Effect::DamageEachPlayer { .. } => { + (AmountEvents::DamageDealt, ZeroSemantics::MeasuredResult) + } + // NOT CLOSED — deliberately pinned at the predecessor behaviour (#6956). + // + // CR 120.10: unlike every other arm, `Fight` sums the EXCESS channel into + // the TOTAL slot, and scopes that sum to the fought creature resolved by + // `fight::resolve_fight_fighters`. That gives it a genuine THIRD state: + // a zero for want of a resolvable subject is neither "zero excess was + // dealt" nor "no fight ran". Settling it needs its own CR determination. + // + // Nothing on the current card pool can observe the difference: the only + // card chaining a `PreviousEffectAmount` consumer off a `Fight` is The + // Last Agni Kai, whose rider is gated on `PreviousEffectAmount { GT 0, + // channel: Excess }` — the EXCESS slot, written unconditionally by + // `previous_effect_excess_amount_from_events` and read via + // `.unwrap_or(0)`. The rider never runs in the zero case, so no + // production-path test can discriminate this arm today. + Effect::Fight { .. } => ( + AmountEvents::FightExcessOnFoughtCreature, + ZeroSemantics::Indistinguishable, + ), + // CR 119.3: "If an effect causes a player to gain life or lose life, + // that player's life total is adjusted accordingly." An effect that + // caused a loss of zero produced a zero. Note the CR has NO life-loss + // analogue of CR 119.9 — the "losing 0 life is not a life-loss event" + // premise cited in #6956 (as CR 118.2, which is the mana-payment rule) + // does not appear anywhere in the rules text. + Effect::LoseLife { .. } => (AmountEvents::LifeLost, ZeroSemantics::MeasuredResult), + // NOT CLOSED — deliberately pinned at the predecessor behaviour (#6956). + // + // CR 118.1: `PayCost`'s `cost` is an arbitrary `AbilityCost`, and only a + // life payment emits the negative `LifeChanged` events this channel sums; + // a mana / sacrifice / discard payment owns no amount here at all. + // CR 119.4b makes paying 0 life a real, always-legal payment whose result + // is zero, so a life payment SHOULD report it — but `pay::resolve` pushes + // no events whatsoever, so it emits no `EffectResolved` terminal marker + // and the completion half of the zero rule cannot be established for it. + // Closing this arm means first giving cost payment a terminal marker, + // which is its own change with its own blast radius (the per-player + // counts helper, the trigger matchers and the game log all read that + // event class). + Effect::PayCost { .. } => (AmountEvents::LifeLost, ZeroSemantics::Indistinguishable), + // CR 119.3 + CR 119.9: gaining zero life is not a life-GAIN EVENT, so + // "whenever you gain life" correctly does not trigger — but CR 119.9 + // governs triggering and replacement, not the arithmetic back-reference. + // The amount gained is still zero, and that is what "that much" reads. + // `life::resolve` stamps its terminal marker on the `final_amount <= 0` + // and CR 119.7 "can't gain life" paths too, so both reach the zero rule. + Effect::GainLife { .. } => (AmountEvents::LifeGained, ZeroSemantics::MeasuredResult), + // CR 122.1: a counter is a marker placed ON an object; removing counters + // from an object that has none removes zero counters, so "for each + // counter removed this way" is zero. + Effect::RemoveCounter { .. } => { + (AmountEvents::CountersRemoved, ZeroSemantics::MeasuredResult) + } + _ => return None, + }; + Some(AmountChannel { events, zero }) +} + fn previous_effect_amount_from_events( state: &GameState, ability: &ResolvedAbility, events: &[GameEvent], ) -> Option { - let amount = match &ability.effect { - Effect::DealDamage { .. } | Effect::DamageAll { .. } | Effect::DamageEachPlayer { .. } => { - events - .iter() - .filter_map(|event| match event { - GameEvent::DamageDealt { amount, .. } => { - Some(crate::game::arithmetic::u32_to_i32_saturating(*amount)) - } - _ => None, - }) - .sum() - } - Effect::Fight { .. } => { + // CR 706.2 + CR 706.4 + CR 608.2c: `roll_die::resolve` is the single + // authority for the scalar value a follow-up `PreviousEffectAmount` or + // `EventContextAmount` consumer reads. That avoids re-deriving from an + // event slice that may contain result-table branch effects or nested + // rolls interleaved with the outer dice. + if matches!(ability.effect, Effect::RollDie { .. }) { + return state.die_result_this_resolution; + } + // CR 121.2 + CR 121.2a + CR 608.2c: `draw::resume_draw_sequence` is the + // single authority for how many cards a draw instruction delivered — it + // commits the whole instruction's post-replacement total to + // `state.last_effect_count` once the sequence completes (a unit replaced + // by something else contributes 0; one doubled by a count modifier + // contributes its post-replacement count). Read that committed total + // instead of re-summing draw events, exactly as the `RollDie` arm above + // defers to `die_result_this_resolution`, so "draw N cards, then discard + // that many" (Varina, Lich Queen; Hordewing Skaab; Horrid Shadowspinner; + // Laquatus's Creativity; Last Stand) reads the true total rather than a + // per-unit or pre-replacement count. The same stamp feeds the condition + // peer `AbilityCondition::PreviousEffectAmount` — Transcendent Archaic's + // "if you draw one or more cards this way, discard two cards". + // + // Returns before the zero policy below: a draw that delivered zero cards is + // a real zero result and must stamp `Some(0)`. "Draw a card for each Island + // you control, then discard that many cards" (Last Stand) controlling no + // Islands has to discard 0, not inherit the life-gain amount its preceding + // chain step left behind in `last_effect_amount`. + if matches!(ability.effect, Effect::Draw { .. }) { + return state.last_effect_count; + } + + let channel = amount_channel(&ability.effect)?; + let amount: i32 = match channel.events { + AmountEvents::DamageDealt => events + .iter() + .filter_map(|event| match event { + GameEvent::DamageDealt { amount, .. } => { + Some(crate::game::arithmetic::u32_to_i32_saturating(*amount)) + } + _ => None, + }) + .sum(), + AmountEvents::FightExcessOnFoughtCreature => { // CR 120.10 + CR 701.14a: "add that much {R}" (The Last Agni Kai) // reads the excess dealt to the *fought* creature — the fight's // damage recipient, which in a dual-target fight is the second @@ -6912,21 +7122,21 @@ fn previous_effect_amount_from_events( }) .sum() } - Effect::LoseLife { .. } | Effect::PayCost { .. } => events + AmountEvents::LifeLost => events .iter() .filter_map(|event| match event { GameEvent::LifeChanged { amount, .. } if *amount < 0 => Some(-*amount), _ => None, }) .sum(), - Effect::GainLife { .. } => events + AmountEvents::LifeGained => events .iter() .filter_map(|event| match event { GameEvent::LifeChanged { amount, .. } if *amount > 0 => Some(*amount), _ => None, }) .sum(), - Effect::RemoveCounter { .. } => events + AmountEvents::CountersRemoved => events .iter() .filter_map(|event| match event { GameEvent::CounterRemoved { count, .. } => { @@ -6935,37 +7145,91 @@ fn previous_effect_amount_from_events( _ => None, }) .sum(), - // CR 706.2 + CR 706.4 + CR 608.2c: `roll_die::resolve` is the single - // authority for the scalar value a follow-up `PreviousEffectAmount` or - // `EventContextAmount` consumer reads. That avoids re-deriving from an - // event slice that may contain result-table branch effects or nested - // rolls interleaved with the outer dice. - Effect::RollDie { .. } => return state.die_result_this_resolution, - // CR 121.2 + CR 121.2a + CR 608.2c: `draw::resume_draw_sequence` is the - // single authority for how many cards a draw instruction delivered — it - // commits the whole instruction's post-replacement total to - // `state.last_effect_count` once the sequence completes (a unit replaced - // by something else contributes 0; one doubled by a count modifier - // contributes its post-replacement count). Read that committed total - // instead of re-summing draw events, exactly as the `RollDie` arm above - // defers to `die_result_this_resolution`, so "draw N cards, then discard - // that many" (Varina, Lich Queen; Hordewing Skaab; Horrid Shadowspinner; - // Laquatus's Creativity; Last Stand) reads the true total rather than a - // per-unit or pre-replacement count. The same stamp feeds the condition - // peer `AbilityCondition::PreviousEffectAmount` — Transcendent Archaic's - // "if you draw one or more cards this way, discard two cards". - // - // Returns early rather than falling through the `> 0` filter below: a - // draw that delivered zero cards is a real zero result and must stamp - // `Some(0)`. "Draw a card for each Island you control, then discard that - // many cards" (Last Stand) controlling no Islands has to discard 0, not - // inherit the life-gain amount its preceding chain step left behind in - // `last_effect_amount`. - Effect::Draw { .. } => return state.last_effect_count, - _ => 0, }; - (amount > 0).then_some(amount) + // CR 608.2c (#6956): the predecessor tail was `(amount > 0).then_some(amount)`, + // which collapsed "this instruction produced ZERO" into "this effect has no + // result on this channel". Both call sites assign only on `Some`, so that + // collapse did not leave the slot empty — it left the PREVIOUS chain step's + // amount standing, and the next "that many" / "that much" clause read that + // instead. `state.last_effect_amount` is ONE slot shared by every step of a + // chain (reset to `None` only at depth 0), so a real zero must overwrite. + // + // The opposite collapse is equally wrong and fails more quietly: stamping + // `Some(0)` unconditionally would let an effect that produced nothing on this + // channel silently zero a legitimately standing earlier value. So the zero + // case is decided per effect, not by the total. + if amount != 0 { + return Some(amount); + } + let zero_is_this_effects_result = channel.zero == ZeroSemantics::MeasuredResult + // CR 608.2c: "…X…, …X…, and …X…, where X is " names ONE + // value that every clause reads. The engine approximates that with a + // single mutable slot plus chain-relative reads, so a clause whose OWN + // quantity is that shared X is a RELAY, not a fresh anchor: letting its + // outcome redefine the slot destroys the X its later siblings still need. + // + // Thorna and Twigtooth is the live case — "remove all counters from + // target creature you control. Each opponent loses X life, you gain X + // life, … where X is the number of counters removed this way" lowers to + // `RemoveCounter -> LoseLife{PreviousEffectAmount} -> GainLife{PreviousEffectAmount}`. + // With an opponent under CR 119.8 "can't lose life" the middle clause + // totals zero, and claiming that zero makes the gain clause read 0 + // instead of the counters removed. + // + // Scoped to the ZERO case on purpose: this is exactly a restoration of + // the predecessor behaviour for relays, so it is provably no wider than + // the #6956 change itself. It does NOT fix the nonzero chain-relative + // divergence (a two-opponent Thorna still stamps the doubled life loss) + // — that is a pre-existing anchored-quantity gap, tracked separately. + && !effect_relays_the_shared_amount(&ability.effect) + && effect_instruction_completed(ability, events); + zero_is_this_effects_result.then_some(0) +} + +/// CR 608.2c: Is this effect's OWN quantity the shared "X" of a co-anchored +/// clause group, rather than a fresh value of its own? See the relay note in +/// `previous_effect_amount_from_events`. +fn effect_relays_the_shared_amount(effect: &Effect) -> bool { + let mut relays = false; + effect.for_each_quantity_expr(&mut |quantity| { + relays |= quantity_expr_any_ref(quantity, &mut |qty| { + matches!( + qty, + QuantityRef::EventContextAmount | QuantityRef::PreviousEffectAmount { .. } + ) + }); + }); + relays +} + +/// CR 608.2c: Did THIS instruction complete within the supplied event window? +/// +/// `EffectResolved { kind, source_id }` is pushed by a resolver as its last act +/// and skipped when the resolver returns early to suspend for a player choice, +/// so its presence is the authority for "the instruction finished". Matched on +/// both the effect kind and the source so a same-window marker from another +/// effect (a chained sub-ability, another permanent's ability) cannot stand in. +/// +/// `previous_effect_counts_by_player_from_events` reads the same marker class, +/// but not identically: it takes the LAST marker's index and then bounds its +/// tally to `events[..=index]`, because events after the marker belong to later +/// work. This predicate only asks whether the marker is present at all — the +/// scalar sums above are not window-bounded, so there is nothing to bound. The +/// two agree on every event slice the call sites produce today; tightening the +/// scalar sums to the same window is a separate change. +fn effect_instruction_completed(ability: &ResolvedAbility, events: &[GameEvent]) -> bool { + let kind = EffectKind::from(&ability.effect); + events.iter().any(|event| { + matches!( + event, + GameEvent::EffectResolved { + kind: event_kind, + source_id: event_source, + .. + } if *event_kind == kind && *event_source == ability.source_id + ) + }) } /// CR 120.10: Resolution-local excess-damage twin of @@ -7135,30 +7399,39 @@ fn effect_consumes_event_context_amount(effect: &Effect) -> bool { consumes } -fn quantity_expr_references_event_context_amount(quantity: &QuantityExpr) -> bool { +/// Walks every `QuantityRef` reachable through `quantity`'s composition forms +/// and reports whether any satisfies `pred`. Single traversal authority for the +/// resolution-local back-reference predicates, so a new `QuantityExpr` +/// composition form is threaded in exactly one place instead of once per +/// predicate. +fn quantity_expr_any_ref( + quantity: &QuantityExpr, + pred: &mut dyn FnMut(&QuantityRef) -> bool, +) -> bool { match quantity { - QuantityExpr::Ref { qty } => matches!(qty, QuantityRef::EventContextAmount), + QuantityExpr::Ref { qty } => pred(qty), QuantityExpr::Offset { inner, .. } | QuantityExpr::ClampMin { inner, .. } | QuantityExpr::Multiply { inner, .. } - | QuantityExpr::DivideRounded { inner, .. } => { - quantity_expr_references_event_context_amount(inner) - } - QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => exprs - .iter() - .any(quantity_expr_references_event_context_amount), - QuantityExpr::UpTo { max } => quantity_expr_references_event_context_amount(max), - QuantityExpr::Power { exponent, .. } => { - quantity_expr_references_event_context_amount(exponent) + | QuantityExpr::DivideRounded { inner, .. } => quantity_expr_any_ref(inner, pred), + QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => { + exprs.iter().any(|expr| quantity_expr_any_ref(expr, pred)) } + QuantityExpr::UpTo { max } => quantity_expr_any_ref(max, pred), + QuantityExpr::Power { exponent, .. } => quantity_expr_any_ref(exponent, pred), QuantityExpr::Difference { left, right } => { - quantity_expr_references_event_context_amount(left) - || quantity_expr_references_event_context_amount(right) + quantity_expr_any_ref(left, pred) || quantity_expr_any_ref(right, pred) } QuantityExpr::Fixed { .. } => false, } } +fn quantity_expr_references_event_context_amount(quantity: &QuantityExpr) -> bool { + quantity_expr_any_ref(quantity, &mut |qty| { + matches!(qty, QuantityRef::EventContextAmount) + }) +} + fn mark_exile_choice_tracks_by_source(state: &mut GameState, source: ObjectId) { if let WaitingFor::EffectZoneChoice { source_id, @@ -12222,7 +12495,7 @@ mod tests { AbilityCondition, AbilityDefinition, AbilityKind, AggregateFunction, BounceSelection, CardPredicateChoice, CastingPermission, ChoiceType, ChoiceValue, Chooser, ChosenAttribute, ChosenCounterCountCondition, Comparator, ContinuousModification, ControllerRef, - DelayedTriggerCondition, Duration, EffectKind, EffectScope, FilterProp, + DamageChannel, DelayedTriggerCondition, Duration, EffectKind, EffectScope, FilterProp, ManaSpendPermission, ObjectProperty, PermissionGrantee, PlayerFilter, PlayerScope, PtValue, QuantityExpr, QuantityRef, SpellContext, StaticDefinition, TapStateChange, TargetFilter, TargetRef, TargetSelectionMode, TypeFilter, TypedFilter, UnlessPayModifier, UntilCondition, @@ -27730,14 +28003,31 @@ mod tests { &PlayerFilter::All, )); - assert!( - !is_player_scope_local_continuation( - &scoped_move, - &scoped_shuffle, - &PlayerFilter::Opponent, - ), - "a non-All player scope must not enter the all-player hand shuffle continuation" - ); + // CR 701.24a + #6957: the `ScopedPlayer` recipients on BOTH halves are + // the discriminator, not the identity of the player set. A SCOPE-position + // filter other than `All` must reach the SAME decision — the predecessor + // `matches!(scope, PlayerFilter::All)` silently detached these, running + // one unbound shuffle after the loop instead of one shuffle per player. + for scope in [ + PlayerFilter::Opponent, + PlayerFilter::AllExcept { + exclude: Box::new(PlayerFilter::Controller), + }, + PlayerFilter::TrackedSetPossessor { + relation: crate::types::ability::PlayerRelation::Opponent, + possession: crate::types::ability::PossessionAxis::Controller, + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + ..Default::default() + }), + caused_by: None, + }, + ] { + assert!( + is_player_scope_local_continuation(&scoped_move, &scoped_shuffle, &scope), + "{scope:?} in SCOPE position must keep the scoped shuffle inside its iteration" + ); + } let non_hand_move = zone_to_library_effect(Zone::Graveyard, TargetFilter::ScopedPlayer); assert!( @@ -27894,4 +28184,623 @@ mod tests { "the draw tail must start only after every scoped move/shuffle pass" ); } + + /// CR 608.2c + CR 701.24a + CR 115.10 (#6957): a NON-`All` SCOPE-position + /// `PlayerFilter` keeps the scoped whole-hand shuffle inside each iteration. + /// + /// Deliberately run with TWO matching opponents. With a single iteration the + /// aggregate "a shuffle happened" is indistinguishable between "kept in + /// scope" and "detached and run once"; with two, the detached form can only + /// produce ONE shuffle, after BOTH hand moves, so both the shuffle count and + /// the move/shuffle interleaving flip when the fix is reverted. + #[test] + fn opponent_scoped_hand_shuffle_stays_inside_each_iteration() { + let mut state = GameState::new(FormatConfig::standard(), 3, 42); + let source = ObjectId(901); + let controller = PlayerId(0); + let opponents = [PlayerId(1), PlayerId(2)]; + for (player, hand_count) in [(PlayerId(0), 4), (PlayerId(1), 3), (PlayerId(2), 5)] { + for card in 0..hand_count { + create_object( + &mut state, + CardId(4_000 + u64::from(player.0) * 100 + card as u64), + player, + format!("P{} hand {card}", player.0), + Zone::Hand, + ); + } + for card in 0..10 { + create_object( + &mut state, + CardId(5_000 + u64::from(player.0) * 100 + card as u64), + player, + format!("P{} library {card}", player.0), + Zone::Library, + ); + } + } + + let mut move_hand = ResolvedAbility::new( + hand_to_library_effect(TargetFilter::ScopedPlayer), + vec![], + source, + controller, + ); + move_hand.player_scope = Some(PlayerFilter::Opponent); + let mut shuffle = ResolvedAbility::new( + Effect::Shuffle { + target: TargetFilter::ScopedPlayer, + }, + vec![], + source, + controller, + ); + let mut draw = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Ref { + qty: QuantityRef::EventContextAmount, + }, + target: TargetFilter::ScopedPlayer, + }, + vec![], + source, + controller, + ); + draw.player_scope = Some(PlayerFilter::Opponent); + shuffle.sub_ability = Some(Box::new(draw)); + move_hand.sub_ability = Some(Box::new(shuffle)); + + let mut events = Vec::new(); + resolve_ability_chain(&mut state, &move_hand, &mut events, 0).unwrap(); + + // `Effect::Shuffle` stamps exactly one `EffectResolved { Shuffle }` per + // invocation, so this counts INSTRUCTION runs — unlike the raw + // `ShuffledLibrary` action, which a whole-hand library insertion also + // emits per card. + let shuffle_instruction_indices: Vec = events + .iter() + .enumerate() + .filter_map(|(index, event)| match event { + GameEvent::EffectResolved { + kind: EffectKind::Shuffle, + .. + } => Some(index), + _ => None, + }) + .collect(); + // Discriminator: the shuffle instruction runs ONCE PER SCOPED OPPONENT. + // The detached form runs the tail exactly once, so this count drops to 1. + assert_eq!( + shuffle_instruction_indices.len(), + 2, + "each of the two scoped opponents must run the shuffle instruction once" + ); + for opponent in opponents { + assert!( + events.iter().any(|event| matches!( + event, + GameEvent::PlayerPerformedAction { + player_id, + action: PlayerActionKind::ShuffledLibrary, + .. + } if *player_id == opponent + )), + "P{} must shuffle their own library", + opponent.0 + ); + } + assert!( + !events.iter().any(|event| matches!( + event, + GameEvent::PlayerPerformedAction { + player_id, + action: PlayerActionKind::ShuffledLibrary, + .. + } if *player_id == controller + )), + "the controller is outside an Opponent scope and must not shuffle" + ); + + // Interleaving discriminator: the FIRST shuffle must land before the LAST + // opponent's hand move. A detached tail runs after every move, so this + // inequality inverts when the scope gate is narrowed back to `All`. + let hand_move_indices: Vec = events + .iter() + .enumerate() + .filter_map(|(index, event)| match event { + GameEvent::ZoneChanged { record, .. } + if record.from_zone == Some(Zone::Hand) + && record.to_zone == Zone::Library + && opponents.contains(&record.owner) => + { + Some(index) + } + _ => None, + }) + .collect(); + assert_eq!( + hand_move_indices.len(), + 8, + "both opponents' whole hands must move (3 + 5 cards)" + ); + assert!( + shuffle_instruction_indices[0] + < *hand_move_indices.last().expect("hand moves occurred"), + "the first opponent's shuffle must precede the second opponent's hand move" + ); + + for (opponent, expected_draws) in opponents.into_iter().zip([3, 5]) { + assert_eq!( + state.players[opponent.0 as usize].cards_drawn_this_turn, expected_draws, + "P{} must draw exactly the number of cards they moved", + opponent.0 + ); + } + } + + // --------------------------------------------------------------------- + // #6956 — a chain step that genuinely produced ZERO must overwrite + // `last_effect_amount`, not leave the previous step's value standing for + // the next "that many" / "that much" clause. + // --------------------------------------------------------------------- + + /// Builds the three-step chain every #6956 test below drives through the + /// real resolution pipeline: + /// + /// 1. `GainLife 7` — a nonzero step that stamps `last_effect_amount = 7`. + /// This is the value the bug leaks. + /// 2. `under_test` — the effect whose zero (or absence) is being probed. + /// 3. `Draw { PreviousEffectAmount { Total } }` — the "that many" consumer. + /// + /// Returns the number of cards the controller drew, i.e. exactly what the + /// following clause read out of the shared slot. + fn previous_effect_amount_seen_by_next_clause( + state: &mut GameState, + source: ObjectId, + under_test: Effect, + probe_targets: Vec, + ) -> u32 { + let controller = PlayerId(0); + let mut gain = ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 7 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + let mut probe = ResolvedAbility::new(under_test, probe_targets, source, controller); + let consumer = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + }, + }, + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + probe.sub_ability = Some(Box::new(consumer)); + gain.sub_ability = Some(Box::new(probe)); + + resolve_ability_chain(state, &gain, &mut Vec::new(), 0).unwrap(); + state.players[controller.0 as usize].cards_drawn_this_turn + } + + fn state_for_previous_amount_probe() -> (GameState, ObjectId) { + let mut state = GameState::new(FormatConfig::standard(), 2, 42); + for card in 0..40u64 { + create_object( + &mut state, + CardId(6_000 + card), + PlayerId(0), + format!("P0 library {card}"), + Zone::Library, + ); + } + let source = create_object( + &mut state, + CardId(6_900), + PlayerId(0), + "Probe Source".to_string(), + Zone::Battlefield, + ); + (state, source) + } + + /// CR 120.8 + CR 614.7a: "If a source would deal 0 damage, it does not deal + /// damage at all." The damage DEALT this way is zero, so the following + /// "that much" clause must read zero — not the 7 the preceding `GainLife` + /// left in the shared slot. + #[test] + fn zero_damage_overwrites_the_previous_chain_steps_amount() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 0 }, + target: TargetFilter::Player, + damage_source: None, + excess: None, + }, + vec![TargetRef::Player(PlayerId(1))], + ); + assert_eq!( + drawn, 0, + "0 damage dealt must be read as 0, not as the preceding GainLife's 7" + ); + } + + /// Reach guard for the test above: the SAME chain with nonzero damage must + /// read the damage amount, proving the consumer is wired to this producer + /// at all and that the zero assertion is not vacuous. + #[test] + fn nonzero_damage_is_still_read_by_the_next_clause() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Player, + damage_source: None, + excess: None, + }, + vec![TargetRef::Player(PlayerId(1))], + ); + assert_eq!(drawn, 3, "the consumer must read this step's own amount"); + } + + /// CR 119.3: an effect that caused a player to lose zero life produced a + /// zero, and "that much" reads zero. + #[test] + fn zero_life_loss_overwrites_the_previous_chain_steps_amount() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::LoseLife { + amount: QuantityExpr::Fixed { value: 0 }, + target: Some(TargetFilter::Controller), + }, + vec![], + ); + assert_eq!( + drawn, 0, + "0 life lost must be read as 0, not as the preceding GainLife's 7" + ); + } + + /// CR 119.3 + CR 119.9: gaining zero life is not a life-GAIN EVENT (so + /// "whenever you gain life" does not trigger), but the amount gained is + /// still zero and that is what the back-reference reads. + #[test] + fn zero_life_gain_overwrites_the_previous_chain_steps_amount() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 0 }, + player: TargetFilter::Controller, + }, + vec![], + ); + assert_eq!( + drawn, 0, + "0 life gained must be read as 0, not as the preceding GainLife's 7" + ); + } + + /// CR 122.1: removing counters from an object that has none removes zero + /// counters. This is #6956's headline shape — "remove all −1/−1 counters + /// from it, then discard a card for each counter removed this way" behind a + /// nonzero life-gain step. + #[test] + fn zero_counters_removed_overwrites_the_previous_chain_steps_amount() { + let (mut state, source) = state_for_previous_amount_probe(); + // The source carries NO counters, so the removal removes zero. + assert!( + state.objects[&source].counters.is_empty(), + "reach guard: the probe object must start with no counters" + ); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::RemoveCounter { + counter_type: Some(CounterType::Minus1Minus1), + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::SelfRef, + }, + vec![], + ); + assert_eq!( + drawn, 0, + "0 counters removed must be read as 0, not as the preceding GainLife's 7" + ); + } + + /// Reach guard for the counter test: the same chain with counters actually + /// present must read the removed count. + #[test] + fn nonzero_counters_removed_is_still_read_by_the_next_clause() { + let (mut state, source) = state_for_previous_amount_probe(); + state + .objects + .get_mut(&source) + .expect("probe source exists") + .counters + .insert(CounterType::Minus1Minus1, 2); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::RemoveCounter { + counter_type: Some(CounterType::Minus1Minus1), + count: QuantityExpr::Fixed { value: 2 }, + target: TargetFilter::SelfRef, + }, + vec![], + ); + assert_eq!(drawn, 2, "the consumer must read this step's own amount"); + } + + /// CR 118.1: `Effect::PayCost` carries an arbitrary `AbilityCost`, so a MANA + /// payment emits no `LifeChanged` event and owns no amount on the life + /// channel at all. Claiming a zero for it would overwrite a legitimately + /// standing earlier value — the mirror bug #6956 warns about. + /// + /// This pins the RULE, not a card. An earlier draft of this comment claimed + /// the Extort cycle as the live case; that is wrong and the census says so: + /// in `PayCost -> LoseLife -> GainLife{PreviousEffectAmount}` the consumer's + /// nearest stamping producer is `LoseLife`, which overwrites any `PayCost` + /// value before `GainLife` reads it (the stamp is written per node before + /// `sub_ability` recurses). ZERO cards on the pool have a `PayCost` as the + /// direct parent of such a consumer. The guard rests on CR 118.1 alone. + #[test] + fn mana_pay_cost_does_not_zero_the_previous_chain_steps_amount() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::PayCost { + cost: AbilityCost::Mana { + cost: ManaCost::default(), + }, + scale: None, + payer: TargetFilter::Controller, + }, + vec![], + ); + assert_eq!( + drawn, 7, + "a mana payment owns no life-loss amount and must leave the preceding step's value" + ); + } + + /// NOT CLOSED (#6956): CR 118.1 + CR 119.4b say paying 0 life is a real, + /// always-legal payment whose result is zero, so this SHOULD read 0 — but + /// `pay::resolve` emits no `EffectResolved` terminal marker, so the + /// completion half of the zero rule cannot be established for + /// `Effect::PayCost`. Pinned at the PREDECESSOR behaviour + /// (the preceding step's 7 survives) so the arm's status is a visible, + /// asserted fact rather than an untested assumption. Flipping this + /// expectation to 0 is the acceptance test for the follow-up. + #[test] + fn zero_life_pay_cost_is_not_yet_distinguished_from_an_absent_producer() { + let (mut state, source) = state_for_previous_amount_probe(); + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::PayCost { + cost: AbilityCost::PayLife { + amount: QuantityExpr::Fixed { value: 0 }, + }, + scale: None, + payer: TargetFilter::Controller, + }, + vec![], + ); + assert_eq!( + drawn, 7, + "PayCost has no terminal marker, so its zero cannot yet be told apart \ + from an absent producer; see `amount_channel`'s PayCost arm" + ); + } + + // --------------------------------------------------------------------- + // #6957 — CR 107.3i: every instance of a co-anchored X reads the SAME + // value. A consumer that is not the direct successor of the anchor must + // still read the anchor, not whatever the intervening RELAY clause + // happened to produce. + // --------------------------------------------------------------------- + + /// Three players so the `player_scope: Opponent` relay fans out twice — + /// that fan-out is what makes the relay's own total (life lost across ALL + /// opponents) differ from the shared X it was reading. + fn state_for_shared_x_probe() -> (GameState, ObjectId) { + let mut state = GameState::new(FormatConfig::standard(), 3, 42); + for card in 0..40u64 { + create_object( + &mut state, + CardId(7_000 + card), + PlayerId(0), + format!("P0 library {card}"), + Zone::Library, + ); + } + let source = create_object( + &mut state, + CardId(7_900), + PlayerId(0), + "Shared X Source".to_string(), + Zone::Battlefield, + ); + (state, source) + } + + /// NOT CLOSED (#6957): CR 107.3c + CR 107.3i — Thorna and Twigtooth, + /// "remove all counters from target creature you control. Each opponent + /// loses X life, you gain X life, … where X is the number of counters + /// removed this way." + /// + /// X is anchored to the `RemoveCounter` clause and read by TWO later + /// clauses. Both lower to `PreviousEffectAmount`, which reads the + /// *immediately preceding* chain step, so the second consumer reads the + /// intervening relay's own total instead of X. With two opponents that + /// total is the SUMMED life loss (2 × X), so the second consumer reads + /// 4 where CR 107.3i requires 2 — "normally, all instances of X on an + /// object have the same value at any given time." + /// + /// Pinned at the CURRENT (wrong) value so the gap is a visible asserted + /// fact rather than an untested assumption, exactly as + /// `zero_life_pay_cost_is_not_yet_distinguished_from_an_absent_producer` + /// pins its own open arm. **Flipping this expectation from 4 to 2 is the + /// acceptance test for the fix.** The fix cannot be made at this stamp + /// site — see + /// `a_relay_that_re_anchors_x_must_keep_publishing_its_own_result` for the + /// counter-shape that makes a stamp-time policy undecidable; it belongs in + /// the parser, binding one X into the single `chosen_x` channel the way + /// `ability_utils::publish_announced_x` already does for CR 601.2b + /// announce-time binders. + /// + /// `Draw` stands in for the "you gain X life" clause so the observation is + /// a clean scalar (`cards_drawn_this_turn`); the defect is in what the + /// consumer READS, which is independent of which effect consumes it. + #[test] + fn second_consumer_of_a_shared_x_reads_the_relay_not_the_anchor() { + let (mut state, source) = state_for_shared_x_probe(); + let controller = PlayerId(0); + state + .objects + .get_mut(&source) + .expect("probe source exists") + .counters + .insert(CounterType::Minus1Minus1, 2); + + // Clause 1 (anchor): remove all counters — X := 2. + let mut anchor = ResolvedAbility::new( + Effect::RemoveCounter { + counter_type: Some(CounterType::Minus1Minus1), + count: QuantityExpr::Fixed { value: -1 }, + target: TargetFilter::SelfRef, + }, + vec![], + source, + controller, + ); + // Clause 2 (relay): each opponent loses X life. Two opponents, so the + // instruction's own total is 4 while the X it read is 2. + let mut relay = ResolvedAbility::new( + Effect::LoseLife { + amount: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + }, + }, + target: None, + }, + vec![], + source, + controller, + ); + relay.player_scope = Some(PlayerFilter::Opponent); + // Clause 3 (second consumer of the SAME X). + let consumer = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + }, + }, + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + relay.sub_ability = Some(Box::new(consumer)); + anchor.sub_ability = Some(Box::new(relay)); + + resolve_ability_chain(&mut state, &anchor, &mut Vec::new(), 0).unwrap(); + + // Reach guard: the relay must actually have fanned out over both + // opponents, otherwise its total would coincide with X and the test + // would pass vacuously. + for opponent in [PlayerId(1), PlayerId(2)] { + assert_eq!( + state.players[opponent.0 as usize].life, 18, + "reach guard: P{} must have lost exactly X = 2 life", + opponent.0 + ); + } + assert_eq!( + state.players[controller.0 as usize].cards_drawn_this_turn, 4, + "pinned gap: the second consumer reads the relay's summed life loss \ + (2 opponents × X) instead of the anchor's X. CR 107.3i requires 2 \ + — flipping this to 2 is the acceptance test for the fix" + ); + } + + /// The counter-shape that makes a stamp-time fix for + /// `second_consumer_of_a_shared_x_reads_the_relay_not_the_anchor` + /// UNDECIDABLE, and therefore the regression guard any such attempt must + /// clear. + /// + /// Magma Pummeler — "prevent that damage and remove that many +1/+1 + /// counters from it. When one or more counters are removed from this + /// creature this way, it deals that much damage to any target." Here the + /// middle clause is a relay (its own count reads the prevented damage) AND + /// is itself the anchor for the third clause: "that much damage" is the + /// number of counters actually REMOVED, which is capped by how many + /// counters were on the object. CR 122.1. + /// + /// That is byte-identical in the AST to Thorna's co-anchored + /// `RemoveCounter -> relay -> consumer` shape, but demands the opposite + /// answer: here the consumer MUST read the relay's own result, there it + /// must reach past it. So "a relay never redefines the shared slot" — the + /// tempting generalization of `effect_relays_the_shared_amount` from the + /// zero case to every case — is wrong, and no runtime policy keyed on the + /// resolved AST can separate the two. The binding has to be recorded when + /// the parser lowers X, not recovered at resolution. + /// + /// The relay reads 7 but the object carries only 2 counters, so the two + /// candidate answers are far apart: 2 (correct, the relay's own result) + /// vs 7 (what declining to overwrite would leak). + #[test] + fn a_relay_that_re_anchors_x_must_keep_publishing_its_own_result() { + let (mut state, source) = state_for_previous_amount_probe(); + state + .objects + .get_mut(&source) + .expect("probe source exists") + .counters + .insert(CounterType::Plus1Plus1, 2); + // `previous_effect_amount_seen_by_next_clause` prefixes `GainLife 7`, + // so the relay's own quantity resolves to 7 while only 2 counters + // exist to remove. + let drawn = previous_effect_amount_seen_by_next_clause( + &mut state, + source, + Effect::RemoveCounter { + counter_type: Some(CounterType::Plus1Plus1), + count: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + }, + }, + target: TargetFilter::SelfRef, + }, + vec![], + ); + assert_eq!( + drawn, 2, + "CR 122.1: only 2 counters existed, so the re-anchoring consumer must \ + read the 2 actually removed — not the 7 the relay asked for" + ); + } } diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 8d4b162053..34a0a50aa3 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -15187,6 +15187,29 @@ mod stage2_injector_tests { // and the other two did NOT move, which located the insertion below them. // #6961 (2ead7aab1) + v0.44.0: `:5918/:5995/:8970 ⇒ :5996/:6073/:9048`, // uniform +78 above all three (whole-file delta +153/-15). + // #6957 (this branch, base 4f524c6014): `:5999/:6076/:9051 ⇒ + // `:6061/:6138/:9113`, uniform +62 above all three. The two hunks above + // them are `-3` (the `matches!(scope, PlayerFilter::All)` gate replaced by + // a named local) and `+65` (`scope_keeps_scoped_whole_hand_shuffle_local` + // and its doc comment); every other hunk in the file is at `:27733+`, i.e. + // BELOW all three. Identity re-established, not assumed: each producer is + // sha256-identical to `4f524c6014:effects/mod.rs` at its old coordinate. + // #6956 (same branch, second unit): `:9113 ⇒ :9243`, +130, and ONLY that + // entry moved — the other four stayed byte-identical AND in place, which is + // the set-preservation evidence. The +109 is the + // zero-policy pair inserted at `:7030` (+131, minus a 1-line comment + // reflow at `:7020`), i.e. below `:6061`/`:6138` and above `:9113`. The + // only other hunks are in `mod tests`. Producer sha256-identical at its + // new coordinate. + // #6956 fix round (review round 1): `:6061/:6138/:9243 ⇒ :6065/:6142/:9324`, + // +4 above the first two and +81 above the third. The +4 is the + // `AllExcept` recursion arm added to + // `scope_keeps_scoped_whole_hand_shuffle_local`; the further +77 is the + // `amount_channel` classifier that collapses the twin `_ => 0` / `_ => false` + // registry, plus the relay guard. All three producers sha256-identical at + // their new coordinates AND still inside the same enclosing functions + // (`drive_sequential_repeated_optional_payment` ×2, `resolve_chain_body`), + // which is stronger evidence than the coordinate alone. // // ⚠ THIS ROW FAILS IN CI BEFORE IT FAILS LOCALLY, and that is not a bug in the // row. CI checks out `refs/pull//merge` — this branch merged with CURRENT @@ -15199,9 +15222,9 @@ mod stage2_injector_tests { // because that is what makes a NEW mint a counted event; a function + // content-hash anchor would end the drift class while keeping that property, // and is offered as a follow-up rather than taken unannounced mid-review. - "game/effects/mod.rs:5999".to_string(), - "game/effects/mod.rs:6076".to_string(), - "game/effects/mod.rs:9051".to_string(), + "game/effects/mod.rs:6065".to_string(), + "game/effects/mod.rs:6142".to_string(), + "game/effects/mod.rs:9324".to_string(), // UNMOVED across the rebase, and that is itself evidence the SET did not // move: a census that had gained or lost a producer would not leave this // entry both byte-identical AND at the same coordinate. diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 8ab4448f94..6a8068f439 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -921,6 +921,7 @@ mod the_immortal_sun; mod the_kingpin_of_crime_combat_damage; mod the_ur_dragon_eminence; mod the_who_opponent_guess_resolution; +mod thorna_and_twigtooth_shared_x_relay_6956; mod thought_distortion; mod thoughtweft_trample_regression; mod throne_of_eldraine_mana_riders; diff --git a/crates/engine/tests/integration/thorna_and_twigtooth_shared_x_relay_6956.rs b/crates/engine/tests/integration/thorna_and_twigtooth_shared_x_relay_6956.rs new file mode 100644 index 0000000000..a71e720f35 --- /dev/null +++ b/crates/engine/tests/integration/thorna_and_twigtooth_shared_x_relay_6956.rs @@ -0,0 +1,196 @@ +//! Regression for the #6956 fix round: a co-anchored "where X is …" clause +//! group must not have its shared X redefined by a middle clause that produced +//! zero. +//! +//! Oracle (Thorna and Twigtooth, verbatim from card data): +//! "Thorna and Twigtooth enters with two -1/-1 counters on it. +//! Whenever Thorna and Twigtooth attacks, remove all counters from target +//! creature you control. Each opponent loses X life, you gain X life, and the +//! topmost creature card in your library perpetually gets +X/+X, where X is +//! the number of counters removed this way." +//! +//! The trigger lowers to a chain-relative +//! `RemoveCounter -> LoseLife{PreviousEffectAmount} -> GainLife{PreviousEffectAmount}` +//! (the perpetual +X/+X clause does not parse at all today), so every clause +//! reads whatever the immediately preceding step left in `last_effect_amount` +//! rather than the anchored X. +//! +//! #6956's first pass made a genuine zero overwrite that slot. That is right for +//! a fresh producer, but the middle `LoseLife` here is a RELAY — its own amount +//! IS the shared X. With an opponent under CR 119.8 "can't lose life" the relay +//! totals zero, and claiming that zero made the gain clause read 0 instead of +//! the counters removed. The card was accidentally correct before #6956 (via +//! exactly the leak #6956 closed) and would have been silently wrong after. +//! +//! CR references: +//! - CR 608.2c: the controller follows the instructions in the order written; +//! "…X…, …X…, and …X…, where X is " names ONE value that every +//! clause reads. +//! - CR 119.8: "If an effect says that a player can't lose life, …" — Platinum +//! Emperion's "Your life total can't change" blocks the middle clause. +//! - CR 122.1: a counter is a marker placed on an object; X is the number of +//! counters actually removed. + +use engine::game::combat::AttackTarget; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::actions::GameAction; +use engine::types::counter::CounterType; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::phase::Phase; + +const THORNA_ORACLE: &str = "Thorna and Twigtooth enters with two -1/-1 counters on it.\n\ +Whenever Thorna and Twigtooth attacks, remove all counters from target creature you control. \ +Each opponent loses X life, you gain X life, and the topmost creature card in your library \ +perpetually gets +X/+X, where X is the number of counters removed this way."; + +const PLATINUM_EMPERION_ORACLE: &str = "Your life total can't change."; + +/// Drive the attack trigger to resolution, answering trigger ordering and the +/// "target creature you control" selection with `target`. +fn resolve_attack_trigger_targeting(runner: &mut GameRunner, target: ObjectId) { + for _ in 0..80 { + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } => { + if runner.state().stack.is_empty() { + return; + } + runner.act(GameAction::PassPriority).expect("pass priority"); + } + WaitingFor::OrderTriggers { triggers, .. } => { + let count = triggers.len(); + runner + .act(GameAction::OrderTriggers { + order: (0..count).collect(), + }) + .expect("order triggers"); + } + WaitingFor::TriggerTargetSelection { .. } | WaitingFor::TargetSelection { .. } => { + runner + .act(GameAction::SelectTargets { + targets: vec![engine::types::ability::TargetRef::Object(target)], + }) + .expect("select the trigger's target creature"); + } + other => panic!("unexpected waiting state during the attack trigger: {other:?}"), + } + } + panic!("the attack trigger never resolved"); +} + +/// P0 attacks with Thorna; the trigger removes two -1/-1 counters from it. P1 +/// controls Platinum Emperion, so the middle "each opponent loses X life" clause +/// produces a ZERO total. +/// +/// Discriminating assertion: P0 must still gain **2** life — the anchored X, the +/// number of counters removed. Reading the relay's own zero yields 0, which is +/// exactly what the un-guarded #6956 change produced. +#[test] +fn thorna_gain_clause_reads_the_anchored_x_when_the_life_loss_clause_totals_zero() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let thorna = scenario + .add_creature_from_oracle(P0, "Thorna and Twigtooth", 4, 4, THORNA_ORACLE) + .id(); + // CR 119.8: P1 cannot lose life, so the middle clause totals zero. + let _emperion = scenario + .add_creature_from_oracle(P1, "Platinum Emperion", 8, 8, PLATINUM_EMPERION_ORACLE) + .id(); + + for _ in 0..20 { + scenario.add_card_to_library_top(P0, "Plains"); + scenario.add_card_to_library_top(P1, "Plains"); + } + + let mut runner = scenario.build(); + runner + .state_mut() + .objects + .get_mut(þa) + .expect("Thorna is on the battlefield") + .counters + .insert(CounterType::Minus1Minus1, 2); + + let life_before = runner.state().players[P0.0 as usize].life; + let opponent_life_before = runner.state().players[P1.0 as usize].life; + + runner.advance_to_combat(); + runner + .declare_attackers(&[(thorna, AttackTarget::Player(P1))]) + .expect("DeclareAttackers must succeed"); + resolve_attack_trigger_targeting(&mut runner, thorna); + + // Reach guard: the removal actually happened, so X really is 2. Without this + // the life assertion below could pass on a chain that never ran. + assert_eq!( + runner.state().objects.get(þa).map(|o| o + .counters + .get(&CounterType::Minus1Minus1) + .copied() + .unwrap_or(0)), + Some(0), + "reach guard: the trigger must have removed both -1/-1 counters (X = 2)" + ); + // Reach guard: the relay clause really did total ZERO — this is the branch + // under test, not the ordinary nonzero path. + assert_eq!( + runner.state().players[P1.0 as usize].life, + opponent_life_before, + "reach guard: CR 119.8 must have blocked the opponent's life loss entirely" + ); + + assert_eq!( + runner.state().players[P0.0 as usize].life - life_before, + 2, + "the gain clause must read the anchored X (2 counters removed), not the \ + relay life-loss clause's own zero" + ); +} + +/// Reach guard for the test above: the SAME card with no "can't lose life" +/// permanent must still work, so the zero-branch assertion is not vacuous and +/// the guard has not disabled the ordinary path. +#[test] +fn thorna_gain_clause_still_matches_the_life_loss_on_the_ordinary_path() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let thorna = scenario + .add_creature_from_oracle(P0, "Thorna and Twigtooth", 4, 4, THORNA_ORACLE) + .id(); + + for _ in 0..20 { + scenario.add_card_to_library_top(P0, "Plains"); + scenario.add_card_to_library_top(P1, "Plains"); + } + + let mut runner = scenario.build(); + runner + .state_mut() + .objects + .get_mut(þa) + .expect("Thorna is on the battlefield") + .counters + .insert(CounterType::Minus1Minus1, 2); + + let life_before = runner.state().players[P0.0 as usize].life; + let opponent_life_before = runner.state().players[P1.0 as usize].life; + + runner.advance_to_combat(); + runner + .declare_attackers(&[(thorna, AttackTarget::Player(P1))]) + .expect("DeclareAttackers must succeed"); + resolve_attack_trigger_targeting(&mut runner, thorna); + + assert_eq!( + opponent_life_before - runner.state().players[P1.0 as usize].life, + 2, + "the sole opponent must lose X = 2 life" + ); + assert_eq!( + runner.state().players[P0.0 as usize].life - life_before, + 2, + "and the controller gains the same X" + ); +}