From 1c74fedb5bef8d76fb45de2c5e22833208179d1d Mon Sep 17 00:00:00 2001 From: jeffrey701 <158072326+jeffrey701@users.noreply.github.com> Date: Thu, 23 Jul 2026 20:35:54 +0200 Subject: [PATCH 1/2] fix(parser): bind source-counter-gated rider pronouns to the source (Gemstone Mine #6507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The depletion-land sacrifice rider ("If there are no mining counters on this land, sacrifice it.") parsed to Sacrifice{ParentTarget}. A mana ability has no targets (CR 605.1a), so ParentTarget resolved to an empty set and the sacrifice silently no-op'd — the land never left play. Root cause is a parse-time anaphor mis-binding, not a runtime gap: the chunk-subject threading in the effect-chain parser already binds a bare "it" to SelfRef when the gating condition references the source object, via condition_refs_source_object. That predicate recognized the source-tapped / source-entered / source-attached conditions but not a source-scoped counter threshold (QuantityCheck over CountersOn{Source}), so the counter-gated riders fell through to ParentTarget (and, on typed triggers, to TriggeringSource). Extend condition_refs_source_object with a QuantityCheck arm that returns true when either side reads counters on the source, via a new exhaustive QuantityExpr walker (no wildcard — a future variant must be classified). This single predicate extension drives both existing consumers: the chunk-subject threading now supplies SelfRef, and the ParentTarget rewrite guard now skips these chunks. Bindings become source-correct for the Mercadian depletion lands (Peat Bog, Hickory Woodlot, Remote Farm, Sandstone Needle, Saprazzan Skerry), Gemstone Mine, Tourach's Gate, Daredevil Dragster, Last Light of Durin's Day, ED-E, and the whole source-counter-conditioned rider class (~21 cards). No runtime files change. CR 122.1 + CR 608.2k annotate the new arm. Closes #6507 Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/engine/src/parser/oracle_effect/mod.rs | 41 +++ .../engine/src/parser/oracle_effect/tests.rs | 76 +++++ crates/engine/src/parser/oracle_tests.rs | 155 +++++++++ .../gemstone_mine_depletion_sacrifice_6507.rs | 299 ++++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + 5 files changed, 572 insertions(+) create mode 100644 crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 7edf095edd..9a5b920074 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 + CR 608.2k: 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) diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 581c9a6c63..f30042a6ca 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -28631,6 +28631,82 @@ fn resolve_it_pronoun_any_subject() { assert_eq!(resolve_it_pronoun(&mut ctx), TargetFilter::SelfRef); } +/// Issue #6507 (CR 122.1 + CR 608.2k): `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), + ], + })); + + // 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 }, + } + )); +} + // --- Suffix condition extraction tests --- #[test] diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 3ac1931516..1fcff263aa 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -22487,6 +22487,161 @@ 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 + ); +} + /// 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..f2ffbc8c13 --- /dev/null +++ b/crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs @@ -0,0 +1,299 @@ +//! 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::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."; + +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" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 81a3ae2ee2..6faff857bd 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -182,6 +182,7 @@ mod gaeas_anthem_team_pump; mod gain_control_multi_target_6205; 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; From 41573c54dca4937dd9f01ec199f53a2267821377 Mon Sep 17 00:00:00 2001 From: jeffrey701 <158072326+jeffrey701@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:41:00 +0200 Subject: [PATCH 2/2] fix(parser): don't rebind a source-counter-gated pronoun over a prior chosen target (#6559 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/engine/src/parser/oracle_effect/mod.rs | 22 +++++- .../engine/src/parser/oracle_effect/tests.rs | 50 ++++++++++++ crates/engine/src/parser/oracle_tests.rs | 76 +++++++++++++++++++ 3 files changed, 146 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index e8137979df..2665257f67 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -29927,7 +29927,21 @@ 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: A source-scoped counter gate rebinds a bare "it" in the gated + // body to the ability's own source ONLY when that source is the pronoun's + // real antecedent — i.e. the ability's cost/trigger named it and no earlier + // clause introduced a chosen target. When a prior clause DID choose a typed + // target ("Target creature gets +2/+2 …"), the gated body's "it" anaphors to + // that target, not the source — Revelation of Power ("… If it has a counter + // on it, it also gains flying and lifelink") mis-scopes its bare "it" to + // CountersOn{Source}, and binding it to the source would drop the grant onto + // the Instant. `chain_has_prior_typed_referent` is false for the depletion + // lands / counter riders (whose prior clause is "Add mana" or "put a counter + // on ~", not a typed target), so every intended heal is unaffected. + let binds_source_counter_pronoun = + condition.as_ref().is_some_and(condition_refs_source_object) + && !chain_has_prior_typed_referent(builder.clauses(), false); + let chunk_subject = if binds_source_counter_pronoun { Some(TargetFilter::SelfRef) } else { if_you_do_anchor.clone().or_else(|| ctx.subject.clone()) @@ -30711,7 +30725,11 @@ pub(crate) fn parse_effect_chain_ir( // Kicker clauses referencing "that creature"/"it" inherit the parent's target. 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 9ac04ceef9..b95cadf1a1 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -29185,6 +29185,56 @@ fn condition_refs_source_object_source_counter_quantity_check() { 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 --- diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index b004e2d755..8100c8bd76 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -22905,6 +22905,82 @@ fn typed_trigger_source_counter_rider_binds_self_ref_not_triggering_source() { ); } +/// 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 +/// mis-scopes the bare "it" to `CountersOn { scope: Source }`. 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" + ); +} + /// 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`