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/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.rs b/crates/engine/src/parser/oracle.rs index ae06345c01..7fb0a1c821 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; @@ -5044,9 +5044,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 @@ -5064,6 +5061,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 a74fc1f2e6..3b7bb0a50c 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 → @@ -4828,21 +4838,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( @@ -4865,6 +4872,82 @@ 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. +/// +/// 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, +) -> 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; + // 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, + 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). @@ -19619,38 +19702,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 { @@ -19663,11 +19768,134 @@ 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 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_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 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. + 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)); + 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: 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" + ); + // 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"); + let Effect::PutCounter { + counter_type, + count, + target: put_target, + } = &*put_counter.effect + else { + panic!("expected PutCounter, got {:?}", put_counter.effect); + }; + assert_eq!(counter_type, &CounterType::Plus1Plus1); + assert_eq!(put_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`) 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."; + 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" + ); + 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 @@ -19676,9 +19904,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/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/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/main.rs b/crates/engine/tests/integration/main.rs index 7e430108e7..46216a29fa 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -814,6 +814,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..053b1a991a --- /dev/null +++ b/crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs @@ -0,0 +1,378 @@ +//! 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 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; 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::{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; + +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); + + 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 + .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(); + + // 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 + .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, 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), + "an MV4 creature must NOT receive any counters (mana value filter \ + 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. 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 + .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)" + ); +} + +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" + ); +}