diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index bf4e2b174c..2b2e5acbc2 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -238,12 +238,53 @@ pub(crate) fn resolve_it_pronoun(ctx: &mut ParseContext) -> TargetFilter { } } +/// CR 122.1 + CR 608.2k: true when the expression reads a counter total on the +/// ability's own source object ("counters on ~" / "counters on this land"). +/// Recognizes `CountersOn { scope: Source }` through the arithmetic wrappers so +/// a wrapped threshold still registers as source-referential. Mirrors the +/// exhaustive wrapper walk of `quantity_expr_uses_recipient` +/// (`game/quantity.rs`) so the compiler flags any future `QuantityExpr` variant. +fn quantity_expr_reads_source_counters(expr: &QuantityExpr) -> bool { + match expr { + QuantityExpr::Fixed { .. } => false, + QuantityExpr::Ref { qty } => matches!( + qty, + QuantityRef::CountersOn { + scope: ObjectScope::Source, + .. + } + ), + QuantityExpr::DivideRounded { inner, .. } + | QuantityExpr::Offset { inner, .. } + | QuantityExpr::ClampMin { inner, .. } + | QuantityExpr::Multiply { inner, .. } => quantity_expr_reads_source_counters(inner), + QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => { + exprs.iter().any(quantity_expr_reads_source_counters) + } + QuantityExpr::UpTo { max } => quantity_expr_reads_source_counters(max), + QuantityExpr::Power { exponent, .. } => quantity_expr_reads_source_counters(exponent), + QuantityExpr::Difference { left, right } => { + quantity_expr_reads_source_counters(left) || quantity_expr_reads_source_counters(right) + } + } +} + fn condition_refs_source_object(condition: &AbilityCondition) -> bool { match condition { AbilityCondition::SourceMatchesFilter { .. } | AbilityCondition::SourceEnteredThisTurn | AbilityCondition::SourceIsTapped | AbilityCondition::SourceAttachedToCreature => true, + // CR 122.1: a counter-threshold gate scoped to the SOURCE + // ("if there are no mining counters on this land", "if it has six or + // more quest counters on it") refers to the ability's own untargeted + // source object, so a bare "it" in the gated body anaphors to the + // source permanent — Gemstone Mine / the Mercadian depletion lands + // (issue #6507), Tourach's Gate, Daredevil Dragster, Last Light of + // Durin's Day. + AbilityCondition::QuantityCheck { lhs, rhs, .. } => { + quantity_expr_reads_source_counters(lhs) || quantity_expr_reads_source_counters(rhs) + } AbilityCondition::Not { condition } | AbilityCondition::ConditionInstead { inner: condition } => { condition_refs_source_object(condition) @@ -255,6 +296,61 @@ fn condition_refs_source_object(condition: &AbilityCondition) -> bool { } } +/// CR 122.1 + CR 608.2c: Bind a source-defaulted counter condition to the +/// prior chosen target only for the leading bare-pronoun grammar. Conditions +/// belong to `ClauseIr` until lowering, so this is the single authority before +/// a continuous grant receives its final `StaticCondition`. +fn rebind_source_counter_condition_to_recipient(condition: &mut AbilityCondition) { + match condition { + AbilityCondition::QuantityCheck { lhs, rhs, .. } => { + rebind_source_counter_quantity_expr_to_recipient(lhs); + rebind_source_counter_quantity_expr_to_recipient(rhs); + } + AbilityCondition::Not { condition } + | AbilityCondition::ConditionInstead { inner: condition } => { + rebind_source_counter_condition_to_recipient(condition); + } + AbilityCondition::And { conditions } | AbilityCondition::Or { conditions } => { + for condition in conditions { + rebind_source_counter_condition_to_recipient(condition); + } + } + _ => {} + } +} + +fn rebind_source_counter_quantity_expr_to_recipient(expr: &mut QuantityExpr) { + match expr { + QuantityExpr::Fixed { .. } => {} + QuantityExpr::Ref { qty } => { + if let QuantityRef::CountersOn { scope, .. } = qty { + if *scope == ObjectScope::Source { + *scope = ObjectScope::Recipient; + } + } + } + QuantityExpr::DivideRounded { inner, .. } + | QuantityExpr::Offset { inner, .. } + | QuantityExpr::ClampMin { inner, .. } + | QuantityExpr::Multiply { inner, .. } => { + rebind_source_counter_quantity_expr_to_recipient(inner); + } + QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => { + for expr in exprs { + rebind_source_counter_quantity_expr_to_recipient(expr); + } + } + QuantityExpr::UpTo { max } => rebind_source_counter_quantity_expr_to_recipient(max), + QuantityExpr::Power { exponent, .. } => { + rebind_source_counter_quantity_expr_to_recipient(exponent); + } + QuantityExpr::Difference { left, right } => { + rebind_source_counter_quantity_expr_to_recipient(left); + rebind_source_counter_quantity_expr_to_recipient(right); + } + } +} + fn condition_refs_cost_paid_object(condition: &AbilityCondition) -> bool { match condition { AbilityCondition::CostPaidObjectMatchesFilter { .. } => true, @@ -28769,6 +28865,10 @@ pub(crate) fn parse_effect_chain_ir( if normalized_text.is_empty() { continue; } + let has_bare_recipient_counter_gate = + crate::parser::oracle_nom::condition::is_leading_if_bare_recipient_counter_condition( + normalized_text, + ); if lower::recognize_zada_copy_distinct_target_rider(&normalized_text.to_ascii_lowercase()) { continue; } @@ -30130,13 +30230,22 @@ pub(crate) fn parse_effect_chain_ir( } else { (None, text) }; - let condition = match (condition, unless_same_name_condition) { + let mut condition = match (condition, unless_same_name_condition) { (Some(existing), Some(unless_cond)) => Some(AbilityCondition::And { conditions: vec![existing, unless_cond], }), (None, Some(unless_cond)) => Some(unless_cond), (existing, None) => existing, }; + let prior_typed_referent = chain_has_prior_typed_referent(builder.clauses(), false); + if prior_typed_referent + && has_bare_recipient_counter_gate + && condition.as_ref().is_some_and(condition_refs_source_object) + { + rebind_source_counter_condition_to_recipient( + condition.as_mut().expect("condition checked above"), + ); + } // CR 701.34a + CR 122.1: keep the whole "for each kind of counter on // target permanent or player, give … another counter of that kind" // clause intact so the targeted-proliferate recognizer in @@ -30474,7 +30583,14 @@ pub(crate) fn parse_effect_chain_ir( } .or_else(|| ctx.actor.clone()); let if_you_do_anchor = if_you_do_object_anchor(builder.clauses(), &condition); - let chunk_subject = if condition.as_ref().is_some_and(condition_refs_source_object) { + // CR 608.2k: An `AbilityCondition` source-counter gate binds a bare + // body pronoun to the source only when no prior clause chose a typed + // target. This preserves the depletion-land / counter-rider class; + // the leading bare-recipient gate is instead rebound on `condition` + // before lowering above. + let binds_source_counter_pronoun = + condition.as_ref().is_some_and(condition_refs_source_object) && !prior_typed_referent; + let chunk_subject = if binds_source_counter_pronoun { Some(TargetFilter::SelfRef) } else { if_you_do_anchor.clone().or_else(|| ctx.subject.clone()) @@ -30487,9 +30603,8 @@ pub(crate) fn parse_effect_chain_ir( // this flag when the outer chain has such a referent (Brilliance Unleashed). // It is false on every top-level and non-else nested parse, so this OR is a // no-op for all pre-existing cards. - let parent_target_available = ctx.parent_target_available - || if_you_do_anchor.is_some() - || chain_has_prior_typed_referent(builder.clauses(), false); + let parent_target_available = + ctx.parent_target_available || if_you_do_anchor.is_some() || prior_typed_referent; // CR 608.2c + CR 601.2a: a strict subset of `parent_target_available` // restricted to chosen-target referents (Emry), excluding impulse // publishers (Territorial Bruntar's `ExileFromTopUntil`). An "if you @@ -31266,7 +31381,11 @@ pub(crate) fn parse_effect_chain_ir( // which is scoped to exactly the reported bug class. if condition.is_some() && !is_distributed_chunk - && !condition.as_ref().is_some_and(condition_refs_source_object) + // CR 608.2k: only a GENUINE source-counter gate (no prior chosen + // target — see `binds_source_counter_pronoun`) keeps the SelfRef + // binding; a mis-scoped bare "it" over a prior typed target + // (Revelation of Power) still rewrites to the parent target here. + && !binds_source_counter_pronoun && !builder.is_empty() && has_anaphoric_reference(&text_lower) && !matches!(if_you_do_anchor, Some(TargetFilter::SelfRef)) diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 53e3af4358..e14a5c0b42 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -29361,6 +29361,146 @@ fn resolve_it_pronoun_any_subject() { assert_eq!(resolve_it_pronoun(&mut ctx), TargetFilter::SelfRef); } +/// Issue #6507 (CR 122.1): `condition_refs_source_object` must +/// recognize a source-scoped counter-threshold `QuantityCheck` ("if there are +/// no mining counters on this land") as source-referential — that gate is what +/// threads `SelfRef` as the chunk subject so the rider's bare "it" binds to +/// the source. Adjacent-variant hostiles: `Target`/`Recipient`-scoped counter +/// reads (the deliberately excluded "if that creature has … counters" class) +/// must stay non-source-referential. +#[test] +fn condition_refs_source_object_source_counter_quantity_check() { + fn counters_check(scope: ObjectScope) -> AbilityCondition { + AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope, + counter_type: Some(crate::types::counter::parse_counter_type("mining")), + }, + }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Fixed { value: 0 }, + } + } + + // Positive: source-scoped counter threshold (the Gemstone Mine class). + assert!(condition_refs_source_object(&counters_check( + ObjectScope::Source + ))); + // Wrapped-expression positive: the walker must see through arithmetic + // wrappers (`Offset { CountersOn { Source } }` still reads the source). + assert!(condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Offset { + inner: Box::new(QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: None, + }, + }), + offset: 1, + }, + comparator: Comparator::GE, + rhs: QuantityExpr::Fixed { value: 2 }, + } + )); + // Existing wrapper recursion still applies to the new arm. + assert!(condition_refs_source_object(&AbilityCondition::Not { + condition: Box::new(counters_check(ObjectScope::Source)), + })); + assert!(condition_refs_source_object(&AbilityCondition::And { + conditions: vec![ + AbilityCondition::IsYourTurn, + counters_check(ObjectScope::Source), + ], + })); + // `QuantityCheck` must inspect both sides of the comparison, not only the + // customary left-hand counter threshold. + assert!(condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Fixed { value: 0 }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: None, + }, + }, + } + )); + + // Adjacent-variant hostiles: target-/recipient-scoped counter reads keep + // their current (non-source) binding. + assert!(!condition_refs_source_object(&counters_check( + ObjectScope::Target + ))); + assert!(!condition_refs_source_object(&counters_check( + ObjectScope::Recipient + ))); + // A QuantityCheck with no counter read at all stays non-source-referential. + assert!(!condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::HandSize { + player: PlayerScope::Controller, + }, + }, + comparator: Comparator::GE, + rhs: QuantityExpr::Fixed { value: 7 }, + } + )); + + // Two-operand wrapper arms: the walker must recurse into BOTH branches so a + // source-counter read hidden in either operand still registers, and a + // wrapper with neither operand reading the source stays negative. These + // arms were previously undriven by this test (only Ref/Offset were). + let source_read = QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: None, + }, + }; + // Sum { exprs } — recurse via `.any(..)`: source read in one operand → true. + assert!(condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Sum { + exprs: vec![QuantityExpr::Fixed { value: 1 }, source_read.clone()], + }, + comparator: Comparator::GE, + rhs: QuantityExpr::Fixed { value: 2 }, + } + )); + // Difference { left, right } — recurse via `left || right`: source read on + // the right operand alone still registers. + assert!(condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Difference { + left: Box::new(QuantityExpr::Fixed { value: 3 }), + right: Box::new(source_read.clone()), + }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Fixed { value: 0 }, + } + )); + // Neither-operand-source Sum stays negative (no false positive). + assert!(!condition_refs_source_object( + &AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Sum { + exprs: vec![ + QuantityExpr::Fixed { value: 1 }, + QuantityExpr::Ref { + qty: QuantityRef::HandSize { + player: PlayerScope::Controller, + }, + }, + ], + }, + comparator: Comparator::GE, + rhs: QuantityExpr::Fixed { value: 2 }, + } + )); +} + // --- Suffix condition extraction tests --- #[test] diff --git a/crates/engine/src/parser/oracle_nom/condition.rs b/crates/engine/src/parser/oracle_nom/condition.rs index 3a52f77af3..2adffab2f0 100644 --- a/crates/engine/src/parser/oracle_nom/condition.rs +++ b/crates/engine/src/parser/oracle_nom/condition.rs @@ -12,6 +12,7 @@ use nom::multi::many0; use nom::sequence::{preceded, terminated}; use nom::Parser; +use super::bridge::nom_on_lower; use super::error::{oracle_err, OracleError, OracleResult}; use super::primitives::{ parse_article, parse_color, parse_keyword_name, parse_mana_cost, parse_number, @@ -2223,6 +2224,20 @@ fn parse_has_counters_axes( Ok((rest, (subject, counters, minimum, maximum))) } +/// CR 122.1 + CR 608.2c: identifies only a leading `if it has … counter on it,` +/// condition. The generic counter-condition parser resolves bare `it` to the +/// source by default; effect-chain parsing uses this narrow lexical fact to +/// rebind that condition when an earlier clause established a typed referent. +/// Explicit source subjects (`~`, `this creature`, `this land`) deliberately do +/// not match and retain their source scope. +pub(crate) fn is_leading_if_bare_recipient_counter_condition(input: &str) -> bool { + let lower = input.to_lowercase(); + nom_on_lower(input, &lower, |i| { + terminated(preceded(tag("if "), parse_has_counters_axes), tag(",")).parse(i) + }) + .is_some_and(|((subject, ..), _)| matches!(subject, CounterConditionSubject::RecipientPronoun)) +} + /// Subject axis for counter-has conditions. Accepts the canonical /// source-referential subjects, the bound pronoun `"it "`, and the /// demonstrative anaphor `"that creature/land/permanent "` used in @@ -19052,6 +19067,25 @@ mod tests { ); } + /// CR 122.1 + CR 608.2c: effect-chain parsing may rebind only the bare + /// recipient-pronoun form after an earlier typed target. Explicit source + /// and demonstrative-recipient subjects must remain distinguishable. + #[test] + fn leading_if_bare_recipient_counter_condition_is_narrow() { + assert!(is_leading_if_bare_recipient_counter_condition( + "If it has a counter on it, it gains flying" + )); + assert!(!is_leading_if_bare_recipient_counter_condition( + "If this creature has a counter on it, it gains flying" + )); + assert!(!is_leading_if_bare_recipient_counter_condition( + "If that creature has a counter on it, it gains flying" + )); + assert!(!is_leading_if_bare_recipient_counter_condition( + "If it is tapped, it gains flying" + )); + } + /// CR 611.3a: the bound pronoun "it" in a self-referential combat-state gate /// binds to the source permanent. Intrepid Ace's "it isn't attacking or /// blocking" must parse to `Not(Or[SourceIsAttacking, SourceIsBlocking])`, diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 697de4f289..9078670d5b 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -23036,6 +23036,299 @@ fn azors_gateway_transform_condition_parses_with_zero_swallowed_clauses() { assert!(transform.sub_ability.is_none()); } +/// Issue #6507 SHAPE: Gemstone Mine's sacrifice rider — "If there are no +/// mining counters on this land, sacrifice it." — must bind the bare "it" to +/// `SelfRef` (CR 608.2k: the rider's condition names the source, so the +/// pronoun anaphors to the source permanent). Pre-fix the sub-ability carried +/// `ParentTarget`, which a mana ability can never resolve (CR 605.1a — a mana +/// ability requires no target), so the rider silently no-oped at runtime. +#[test] +fn depletion_land_rider_sacrifice_binds_self_ref() { + let text = "This land enters with three mining counters on it.\n{T}, Remove a mining \ + counter from this land: Add one mana of any color. If there are no mining \ + counters on this land, sacrifice it."; + let parsed = parse(text, "Gemstone Mine", &[], &["Land"], &[]); + + // Reach-guards: the parse succeeded with zero swallowed clauses and zero + // Unimplemented markers, so the shape assertions below are not vacuous. + assert!( + parsed.parse_warnings.is_empty(), + "expected zero parse warnings, got {:#?}", + parsed.parse_warnings + ); + fn has_unimpl(def: &AbilityDefinition) -> bool { + matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) + || def.sub_ability.as_deref().is_some_and(has_unimpl) + } + assert!( + !parsed.abilities.iter().any(has_unimpl), + "no Unimplemented anywhere in the parse: {:#?}", + parsed.abilities + ); + assert_eq!(parsed.abilities.len(), 1); + + let mana = &parsed.abilities[0]; + assert!( + matches!(mana.effect.as_ref(), Effect::Mana { .. }), + "head effect must be the mana production, got {:?}", + mana.effect + ); + + let rider = mana.sub_ability.as_deref().expect("sacrifice sub-ability"); + // CR 122.1: the gate reads the mining-counter total on the source. + assert_eq!( + rider.condition, + Some(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: Some(crate::types::counter::parse_counter_type("mining")), + }, + }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Fixed { value: 0 }, + }) + ); + // CR 608.2k: "sacrifice it" = sacrifice the source land itself. + assert!( + matches!( + rider.effect.as_ref(), + Effect::Sacrifice { + target: TargetFilter::SelfRef, + count: QuantityExpr::Fixed { value: 1 }, + .. + } + ), + "the rider must sacrifice SelfRef, got {:?}", + rider.effect + ); +} + +/// Issue #6507 SHAPE (typed-subject trigger sibling): Last Light of Durin's +/// Day's rider — "If it has six or more quest counters on it, sacrifice it." +/// — must bind to `SelfRef`, not `TriggeringSource` (pre-fix the anaphor +/// rewriter bound the gated sacrifice to the triggering Mountain). The head +/// clause's explicit "on this enchantment" binding must be untouched (guards +/// against over-rewrite). +#[test] +fn typed_trigger_source_counter_rider_binds_self_ref_not_triggering_source() { + let text = "Whenever a Mountain you control enters, put a quest counter on this \ + enchantment. If it has six or more quest counters on it, sacrifice it. If you \ + do, search your hand and/or library for a Dragon card and put it onto the \ + battlefield. If you search your library this way, shuffle."; + let parsed = parse( + text, + "Last Light of Durin's Day", + &[], + &["Enchantment"], + &[], + ); + + // Reach-guards: full trigger parse, zero warnings, zero Unimplemented. + assert!( + parsed.parse_warnings.is_empty(), + "expected zero parse warnings, got {:#?}", + parsed.parse_warnings + ); + fn has_unimpl(def: &AbilityDefinition) -> bool { + matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) + || def.sub_ability.as_deref().is_some_and(has_unimpl) + } + assert_eq!(parsed.triggers.len(), 1); + let execute = parsed.triggers[0] + .execute + .as_deref() + .expect("trigger must carry an execute chain"); + assert!( + !has_unimpl(execute), + "no Unimplemented anywhere in the trigger chain: {execute:#?}" + ); + + // Head clause: the explicit "on this enchantment" subject stays SelfRef. + assert!( + matches!( + execute.effect.as_ref(), + Effect::PutCounter { + target: TargetFilter::SelfRef, + .. + } + ), + "the head counter placement must stay on the source, got {:?}", + execute.effect + ); + + let rider = execute + .sub_ability + .as_deref() + .expect("sacrifice sub-ability"); + // CR 122.1: source-scoped quest-counter threshold gate. + assert_eq!( + rider.condition, + Some(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: Some(crate::types::counter::parse_counter_type("quest")), + }, + }, + comparator: Comparator::GE, + rhs: QuantityExpr::Fixed { value: 6 }, + }) + ); + // CR 608.2k: the gated "sacrifice it" anaphors to the source enchantment, + // NOT the triggering Mountain. + assert!( + matches!( + rider.effect.as_ref(), + Effect::Sacrifice { + target: TargetFilter::SelfRef, + count: QuantityExpr::Fixed { value: 1 }, + .. + } + ), + "the rider must sacrifice SelfRef (the source), got {:?}", + rider.effect + ); +} + +/// Issue #6559 review regression guard: the source-counter arm must NOT fire +/// when an EARLIER clause already chose a typed target. Revelation of Power +/// (Instant): "Target creature gets +2/+2 until end of turn. If it has a counter +/// on it, it also gains flying and lifelink until end of turn." Both bare "it" +/// pronouns anaphor to the TARGET creature, but the intervening-if condition +/// must be recipient-scoped instead of source-scoped. Binding the gated grant +/// to the source (SelfRef) would drop flying/lifelink onto the one-shot Instant +/// — the card would lose its second sentence (an `engine_regress`). +/// `chain_has_prior_typed_referent` sees the prior "Target creature gets +2/+2" +/// clause and suppresses the source binding, so the grant stays on the parent +/// (target) creature. CR 608.2k only licenses a source anaphor when the ability's +/// COST or TRIGGER CONDITION names the source — never a mid-effect intervening-if +/// over a previously chosen target. The depletion-land / counter riders have no +/// prior typed target, so their SelfRef heals are unaffected (see the two tests +/// above). +#[test] +fn source_counter_gate_over_prior_target_keeps_parent_not_self_ref() { + let text = "Target creature gets +2/+2 until end of turn. If it has a counter on it, \ + it also gains flying and lifelink until end of turn."; + let parsed = parse(text, "Revelation of Power", &[], &["Instant"], &[]); + + // Reach-guards: the parse succeeded with zero warnings and zero Unimplemented, + // so the shape assertions below are not vacuous. + assert!( + parsed.parse_warnings.is_empty(), + "expected zero parse warnings, got {:#?}", + parsed.parse_warnings + ); + fn has_unimpl(def: &AbilityDefinition) -> bool { + matches!(def.effect.as_ref(), Effect::Unimplemented { .. }) + || def.sub_ability.as_deref().is_some_and(has_unimpl) + } + assert!( + !parsed.abilities.iter().any(has_unimpl), + "no Unimplemented anywhere in the parse: {:#?}", + parsed.abilities + ); + + // The flying/lifelink grant is the sub-ability of the +2/+2 pump. + let pump = &parsed.abilities[0]; + assert!( + matches!(pump.effect.as_ref(), Effect::Pump { .. }), + "head effect is the +2/+2 pump, got {:?}", + pump.effect + ); + let grant = pump + .sub_ability + .as_deref() + .expect("flying/lifelink grant sub-ability"); + let Effect::GenericEffect { + static_abilities, + target, + .. + } = grant.effect.as_ref() + else { + panic!( + "grant must be a continuous GenericEffect, got {:?}", + grant.effect + ); + }; + // CR 608.2k: "it" = the prior chosen target creature (ParentTarget), never + // the source Instant (SelfRef). + assert_eq!( + *target, + Some(TargetFilter::ParentTarget), + "the grant's target must be the parent (target) creature, not the source Instant" + ); + assert_eq!(static_abilities.len(), 1); + assert_eq!( + static_abilities[0].affected, + Some(TargetFilter::ParentTarget), + "the flying/lifelink continuous grant must affect the target creature (ParentTarget), \ + not the source Instant (SelfRef) — the #6559 regression this guard prevents" + ); + assert!( + matches!( + &static_abilities[0].condition, + Some(StaticCondition::QuantityComparison { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Recipient, + .. + }, + }, + .. + }) + ), + "the bare counter-condition pronoun must gate the recipient, got {:?}", + static_abilities[0].condition + ); +} + +/// CR 122.1 + CR 608.2c: A prior typed target does not rewrite an explicit +/// source subject. This sibling keeps source-counter cards such as Gemstone +/// Mine and Last Light of Durin's Day on their established Source scope. +#[test] +fn explicit_source_counter_gate_over_prior_target_stays_source_scoped() { + let parsed = parse( + "Target creature gets +2/+2 until end of turn. If this creature has a counter on it, \ + it also gains flying until end of turn.", + "Explicit Source Counter Guard", + &[], + &["Creature"], + &[], + ); + + let rider = parsed.abilities[0] + .sub_ability + .as_deref() + .expect("conditional flying rider"); + let Effect::GenericEffect { + static_abilities, .. + } = rider.effect.as_ref() + else { + panic!( + "explicit source-counter rider must be a GenericEffect, got {:?}", + rider.effect + ); + }; + assert_eq!(static_abilities.len(), 1); + assert!( + matches!( + &static_abilities[0].condition, + Some(StaticCondition::QuantityComparison { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + .. + }, + }, + .. + }) + ), + "explicit source text must retain CountersOn(Source), got {:?}", + static_abilities[0].condition + ); +} + /// CR 104.2b + CR 104.3e + CR 114.1 + CR 611.3a + CR 205.3j: Gideon of the /// Trials (verbatim MTGJSON Oracle text) — the third loyalty ability creates /// an emblem carrying BOTH game-outcome locks, each gated on an `IsPresent` diff --git a/crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs b/crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs new file mode 100644 index 0000000000..00adc634c4 --- /dev/null +++ b/crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs @@ -0,0 +1,329 @@ +//! Issue #6507: the depletion-land sacrifice rider never fires (Gemstone Mine +//! class). "{T}, Remove a mining counter from this land: Add one mana of any +//! color. If there are no mining counters on this land, sacrifice it." — the +//! rider's bare "it" mis-bound to `ParentTarget` at parse time; a mana ability +//! has no targets (CR 605.1a), so the sacrifice sub-chain silently no-oped and +//! the land survived with zero counters. +//! +//! Fix: `condition_refs_source_object` now recognizes a source-scoped counter +//! `QuantityCheck` (CR 122.1 + CR 608.2k), so the chunk subject threads +//! `SelfRef` and `resolve_it_pronoun` binds "sacrifice it" to the source. +//! +//! Discriminators (flip when the fix is reverted): +//! - last-counter activations leave the land on the battlefield instead of in +//! the graveyard (tests 1, 3, 4); +//! - the typed-subject trigger sibling (Last Light of Durin's Day shape) +//! sacrifices the TRIGGERING Mountain instead of the source enchantment +//! (test 5 — both zone assertions flip). + +use engine::game::scenario::{GameScenario, P0}; +use engine::types::actions::GameAction; +use engine::types::counter::parse_counter_type; +use engine::types::game_state::{ManaChoice, WaitingFor}; +use engine::types::identifiers::ObjectId; +use engine::types::keywords::Keyword; +use engine::types::mana::{ManaCost, ManaCostShard, ManaType}; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +// Verbatim Oracle texts (card-data.json, verified 2026-07-23). +const GEMSTONE_MINE_ORACLE: &str = "This land enters with three mining counters on it.\n{T}, Remove a mining counter from this land: Add one mana of any color. If there are no mining counters on this land, sacrifice it."; +const PEAT_BOG_ORACLE: &str = "This land enters tapped with two depletion counters on it.\n{T}, Remove a depletion counter from this land: Add {B}{B}. If there are no depletion counters on this land, sacrifice it."; +const DARK_RITUAL_ORACLE: &str = "Add {B}{B}{B}."; +// Trigger line of Last Light of Durin's Day (the Mountaincycling keyword line +// is irrelevant to the rider under test and omitted). +const LAST_LIGHT_ORACLE: &str = "Whenever a Mountain you control enters, put a quest counter on this enchantment. If it has six or more quest counters on it, sacrifice it. If you do, search your hand and/or library for a Dragon card and put it onto the battlefield. If you search your library this way, shuffle."; +const REVELATION_OF_POWER_ORACLE: &str = "Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink until end of turn."; + +fn counter_count(runner: &engine::game::scenario::GameRunner, id: ObjectId, counter: &str) -> u32 { + runner.state().objects[&id] + .counters + .get(&parse_counter_type(counter)) + .copied() + .unwrap_or(0) +} + +fn pool_count( + runner: &engine::game::scenario::GameRunner, + player: engine::types::player::PlayerId, + mana: ManaType, +) -> usize { + runner.state().players[player.0 as usize] + .mana_pool + .count_color(mana) +} + +fn gemstone_mine_scenario(mining_counters: u32) -> (engine::game::scenario::GameRunner, ObjectId) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let land = scenario + .add_land_from_oracle(P0, "Gemstone Mine", GEMSTONE_MINE_ORACLE) + .id(); + // Scenario-seeded permanents skip ETB replacements, so seed the counters + // directly (the real card enters with three). + scenario.with_counter(land, parse_counter_type("mining"), mining_counters); + (scenario.build(), land) +} + +fn activate_and_choose_green(runner: &mut engine::game::scenario::GameRunner, land: ObjectId) { + runner + .act(GameAction::ActivateAbility { + source_id: land, + ability_index: 0, + }) + .expect("Gemstone Mine's mana ability must be payable while a mining counter remains"); + // CR 605.3b: the mana ability resolves immediately after the color choice — + // no stack, no priority round-trip. + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::ChooseManaColor { .. } + ), + "AnyOneColor production must prompt for a color, got {:?}", + runner.state().waiting_for + ); + runner + .act(GameAction::ChooseManaColor { + choice: ManaChoice::SingleColor(ManaType::Green), + count: 1, + }) + .expect("choose green"); +} + +/// Test 1 (discriminating): removing the LAST mining counter must sacrifice +/// Gemstone Mine after the mana is produced. Pre-fix the rider's `ParentTarget` +/// matched nothing (a mana ability has no targets, CR 605.1a) and the land +/// wrongly stayed on the battlefield. +#[test] +fn gemstone_mine_last_counter_sacrifices_after_mana() { + let (mut runner, land) = gemstone_mine_scenario(1); + activate_and_choose_green(&mut runner, land); + + // Reach-guards: the ability resolved — mana was produced and the cost's + // counter removal happened — so the zone assertion is not vacuous. + assert_eq!( + pool_count(&runner, P0, ManaType::Green), + 1, + "the mana ability must resolve and add one green mana" + ); + assert_eq!( + counter_count(&runner, land, "mining"), + 0, + "the activation cost must have removed the last mining counter" + ); + // CR 701.21a: the rider sacrifices the land itself. + assert_eq!( + runner.state().objects[&land].zone, + Zone::Graveyard, + "with no mining counters left, Gemstone Mine must sacrifice itself" + ); +} + +/// Test 2 (negative sibling): with a counter remaining the gate is false and +/// the land stays. The positive pair (mana produced + exactly one counter +/// left) proves resolution reached the gate and the gate evaluated false — +/// not that resolution never ran. +#[test] +fn gemstone_mine_two_counters_stays_on_battlefield() { + let (mut runner, land) = gemstone_mine_scenario(2); + activate_and_choose_green(&mut runner, land); + + assert_eq!( + pool_count(&runner, P0, ManaType::Green), + 1, + "the mana ability must resolve and add one green mana" + ); + assert_eq!( + counter_count(&runner, land, "mining"), + 1, + "one activation must remove exactly one mining counter" + ); + assert_eq!( + runner.state().objects[&land].zone, + Zone::Battlefield, + "with a mining counter remaining, the sacrifice gate is false and the land stays" + ); +} + +/// Test 3 (class sibling, discriminating): Peat Bog — different counter type, +/// fixed multi-mana production, no color prompt. Removing the last depletion +/// counter must sacrifice the land. +#[test] +fn peat_bog_last_depletion_counter_sacrifices() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let land = scenario + .add_land_from_oracle(P0, "Peat Bog", PEAT_BOG_ORACLE) + .id(); + scenario.with_counter(land, parse_counter_type("depletion"), 1); + let mut runner = scenario.build(); + + runner + .act(GameAction::ActivateAbility { + source_id: land, + ability_index: 0, + }) + .expect("Peat Bog's mana ability must be payable while a depletion counter remains"); + + // Reach-guard: {B}{B} landed in the pool, so the ability resolved. + assert_eq!( + pool_count(&runner, P0, ManaType::Black), + 2, + "Peat Bog must add {{B}}{{B}}" + ); + assert_eq!( + runner.state().objects[&land].zone, + Zone::Graveyard, + "with no depletion counters left, Peat Bog must sacrifice itself" + ); +} + +/// Test 4 (auto-tap payment path, discriminating): casting a {B} spell with +/// Peat Bog (one depletion counter) as the only mana source. Auto payment taps +/// Peat Bog, removing its last counter; the rider must sacrifice it during +/// payment. Pre-fix the spell resolved but the land wrongly survived. +#[test] +fn peat_bog_autotap_payment_sacrifices_after_last_counter() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let land = scenario + .add_land_from_oracle(P0, "Peat Bog", PEAT_BOG_ORACLE) + .id(); + scenario.with_counter(land, parse_counter_type("depletion"), 1); + let spell = { + let mut b = + scenario.add_spell_to_hand_from_oracle(P0, "Dark Ritual", true, DARK_RITUAL_ORACLE); + b.with_mana_cost(ManaCost::Cost { + shards: vec![ManaCostShard::Black], + generic: 0, + }); + b.id() + }; + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).resolve(); + + // Reach-guard: the cast and its payment executed — Peat Bog produced + // {B}{B}, one paid the cost, and Dark Ritual's resolution added {B}{B}{B}, + // leaving four black mana in the pool. + assert_eq!( + outcome + .state() + .players[P0.0 as usize] + .mana_pool + .count_color(ManaType::Black), + 4, + "auto-tap must pay {{B}} from Peat Bog's {{B}}{{B}} and Dark Ritual must add {{B}}{{B}}{{B}}" + ); + outcome.assert_zone(&[spell], Zone::Graveyard); + // CR 701.21a: the rider fires on the auto-tap path too — the sub-chain runs + // inside `resolve_mana_ability` regardless of how the activation happened. + assert_eq!( + outcome.zone_of(land), + Zone::Graveyard, + "paying with Peat Bog's last depletion counter must sacrifice it" + ); +} + +/// Test 5 (multi-authority hostile, discriminating): the typed-subject trigger +/// sibling. Two live candidate objects — the triggering Mountain and the +/// source enchantment. Pre-fix the rider bound to `TriggeringSource` and +/// sacrificed the MOUNTAIN while the enchantment survived; post-fix the +/// enchantment is sacrificed and the Mountain stays (CR 608.2k — "it" refers +/// to the source named by the rider's own condition). +#[test] +fn source_counter_rider_sacrifices_source_not_triggering_object() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let enchantment = { + let mut b = scenario.add_creature(P0, "Last Light of Durin's Day", 0, 0); + b.as_enchantment().from_oracle_text(LAST_LIGHT_ORACLE); + b.id() + }; + scenario.with_counter(enchantment, parse_counter_type("quest"), 5); + let mountain = { + let mut b = scenario.add_land_to_hand(P0, "Mountain"); + b.with_subtypes(vec!["Mountain"]); + b.id() + }; + // A findable Dragon in the library so the post-sacrifice "search your hand + // and/or library for a Dragon card" surfaces a mandatory `SearchChoice` + // prompt — the halt point the trigger only reaches if the sacrifice was + // performed ("If you do"). Without a findable Dragon the search silently + // fails to find and the trigger fully resolves, so there is no clean + // boundary to inspect. + { + let mut b = scenario.add_spell_to_library_top(P0, "Shivan Dragon", false); + b.as_creature().with_subtypes(vec!["Dragon"]); + } + let mut runner = scenario.build(); + + let card_id = runner.state().objects[&mountain].card_id; + runner + .act(GameAction::PlayLand { + object_id: mountain, + card_id, + }) + .expect("play the Mountain from hand"); + + // Drain priority until the trigger resolves; it halts at the mandatory + // "search your hand and/or library" prompt (the /card-test boundary rule). + for _ in 0..20 { + match &runner.state().waiting_for { + WaitingFor::Priority { .. } => { + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + _ => break, + } + } + + // Reach-guard: the trigger body ran through the counter placement AND the + // sacrifice gate — the "If you do" search prompt only surfaces after a + // sacrifice was performed, so neither zone assertion below is vacuous. + assert!( + matches!(runner.state().waiting_for, WaitingFor::SearchChoice { .. }), + "the post-sacrifice search prompt must surface, got {:?}", + runner.state().waiting_for + ); + assert_eq!( + runner.state().objects[&enchantment].zone, + Zone::Graveyard, + "the rider must sacrifice the SOURCE enchantment (sixth quest counter reached)" + ); + assert_eq!( + runner.state().objects[&mountain].zone, + Zone::Battlefield, + "the triggering Mountain must survive — pre-fix the TriggeringSource \ + mis-binding sacrificed it instead of the source" + ); +} + +/// Issue #6559 regression: the intervening counter check reads the previously +/// chosen creature, not the Instant. This drives casting, target selection, +/// condition evaluation, and layer application; it fails if the source-counter +/// guard binds the rider's bare pronouns to `SelfRef`. +#[test] +fn revelation_of_power_grants_keywords_to_its_countered_target() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let creature = scenario.add_creature(P0, "Countered Creature", 2, 2).id(); + scenario.with_counter(creature, engine::types::counter::CounterType::Plus1Plus1, 1); + let revelation = scenario + .add_spell_to_hand_from_oracle(P0, "Revelation of Power", true, REVELATION_OF_POWER_ORACLE) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(revelation).target_object(creature).resolve(); + + assert!( + outcome.state().objects[&creature].has_keyword(&Keyword::Flying), + "the countered target must gain flying" + ); + assert!( + outcome.state().objects[&creature].has_keyword(&Keyword::Lifelink), + "the countered target must gain lifelink" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 90e0f718d3..54190e9943 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -210,6 +210,7 @@ mod game_state_boxed_ability_serde; mod game_state_stack_budget; mod gatta_and_luzzu_regression; mod gemstone_caverns_begin_game; +mod gemstone_mine_depletion_sacrifice_6507; mod gev_scaled_scorch_enter_counters; mod giada_angel_counters; mod giant_ox_crew_toughness;