From a475003e30bf127aba9482c4b06553ff43122fc4 Mon Sep 17 00:00:00 2001 From: nghequyettien Date: Wed, 29 Jul 2026 01:18:39 +0700 Subject: [PATCH 1/3] fix(parser): Runadi, Behemoth Caller ETB counters and haste threshold Runadi's first ability failed to parse its composite "where X is its mana value minus 4" quantity clause, misrouting the whole ability to the generic self-ETB fallback and scoping the counter grant to Runadi herself instead of the cast creature. Delegate to the shared where-X-is suffix parser (already used by the sibling self-ETB path) so composite/offset quantities resolve here too, and thread the entering object through the Recipient scope resolver so "its mana value" reads the creature actually entering, not the static source. Separately, the haste static's "with three or more +1/+1 counters" filter left "or more" stuck onto the counter-type text (a garbage Generic("or more +1/+1") counter type), so no creature ever matched and haste never applied. Strip the redundant or-more/or-greater qualifier after the count, mirroring the existing mana-value handling. --- crates/engine/src/game/quantity.rs | 13 ++ .../engine/src/parser/oracle_replacement.rs | 98 ++++++++++++-- crates/engine/src/parser/oracle_target.rs | 52 ++++++++ crates/engine/src/types/ability.rs | 7 +- crates/engine/tests/integration/main.rs | 1 + .../runadi_behemoth_caller_etb_counters.rs | 126 ++++++++++++++++++ 6 files changed, 282 insertions(+), 15 deletions(-) create mode 100644 crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index 409769adf5..716dbab427 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -4628,6 +4628,17 @@ fn object_for_scope<'a>( _ => None, }) }) + // CR 614.12 + CR 613.4c: in an ETB-scoped replacement ("that + // creature enters with ... counters on it, where X is its mana + // value/power/toughness ..."), the recipient IS the entering + // object, not the static replacement source. `ctx.entering` + // carries that identity (mirrors `QuantityContext::self_object`, + // the same convention `CastManaObjectScope::SelfObject` uses for + // Wildgrowth Archaic's "it"). Outside ETB-replacement contexts + // `ctx.entering` is always `None` (only ETB-counter extraction + // sets it), so this fallback is inert for every layer-evaluation + // `Recipient` caller (Blessing of the Nephilim, Civic Saber). + .or_else(|| ctx.entering.and_then(|id| state.objects.get(&id))) .or_else(|| source_object_for_context(state, ctx.source, ctx.trigger_source.as_ref())), // CR 603.4: an intervening-if condition is checked at trigger detection // (current_trigger_event is None then) and re-checked on resolution. @@ -4692,6 +4703,8 @@ fn object_id_for_scope( _ => None, }) }) + // CR 614.12 + CR 613.4c: see the parallel arm in `object_for_scope`. + .or(ctx.entering) .or_else(|| { source_object_for_context(state, ctx.source, ctx.trigger_source.as_ref()) .map(|object| object.id) diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index a74fc1f2e6..89a9b741ff 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -4828,21 +4828,18 @@ fn parse_whenever_you_cast_enters_with( .parse(rest) .ok()?; - // Optional trailing "where X is [quantity]" clause. + // Optional trailing "where X is [quantity]" clause. Delegate to + // `parse_enters_with_where_x_suffix` — the single authority for this tail + // grammar, already shared with the self-ETB `parse_enters_with_counters` + // path — so composite/offset quantities ("its mana value minus 4"; CR + // 107.1 arithmetic over a CR 202.3 mana-value reference) resolve here too, + // not just atomic `QuantityRef`s. The previous atomic-only + // `parse_quantity_ref` call silently failed (via `?`) on any composite + // expression, misrouting the whole ability to the generic self-ETB + // fallback (Runadi, Behemoth Caller — issue #6492). let count_expr = match fixed_count { Some(n) => QuantityExpr::Fixed { value: n as i32 }, - None => { - // Expect ", where x is " then a quantity ref. - let (rest, _) = alt(( - tag::<_, _, OracleError<'_>>(", where x is "), - tag(", where X is "), - )) - .parse(rest) - .ok()?; - let qty_text = rest.trim_end_matches('.').trim(); - let qty = crate::parser::oracle_quantity::parse_quantity_ref(qty_text)?; - QuantityExpr::Ref { qty } - } + None => parse_enters_with_where_x_suffix(rest)?, }; let put_counter = AbilityDefinition::new( @@ -19670,6 +19667,81 @@ mod tests { assert!(parse_replacement_line(text, "Filler").is_none()); } + /// CR 614.1c + CR 202.3 + CR 107.1: Runadi, Behemoth Caller's first ability + /// ("Whenever you cast a creature spell with mana value 5 or greater, that + /// creature enters with X additional +1/+1 counters on it, where X is its + /// mana value minus 4.") parses into a `ChangeZone` replacement scoped to + /// the entering creature (not Runadi herself — issue #6492 regression), + /// with a composite offset quantity over that creature's own mana value. + #[test] + fn parses_runadi_behemoth_caller_replacement() { + let text = "Whenever you cast a creature spell with mana value 5 or greater, that creature enters with X additional +1/+1 counters on it, where X is its mana value minus 4."; + let def = parse_replacement_line(text, "Runadi, Behemoth Caller") + .expect("Runadi's first ability should parse as a replacement"); + assert_eq!(def.event, ReplacementEvent::ChangeZone); + assert_eq!(def.destination_zone, Some(Zone::Battlefield)); + + // valid_card: creature with mana value >= 5, controlled by Runadi's + // controller — NOT SelfRef (the pre-fix regression). + let TargetFilter::Typed(ref tf) = def.valid_card.as_ref().expect("valid_card set") else { + panic!("expected Typed filter, got {:?}", def.valid_card); + }; + assert_eq!(tf.type_filters, vec![TypeFilter::Creature]); + assert_eq!(tf.controller, Some(ControllerRef::You)); + assert!( + tf.properties.iter().any(|p| matches!( + p, + FilterProp::Cmc { + comparator: Comparator::GE, + value: QuantityExpr::Fixed { value: 5 }, + } + )), + "valid_card must gate on mana value >= 5, got {:?}", + tf.properties + ); + + // execute: PutCounter { target: SelfRef, count: Offset(its mana value, -4) }. + let exec = def.execute.as_ref().expect("execute set"); + let Effect::PutCounter { + counter_type, + count, + target, + } = &*exec.effect + else { + panic!("expected PutCounter, got {:?}", exec.effect); + }; + assert_eq!(counter_type, &CounterType::Plus1Plus1); + assert_eq!(target, &TargetFilter::SelfRef); + assert_eq!( + count, + &QuantityExpr::Offset { + inner: Box::new(QuantityExpr::Ref { + qty: QuantityRef::ObjectManaValue { + scope: crate::types::ability::ObjectScope::Recipient, + }, + }), + offset: -4, + }, + "count must be the entering creature's own mana value minus 4, not a \ + garbage literal or Runadi's own mana value" + ); + } + + /// Regression: an unparseable composite quantity in the "where X is" clause + /// must still fail closed (return `None`, falling through to the generic + /// self-ETB fallback) rather than silently absorbing the condition text as + /// a garbage counter-type literal — the exact failure mode issue #6492 + /// reported before the fix. + #[test] + fn whenever_you_cast_enters_with_garbage_quantity_fails_closed() { + let text = "Whenever you cast a creature spell with mana value 5 or greater, that creature enters with X additional +1/+1 counters on it, where X is its unrecognized nonsense value minus 4."; + assert!( + parse_whenever_you_cast_enters_with(&text.to_lowercase(), text).is_none(), + "an unparseable quantity clause must fail this combinator closed, not \ + succeed with a wrong AST" + ); + } + /// Regression: "Whenever you cast" with a fixed additional counter amount /// (no "where X is …" tail) also parses cleanly. Covers the cousin shape /// where the count is a literal number. diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index 2d404c106b..80e17261a1 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -5532,6 +5532,18 @@ fn parse_counter_spec_after_lead( |input| { let (input, expr) = nom_quantity::parse_quantity_expr_number(input)?; let (input, _) = tag_e::<_, _, OracleError<'_>>(" ").parse(input)?; + // CR 122.1: "with N or more/or greater counters" — redundant + // with the already-GE `with` lead (mirrors the CMC "N or greater" + // handling above `parse_counter_suffix`'s call site), but the + // qualifier must still be consumed here or it leaks into the + // counter-type slice below (issue #6492: "or more +1/+1" parsed as + // a garbage counter type on Runadi, Behemoth Caller's haste static). + let input = alt(( + tag_e::<_, _, OracleError<'_>>("or more "), + tag_e("or greater "), + )) + .parse(input) + .map_or(input, |(rest, _)| rest); Ok((input, expr)) }, )); @@ -11796,6 +11808,46 @@ mod tests { )); } + /// CR 122.1 + CR 613.4c: issue #6492 — Runadi, Behemoth Caller's haste + /// static ("Creatures you control with three or more +1/+1 counters on + /// them have haste.") requires "three or more" to consume cleanly instead + /// of leaking "or more" into the counter-type slice (`Generic("or more + /// +1/+1")` pre-fix — no creature ever matched the filter, so haste never + /// applied). "with N counters" is already GE per the `with` lead, so "or + /// more"/"or greater" is a redundant qualifier that must be consumed, not + /// carried into the counter type. + #[test] + fn parse_counter_suffix_three_or_more_plus1plus1() { + let result = parse_counter_suffix(" with three or more +1/+1 counters on them"); + assert!(result.is_some()); + let (prop, _consumed) = result.unwrap(); + assert!(matches!( + prop, + FilterProp::Counters { + counters: CounterMatch::OfType(CounterType::Plus1Plus1), + comparator: Comparator::GE, + count: QuantityExpr::Fixed { value: 3 }, + } + )); + } + + /// Sibling coverage: "or greater" (not just "or more") must also be + /// stripped cleanly. + #[test] + fn parse_counter_suffix_two_or_greater_stun() { + let result = parse_counter_suffix(" with two or greater stun counters on it"); + assert!(result.is_some()); + let (prop, _consumed) = result.unwrap(); + assert!(matches!( + prop, + FilterProp::Counters { + counters: CounterMatch::OfType(CounterType::Stun), + comparator: Comparator::GE, + count: QuantityExpr::Fixed { value: 2 }, + } + )); + } + #[test] fn parse_counter_suffix_not_counter_phrase() { let result = parse_counter_suffix(" with power 3 or greater"); diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 3a8322bce2..2c8995a3b6 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -5294,9 +5294,12 @@ pub enum ObjectScope { Target, /// CR 613.4c + CR 115.10: The object currently receiving an effect. /// In layer evaluation this is the per-object recipient. Outside layers, - /// it resolves to the first object target when present, then to the source. + /// it resolves to the first object target when present, then to the + /// entering object of an ETB-scoped replacement, then to the source. /// Used for recipient-relative "its colors" boosts such as Blessing of - /// the Nephilim and Civic Saber. + /// the Nephilim and Civic Saber, and for "its mana value"/"its power" + /// quantities inside "that creature enters with ... counters" replacement + /// effects (Runadi, Behemoth Caller). Recipient, /// CR 603.2: The object referenced by the current trigger event. EventSource, diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index cf90c0bbf6..5f0c7345e1 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -810,6 +810,7 @@ mod roots_of_wisdom_if_you_cant_draw; mod roughshod_mentor_green_trample_grant; mod rules; mod run_for_your_life_escape; +mod runadi_behemoth_caller_etb_counters; mod saddle_become_effect; mod saddle_state_model; mod saruman_white_hand_amass; diff --git a/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs new file mode 100644 index 0000000000..121847e75d --- /dev/null +++ b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs @@ -0,0 +1,126 @@ +//! Runadi, Behemoth Caller — RUNTIME witness for the ETB-counter replacement +//! and its downstream haste consequence (issue #6492). +//! +//! Oracle (verified via Scryfall, card j22/44): +//! "Whenever you cast a creature spell with mana value 5 or greater, that +//! creature enters with X additional +1/+1 counters on it, where X is its +//! mana value minus 4." +//! "Creatures you control with three or more +1/+1 counters on them have +//! haste." +//! "{T}: Add {G}." +//! +//! Pre-fix, the composite "its mana value minus 4" clause failed to parse +//! (the combinator only supported atomic quantity refs), misrouting the +//! ability to the self-ETB fallback with `valid_card: SelfRef` — Runadi would +//! try to put counters on HERSELF, not the cast creature, and the cast +//! creature would enter with 0 counters regardless of its mana value. +//! +//! CR references (verified against docs/MagicCompRules.txt): +//! - CR 614.1c: "[this permanent] enters with ..." is a replacement effect. +//! - CR 202.3: mana value. +//! - CR 122.1a: a +1/+1 counter adds 1 to power and 1 to toughness. +//! +//! Discrimination: a mana-value-8 creature must enter with 4 counters (8-4) +//! and gain Haste from the second ability's 3-or-more threshold; a +//! mana-value-4 creature must enter with 0 counters (filter excludes it) and +//! no Haste. + +use engine::game::layers::evaluate_layers; +use engine::game::scenario::{GameScenario, P0}; +use engine::types::counter::CounterType; +use engine::types::keywords::Keyword; +use engine::types::mana::{ManaCost, ManaCostShard, ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::zones::Zone; +use engine::types::ObjectId; + +const RUNADI: &str = "Whenever you cast a creature spell with mana value 5 or greater, that creature enters with X additional +1/+1 counters on it, where X is its mana value minus 4.\nCreatures you control with three or more +1/+1 counters on them have haste.\n{T}: Add {G}."; + +/// Cast a green creature of the given mana value (shards: GG, generic = mv - +/// 2) while Runadi is on P0's battlefield. Returns `(counters on the +/// entrant, entrant has Haste)`. +fn cast_creature_with_runadi(name: &str, mana_value: u32) -> (u32, bool) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + scenario.add_creature_from_oracle(P0, "Runadi, Behemoth Caller", 1, 3, RUNADI); + + let generic = mana_value.saturating_sub(2); + let spell = scenario + .add_creature_to_hand_from_oracle(P0, name, 1, 1, "") + .with_mana_cost(ManaCost::Cost { + shards: vec![ManaCostShard::Green, ManaCostShard::Green], + generic, + }) + .id(); + + scenario.with_mana_pool( + P0, + (0..generic) + .map(|_| ManaUnit::new(ManaType::Colorless, ObjectId(0), false, Vec::new())) + .chain((0..2).map(|_| ManaUnit::new(ManaType::Green, ObjectId(0), false, Vec::new()))) + .collect(), + ); + + let mut runner = scenario.build(); + let outcome = runner.cast(spell).resolve(); + + let entered = outcome + .find_object(|o| o.name == name && o.zone == Zone::Battlefield) + .expect("cast creature must have entered the battlefield"); + + let counters = outcome.counters(entered, CounterType::Plus1Plus1); + + // Force a full layers re-evaluation to read the haste static's current + // grant — mirrors the established convention (see + // `frostcliff_siege_anchor_word_modes.rs`) since not every action reliably + // bumps `layers_dirty` on its own; this test cares about the counters + // (the actual bug) driving the static's condition, not about dirty-bit + // plumbing. + let state = runner.state_mut(); + state.layers_dirty.mark_full(); + evaluate_layers(state); + let has_haste = runner + .state() + .objects + .get(&entered) + .is_some_and(|obj| obj.keywords.contains(&Keyword::Haste)); + (counters, has_haste) +} + +#[test] +fn runadi_grants_mv_minus_4_counters_and_downstream_haste_at_mv8() { + // MV 8: X = 8 - 4 = 4 counters, crossing the "three or more" haste + // threshold on the SAME creature. + assert_eq!( + cast_creature_with_runadi("Test Behemoth", 8), + (4, true), + "an MV8 creature must enter with 4 counters and gain haste from the \ + 3-or-more threshold; (0, false) means the ETB-counter replacement \ + never fired (issue #6492 regression)" + ); +} + +#[test] +fn runadi_grants_exactly_one_counter_at_mv5_threshold() { + // MV 5: X = 5 - 4 = 1 counter — below the haste threshold. + assert_eq!( + cast_creature_with_runadi("Test Whelp", 5), + (1, false), + "an MV5 creature (the exact threshold) must enter with exactly 1 \ + counter and not yet have haste" + ); +} + +#[test] +fn runadi_grants_no_counters_below_mv5_threshold() { + // MV 4: below the "mana value 5 or greater" filter — the replacement + // must not apply at all (proves the Cmc filter still gates correctly and + // the fix didn't turn this into an unconditional counter grant). + assert_eq!( + cast_creature_with_runadi("Test Sprite", 4), + (0, false), + "an MV4 creature must NOT receive any counters (mana value filter \ + excludes it) and must not have haste" + ); +} From 6f7275e5287ac73aab64875fae7875fd796f9d08 Mon Sep 17 00:00:00 2001 From: nghequyettien Date: Wed, 29 Jul 2026 02:38:21 +0700 Subject: [PATCH 2/3] fix(PR-6735): address review feedback --- crates/engine/src/parser/oracle.rs | 22 +- .../engine/src/parser/oracle_replacement.rs | 241 +++++++++++++----- .../runadi_behemoth_caller_etb_counters.rs | 73 +++++- 3 files changed, 268 insertions(+), 68 deletions(-) diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 41a45dacfe..85a0c45360 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -90,7 +90,7 @@ use super::oracle_modal::{ use super::oracle_replacement::{ find_copy_verb_present, lower_as_enters_becomes_choice_modal, lower_as_enters_or_face_up_counters, lower_replacement_ir, parse_replacement_line, - parse_replacement_line_ir, + parse_replacement_line_ir, parse_whenever_you_cast_enters_with_trigger, }; use super::oracle_saga::{is_saga_chapter, parse_saga_chapters}; use super::oracle_spacecraft::parse_spacecraft_threshold_lines; @@ -4887,9 +4887,6 @@ pub(crate) fn parse_oracle_ir( // are CR 614.1c replacement effects, not triggered abilities — despite // the "whenever"/"when" framing. Intercept before the generic trigger // dispatch routes them through the SpellCast / ChangesZone matcher. - // Applies to Wildgrowth Archaic and cousin cards (Runadi, Boreal - // Outrider, Torgal, Dragon Broodmother, …). `parse_replacement_line` - // handles all the compositional variants (fixed / X / "where X is …"). // // CR 603.2 exclusion: an ETB-with-counter TRIGGER ("… enters with a // counter on it, ") watches for ANY (untyped) counter and @@ -4907,6 +4904,23 @@ pub(crate) fn parse_oracle_ir( && scan_contains(&lower, "enters with") && !scan_contains(&lower, "enters this way,") { + // CR 603.1 + CR 603.3 + CR 614.1c/614.12: "Whenever you cast [spell], + // that [subject] enters with … counter(s) on it[, where X is …]" + // (Wildgrowth Archaic and cousin cards — Runadi, Boreal Outrider, + // Torgal, Dragon Broodmother, …) is a TRIGGERED ability (CR 603.1), + // not an object-hosted static replacement — the entering-with-counters + // effect must survive the source leaving the battlefield after the + // trigger resolves but before the cast spell does (issue #6492 + // review). Try this shape's dedicated trigger recognizer FIRST so it + // never falls through to the generic object-hosted replacement path. + if let Some(trigger) = parse_whenever_you_cast_enters_with_trigger(&line, card_name) { + emitter.trigger_ir_at(item_line, TriggerNodeIr::from_definition(&line, trigger)); + i += 1; + continue; + } + // Every other "… enters with …" shape here (kicker-conditional + // "if ~ was kicked, it enters with …", external "[type] enters + // with …", etc.) is a genuine CR 614.1c object-hosted replacement. if let Some(replacement_ir) = parse_replacement_line_ir(&line, card_name) { emitter.emit_at(item_line, OracleNodeIr::Replacement(replacement_ir)); i += 1; diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 89a9b741ff..4bdd755582 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -43,13 +43,14 @@ use crate::types::ability::{ PermissionGrantee, PlayerFilter, PreventionAmount, QuantityExpr, QuantityModification, QuantityRef, ReplacementCondition, ReplacementDefinition, ReplacementMode, ReplacementPlayerScope, StaticCondition, StaticDefinition, TapStateChange, TargetFilter, - TypeFilter, TypedFilter, + TriggerDefinition, TypeFilter, TypedFilter, }; use crate::types::card_type::Supertype; use crate::types::counter::{CounterMatch, CounterType}; use crate::types::mana::{ManaColor, ManaCost, ManaType}; use crate::types::replacements::ReplacementEvent; use crate::types::statics::CastFrequency; +use crate::types::triggers::TriggerMode; use crate::types::zones::Zone; /// Parse a replacement effect line into a ReplacementDefinition. @@ -698,16 +699,17 @@ fn parse_replacement_line_inner(text: &str, card_name: &str) -> Option) -> Ab /// CR 614.1c + CR 601.2: Parse "Whenever you cast a [spell], that [subject] /// enters with [an additional] [count] [type] counter(s) on it[, where X is -/// [quantity]]" as a replacement effect on the *cast spell itself*. +/// [quantity]]" into the `ChangeZone` + `PutCounter` replacement payload for +/// this shape. Wildgrowth Archaic and its cousin family (Runadi, Boreal +/// Outrider, Torgal, …) all share this shape. /// -/// Despite the "whenever you cast" framing, CR 614.1c classifies "enters with" -/// as a replacement effect, not a triggered ability. Wildgrowth Archaic and its -/// cousin family (Runadi, Boreal Outrider, Torgal, …) all share this shape. +/// CR 603.1 + CR 603.3: "whenever" is a triggered ability that goes on the +/// stack — the entering-with-counters EFFECT (CR 614.1c/614.12) only applies +/// once that ability resolves. This function builds only the reusable +/// replacement PAYLOAD (the `ChangeZone`/`PutCounter` shape keyed to the spell +/// filter); it is never returned as a top-level, object-hosted replacement. +/// `parse_whenever_you_cast_enters_with_trigger` is the actual recognizer — +/// it wraps this payload in `Effect::AddTargetReplacement { target: None, .. }` +/// so the triggered ability installs a floating, filter-scoped replacement +/// that survives the source leaving the battlefield (issue #6492 review). /// /// Composition: /// "whenever you cast " → spell filter → ", that " → subject → @@ -4862,6 +4872,60 @@ fn parse_whenever_you_cast_enters_with( ) } +/// CR 603.1 + CR 603.3 + CR 614.1c/614.12: The actual recognizer for "Whenever +/// you cast a [spell], that [subject] enters with ... counter(s) on it[, +/// where X is [quantity]]" (Wildgrowth Archaic, Runadi, Boreal Outrider, +/// Torgal, …). +/// +/// "Whenever" is a triggered ability — it goes on the stack ABOVE the spell +/// that triggered it (CR 603.3b) and resolves first. Modeling this whole +/// sentence as an object-hosted static replacement (the pre-#6492-review +/// design) is rules-wrong: the entering-with-counters effect must survive the +/// source leaving the battlefield after the trigger resolves but before the +/// cast spell does. Instead, this builds a real `TriggerDefinition` +/// (`TriggerMode::SpellCast`, matching the same spell filter) whose resolving +/// effect installs a FLOATING replacement via `Effect::AddTargetReplacement { +/// target: TargetFilter::None, .. }` — pushed to `GameState::pending_damage_replacements` +/// under the `ObjectId(0)` sentinel (see `add_target_replacement.rs`), which the +/// replacement scan (`find_applicable_replacements`) admits independent of any +/// object's zone. `consume_on_apply` makes it one-shot: it fires on the first +/// qualifying `ChangeZone`-to-battlefield event (the same spell this trigger's +/// resolution was itself created for — nothing else can interleave between a +/// trigger resolving and the spell directly below it on the stack resolving +/// without an intervening priority pass) and then self-destructs, so it never +/// lingers to affect a later, unrelated qualifying spell. +pub(crate) fn parse_whenever_you_cast_enters_with_trigger( + text: &str, + card_name: &str, +) -> Option { + let text = strip_reminder_text(text); + let normalized = replace_self_refs(&text, card_name); + let norm_lower = normalized.to_lowercase(); + + let mut replacement = parse_whenever_you_cast_enters_with(&norm_lower, &text)?; + let spell_filter = replacement.valid_card.clone()?; + // CR 614.1c: one qualifying entry, then gone — this floating install must + // never persist to affect a second, later cast of the same shape. + replacement.consume_on_apply = true; + + let install = AbilityDefinition::new( + AbilityKind::Spell, + Effect::AddTargetReplacement { + replacement: Box::new(replacement), + target: TargetFilter::None, + }, + ); + + Some( + TriggerDefinition::new(TriggerMode::SpellCast) + .valid_card(spell_filter) + .valid_target(TargetFilter::Controller) + .trigger_zones(vec![Zone::Battlefield]) + .execute(install) + .description(text.to_string()), + ) +} + /// Extract kicker-conditional prefix from "if ~ was kicked [with its {cost} kicker], it enters with..." /// Returns `(Option, remaining_text)` where remaining_text has the /// conditional prefix stripped (just "it enters with..." or the original text if no prefix). @@ -19616,38 +19680,60 @@ mod tests { ); } - /// CR 614.1c + CR 601.2h + CR 202.2: Wildgrowth Archaic's replacement line + /// CR 603.1 + CR 603.3 + CR 614.1c/614.12: Wildgrowth Archaic's ability /// ("Whenever you cast a creature spell, that creature enters with X /// additional +1/+1 counters on it, where X is the number of colors of - /// mana spent to cast it.") parses into a `ChangeZone` replacement on the - /// entering creature with a self-scoped spent-mana counter quantity. - #[test] - fn parses_wildgrowth_archaic_replacement() { + /// mana spent to cast it.") parses into a `SpellCast` TRIGGER — not an + /// object-hosted replacement (issue #6492 review: "whenever" is a + /// triggered ability per CR 603.1/603.3, so the entering-with-counters + /// effect must survive the source leaving the battlefield after the + /// trigger resolves but before the cast spell does). The trigger's + /// resolution installs a floating, one-shot `ChangeZone` replacement via + /// `Effect::AddTargetReplacement`. + #[test] + fn parses_wildgrowth_archaic_trigger() { let text = "Whenever you cast a creature spell, that creature enters with X additional +1/+1 counters on it, where X is the number of colors of mana spent to cast it."; - let def = parse_replacement_line(text, "Wildgrowth Archaic") - .expect("Wildgrowth line should parse as a replacement"); - assert_eq!(def.event, ReplacementEvent::ChangeZone); - assert_eq!(def.destination_zone, Some(Zone::Battlefield)); + let trigger = parse_whenever_you_cast_enters_with_trigger(text, "Wildgrowth Archaic") + .expect("Wildgrowth line should parse as a trigger"); + assert_eq!(trigger.mode, TriggerMode::SpellCast); + assert_eq!(trigger.valid_target, Some(TargetFilter::Controller)); - // valid_card: creature controlled by the Archaic's controller. - let TargetFilter::Typed(ref tf) = def.valid_card.as_ref().expect("valid_card set") else { - panic!("expected Typed filter, got {:?}", def.valid_card); + // valid_card: creature spell. + let TargetFilter::Typed(ref tf) = trigger.valid_card.as_ref().expect("valid_card set") + else { + panic!("expected Typed filter, got {:?}", trigger.valid_card); }; assert_eq!(tf.type_filters, vec![TypeFilter::Creature]); assert_eq!(tf.controller, Some(ControllerRef::You)); - // execute: PutCounter { target: SelfRef, count: Ref(self spent-mana colors) }. - let exec = def.execute.as_ref().expect("execute set"); + // execute: AddTargetReplacement { target: None, replacement: one-shot + // ChangeZone + PutCounter { target: SelfRef, count: Ref(self spent-mana colors) } }. + let exec = trigger.execute.as_ref().expect("execute set"); + let Effect::AddTargetReplacement { + replacement, + target, + } = &*exec.effect + else { + panic!("expected AddTargetReplacement, got {:?}", exec.effect); + }; + assert_eq!(target, &TargetFilter::None); + assert!( + replacement.consume_on_apply, + "floating install must be one-shot (CR 614.1c: one qualifying entry, then gone)" + ); + assert_eq!(replacement.event, ReplacementEvent::ChangeZone); + assert_eq!(replacement.destination_zone, Some(Zone::Battlefield)); + let put_counter = replacement.execute.as_ref().expect("execute set"); let Effect::PutCounter { counter_type, count, - target, - } = &*exec.effect + target: put_target, + } = &*put_counter.effect else { - panic!("expected PutCounter, got {:?}", exec.effect); + panic!("expected PutCounter, got {:?}", put_counter.effect); }; assert_eq!(counter_type, &CounterType::Plus1Plus1); - assert_eq!(target, &TargetFilter::SelfRef); + assert_eq!(put_target, &TargetFilter::SelfRef); assert_eq!( count, &QuantityExpr::Ref { @@ -19660,31 +19746,36 @@ mod tests { } /// Regression: a plain "Whenever you cast" trigger without an "enters with" - /// body must NOT be misrouted to the replacement path. + /// body must NOT be misrouted to either the trigger recognizer above or the + /// object-hosted replacement path. #[test] fn plain_whenever_you_cast_is_not_replacement() { let text = "Whenever you cast a creature spell, draw a card."; assert!(parse_replacement_line(text, "Filler").is_none()); + assert!(parse_whenever_you_cast_enters_with_trigger(text, "Filler").is_none()); } - /// CR 614.1c + CR 202.3 + CR 107.1: Runadi, Behemoth Caller's first ability - /// ("Whenever you cast a creature spell with mana value 5 or greater, that - /// creature enters with X additional +1/+1 counters on it, where X is its - /// mana value minus 4.") parses into a `ChangeZone` replacement scoped to - /// the entering creature (not Runadi herself — issue #6492 regression), - /// with a composite offset quantity over that creature's own mana value. + /// CR 603.1 + CR 603.3 + CR 614.1c/614.12 + CR 202.3 + CR 107.1: Runadi, + /// Behemoth Caller's first ability ("Whenever you cast a creature spell + /// with mana value 5 or greater, that creature enters with X additional + /// +1/+1 counters on it, where X is its mana value minus 4.") parses into + /// a `SpellCast` trigger (not a static replacement scoped to Runadi + /// herself — issue #6492 regression), whose resolution installs a + /// floating one-shot `ChangeZone` replacement gated on mana value >= 5, + /// with a composite offset quantity over the entering creature's own + /// mana value. #[test] - fn parses_runadi_behemoth_caller_replacement() { + fn parses_runadi_behemoth_caller_trigger() { let text = "Whenever you cast a creature spell with mana value 5 or greater, that creature enters with X additional +1/+1 counters on it, where X is its mana value minus 4."; - let def = parse_replacement_line(text, "Runadi, Behemoth Caller") - .expect("Runadi's first ability should parse as a replacement"); - assert_eq!(def.event, ReplacementEvent::ChangeZone); - assert_eq!(def.destination_zone, Some(Zone::Battlefield)); + let trigger = parse_whenever_you_cast_enters_with_trigger(text, "Runadi, Behemoth Caller") + .expect("Runadi's first ability should parse as a trigger"); + assert_eq!(trigger.mode, TriggerMode::SpellCast); + assert_eq!(trigger.valid_target, Some(TargetFilter::Controller)); - // valid_card: creature with mana value >= 5, controlled by Runadi's - // controller — NOT SelfRef (the pre-fix regression). - let TargetFilter::Typed(ref tf) = def.valid_card.as_ref().expect("valid_card set") else { - panic!("expected Typed filter, got {:?}", def.valid_card); + // valid_card: creature with mana value >= 5, controlled by Runadi's controller. + let TargetFilter::Typed(ref tf) = trigger.valid_card.as_ref().expect("valid_card set") + else { + panic!("expected Typed filter, got {:?}", trigger.valid_card); }; assert_eq!(tf.type_filters, vec![TypeFilter::Creature]); assert_eq!(tf.controller, Some(ControllerRef::You)); @@ -19700,18 +19791,35 @@ mod tests { tf.properties ); - // execute: PutCounter { target: SelfRef, count: Offset(its mana value, -4) }. - let exec = def.execute.as_ref().expect("execute set"); + // execute: AddTargetReplacement { target: None, replacement: one-shot + // ChangeZone + PutCounter { target: SelfRef, count: Offset(its mana value, -4) } }. + let exec = trigger.execute.as_ref().expect("execute set"); + let Effect::AddTargetReplacement { + replacement, + target, + } = &*exec.effect + else { + panic!("expected AddTargetReplacement, got {:?}", exec.effect); + }; + assert_eq!(target, &TargetFilter::None); + assert!( + replacement.consume_on_apply, + "floating install must be one-shot, or it would apply to a later, \ + unrelated qualifying spell too" + ); + assert_eq!(replacement.event, ReplacementEvent::ChangeZone); + assert_eq!(replacement.destination_zone, Some(Zone::Battlefield)); + let put_counter = replacement.execute.as_ref().expect("execute set"); let Effect::PutCounter { counter_type, count, - target, - } = &*exec.effect + target: put_target, + } = &*put_counter.effect else { - panic!("expected PutCounter, got {:?}", exec.effect); + panic!("expected PutCounter, got {:?}", put_counter.effect); }; assert_eq!(counter_type, &CounterType::Plus1Plus1); - assert_eq!(target, &TargetFilter::SelfRef); + assert_eq!(put_target, &TargetFilter::SelfRef); assert_eq!( count, &QuantityExpr::Offset { @@ -19728,10 +19836,10 @@ mod tests { } /// Regression: an unparseable composite quantity in the "where X is" clause - /// must still fail closed (return `None`, falling through to the generic - /// self-ETB fallback) rather than silently absorbing the condition text as - /// a garbage counter-type literal — the exact failure mode issue #6492 - /// reported before the fix. + /// must still fail closed (return `None`) rather than silently absorbing + /// the condition text as a garbage counter-type literal — the exact + /// failure mode issue #6492 reported before the fix. Checked against both + /// the internal payload builder and the public trigger recognizer. #[test] fn whenever_you_cast_enters_with_garbage_quantity_fails_closed() { let text = "Whenever you cast a creature spell with mana value 5 or greater, that creature enters with X additional +1/+1 counters on it, where X is its unrecognized nonsense value minus 4."; @@ -19740,6 +19848,10 @@ mod tests { "an unparseable quantity clause must fail this combinator closed, not \ succeed with a wrong AST" ); + assert!( + parse_whenever_you_cast_enters_with_trigger(text, "Filler").is_none(), + "the trigger recognizer must also fail closed when its payload builder does" + ); } /// Regression: "Whenever you cast" with a fixed additional counter amount @@ -19748,9 +19860,14 @@ mod tests { #[test] fn parses_fixed_count_variant() { let text = "Whenever you cast a creature spell, that creature enters with an additional +1/+1 counter on it."; - let def = parse_replacement_line(text, "Filler").expect("should parse"); - let exec = def.execute.as_ref().expect("execute set"); - let Effect::PutCounter { count, .. } = &*exec.effect else { + let trigger = + parse_whenever_you_cast_enters_with_trigger(text, "Filler").expect("should parse"); + let exec = trigger.execute.as_ref().expect("execute set"); + let Effect::AddTargetReplacement { replacement, .. } = &*exec.effect else { + panic!("expected AddTargetReplacement, got {:?}", exec.effect); + }; + let put_counter = replacement.execute.as_ref().expect("execute set"); + let Effect::PutCounter { count, .. } = &*put_counter.effect else { panic!("expected PutCounter"); }; assert_eq!(count, &QuantityExpr::Fixed { value: 1 }); diff --git a/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs index 121847e75d..54db4aab9d 100644 --- a/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs +++ b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs @@ -15,15 +15,27 @@ //! try to put counters on HERSELF, not the cast creature, and the cast //! creature would enter with 0 counters regardless of its mana value. //! +//! CR 603.1 + CR 603.3: "Whenever you cast ..." is a TRIGGERED ability — it +//! goes on the stack and resolves independently of Runadi's continued +//! presence. The first ability is modeled as a `SpellCast` trigger whose +//! resolution installs a floating (not object-hosted), one-shot `ChangeZone` +//! replacement, so the entering-with-counters effect survives Runadi leaving +//! the battlefield between the trigger resolving and the cast creature +//! resolving (maintainer review on issue #6492 / PR #6735). +//! //! CR references (verified against docs/MagicCompRules.txt): //! - CR 614.1c: "[this permanent] enters with ..." is a replacement effect. //! - CR 202.3: mana value. //! - CR 122.1a: a +1/+1 counter adds 1 to power and 1 to toughness. +//! - CR 603.3b: a resolving triggered ability functions independently of +//! its source once it exists on the stack. //! //! Discrimination: a mana-value-8 creature must enter with 4 counters (8-4) //! and gain Haste from the second ability's 3-or-more threshold; a //! mana-value-4 creature must enter with 0 counters (filter excludes it) and -//! no Haste. +//! no Haste; a mana-value-8 creature must still enter with 4 counters even +//! when Runadi leaves the battlefield after the qualifying spell is cast but +//! before it resolves. use engine::game::layers::evaluate_layers; use engine::game::scenario::{GameScenario, P0}; @@ -116,7 +128,9 @@ fn runadi_grants_exactly_one_counter_at_mv5_threshold() { fn runadi_grants_no_counters_below_mv5_threshold() { // MV 4: below the "mana value 5 or greater" filter — the replacement // must not apply at all (proves the Cmc filter still gates correctly and - // the fix didn't turn this into an unconditional counter grant). + // the fix didn't turn this into an unconditional counter grant, and + // doubles as "no qualifying spell → no floating replacement installed" + // coverage: the trigger never fires, so nothing is affected). assert_eq!( cast_creature_with_runadi("Test Sprite", 4), (0, false), @@ -124,3 +138,58 @@ fn runadi_grants_no_counters_below_mv5_threshold() { excludes it) and must not have haste" ); } + +/// CR 603.1 + CR 603.3b + CR 614.1c/614.12: Runadi's ability is a TRIGGERED +/// ability — once her trigger exists on the stack (queued the moment the +/// qualifying spell is cast), it resolves independently of whether Runadi +/// herself is still around. Removing her from the battlefield after the cast +/// commits (her trigger has been created and stacked above the spell) but +/// before the whole stack resolves must NOT prevent the entering creature +/// from getting its counters — the floating replacement the trigger installs +/// is source-independent, unlike an object-hosted static replacement, which +/// would have vanished with Runadi (the pre-review design this test guards +/// against regressing to). +#[test] +fn runadi_leaving_before_spell_resolves_still_grants_counters() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let runadi = scenario + .add_creature_from_oracle(P0, "Runadi, Behemoth Caller", 1, 3, RUNADI) + .id(); + + let mana_value = 8u32; + let generic = mana_value.saturating_sub(2); + let spell = scenario + .add_creature_to_hand_from_oracle(P0, "Test Behemoth", 1, 1, "") + .with_mana_cost(ManaCost::Cost { + shards: vec![ManaCostShard::Green, ManaCostShard::Green], + generic, + }) + .id(); + scenario.with_mana_pool( + P0, + (0..generic) + .map(|_| ManaUnit::new(ManaType::Colorless, ObjectId(0), false, Vec::new())) + .chain((0..2).map(|_| ManaUnit::new(ManaType::Green, ObjectId(0), false, Vec::new()))) + .collect(), + ); + + let mut runner = scenario.build(); + let mut commit = runner.cast(spell).commit(); + // Runadi leaves the battlefield WHILE the spell — and her own trigger, + // already queued by the cast — are still unresolved on the stack. + commit.state_mut().objects.get_mut(&runadi).unwrap().zone = Zone::Graveyard; + let outcome = commit.resolve(); + + let entered = outcome + .find_object(|o| o.name == "Test Behemoth" && o.zone == Zone::Battlefield) + .expect("cast creature must have entered the battlefield"); + assert_eq!( + outcome.counters(entered, CounterType::Plus1Plus1), + 4, + "the entering creature must still get its mana-value-minus-4 counters \ + even though Runadi left the battlefield before the spell resolved — \ + the floating replacement her trigger installs must not depend on her \ + continued presence (issue #6492 maintainer review)" + ); +} From 3ddbe75db323f6b4957285bc5319772f049d5cf7 Mon Sep 17 00:00:00 2001 From: nghequyettien Date: Wed, 29 Jul 2026 14:01:51 +0700 Subject: [PATCH 3/3] fix(PR-6735): bind Runadi's floating replacement to its triggering spell The one-shot floating replacement Runadi's SpellCast trigger installs was scoped only by type/mana-value filter, so a different qualifying creature entering the battlefield during the priority window between the trigger resolving and the originally-cast spell resolving could consume it first, leaving the intended entrant uncountered. Bind the install to the specific spell that caused the trigger: the parser embeds a TRIGGERING_SPELL_PLACEHOLDER sentinel inside the replacement's valid_card (AND-combined with the existing filter), and Effect::AddTargetReplacement's resolve function concretizes it to the real triggering spell's id from the current trigger event at install time (or to a match-nothing id if none is extractable). Using a sentinel object id instead of a new ReplacementDefinition field avoids rippling a struct change through every exhaustive construction site, including the dormant mtgish-import crate. Also address review nits: the "Runadi leaves before the spell resolves" regression now drives the departure through the production zone-change pipeline instead of poking the object's zone field directly, and the negative mana-value test asserts the trigger actually registered before checking the zero-counter outcome. Added a new regression that casts two qualifying creatures back to back, proving each entrant gets only its own counters. --- .../game/effects/add_target_replacement.rs | 76 ++++++- .../engine/src/parser/oracle_replacement.rs | 56 ++++- crates/engine/src/types/identifiers.rs | 15 ++ .../runadi_behemoth_caller_etb_counters.rs | 191 +++++++++++++++++- 4 files changed, 326 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/game/effects/add_target_replacement.rs b/crates/engine/src/game/effects/add_target_replacement.rs index fa3d885445..bee07c2fbd 100644 --- a/crates/engine/src/game/effects/add_target_replacement.rs +++ b/crates/engine/src/game/effects/add_target_replacement.rs @@ -1,4 +1,4 @@ -use crate::game::targeting::resolve_event_context_target; +use crate::game::targeting::{extract_source_from_event, resolve_event_context_target}; use crate::types::ability::{ AbilityDefinition, DamageTargetFilter, DamageTargetPlayerScope, Duration, Effect, EffectError, EffectKind, ReplacementCondition, ReplacementDefinition, ResolvedAbility, RestrictionExpiry, @@ -6,6 +6,7 @@ use crate::types::ability::{ }; use crate::types::events::GameEvent; use crate::types::game_state::GameState; +use crate::types::identifiers::ObjectId; use crate::types::replacements::ReplacementEvent; pub(crate) fn expiry_from_duration( @@ -47,6 +48,76 @@ fn replacement_with_ability_expiry( replacement } +/// CR 603.2 + CR 603.3b + CR 117.3b: Concretize +/// `TRIGGERING_SPELL_PLACEHOLDER` — the parse-time sentinel +/// `parse_whenever_you_cast_enters_with_trigger` embeds inside a floating +/// (`TargetFilter::None`) replacement's `valid_card` — to the SPECIFIC spell +/// object referenced by the currently-resolving triggered ability's own +/// originating event (Runadi, Behemoth Caller and the Wildgrowth Archaic +/// cousin family — issue #6492 review). +/// +/// Without this, a bare type/mana-value filter would let a DIFFERENT +/// qualifying creature — cast by the active player during the CR 117.3b +/// priority window between this trigger resolving and the originally-cast +/// spell resolving — consume the one-shot install first, leaving the intended +/// entrant uncountered. `state.current_trigger_event` is exactly this +/// ability's own trigger event (set by `push_resolving_trigger_context` for +/// the duration of its resolution — see `game/triggers.rs`), so +/// `extract_source_from_event` yields the specific cast spell's `ObjectId`. +/// +/// If the event carries no extractable source, this fails CLOSED — matching +/// no object via the `ObjectId(0)` sentinel (never a real permanent) — rather +/// than silently widening back to the bare filter, which would reopen the +/// exact bug this binding exists to close. +/// +/// No-op for every other floating-replacement install (Kaya's until-EOT token +/// doubler, Rankle and Torbran's damage-modification shields): none of them +/// ever embed the placeholder, so the walk finds nothing to replace. +fn bind_replacement_to_trigger_source(replacement: &mut ReplacementDefinition, state: &GameState) { + let Some(valid_card) = replacement.valid_card.as_mut() else { + return; + }; + if !target_filter_contains_placeholder(valid_card) { + return; + } + let bound = state + .current_trigger_event + .as_ref() + .and_then(extract_source_from_event) + .unwrap_or(ObjectId(0)); + concretize_triggering_spell_placeholder(valid_card, bound); +} + +fn target_filter_contains_placeholder(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::SpecificObject { id } => { + *id == crate::types::identifiers::TRIGGERING_SPELL_PLACEHOLDER + } + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().any(target_filter_contains_placeholder) + } + TargetFilter::Not { filter } => target_filter_contains_placeholder(filter), + _ => false, + } +} + +fn concretize_triggering_spell_placeholder(filter: &mut TargetFilter, bound: ObjectId) { + match filter { + TargetFilter::SpecificObject { id } + if *id == crate::types::identifiers::TRIGGERING_SPELL_PLACEHOLDER => + { + *id = bound; + } + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + for f in filters.iter_mut() { + concretize_triggering_spell_placeholder(f, bound); + } + } + TargetFilter::Not { filter } => concretize_triggering_spell_placeholder(filter, bound), + _ => {} + } +} + // CR 614.12a + CR 707.2: If the resolving spell chose the object to copy, bind // that object into the delayed enter-as-copy replacement when the shield is // created so the later entry event does not ask for a new copy source. @@ -237,7 +308,8 @@ pub fn resolve( // Slaughter's "If a source you control would deal damage this turn, // it deals that much damage plus 1 instead."). if matches!(target, TargetFilter::None) { - let replacement = replacement_with_ability_expiry(replacement, ability); + let mut replacement = replacement_with_ability_expiry(replacement, ability); + bind_replacement_to_trigger_source(&mut replacement, state); state.pending_damage_replacements.push(replacement); attached += 1; } else { diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 4bdd755582..3b7bb0a50c 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -4888,12 +4888,20 @@ fn parse_whenever_you_cast_enters_with( /// target: TargetFilter::None, .. }` — pushed to `GameState::pending_damage_replacements` /// under the `ObjectId(0)` sentinel (see `add_target_replacement.rs`), which the /// replacement scan (`find_applicable_replacements`) admits independent of any -/// object's zone. `consume_on_apply` makes it one-shot: it fires on the first -/// qualifying `ChangeZone`-to-battlefield event (the same spell this trigger's -/// resolution was itself created for — nothing else can interleave between a -/// trigger resolving and the spell directly below it on the stack resolving -/// without an intervening priority pass) and then self-destructs, so it never -/// lingers to affect a later, unrelated qualifying spell. +/// object's zone. `consume_on_apply` makes it one-shot. +/// +/// CR 117.3b: after Runadi's trigger resolves, the active player receives +/// priority BEFORE the originally-cast spell resolves, and could cast a +/// second qualifying flash creature in response — a bare filter-scoped +/// one-shot install would let that INTERLOPING spell's battlefield entry +/// consume the replacement first, leaving the original entrant uncountered. +/// This is closed by AND-ing the spell filter with +/// `TargetFilter::SpecificObject { id: TRIGGERING_SPELL_PLACEHOLDER }` — a +/// parse-time placeholder id that `Effect::AddTargetReplacement`'s resolve +/// function (`add_target_replacement.rs`) concretizes to the SPECIFIC spell +/// object named by `state.current_trigger_event` (this trigger's own +/// originating `SpellCast` event — CR 603.2) at install time, so only that +/// exact spell's entry can ever satisfy it. pub(crate) fn parse_whenever_you_cast_enters_with_trigger( text: &str, card_name: &str, @@ -4907,6 +4915,20 @@ pub(crate) fn parse_whenever_you_cast_enters_with_trigger( // CR 614.1c: one qualifying entry, then gone — this floating install must // never persist to affect a second, later cast of the same shape. replacement.consume_on_apply = true; + // CR 603.2 + CR 117.3b: bind to the SPECIFIC spell that caused this trigger, + // not just any spell matching the type/mana-value filter — see the + // interleaving-flash-creature note in the doc comment above. + // `TRIGGERING_SPELL_PLACEHOLDER` is concretized to the real triggering + // spell's id (or `ObjectId(0)`, matching nothing) by + // `Effect::AddTargetReplacement`'s resolve function at install time. + replacement.valid_card = Some(TargetFilter::And { + filters: vec![ + spell_filter.clone(), + TargetFilter::SpecificObject { + id: crate::types::identifiers::TRIGGERING_SPELL_PLACEHOLDER, + }, + ], + }); let install = AbilityDefinition::new( AbilityKind::Spell, @@ -19807,6 +19829,28 @@ mod tests { "floating install must be one-shot, or it would apply to a later, \ unrelated qualifying spell too" ); + // valid_card must AND the spell filter with a `SpecificObject` leaf + // carrying the trigger-source placeholder — `Effect::AddTargetReplacement` + // concretizes this to the SPECIFIC triggering spell's id at install + // time, so a different qualifying creature entering during the + // post-trigger priority window can't steal the install. + let TargetFilter::And { filters } = + replacement.valid_card.as_ref().expect("valid_card set") + else { + panic!( + "expected valid_card to be an And{{spell filter, trigger-source \ + placeholder}}, got {:?}", + replacement.valid_card + ); + }; + assert!( + filters.iter().any(|f| matches!( + f, + TargetFilter::SpecificObject { id } + if *id == crate::types::identifiers::TRIGGERING_SPELL_PLACEHOLDER + )), + "valid_card must carry the trigger-source placeholder, got {filters:?}" + ); assert_eq!(replacement.event, ReplacementEvent::ChangeZone); assert_eq!(replacement.destination_zone, Some(Zone::Battlefield)); let put_counter = replacement.execute.as_ref().expect("execute set"); diff --git a/crates/engine/src/types/identifiers.rs b/crates/engine/src/types/identifiers.rs index d593728738..abc8bade2f 100644 --- a/crates/engine/src/types/identifiers.rs +++ b/crates/engine/src/types/identifiers.rs @@ -11,6 +11,21 @@ pub struct CardId(pub u64); #[serde(transparent)] pub struct ObjectId(pub u64); +/// CR 603.2 + CR 603.3b + CR 117.3b: parse-time placeholder for "the specific +/// spell object that will cause this trigger to fire", embedded inside a +/// `TargetFilter::SpecificObject` leaf of a floating (`TargetFilter::None`) +/// replacement's `valid_card` tree by `parse_whenever_you_cast_enters_with_trigger`. +/// `Effect::AddTargetReplacement`'s resolve function (`add_target_replacement.rs`) +/// concretizes this to the real triggering spell's id (from +/// `state.current_trigger_event`) — or to `ObjectId(0)` (matches nothing) if +/// none is extractable — before the install is pushed. Never a real object's +/// id (the allocator starts well below `u64::MAX`), so this is safe to use as +/// a sentinel without a dedicated `TargetFilter`/`ReplacementDefinition` +/// variant, which would ripple through every exhaustive match on those types +/// across the workspace (including the dormant `mtgish-import` crate, which +/// must remain untouched). +pub(crate) const TRIGGERING_SPELL_PLACEHOLDER: ObjectId = ObjectId(u64::MAX); + /// Monotonic identity for one logical simultaneous zone-change action. /// /// This remains distinct from an [`ObjectId`]: a logical group can contain diff --git a/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs index 54db4aab9d..053b1a991a 100644 --- a/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs +++ b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs @@ -38,11 +38,15 @@ //! before it resolves. use engine::game::layers::evaluate_layers; -use engine::game::scenario::{GameScenario, P0}; +use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::game::triggers::drain_order_triggers_with_identity; +use engine::types::actions::GameAction; use engine::types::counter::CounterType; +use engine::types::game_state::{CastPaymentMode, WaitingFor}; use engine::types::keywords::Keyword; use engine::types::mana::{ManaCost, ManaCostShard, ManaType, ManaUnit}; use engine::types::phase::Phase; +use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; use engine::types::ObjectId; @@ -55,7 +59,9 @@ fn cast_creature_with_runadi(name: &str, mana_value: u32) -> (u32, bool) { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); - scenario.add_creature_from_oracle(P0, "Runadi, Behemoth Caller", 1, 3, RUNADI); + let runadi = scenario + .add_creature_from_oracle(P0, "Runadi, Behemoth Caller", 1, 3, RUNADI) + .id(); let generic = mana_value.saturating_sub(2); let spell = scenario @@ -75,6 +81,26 @@ fn cast_creature_with_runadi(name: &str, mana_value: u32) -> (u32, bool) { ); let mut runner = scenario.build(); + + // Positive reach guard (CodeRabbit review): prove Runadi's first ability + // actually registered as a real `SpellCast` trigger on the permanent + // BEFORE casting anything, so a (0, false) result below is attributable to + // the mana-value filter rejecting the spell, not to the trigger having + // silently failed to parse/attach for every mana value. + assert!( + runner + .state() + .objects + .get(&runadi) + .expect("Runadi must be on the battlefield") + .trigger_definitions + .as_slice() + .iter() + .any(|entry| entry.definition.mode == TriggerMode::SpellCast), + "Runadi's first ability must register as a SpellCast trigger — a missing \ + registration would make every mana value silently give (0, false)" + ); + let outcome = runner.cast(spell).resolve(); let entered = outcome @@ -177,8 +203,16 @@ fn runadi_leaving_before_spell_resolves_still_grants_counters() { let mut runner = scenario.build(); let mut commit = runner.cast(spell).commit(); // Runadi leaves the battlefield WHILE the spell — and her own trigger, - // already queued by the cast — are still unresolved on the stack. - commit.state_mut().objects.get_mut(&runadi).unwrap().zone = Zone::Graveyard; + // already queued by the cast — are still unresolved on the stack. Drive + // this through the production zone-change pipeline (`game::zones::move_to_zone`) + // rather than poking `GameObject.zone` directly, so the departure is a real + // CR 400.7 zone change, not a test-only shortcut. + engine::game::zones::move_to_zone(commit.state_mut(), runadi, Zone::Graveyard, &mut Vec::new()); + assert_eq!( + commit.state().objects.get(&runadi).map(|o| o.zone), + Some(Zone::Graveyard), + "Runadi must actually be in the graveyard before the stack resolves" + ); let outcome = commit.resolve(); let entered = outcome @@ -193,3 +227,152 @@ fn runadi_leaving_before_spell_resolves_still_grants_counters() { continued presence (issue #6492 maintainer review)" ); } + +fn cast_spell(runner: &mut GameRunner, spell: ObjectId) { + let card_id = runner + .state() + .objects + .get(&spell) + .expect("spell object exists") + .card_id; + runner + .act(GameAction::CastSpell { + object_id: spell, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("cast must be accepted"); +} + +/// Pass priority (draining any trigger-ordering prompt) until the stack has +/// exactly `len` items. Runadi's trigger has no target to select, so the only +/// prompts this scenario can surface are `Priority` and `OrderTriggers`. +fn pass_priority_until_stack_len(runner: &mut GameRunner, len: usize) { + for _ in 0..64 { + if runner.state().stack.len() == len { + return; + } + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } => { + runner + .act(GameAction::PassPriority) + .expect("pass priority must be accepted"); + } + WaitingFor::OrderTriggers { .. } => { + drain_order_triggers_with_identity(runner.state_mut()); + } + other => panic!("unexpected WaitingFor while pumping to stack length {len}: {other:?}"), + } + } + panic!( + "stack never reached length {len} (currently {})", + runner.state().stack.len() + ); +} + +/// CR 603.2 + CR 117.3b + CR 614.1c/614.12: The floating replacement Runadi's +/// trigger installs must be bound to the SPECIFIC spell that caused it, not +/// just any spell matching the "creature, mana value >= 5" filter — reviewer +/// finding on PR #6735. Cast a first qualifying creature (A), let its trigger +/// resolve (installing a floating replacement bound to A), then cast a SECOND +/// qualifying creature (B) in response — during the CR 117.3b priority window +/// before A resolves — and let everything resolve. B's own trigger installs a +/// second floating replacement bound to B; B resolves first (LIFO) and must +/// get exactly its own counters, NOT steal/consume the replacement meant for +/// A. A must then still resolve with its own correct counters. +/// +/// Revert-to-red: a bare filter-scoped one-shot install (no +/// `bind_to_trigger_source`) lets B's earlier battlefield entry consume A's +/// still-pending floating replacement (insertion-order-first in +/// `pending_damage_replacements`), leaving A uncountered when it resolves. +#[test] +fn runadi_binds_floating_replacement_to_the_specific_triggering_spell() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_creature_from_oracle(P0, "Runadi, Behemoth Caller", 1, 3, RUNADI); + + // Both A and B are MV8 (X = 4 counters each) so a correct result is + // symmetric and unambiguous: (4, 4), never (0, 8) or (8, 0) from a stolen + // replacement. + let generic = 8u32.saturating_sub(2); + // CR 117.1a: creature spells are normally sorcery-speed only. Flash lets B + // be cast in response, on the stack above A — the exact CR 117.3b window + // the maintainer's review calls out. + let make_spell = |scenario: &mut GameScenario, name: &str| { + scenario + .add_creature_to_hand_from_oracle(P0, name, 1, 1, "Flash") + .with_mana_cost(ManaCost::Cost { + shards: vec![ManaCostShard::Green, ManaCostShard::Green], + generic, + }) + .id() + }; + let spell_a = make_spell(&mut scenario, "Test Behemoth A"); + let spell_b = make_spell(&mut scenario, "Test Behemoth B"); + + // Ample combined pool for both GG+6-generic casts. + scenario.with_mana_pool( + P0, + (0..generic * 2) + .map(|_| ManaUnit::new(ManaType::Colorless, ObjectId(0), false, Vec::new())) + .chain((0..4).map(|_| ManaUnit::new(ManaType::Green, ObjectId(0), false, Vec::new()))) + .collect(), + ); + + let mut runner = scenario.build(); + + cast_spell(&mut runner, spell_a); + // Stack: [A, triggerA]. Let triggerA resolve (installs the floating + // replacement bound to A), leaving just A on the stack. + pass_priority_until_stack_len(&mut runner, 1); + + // Cast B IN RESPONSE, while A is still on the stack unresolved. + cast_spell(&mut runner, spell_b); + // Stack: [A, B, triggerB]. Let triggerB resolve (installs the floating + // replacement bound to B), leaving [A, B]. + pass_priority_until_stack_len(&mut runner, 2); + + // Resolve the rest of the stack: B resolves first (LIFO), then A. + pass_priority_until_stack_len(&mut runner, 0); + + let state = runner.state(); + let entered_a = state + .objects + .values() + .find(|o| o.name == "Test Behemoth A" && o.zone == Zone::Battlefield) + .map(|o| o.id) + .expect("Test Behemoth A must have entered the battlefield"); + let entered_b = state + .objects + .values() + .find(|o| o.name == "Test Behemoth B" && o.zone == Zone::Battlefield) + .map(|o| o.id) + .expect("Test Behemoth B must have entered the battlefield"); + + let counters_a = state + .objects + .get(&entered_a) + .unwrap() + .counters + .get(&CounterType::Plus1Plus1) + .copied() + .unwrap_or(0); + let counters_b = state + .objects + .get(&entered_b) + .unwrap() + .counters + .get(&CounterType::Plus1Plus1) + .copied() + .unwrap_or(0); + + assert_eq!( + (counters_a, counters_b), + (4, 4), + "each entrant must get its OWN 4 counters; anything else means the \ + floating replacement was stolen by (or applied to) the wrong spell — \ + (0, 8) or (8, 0) means B's earlier entry consumed the replacement \ + meant for A" + ); +}