From 691c4e103eda4ee195c13193b64a500fc74091ee Mon Sep 17 00:00:00 2001 From: matthewevans Date: Mon, 3 Aug 2026 03:37:23 -0700 Subject: [PATCH] fix(engine): stamp the draw total so a chained "discard that many" reads it "Draw N cards, then discard that many" discarded nothing. Varina, Lich Queen drew and gained life on attack but never discarded. `QuantityRef::PreviousEffectAmount { channel: Total }` reads only `state.last_effect_amount` and has no fallback, but `Effect::Draw` commits its total to `state.last_effect_count`. Neither extractor that populates `last_effect_amount` had an `Effect::Draw` arm, so both fell through to their `0` / `None` default: the discard count resolved to 0 and `discard.rs` short-circuited to a no-op before the `DiscardChoice` branch was ever reached. This is a regression. `ea9de8764b` ("bind discard that many to previous draw count") added an unconditional parser rewrite retargeting `Discard`'s `EventContextAmount` to `PreviousEffectAmount`, but was parser-only -- the engine-side producer arm was never added, so the rewritten reference has pointed at an unfilled channel ever since. The obligation was already documented on the `PreviousEffectAmount` type: "Every non-damage producer (life lost, counters removed, cards drawn) stamps only this channel." Read the committed total rather than re-summing draw events, exactly as the neighbouring `RollDie` arm defers to `die_result_this_resolution`, so replacement effects are respected: a draw replaced by something else contributes 0, one doubled by a count modifier contributes its post-replacement count. The arm returns early instead of falling through the `> 0` filter below it. A draw that delivered zero cards is a real zero and must stamp `Some(0)` -- otherwise "draw a card for each Island you control, then discard that many" with no Islands would inherit the amount its preceding chain step left behind. Also fixes the existing #3296 regression test, which passed vacuously. Its helper exited via the `Priority` arm after `advance_until_stack_empty` without ever dispatching `DecideOptionalEffect`, so the draw never happened and "net hand size unchanged" held for the wrong reason -- it asserted nothing about library size or that a discard occurred. Confirmed by running its original body against base behaviour with the bug fully present: it passed. It now keeps advancing until the optional draw is genuinely taken, panics if it never settles, and asserts the library actually shrank. --- crates/engine/src/game/effects/mod.rs | 21 ++ .../issue_3296_hordewing_skaab_discard.rs | 67 +++- .../issue_6858_draw_that_many_discard.rs | 298 ++++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + 4 files changed, 381 insertions(+), 6 deletions(-) create mode 100644 crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 7cf88119ab..7e0278c4ad 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -6857,6 +6857,27 @@ fn previous_effect_amount_from_events( // 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, }; diff --git a/crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs b/crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs index 99b01708c3..fc722986ff 100644 --- a/crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs +++ b/crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs @@ -28,8 +28,29 @@ fn hand_len(runner: &GameRunner, player: PlayerId) -> usize { .unwrap_or(0) } +fn library_len(runner: &GameRunner, player: PlayerId) -> usize { + runner + .state() + .players + .iter() + .find(|p| p.id == player) + .map(|p| p.library.len()) + .unwrap_or(0) +} + +/// Drive the trigger to the point where its "you may draw" decision is live and +/// accept it. +/// +/// The `Priority`-arm-then-`break` shape this replaces exited before ever +/// dispatching `DecideOptionalEffect`: after `run_combat` the trigger is on the +/// stack under `WaitingFor::Priority`, so the very first iteration took the +/// priority arm, ran `advance_until_stack_empty` (which stops as soon as +/// `PassPriority` is rejected under `OptionalEffectChoice`) and broke out. The +/// draw therefore never happened and the "net hand size unchanged" assertion +/// held vacuously (issue #6858). Keep advancing and accepting until neither is +/// possible so the optional draw is genuinely taken. fn accept_optional_effect(runner: &mut GameRunner) { - loop { + for _ in 0..8 { match &runner.state().waiting_for { WaitingFor::OptionalEffectChoice { .. } => { runner @@ -38,12 +59,14 @@ fn accept_optional_effect(runner: &mut GameRunner) { } WaitingFor::Priority { .. } if !runner.state().stack.is_empty() => { runner.advance_until_stack_empty(); - break; } - _ => break, + _ => return, } } - runner.advance_until_stack_empty(); + panic!( + "optional draw never settled; stuck on {:?}", + runner.state().waiting_for + ); } #[test] @@ -69,14 +92,46 @@ fn hordewing_skaab_discards_only_as_many_as_drawn_not_entire_hand() { let mut runner = scenario.build(); let hand_before = hand_len(&runner, P0); + let library_before = library_len(&runner, P0); assert_eq!(hand_before, 7, "precondition: seven cards in hand"); run_combat(&mut runner, vec![zombie], vec![]); accept_optional_effect(&mut runner); - let hand_after = hand_len(&runner, P0); + // Reach-guard (issue #6858): the net-hand-size assertion below is satisfied + // just as well by a trigger that drew nothing and discarded nothing, so it + // cannot stand alone. Pin the draw against the library and the discard + // against the live prompt before reading hand size. + assert_eq!( + library_len(&runner, P0), + library_before - 1, + "one opponent was damaged: the optional draw must have taken a card" + ); + let WaitingFor::DiscardChoice { + player, + count, + cards, + .. + } = runner.state().waiting_for.clone() + else { + panic!( + "\"If you do, discard that many cards\" must prompt for one discard, got {:?}", + runner.state().waiting_for + ); + }; + assert_eq!(player, P0); + assert_eq!(count, 1, "discard exactly as many as were drawn"); + + runner + .act(GameAction::SelectCards { + cards: cards.iter().copied().take(1).collect(), + }) + .expect("submitting the discard selection must succeed"); + accept_optional_effect(&mut runner); + assert_eq!( - hand_after, hand_before, + hand_len(&runner, P0), + hand_before, "one opponent was damaged: draw 1, then discard 1 — net hand size unchanged" ); } diff --git a/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs b/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs new file mode 100644 index 0000000000..190ecbccc6 --- /dev/null +++ b/crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs @@ -0,0 +1,298 @@ +//! GitHub issue #6858 — a chained "discard that many" that follows a draw +//! always resolved to 0. +//! +//! CR 608.2c + CR 121.2: `QuantityRef::PreviousEffectAmount { channel: Total }` +//! reads one resolution-local slot, `GameState::last_effect_amount`. Every +//! non-damage producer is contracted to stamp it (see the type doc on +//! `QuantityRef::PreviousEffectAmount`, which names "cards drawn" explicitly), +//! but `Effect::Draw` committed its instruction total only to +//! `state.last_effect_count`, so the chained consumer read `None` → 0 and the +//! discard silently short-circuited before `WaitingFor::DiscardChoice`. +//! +//! These tests pin the CHANNEL CONTRACT (`Draw` → `last_effect_amount`), not one +//! card: the first drives a synthetic "draw N, then discard that many" chain, the +//! second drives Varina, Lich Queen's real attack trigger end to end, and the +//! third pins the zero-draw case that keeps a preceding chain step's amount from +//! leaking into the discard count (Last Stand with no Islands). + +use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::types::ability::{ + AbilityDefinition, AbilityKind, CardSelectionMode, DamageChannel, Effect, QuantityExpr, + QuantityRef, TargetFilter, +}; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; + +use super::rules::run_combat; + +/// Verified against Scryfall (`/cards/named?exact=Varina, Lich Queen`). +const VARINA_ORACLE: &str = "Whenever you attack with one or more Zombies, draw that many cards, \ +then discard that many cards. You gain that much life.\n\ +{2}, Exile two cards from your graveyard: Create a tapped 2/2 black Zombie creature token."; + +fn hand_len(runner: &GameRunner, player: PlayerId) -> usize { + runner + .state() + .players + .iter() + .find(|p| p.id == player) + .map(|p| p.hand.len()) + .unwrap_or(0) +} + +fn library_len(runner: &GameRunner, player: PlayerId) -> usize { + runner + .state() + .players + .iter() + .find(|p| p.id == player) + .map(|p| p.library.len()) + .unwrap_or(0) +} + +/// "Draw `draw_count` cards, then discard that many cards" as a bare chain — +/// the building block the four affected Oracle shapes all compile down to. +fn draw_then_discard_that_many(draw: Effect) -> AbilityDefinition { + let mut ability = AbilityDefinition::new(AbilityKind::Activated, draw); + ability.sub_ability = Some(Box::new(AbilityDefinition::new( + AbilityKind::Activated, + Effect::Discard { + count: QuantityExpr::Ref { + qty: QuantityRef::PreviousEffectAmount { + channel: DamageChannel::Total, + }, + }, + target: TargetFilter::Controller, + selection: CardSelectionMode::Chosen, + unless_filter: None, + filter: None, + }, + ))); + ability +} + +fn stock_library(scenario: &mut GameScenario, player: PlayerId, count: usize) { + for i in 0..count { + scenario.add_card_to_library_top(player, &format!("Library Card {i}")); + } +} + +fn stock_hand(scenario: &mut GameScenario, player: PlayerId, count: usize) { + for i in 0..count { + scenario.add_creature_to_hand(player, &format!("Hand Card {i}"), 1, 1); + } +} + +fn activate(runner: &mut GameRunner, source: ObjectId) { + runner + .act(GameAction::ActivateAbility { + source_id: source, + ability_index: 0, + }) + .expect("costless activation must succeed"); + runner.advance_until_stack_empty(); +} + +#[test] +fn draw_stamps_the_total_channel_a_chained_that_many_discard_reads() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let source = scenario + .add_creature(P0, "Draw Then Discard That Many", 1, 1) + .with_ability_definition(draw_then_discard_that_many(Effect::Draw { + count: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Controller, + })) + .id(); + stock_library(&mut scenario, P0, 5); + stock_hand(&mut scenario, P0, 2); + + let mut runner = scenario.build(); + let hand_before = hand_len(&runner, P0); + let library_before = library_len(&runner, P0); + + activate(&mut runner, source); + + // The draw itself must have happened — without this the discard assertions + // below could pass for the wrong reason (a chain that never ran at all). + assert_eq!( + library_len(&runner, P0), + library_before - 3, + "the three-card draw must have left the library" + ); + assert_eq!( + hand_len(&runner, P0), + hand_before + 3, + "the three drawn cards must be in hand while the discard choice is open" + ); + + // CR 608.2c: at the moment the chain pauses on the discard prompt, BOTH + // resolution-local channels must still carry the draw instruction's + // committed total. `last_effect_count` is the slot `draw::resume_draw_ + // sequence` commits to; `last_effect_amount` is the slot + // `PreviousEffectAmount { channel: Total }` actually reads, and the gap + // between them was the defect. Both are observed here rather than only the + // second, so a future producer change that fills one and drops the other + // cannot pass. + assert_eq!( + runner.state().last_effect_count, + Some(3), + "the draw sequence commits its total to last_effect_count" + ); + assert_eq!( + runner.state().last_effect_amount, + Some(3), + "the draw must also stamp the slot that \ + PreviousEffectAmount {{ channel: Total }} reads" + ); + + // The discard must actually reach the interactive branch with count 3 — a + // count of 0 short-circuits to a no-op before `DiscardChoice` is raised. + let WaitingFor::DiscardChoice { player, count, .. } = &runner.state().waiting_for else { + panic!( + "chained \"discard that many\" must raise DiscardChoice, got {:?}", + runner.state().waiting_for + ); + }; + assert_eq!(*player, P0); + assert_eq!(*count, 3, "discard that many == the three cards drawn"); +} + +#[test] +fn varina_attack_trigger_discards_as_many_as_it_drew() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + scenario + .add_creature_from_oracle(P0, "Varina, Lich Queen", 3, 4, VARINA_ORACLE) + .with_subtypes(vec!["Zombie", "Wizard"]); + let zombie_a = scenario + .add_creature(P0, "Zombie Attacker A", 2, 2) + .with_subtypes(vec!["Zombie"]) + .id(); + let zombie_b = scenario + .add_creature(P0, "Zombie Attacker B", 2, 2) + .with_subtypes(vec!["Zombie"]) + .id(); + stock_library(&mut scenario, P0, 5); + stock_hand(&mut scenario, P0, 2); + + let mut runner = scenario.build(); + let hand_before = hand_len(&runner, P0); + let library_before = library_len(&runner, P0); + let life_before = runner + .state() + .players + .iter() + .find(|p| p.id == P0) + .map(|p| p.life) + .expect("P0 exists"); + + run_combat(&mut runner, vec![zombie_a, zombie_b], vec![]); + + assert_eq!( + library_len(&runner, P0), + library_before - 2, + "two attacking Zombies draw two cards" + ); + assert_eq!( + runner.state().last_effect_amount, + Some(2), + "the draw must stamp the total channel the discard reads" + ); + + let WaitingFor::DiscardChoice { count, cards, .. } = runner.state().waiting_for.clone() else { + panic!( + "Varina's \"then discard that many cards\" must prompt a discard, got {:?}", + runner.state().waiting_for + ); + }; + assert_eq!(count, 2, "discard as many as were drawn"); + + let chosen: Vec = cards.into_iter().take(2).collect(); + runner + .act(GameAction::SelectCards { cards: chosen }) + .expect("submitting the discard selection must succeed"); + runner.advance_until_stack_empty(); + + assert_eq!( + hand_len(&runner, P0), + hand_before, + "drew two and discarded two — net hand size unchanged" + ); + assert_eq!( + runner + .state() + .players + .iter() + .find(|p| p.id == P0) + .map(|p| p.life) + .expect("P0 exists"), + life_before + 2, + "\"You gain that much life\" still reads the attacking-Zombie count" + ); + assert_eq!( + runner + .state() + .players + .iter() + .find(|p| p.id == P0) + .map(|p| p.graveyard.len()) + .expect("P0 exists"), + 2, + "the two discarded cards reached the graveyard" + ); +} + +#[test] +fn a_zero_card_draw_stamps_zero_instead_of_inheriting_the_previous_step() { + // Last Stand's tail: "You gain 2 life for each Plains you control. Draw a + // card for each Island you control, then discard that many cards." With no + // Islands the draw delivers 0 and the discard must be 0 — never the life + // total the preceding chain step left in `last_effect_amount`. + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let mut ability = AbilityDefinition::new( + AbilityKind::Activated, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 4 }, + player: TargetFilter::Controller, + }, + ); + ability.sub_ability = Some(Box::new(draw_then_discard_that_many(Effect::Draw { + count: QuantityExpr::Fixed { value: 0 }, + target: TargetFilter::Controller, + }))); + let source = scenario + .add_creature(P0, "Gain Then Draw Zero", 1, 1) + .with_ability_definition(ability) + .id(); + stock_library(&mut scenario, P0, 5); + stock_hand(&mut scenario, P0, 4); + + let mut runner = scenario.build(); + let hand_before = hand_len(&runner, P0); + + activate(&mut runner, source); + + assert_eq!( + runner.state().last_effect_amount, + Some(0), + "a zero-card draw is a real zero result — it must overwrite the \ + preceding GainLife stamp, not leave it standing" + ); + assert!( + !matches!(runner.state().waiting_for, WaitingFor::DiscardChoice { .. }), + "discarding zero cards must not prompt, got {:?}", + runner.state().waiting_for + ); + assert_eq!( + hand_len(&runner, P0), + hand_before, + "no cards drawn and none discarded" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index a39af9f88f..4924af90ef 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -649,6 +649,7 @@ mod issue_6678_captain_america_shield_tap_defender; mod issue_6691_enters_under_their_control; mod issue_680_shalai_and_hallar_forgotten_ancient; mod issue_680_shalai_upkeep_move; +mod issue_6858_draw_that_many_discard; mod issue_688_mind_into_matter; mod issue_689_resonating_lute_hand_size; mod issue_691_sheoldred_saga_lore;