From 2aff5d834faf507b05c132f3243e2d74826509e6 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 09:55:17 -0700 Subject: [PATCH 1/4] fix(engine): make SCOPE-position PlayerFilter continuation decisions exhaustive (#6957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_player_scope_local_continuation` ended on `matches!(scope, PlayerFilter::All)`. That is a hand-maintained allowlist, not an exhaustive match, so every other `PlayerFilter` in SCOPE position silently answered "detach" with no compile error — and that answer is structural, not inert: `detach_after_player_scope_local_chain` uses it to pull the continuation out of the loop and run it ONCE as an unscoped tail, where `TargetFilter::ScopedPlayer` has no iteration player to bind to. Replace the allowlist with `scope_keeps_scoped_whole_hand_shuffle_local`, an exhaustive `match` over `PlayerFilter` (no `_` arm), mirroring `player_filter_references_tracked_set`. The answer is the same for every filter, which is the finding: the discriminator is the `ScopedPlayer` recipient on BOTH halves of the whole-hand move/shuffle pair (CR 115.10 + CR 701.24a), never the identity of the player set. The `All` conjunct was a conservative narrowing from #6730, not a rules condition. Behaviour on the current card pool is unchanged: a census of `data/card-data.json` finds exactly four cards with this effect pair (Molten Psyche, Once More with Feeling, Whirlpool Warrior, Winds of Change) and all four already carry `player_scope: All`. Regression runs TWO scoped opponents on purpose — with one iteration the aggregate "a shuffle happened" cannot distinguish "kept in scope" from "detached and run once". Under the reverted gate it fails with 1 shuffle instruction instead of 2. Also re-pins the CR 603.5 prompt census (+62, uniform, producers verified sha256-identical at their new coordinates). --- crates/engine/src/game/effects/mod.rs | 286 +++++++++++++++++++++++--- crates/engine/src/game/engine.rs | 13 +- 2 files changed, 269 insertions(+), 30 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 8ae1ad4e59..0541a800e0 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -3326,26 +3326,88 @@ 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 + | PlayerFilter::AllExcept { .. } + // 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, + } } /// CR 109.5 + CR 115.10 + CR 119.3: Detect that an effect's recipient is bound @@ -27730,14 +27792,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 +27973,157 @@ 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 + ); + } + } } diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 8d4b162053..a1bf8938f6 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -15187,6 +15187,13 @@ 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. // // ⚠ 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 +15206,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:6061".to_string(), + "game/effects/mod.rs:6138".to_string(), + "game/effects/mod.rs:9113".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. From 00e5a949a8ec61ab40df9e413cbd74cf852f0c2e Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 10:43:45 -0700 Subject: [PATCH 2/4] fix(engine): stamp a real zero result instead of leaking the previous chain step (#6956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `previous_effect_amount_from_events` ended on `(amount > 0).then_some(amount)`, collapsing "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. Stamping `Some(sum)` unconditionally is the mirror bug and fails more quietly. It is also live, not hypothetical: `Effect::PayCost` carries an arbitrary `AbilityCost`, and 19 shipping cards (the Extort cycle) chain a `PreviousEffectAmount` consumer behind a MANA `PayCost` that emits no `LifeChanged` event at all. So the zero case is decided per effect by two independent conditions: 1. the effect OWNS the channel that arm sums, and 2. the instruction COMPLETED — `EffectResolved { kind, source_id }`, pushed by every resolver as its last act and skipped when it suspends for a player choice. This is the same anchor the already-correct sibling `previous_effect_counts_by_player_from_events` uses. Nonzero behaviour is unchanged. Arms closed, each with its own CR determination and its own zero-case test: DealDamage/DamageAll/DamageEachPlayer CR 120.8 + CR 614.7a + CR 615.1 LoseLife CR 119.3 GainLife CR 119.3 + CR 119.9 RemoveCounter CR 122.1 Arms NOT closed, left on predecessor behaviour with the reasoning inline: PayCost CR 118.1 + CR 119.4b say paying 0 life is a real payment whose result is zero, but `pay::resolve` pushes no events at all, so it has no terminal marker and completion cannot be established. Both directions are pinned by tests instead. Fight sums the EXCESS channel into the TOTAL slot and scopes it to a fought creature that may not resolve — a genuine third state. Unobservable today: The Last Agni Kai, the only consumer, gates on the Excess channel, which is written unconditionally. Note: #6956 cites CR 118.2 for "losing 0 life is not a life-loss event". CR 118.2 is the mana-payment rule, and the rules text has no life-loss analogue of CR 119.9 at all, so that premise does not hold and LoseLife is treated under CR 119.3 like the others. Also re-pins the CR 603.5 prompt census (+130 on one entry; producer verified sha256-identical at its new coordinate, other four unmoved). --- crates/engine/src/game/effects/mod.rs | 404 +++++++++++++++++++++++++- crates/engine/src/game/engine.rs | 10 +- 2 files changed, 410 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 0541a800e0..28263e0fd4 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -7017,7 +7017,7 @@ fn previous_effect_amount_from_events( // 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 + // Returns early rather than falling through 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 @@ -7027,7 +7027,137 @@ fn previous_effect_amount_from_events( _ => 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); + } + zero_amount_is_this_effects_result(ability, events).then_some(0) +} + +/// CR 608.2c: Is a zero total a REAL zero result for this effect, or the absence +/// of any result on the channel `previous_effect_amount_from_events` just summed? +/// +/// Two independent conditions must hold, and each one alone is insufficient. +/// +/// 1. The effect must OWN the channel. Every arm above sums one specific event +/// class, and one arm can be reached by an effect that emits none of it: +/// `Effect::PayCost` carries an arbitrary `AbilityCost`, so a mana, sacrifice +/// or discard payment produces no `LifeChanged` event at all and has no amount +/// to report. Likewise the `Fight` arm scopes its sum to the fought creature, +/// so an unresolvable fighter pair yields zero for want of a subject rather +/// than for want of excess. +/// 2. The instruction must have COMPLETED. Every resolver pushes +/// `EffectResolved { kind, source_id }` as its last act and returns BEFORE that +/// push when it suspends for a player choice (`DamageResult::NeedsChoice`, the +/// replacement-choice branch in `life::resolve`, the pending-counter drain). +/// A suspended instruction has not produced its result yet, so its +/// partial-window zero must not overwrite anything. This is the same anchor +/// the sibling `previous_effect_counts_by_player_from_events` already uses to +/// separate "producer ran and moved nothing" from "no producer". +fn zero_amount_is_this_effects_result(ability: &ResolvedAbility, events: &[GameEvent]) -> bool { + let owns_channel = match &ability.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 precisely what a following "that much" back-references + // Reading an earlier step's amount instead is never correct. + Effect::DealDamage { .. } | Effect::DamageAll { .. } | Effect::DamageEachPlayer { .. } + // 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 exist in the rules text; see FINDINGS. + | Effect::LoseLife { .. } + // 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 here. + | Effect::GainLife { .. } + // 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 { .. } => true, + // NOT CLOSED — deliberately left on the predecessor "zero is an absence" + // behaviour, not overlooked (#6956). + // + // CR 120.10: unlike every arm above, the `Fight` arm sums the EXCESS + // channel into the TOTAL slot, and it scopes that sum to the fought + // creature resolved by `fight::resolve_fight_fighters`. That gives it a + // genuine THIRD state the others do not have: 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 about what a fight + // with an unresolvable fighter pair reports. + // + // 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 through + // `.unwrap_or(0)`. So the rider never runs in the zero case and no + // production-path test can discriminate this arm's behaviour today. + Effect::Fight { .. } => false, + // NOT CLOSED — deliberately left on the predecessor behaviour (#6956). + // + // CR 118.1: `PayCost`'s `cost` is an arbitrary `AbilityCost`, and only a + // life payment emits the negative `LifeChanged` events the `LoseLife | + // PayCost` arm sums. A mana / sacrifice / discard payment owns NO amount + // on this channel and must leave the preceding step's value standing — + // that is the live mirror-bug case, not a hypothetical: 19 cards (the + // Extort cycle) chain a `PreviousEffectAmount` consumer behind a mana + // `PayCost`, and an unconditional `Some(0)` would zero the life-loss + // total they are meant to read. + // + // CR 119.4b makes paying 0 life a real, always-legal payment whose + // result is zero, so a `CostCategory::PaysLife` cost SHOULD stamp + // `Some(0)` — but `pay::resolve` pushes no events at all, so it emits no + // `EffectResolved` terminal marker and the completion half of this + // predicate 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 (`previous_effect_counts_by_player_from_events`, + // the trigger matchers, and the game log all read that event class). + // `mana_pay_cost_does_not_zero_the_previous_chain_steps_amount` pins the + // Extort direction so the mirror bug cannot be introduced meanwhile. + Effect::PayCost { .. } => false, + // Mirrors the `_ => 0` arm of the sum above: these effects have no + // scalar channel here, so their zero is an absence, never a result. + _ => false, + }; + owns_channel && effect_instruction_completed(ability, events) +} + +/// 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. +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 @@ -12284,7 +12414,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, @@ -28126,4 +28256,272 @@ mod tests { ); } } + + // --------------------------------------------------------------------- + // #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: THE MIRROR BUG. `Effect::PayCost` carries an arbitrary + /// `AbilityCost`, so a MANA payment emits no `LifeChanged` event and has no + /// amount on the life channel at all. Stamping `Some(0)` for it would zero + /// the value the next clause is meant to read — which is exactly the Extort + /// cycle's shape (19 cards chain a `PreviousEffectAmount` consumer behind a + /// mana `PayCost`). The preceding step's 7 must survive. + #[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 `zero_amount_is_this_effects_result` 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 zero_amount_is_this_effects_result" + ); + } } diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index a1bf8938f6..27780a8d90 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -15194,6 +15194,14 @@ mod stage2_injector_tests { // 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_amount_is_this_effects_result` / `effect_instruction_completed` + // 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`, at `:12417`/`:28259`. Producer + // sha256-identical at its new coordinate. // // ⚠ 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 @@ -15208,7 +15216,7 @@ mod stage2_injector_tests { // and is offered as a follow-up rather than taken unannounced mid-review. "game/effects/mod.rs:6061".to_string(), "game/effects/mod.rs:6138".to_string(), - "game/effects/mod.rs:9113".to_string(), + "game/effects/mod.rs:9243".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. From d1c1c038ba66447e58c314ac338a371e302b8144 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 11:51:09 -0700 Subject: [PATCH 3/4] fix(engine): guard the shared-X relay shape and collapse the twin registry (#6956 review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the HIGH plus items 2-6 from review. #6957 untouched except item 6. [HIGH] Thorna and Twigtooth regressed. Its trigger lowers to `RemoveCounter -> LoseLife{PreviousEffectAmount} -> GainLife{PreviousEffectAmount}` for "…each opponent loses X life, you gain X life, … where X is the number of counters removed this way". With an opponent under CR 119.8 "can't lose life" the middle clause totals zero; #6956's first pass claimed that zero and the gain clause read 0 instead of 2. Reproduced exactly as reported (gained=0 vs 2). The generalisation, not the card: CR 608.2c — "…X…, …X…, and …X…, where X is " names ONE value 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 need. `zero_is_a_result` now requires `!effect_relays_the_shared_amount(..)`. Scoped to the ZERO case on purpose, so it is exactly a restoration of predecessor behaviour for relays and provably no wider than #6956 itself. It does NOT fix the nonzero chain-relative divergence (a two-opponent Thorna still stamps the doubled loss) — that is the pre-existing anchored-quantity gap, left for separate routing. The card-pool census #6956 never got, by nearest stamping producer (the only one a consumer can read): 114 cards read a changed-kind producer directly -> helped 2 had a stamper above it, i.e. real leak content -> Thorna, Groaaaaag 0 harm remaining after the relay guard 4 relay-shaped cards with nothing above them revert to predecessor behaviour (comeuppance, lulu, magma pummeler, new way forward) Last Stand drops out: its consumer's nearest producer is `Draw`, which this diff never changed. [MED] The PayCost justification was factually inverted and is corrected. The 19 Extort cards do NOT have `PayCost` as the direct parent of a `PreviousEffectAmount` consumer — `LoseLife` sits between and overwrites first. ZERO cards do. The guard now rests on CR 118.1 alone ("owns no channel"), and the test docstring says so instead of claiming to pin an Extort direction it never protected. [LOW-MED] Twin registry collapsed. The `_ => 0` summing arm and the separate `_ => false` zero-policy arm could drift, silently restoring #6956 with no compile error — the exact mechanism #6957 removes. Both are now derived from one `amount_channel` classifier returning a typed descriptor carrying the event channel and the zero semantics. Verified: adding a channel is E0004. [LOW] Dangling "see FINDINGS" pointer removed. [LOW] `effect_instruction_completed` no longer overstates parity with the sibling; the comment now names the difference (marker presence vs window-bounded tally). [LOW] `AllExcept` recurses into its anchor in #6957's helper, matching the cited precedent. Behaviour-identical while every arm is `true`. Also adds the shared `quantity_expr_any_ref` walker so the back-reference predicates do not each carry a copy of the composition-form recursion. --- crates/engine/src/game/effects/mod.rs | 407 +++++++++++------- crates/engine/src/game/engine.rs | 24 +- crates/engine/tests/integration/main.rs | 1 + ...horna_and_twigtooth_shared_x_relay_6956.rs | 196 +++++++++ 4 files changed, 460 insertions(+), 168 deletions(-) create mode 100644 crates/engine/tests/integration/thorna_and_twigtooth_shared_x_relay_6956.rs diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 28263e0fd4..7b9c144a94 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -3380,7 +3380,6 @@ fn scope_keeps_scoped_whole_hand_shuffle_local(scope: &PlayerFilter) -> bool { PlayerFilter::Controller | PlayerFilter::Opponent | PlayerFilter::All - | PlayerFilter::AllExcept { .. } // Event-context and trigger anchors. | PlayerFilter::DefendingPlayer | PlayerFilter::TriggeringPlayer @@ -3407,6 +3406,11 @@ fn scope_keeps_scoped_whole_hand_shuffle_local(scope: &PlayerFilter) -> bool { // 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), } } @@ -6929,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 @@ -6974,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, .. } => { @@ -6997,34 +7145,6 @@ 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 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`. - Effect::Draw { .. } => return state.last_effect_count, - _ => 0, }; // CR 608.2c (#6956): the predecessor tail was `(amount > 0).then_some(amount)`, @@ -7042,101 +7162,45 @@ fn previous_effect_amount_from_events( if amount != 0 { return Some(amount); } - zero_amount_is_this_effects_result(ability, events).then_some(0) -} - -/// CR 608.2c: Is a zero total a REAL zero result for this effect, or the absence -/// of any result on the channel `previous_effect_amount_from_events` just summed? -/// -/// Two independent conditions must hold, and each one alone is insufficient. -/// -/// 1. The effect must OWN the channel. Every arm above sums one specific event -/// class, and one arm can be reached by an effect that emits none of it: -/// `Effect::PayCost` carries an arbitrary `AbilityCost`, so a mana, sacrifice -/// or discard payment produces no `LifeChanged` event at all and has no amount -/// to report. Likewise the `Fight` arm scopes its sum to the fought creature, -/// so an unresolvable fighter pair yields zero for want of a subject rather -/// than for want of excess. -/// 2. The instruction must have COMPLETED. Every resolver pushes -/// `EffectResolved { kind, source_id }` as its last act and returns BEFORE that -/// push when it suspends for a player choice (`DamageResult::NeedsChoice`, the -/// replacement-choice branch in `life::resolve`, the pending-counter drain). -/// A suspended instruction has not produced its result yet, so its -/// partial-window zero must not overwrite anything. This is the same anchor -/// the sibling `previous_effect_counts_by_player_from_events` already uses to -/// separate "producer ran and moved nothing" from "no producer". -fn zero_amount_is_this_effects_result(ability: &ResolvedAbility, events: &[GameEvent]) -> bool { - let owns_channel = match &ability.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 precisely what a following "that much" back-references - // Reading an earlier step's amount instead is never correct. - Effect::DealDamage { .. } | Effect::DamageAll { .. } | Effect::DamageEachPlayer { .. } - // 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 exist in the rules text; see FINDINGS. - | Effect::LoseLife { .. } - // 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 here. - | Effect::GainLife { .. } - // 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 { .. } => true, - // NOT CLOSED — deliberately left on the predecessor "zero is an absence" - // behaviour, not overlooked (#6956). - // - // CR 120.10: unlike every arm above, the `Fight` arm sums the EXCESS - // channel into the TOTAL slot, and it scopes that sum to the fought - // creature resolved by `fight::resolve_fight_fighters`. That gives it a - // genuine THIRD state the others do not have: 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 about what a fight - // with an unresolvable fighter pair reports. - // - // 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 through - // `.unwrap_or(0)`. So the rider never runs in the zero case and no - // production-path test can discriminate this arm's behaviour today. - Effect::Fight { .. } => false, - // NOT CLOSED — deliberately left on the predecessor behaviour (#6956). + 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. // - // CR 118.1: `PayCost`'s `cost` is an arbitrary `AbilityCost`, and only a - // life payment emits the negative `LifeChanged` events the `LoseLife | - // PayCost` arm sums. A mana / sacrifice / discard payment owns NO amount - // on this channel and must leave the preceding step's value standing — - // that is the live mirror-bug case, not a hypothetical: 19 cards (the - // Extort cycle) chain a `PreviousEffectAmount` consumer behind a mana - // `PayCost`, and an unconditional `Some(0)` would zero the life-loss - // total they are meant to read. + // 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. // - // CR 119.4b makes paying 0 life a real, always-legal payment whose - // result is zero, so a `CostCategory::PaysLife` cost SHOULD stamp - // `Some(0)` — but `pay::resolve` pushes no events at all, so it emits no - // `EffectResolved` terminal marker and the completion half of this - // predicate 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 (`previous_effect_counts_by_player_from_events`, - // the trigger matchers, and the game log all read that event class). - // `mana_pay_cost_does_not_zero_the_previous_chain_steps_amount` pins the - // Extort direction so the mirror bug cannot be introduced meanwhile. - Effect::PayCost { .. } => false, - // Mirrors the `_ => 0` arm of the sum above: these effects have no - // scalar channel here, so their zero is an absence, never a result. - _ => false, - }; - owns_channel && effect_instruction_completed(ability, events) + // 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? @@ -7146,6 +7210,14 @@ fn zero_amount_is_this_effects_result(ability: &ResolvedAbility, events: &[GameE /// 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| { @@ -7327,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, @@ -28468,12 +28549,18 @@ mod tests { assert_eq!(drawn, 2, "the consumer must read this step's own amount"); } - /// CR 118.1: THE MIRROR BUG. `Effect::PayCost` carries an arbitrary - /// `AbilityCost`, so a MANA payment emits no `LifeChanged` event and has no - /// amount on the life channel at all. Stamping `Some(0)` for it would zero - /// the value the next clause is meant to read — which is exactly the Extort - /// cycle's shape (19 cards chain a `PreviousEffectAmount` consumer behind a - /// mana `PayCost`). The preceding step's 7 must survive. + /// 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(); @@ -28498,8 +28585,8 @@ mod tests { /// 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 `zero_amount_is_this_effects_result` cannot be - /// established for `Effect::PayCost`. Pinned at the PREDECESSOR behaviour + /// 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. @@ -28521,7 +28608,7 @@ mod tests { assert_eq!( drawn, 7, "PayCost has no terminal marker, so its zero cannot yet be told apart \ - from an absent producer; see zero_amount_is_this_effects_result" + from an absent producer; see `amount_channel`'s PayCost arm" ); } } diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 27780a8d90..34a0a50aa3 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -15197,11 +15197,19 @@ mod stage2_injector_tests { // #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_amount_is_this_effects_result` / `effect_instruction_completed` - // 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`, at `:12417`/`:28259`. Producer - // sha256-identical at its new coordinate. + // 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 @@ -15214,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:6061".to_string(), - "game/effects/mod.rs:6138".to_string(), - "game/effects/mod.rs:9243".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" + ); +} From e98437f61282fb5aa570924d2081aa086ca0ba23 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 12:36:28 -0700 Subject: [PATCH 4/4] test(engine): pin the co-anchored-X gap and its undecidability guard (#6956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR 107.3i — "normally, all instances of X on an object have the same value at any given time." The engine lowers every X in a co-anchored clause group to `QuantityRef::PreviousEffectAmount`, which reads the immediately preceding chain step. When the anchor is not the direct predecessor, a later clause reads the intervening relay's result instead of X. Thorna and Twigtooth is the live case: `RemoveCounter -> LoseLife{Prev} -> GainLife{Prev}` with two opponents makes the third clause read the SUMMED life loss (2 x X) rather than X. Two tests, both watched go red against this tree: - `second_consumer_of_a_shared_x_reads_the_relay_not_the_anchor` pins the gap at its current (wrong) value of 4, following the convention already used by `zero_life_pay_cost_is_not_yet_distinguished_from_an_absent_producer` — the open arm becomes an asserted fact rather than an untested assumption. Flipping the expectation to 2 is the acceptance test. - `a_relay_that_re_anchors_x_must_keep_publishing_its_own_result` pins the counter-shape (Magma Pummeler: "remove that many counters ... deals that much damage") where the intervening relay IS the correct anchor. It is byte-identical in the AST to the Thorna shape but demands the opposite answer. Together they prove a stamp-time fix is undecidable: generalizing `effect_relays_the_shared_amount` from the zero case to every case was measured to flip the first test 4 -> 2 (correct) while breaking the second 7 -> 2 (regression). The binding must be recorded where the parser lowers X, not recovered at resolution. No production code changed. --- crates/engine/src/game/effects/mod.rs | 192 ++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 7b9c144a94..0c5b7943de 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -28611,4 +28611,196 @@ mod tests { 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" + ); + } }