From bddbfd4ff8d1549b37337f2483470a6c3dd6393d Mon Sep 17 00:00:00 2001 From: Clayton Date: Wed, 29 Jul 2026 18:02:37 -0500 Subject: [PATCH 1/4] fix(engine): Gemstone Mine-class conditional sacrifice binds to source, not an unbound ParentTarget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An anaphoric-target rewrite in the chain parser unconditionally bound a following clause's bare "it"/"that creature" to ParentTarget whenever any prior clause existed in the chain, even when that prior clause established no target at all (e.g. a mana ability's untargeted "Add one mana of any color"). For Gemstone Mine's "{T}, Remove a mining counter: Add one mana of any color. If there are no mining counters on this land, sacrifice it.", this clobbered the sacrifice's target into a ParentTarget that resolves to nothing at runtime, so the land was never sacrificed after its last counter was removed (#6507). Gate the rewrite on the prior clause actually exposing a target concept, and rebind a sacrifice imperative's unbound ParentTarget default to the ability's own source when no such antecedent exists — covering the whole class of targetless activated abilities with a trailing conditional self-sacrifice, not just this card. Co-Authored-By: Claude Sonnet 5 --- crates/engine/src/parser/oracle_effect/mod.rs | 42 +++++++++++++++++- crates/engine/src/parser/oracle_tests.rs | 44 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 46ce49b490..86063eb6ad 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -30668,10 +30668,31 @@ pub(crate) fn parse_effect_chain_ir( // which is exactly why a head-only rewrite corrupts the chain. let is_distributed_chunk = text_is_compound_subject_distribution(&text); // Kicker clauses referencing "that creature"/"it" inherit the parent's target. + // + // CR 608.2c + CR 122.1 (issue #6507): gated on the immediately preceding + // clause actually exposing SOME target concept (`target_filter().is_some()`), + // not merely `!builder.is_empty()`. A prior clause can establish a legitimate + // antecedent through several different shapes (a chosen typed target — the + // kicker_instead_chain_produces_correct_condition DealDamage head; a + // self-reference by printed name — Managorger Phoenix's `PutCounter{SelfRef}`; + // a cost-paid object — Jhoira of the Ghitu's exiled card), so this deliberately + // does not enumerate those shapes — it only excludes the one shape that has NO + // target concept at all. A mana ability's `Effect::Mana` has an `Option` + // that is `None` for every ordinary mana ability (Gemstone Mine's "Add one mana + // of any color" chooses nothing), so `target_filter()` returns `None` and there + // is no antecedent for a following "it" to inherit. Previously this rewrite + // fired unconditionally whenever the builder was non-empty, clobbering an + // unbound sacrifice target into `ParentTarget`, which resolves to nothing at + // runtime — see the sibling fixup immediately below for the other half of the + // fix (rebinding that unbound default to the ability's own source instead). + let prior_clause_has_no_target_concept = builder + .clauses() + .last() + .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()); if condition.is_some() && !is_distributed_chunk && !condition.as_ref().is_some_and(condition_refs_source_object) - && !builder.is_empty() + && !prior_clause_has_no_target_concept && has_anaphoric_reference(&text_lower) && !matches!(if_you_do_anchor, Some(TargetFilter::SelfRef)) && !typed_trigger_subject @@ -30683,6 +30704,25 @@ pub(crate) fn parse_effect_chain_ir( { replace_target_with_parent(&mut clause.effect); } + // CR 608.2c + CR 122.1 (issue #6507): the sacrifice imperative parser + // (`parse_targeted_action_ast`) defaults a bare "it"/"them" pronoun with no + // trigger subject to `ParentTarget`, on the assumption that SOME earlier + // clause in the chain chose a real target for it to inherit — the common case + // (`bare_it_without_trigger_subject_preserves_parent_target`). When the + // immediately preceding clause has no target concept at all (the Kicker-clause + // guard above), that assumption is false: there is nothing for `ParentTarget` + // to resolve to, so a conditional "sacrifice it" tail (Gemstone Mine's "{T}, + // Remove a mining counter: Add one mana of any color. If there are no mining + // counters on this land, sacrifice it.") silently never sacrifices anything. + // Rebind it to the ability's own source, the only object "it" can plausibly + // name here. + if condition.is_some() && !is_distributed_chunk && prior_clause_has_no_target_concept { + if let Effect::Sacrifice { target, .. } = &mut clause.effect { + if matches!(target, TargetFilter::ParentTarget) { + *target = TargetFilter::SelfRef; + } + } + } // CR 608.2c: Pronoun clause following a conditional targeted effect. if condition.is_none() && !is_distributed_chunk diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index ee50378e7f..5c49fcd37e 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -8214,6 +8214,50 @@ fn mox_pearl_mana_ability() { assert_eq!(r.abilities[0].kind, AbilityKind::Activated); } +/// Issue #6507: Gemstone Mine's mana ability is targetless ("Add one mana of +/// any color" chooses no target), so the trailing "If there are no mining +/// counters on this land, sacrifice it" conditional has no parent target to +/// inherit. The bare "it" pronoun must bind to the ability's own source +/// (`TargetFilter::SelfRef`), not `ParentTarget` — `ParentTarget` resolves to +/// nothing here and the land was never sacrificed. This is the +/// depletion-land class, not a one-off: any targetless activated ability +/// whose trailing conditional says "sacrifice it" shares the exposure. +#[test] +fn gemstone_mine_conditional_sacrifice_binds_to_self_ref() { + let r = parse( + "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.", + "Gemstone Mine", + &[], + &["Land"], + &[], + ); + assert_eq!(r.abilities.len(), 1); + let ability = &r.abilities[0]; + assert!( + matches!(*ability.effect, Effect::Mana { .. }), + "expected Effect::Mana, got {:?}", + ability.effect + ); + let sub_ability = ability + .sub_ability + .as_ref() + .expect("expected a conditional sub_ability for the trailing sacrifice sentence"); + assert!( + sub_ability.condition.is_some(), + "expected the 'if there are no mining counters' gate to survive as a condition" + ); + let Effect::Sacrifice { target, .. } = &*sub_ability.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sub_ability.effect); + }; + assert_eq!( + *target, + TargetFilter::SelfRef, + "sacrifice target must bind to the source land, not an unestablished ParentTarget" + ); +} + #[test] fn parses_return_forest_cost_untap_activated_ability() { let r = parse( From a43be74b86aa8d8dd9de3e9129897b75f399b26c Mon Sep 17 00:00:00 2001 From: Clayton Date: Wed, 29 Jul 2026 19:04:52 -0500 Subject: [PATCH 2/4] fix(engine): scope the ParentTarget-antecedent guard to exclude known non-target referents; add integration coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #6797: - HIGH: the new "no antecedent" fallback compared only `target_filter().is_none()` on the prior clause, which also fires for a targetless clause that DOES establish a real referent through a different channel — a token/copy publisher (Effect::Populate has no target field but publishes a created-token referent via chain_prior_referent_is_created_token) or a GenericEffect grant surfaced only through its static's `affected` field (if_you_do_object_anchor). Excluding both from the guard prevents the sacrifice-target rewrite from firing on those chains, so a conditional "sacrifice it" after Populate keeps binding to the populated token (LastCreated) instead of being misrouted to the ability's own source. Added a regression (populate_conditional_sacrifice_keeps_created_token_not_self_ref) that fails against the prior guard and passes against this one. - MED: added a runtime integration test pair (gemstone_mine_survives_activation_with_counters_remaining, gemstone_mine_sacrifices_itself_on_last_counter_removed) that activates the real mana+conditional-sacrifice ability shape through resolve_mana_ability/resolve_mana_ability_sub_chain — proving mana production and the SelfRef sacrifice resolution path both work end-to-end, not just that the parser emits the right AST node. Co-Authored-By: Claude Sonnet 5 --- crates/engine/src/game/mana_abilities.rs | 164 ++++++++++++++++++ crates/engine/src/parser/oracle_effect/mod.rs | 17 +- .../engine/src/parser/oracle_effect/tests.rs | 32 ++++ 3 files changed, 212 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/game/mana_abilities.rs b/crates/engine/src/game/mana_abilities.rs index 97fb7f57a2..5795d85d37 100644 --- a/crates/engine/src/game/mana_abilities.rs +++ b/crates/engine/src/game/mana_abilities.rs @@ -9947,6 +9947,170 @@ mod tests { ); } + // Issue #6507 integration follow-up: `make_gemstone_mine` above omits the + // trailing "If there are no mining counters on this land, sacrifice it." + // sub-ability entirely, so it never exercised the reported bug or its fix + // through the real activation pipeline — only the parser-level AST shape + // is covered by `oracle::tests::gemstone_mine_conditional_sacrifice_binds_to_self_ref`. + // This fixture mirrors the actual end-to-end parsed shape (mana effect + + // conditional `Sacrifice { target: SelfRef }` sub-ability gated on zero + // mining counters) so activation itself proves the fix: mana is produced + // regardless, and the land is sacrificed only on the activation that + // removes its LAST counter. + fn make_gemstone_mine_with_sacrifice_sub_ability( + state: &mut GameState, + player: PlayerId, + initial_mining_counters: u32, + ) -> ObjectId { + let land = create_object( + state, + CardId(8003), + player, + "Gemstone Mine".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&land).unwrap(); + obj.card_types + .core_types + .push(crate::types::card_type::CoreType::Land); + let mining_key = crate::types::counter::parse_counter_type("MINING"); + obj.counters.insert(mining_key, initial_mining_counters); + + let sacrifice_if_depleted = AbilityDefinition::new( + AbilityKind::Spell, + Effect::Sacrifice { + target: TargetFilter::SelfRef, + count: QuantityExpr::Fixed { value: 1 }, + min_count: 0, + }, + ) + .condition(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: Some(CounterType::Generic("mining".to_string())), + }, + }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Fixed { value: 0 }, + }); + + let ability = AbilityDefinition::new( + AbilityKind::Activated, + Effect::Mana { + produced: ManaProduction::AnyOneColor { + count: QuantityExpr::Fixed { value: 1 }, + color_options: vec![ + ManaColor::White, + ManaColor::Blue, + ManaColor::Black, + ManaColor::Red, + ManaColor::Green, + ], + contribution: ManaContribution::Base, + }, + restrictions: Vec::new(), + grants: Vec::new(), + expiry: None, + target: None, + }, + ) + .cost(AbilityCost::Composite { + costs: vec![ + AbilityCost::Tap, + AbilityCost::RemoveCounter { + count: 1, + counter_type: CounterMatch::OfType(CounterType::Generic("mining".to_string())), + target: None, + selection: crate::types::ability::CounterCostSelection::SingleObject, + }, + ], + }) + .sub_ability(sacrifice_if_depleted); + Arc::make_mut(&mut obj.abilities).push(ability); + land + } + + #[test] + fn gemstone_mine_survives_activation_with_counters_remaining() { + let mut state = GameState::new_two_player(42); + let player = PlayerId(0); + let land = make_gemstone_mine_with_sacrifice_sub_ability(&mut state, player, 2); + + let def = state + .objects + .get(&land) + .unwrap() + .abilities + .first() + .cloned() + .unwrap(); + let mut events = Vec::new(); + resolve_mana_ability( + &mut state, + land, + player, + &def, + &mut events, + Some(ProductionOverride::SingleColor(ManaType::Green)), + ) + .expect("Gemstone Mine activation must not fail with counters present"); + + assert_eq!( + state.players[player.0 as usize] + .mana_pool + .count_color(ManaType::Green), + 1, + "mana must be produced regardless of the trailing conditional sacrifice" + ); + assert!( + state.battlefield.contains(&land), + "Gemstone Mine must remain on the battlefield while a mining counter remains after activation" + ); + } + + #[test] + fn gemstone_mine_sacrifices_itself_on_last_counter_removed() { + let mut state = GameState::new_two_player(42); + let player = PlayerId(0); + let land = make_gemstone_mine_with_sacrifice_sub_ability(&mut state, player, 1); + + let def = state + .objects + .get(&land) + .unwrap() + .abilities + .first() + .cloned() + .unwrap(); + let mut events = Vec::new(); + resolve_mana_ability( + &mut state, + land, + player, + &def, + &mut events, + Some(ProductionOverride::SingleColor(ManaType::Green)), + ) + .expect("Gemstone Mine activation must not fail on its last counter"); + + assert_eq!( + state.players[player.0 as usize] + .mana_pool + .count_color(ManaType::Green), + 1, + "mana must still be produced on the activation that empties the last counter" + ); + assert!( + !state.battlefield.contains(&land), + "Gemstone Mine must be sacrificed once its last mining counter is removed (issue #6507)" + ); + assert!( + state.players[player.0 as usize].graveyard.contains(&land), + "the sacrificed Gemstone Mine must land in its controller's graveyard" + ); + } + #[test] fn cabal_coffers_pays_generic_taps_and_counts_swamps() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 86063eb6ad..556a69b28f 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -30685,10 +30685,25 @@ pub(crate) fn parse_effect_chain_ir( // unbound sacrifice target into `ParentTarget`, which resolves to nothing at // runtime — see the sibling fixup immediately below for the other half of the // fix (rebinding that unbound default to the ability's own source instead). + // + // `target_filter().is_none()` alone over-fires on two OTHER legitimate + // non-target antecedents, so both must be excluded here too: + // - `Effect::Populate` has no `target` field (`target_filter()` is `None`) + // but DOES publish a created-token referent — the exact "populate anaphor + // chain" class already documented at `parse_targeted_action_ast`'s bare-"it" + // sacrifice binding. `chain_prior_referent_is_created_token` is the single + // authority that already recognizes this (Token/CopyTokenOf are excluded + // for free since both DO have a `target_filter()`). + // - A `GenericEffect` whose referent lives only in a granted static's + // `affected` field (not its own top-level `target`) still has + // `target_filter() == None`, but `if_you_do_object_anchor` — computed just + // above for this exact clause's "if you do" gate — already finds it. let prior_clause_has_no_target_concept = builder .clauses() .last() - .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()); + .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()) + && !chain_prior_referent_is_created_token(builder.clauses()) + && if_you_do_anchor.is_none(); if condition.is_some() && !is_distributed_chunk && !condition.as_ref().is_some_and(condition_refs_source_object) diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 76a8e8b96d..ca647ced04 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -18715,6 +18715,38 @@ fn kicker_instead_chain_produces_correct_condition() { )); } +/// Issue #6507 review follow-up: `Effect::Populate` has no `target` field +/// (`target_filter()` is `None`, just like a mana ability's untargeted `Add`), +/// but it DOES publish a created-token referent via +/// `chain_prior_referent_is_created_token` — the same "populate anaphor +/// chain" class documented at the sacrifice imperative's bare-"it" binding. +/// A conditional "sacrifice it" tail after Populate must keep inheriting the +/// populated token (`ParentTarget`, rewritten to `LastCreated` by +/// `rewrite_parent_target_to_last_created`) — NOT get rebound to `SelfRef`, +/// which would sacrifice the ability's source instead of the populated copy. +#[test] +fn populate_conditional_sacrifice_keeps_created_token_not_self_ref() { + let ability = parse_effect_chain( + "Populate. If it was kicked, sacrifice it.", + AbilityKind::Spell, + ); + assert!(matches!(&*ability.effect, Effect::Populate)); + let sub = ability.sub_ability.as_ref().expect("expected sub_ability"); + assert!( + sub.condition.is_some(), + "expected the 'if it was kicked' gate to survive as a condition" + ); + let Effect::Sacrifice { target, .. } = &*sub.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sub.effect); + }; + assert_eq!( + *target, + TargetFilter::LastCreated, + "sacrifice target must bind to the populated token (LastCreated), \ + not fall back to SelfRef (the ability's own source)" + ); +} + #[test] fn kicker_leading_instead_produces_correct_condition() { // CR 608.2c: "if kicked, instead [effect]" — leading "instead" variant. From 03d7cd27f4823fb1041717e436b7f2d0ac35d262 Mon Sep 17 00:00:00 2001 From: Clayton Date: Wed, 29 Jul 2026 20:02:06 -0500 Subject: [PATCH 3/4] fix(engine): scope the sacrifice-antecedent fixup to Effect::Sacrifice only, revert the shared Kicker-clause gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #6797 (MED: blast-radius concern). Narrowing the shared "Kicker clause" rewrite's gate (used by `replace_target_with_parent` for every targetable effect kind, not just Sacrifice) was unnecessary and wrong: a full coverage-parse-diff against main's fd53e6af9 showed it flipped 59 cards across 49 signatures, including a genuine regression in Feather, the Redeemed (its ExileResolvingSpell return-at/return-to rider dropped entirely) and target-binding churn across PutCounter, Pump, grant-Flying/DoubleStrike/FirstStrike/Trample/Haste/ Indestructible, ChangeZone, DealDamage, Destroy, and GainControl on unrelated cards (Managorger Phoenix, Skizzik, Searing Blaze, Savage Punch, ED-E, and more) — none of which are the reported bug's class. The Kicker-clause gate is restored to its original `!builder.is_empty()`. The actual fix is the standalone Sacrifice-specific fixup added below it, which only rebinds `Effect::Sacrifice`'s unbound `ParentTarget` default to `SelfRef` — that alone was already sufficient to fix Gemstone Mine; touching the shared gate never contributed to the fix and only caused collateral damage. Re-verified with the same parse-diff pipeline: this narrowed change now touches exactly 6 cards / 1 signature (`ability/Sacrifice`, target `parent target` → `self`) — Gemstone Mine and the five genuine Homelands depletion lands (Hickory Woodlot, Peat Bog, Remote Farm, Sandstone Needle, Saprazzan Skerry), all sharing the identical "{T}, Remove a depletion counter: Add mana. If there are no depletion counters on this land, sacrifice it." shape. Zero unrelated cards changed; the full engine test suite (18015 tests) still passes. Co-Authored-By: Claude Sonnet 5 --- crates/engine/src/parser/oracle_effect/mod.rs | 79 ++++++++----------- 1 file changed, 34 insertions(+), 45 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 556a69b28f..83c0e19221 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -30668,46 +30668,18 @@ pub(crate) fn parse_effect_chain_ir( // which is exactly why a head-only rewrite corrupts the chain. let is_distributed_chunk = text_is_compound_subject_distribution(&text); // Kicker clauses referencing "that creature"/"it" inherit the parent's target. - // - // CR 608.2c + CR 122.1 (issue #6507): gated on the immediately preceding - // clause actually exposing SOME target concept (`target_filter().is_some()`), - // not merely `!builder.is_empty()`. A prior clause can establish a legitimate - // antecedent through several different shapes (a chosen typed target — the - // kicker_instead_chain_produces_correct_condition DealDamage head; a - // self-reference by printed name — Managorger Phoenix's `PutCounter{SelfRef}`; - // a cost-paid object — Jhoira of the Ghitu's exiled card), so this deliberately - // does not enumerate those shapes — it only excludes the one shape that has NO - // target concept at all. A mana ability's `Effect::Mana` has an `Option` - // that is `None` for every ordinary mana ability (Gemstone Mine's "Add one mana - // of any color" chooses nothing), so `target_filter()` returns `None` and there - // is no antecedent for a following "it" to inherit. Previously this rewrite - // fired unconditionally whenever the builder was non-empty, clobbering an - // unbound sacrifice target into `ParentTarget`, which resolves to nothing at - // runtime — see the sibling fixup immediately below for the other half of the - // fix (rebinding that unbound default to the ability's own source instead). - // - // `target_filter().is_none()` alone over-fires on two OTHER legitimate - // non-target antecedents, so both must be excluded here too: - // - `Effect::Populate` has no `target` field (`target_filter()` is `None`) - // but DOES publish a created-token referent — the exact "populate anaphor - // chain" class already documented at `parse_targeted_action_ast`'s bare-"it" - // sacrifice binding. `chain_prior_referent_is_created_token` is the single - // authority that already recognizes this (Token/CopyTokenOf are excluded - // for free since both DO have a `target_filter()`). - // - A `GenericEffect` whose referent lives only in a granted static's - // `affected` field (not its own top-level `target`) still has - // `target_filter() == None`, but `if_you_do_object_anchor` — computed just - // above for this exact clause's "if you do" gate — already finds it. - let prior_clause_has_no_target_concept = builder - .clauses() - .last() - .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()) - && !chain_prior_referent_is_created_token(builder.clauses()) - && if_you_do_anchor.is_none(); + // CR 608.2c: untouched — its `!builder.is_empty()` gate is intentionally broad + // (many antecedent shapes reach this without a `target_filter()`, e.g. a typed + // trigger subject rebind chain), and narrowing it here regressed dozens of + // unrelated cards (Feather, the Redeemed's exile-return rider; Managorger + // Phoenix; dropped/added parent-target bindings across PutCounter/Pump/grant/ + // ChangeZone/DealDamage/Destroy/GainControl) that a full parse-diff against + // main surfaced. See the Sacrifice-specific fixup immediately below instead, + // 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) - && !prior_clause_has_no_target_concept + && !builder.is_empty() && has_anaphoric_reference(&text_lower) && !matches!(if_you_do_anchor, Some(TargetFilter::SelfRef)) && !typed_trigger_subject @@ -30723,14 +30695,31 @@ pub(crate) fn parse_effect_chain_ir( // (`parse_targeted_action_ast`) defaults a bare "it"/"them" pronoun with no // trigger subject to `ParentTarget`, on the assumption that SOME earlier // clause in the chain chose a real target for it to inherit — the common case - // (`bare_it_without_trigger_subject_preserves_parent_target`). When the - // immediately preceding clause has no target concept at all (the Kicker-clause - // guard above), that assumption is false: there is nothing for `ParentTarget` - // to resolve to, so a conditional "sacrifice it" tail (Gemstone Mine's "{T}, - // Remove a mining counter: Add one mana of any color. If there are no mining - // counters on this land, sacrifice it.") silently never sacrifices anything. - // Rebind it to the ability's own source, the only object "it" can plausibly - // name here. + // (`bare_it_without_trigger_subject_preserves_parent_target`). That assumption + // is false when the immediately preceding clause exposes no target concept at + // all (`target_filter().is_none()`), publishes no created-token referent + // (`chain_prior_referent_is_created_token` — Populate has no `target` field but + // DOES publish one; Token/CopyTokenOf are excluded for free since both DO have + // a `target_filter()`), and there is no "if you do" object anchor + // (`if_you_do_object_anchor`, computed above — a GenericEffect referent + // surfaced only through its granted static's `affected` field, not its own + // `target`). With none of those three antecedents present, there is nothing + // for `ParentTarget` to resolve to, so a conditional "sacrifice it" tail + // (Gemstone Mine's "{T}, Remove a mining counter: Add one mana of any color. If + // there are no mining counters on this land, sacrifice it.") silently never + // sacrifices anything. Rebind it to the ability's own source, the only object + // "it" can plausibly name here. + // + // Deliberately scoped to `Effect::Sacrifice` only (not folded into the + // Kicker-clause rewrite's gate above): that gate is shared by every + // `replace_target_with_parent`-eligible effect kind, and narrowing it there + // regressed unrelated cards — see the comment above. + let prior_clause_has_no_target_concept = builder + .clauses() + .last() + .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()) + && !chain_prior_referent_is_created_token(builder.clauses()) + && if_you_do_anchor.is_none(); if condition.is_some() && !is_distributed_chunk && prior_clause_has_no_target_concept { if let Effect::Sacrifice { target, .. } = &mut clause.effect { if matches!(target, TargetFilter::ParentTarget) { From 1d6d21cf9e96ecc5efd516efaf367a66f27c2c0b Mon Sep 17 00:00:00 2001 From: Clayton Date: Wed, 29 Jul 2026 21:57:40 -0500 Subject: [PATCH 4/4] fix(engine): sacrifice antecedent fallback defers to parent_target_available; drop mismatched CR citation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 follow-up on #6797. - HIGH: the no-antecedent guard checked only the immediately-preceding clause's target_filter(), which cannot see a referent established BEFORE a paired conditional when the current clause is inside a recursively-parsed `Otherwise` else-branch (its own `clauses` start empty). Now defers first to `parent_target_available` (already computed above, and already carrying `ctx.parent_target_available` across exactly this recursive-else-branch boundary for the same reason — see the "Otherwise" handler's own comment). Added conditional_sacrifice_defers_to_seeded_parent_target_available, which drives parse_effect_chain_ir directly with parent_target_available seeded (mirroring what a real outer Otherwise referent seeds) and fails against the old guard. - MED: the lookback now uses the nearest non-`ClauseDisposition::Continue` clause instead of a bare `.last()`, matching the existing lookback idiom `if_you_do_object_anchor` already uses for the same reason — an absorbed rider carries no independent referent and must not be mistaken for a targetless antecedent. - MED: dropped the CR 122.1 citation from the Sacrifice-fixup comment. CR 122.1 defines counters; it does not support pronoun/antecedent-binding behavior. CR 608.2c (order-of-instructions + English-rules anaphora) is the citation this code actually implements, and is now the sole one. - MED: regenerated the parse-diff against this exact head (oracle-gen + coverage-report --all + coverage-parse-diff, same base fd53e6af9c07 used throughout this PR's review). Unchanged: 6 cards / 1 signature (ability/Sacrifice target parent target -> self — Gemstone Mine and the five genuine Homelands depletion lands). The two code fixes above are purely defensive additions gated on conditions none of those six cards' parse trees trigger, so the verified scope is stable. Co-Authored-By: Claude Sonnet 5 --- crates/engine/src/parser/oracle_effect/mod.rs | 52 ++++++++++++------- .../engine/src/parser/oracle_effect/tests.rs | 43 +++++++++++++++ 2 files changed, 76 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 83c0e19221..4dc65025ef 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -30691,35 +30691,49 @@ pub(crate) fn parse_effect_chain_ir( { replace_target_with_parent(&mut clause.effect); } - // CR 608.2c + CR 122.1 (issue #6507): the sacrifice imperative parser + // CR 608.2c (issue #6507): the sacrifice imperative parser // (`parse_targeted_action_ast`) defaults a bare "it"/"them" pronoun with no // trigger subject to `ParentTarget`, on the assumption that SOME earlier - // clause in the chain chose a real target for it to inherit — the common case + // clause chose a real target for it to inherit — the common case // (`bare_it_without_trigger_subject_preserves_parent_target`). That assumption - // is false when the immediately preceding clause exposes no target concept at - // all (`target_filter().is_none()`), publishes no created-token referent - // (`chain_prior_referent_is_created_token` — Populate has no `target` field but - // DOES publish one; Token/CopyTokenOf are excluded for free since both DO have - // a `target_filter()`), and there is no "if you do" object anchor - // (`if_you_do_object_anchor`, computed above — a GenericEffect referent - // surfaced only through its granted static's `affected` field, not its own - // `target`). With none of those three antecedents present, there is nothing - // for `ParentTarget` to resolve to, so a conditional "sacrifice it" tail - // (Gemstone Mine's "{T}, Remove a mining counter: Add one mana of any color. If - // there are no mining counters on this land, sacrifice it.") silently never - // sacrifices anything. Rebind it to the ability's own source, the only object - // "it" can plausibly name here. + // is false, and the fallback must not fire, when ANY of the established + // antecedent authorities finds a referent: + // - `parent_target_available` (computed above) — a chosen typed target + // anywhere in this chain, an `if you do` object anchor, OR an outer + // chain's referent inherited into a recursively-parsed `Otherwise` + // else-branch via `ctx.parent_target_available` (Brilliance Unleashed's + // class). Omitting this last case would let an else-branch whose own + // internal clauses are targetless rebind a valid OUTER `ParentTarget` to + // `SelfRef`, sacrificing the ability's source instead of the object + // selected before the paired conditional. + // - `chain_prior_referent_is_created_token` — a token/copy publisher + // (`Effect::Populate` has no `target` field but DOES publish a + // created-token referent; `Token`/`CopyTokenOf` are covered by + // `parent_target_available`'s typed-referent scan for free since both DO + // have a `target_filter()`). + // - the nearest non-`Continue` clause (an absorbed rider carries no + // independent referent, so it must not be mistaken for a targetless + // antecedent — mirrors `if_you_do_object_anchor`'s same lookback) + // exposing SOME target concept (`target_filter().is_some()`). + // With none of those present, there is nothing for `ParentTarget` to resolve + // to, so a conditional "sacrifice it" tail (Gemstone Mine's "{T}, Remove a + // mining counter: Add one mana of any color. If there are no mining counters + // on this land, sacrifice it.") silently never sacrifices anything. Rebind it + // to the ability's own source, the only object "it" can plausibly name here. // // Deliberately scoped to `Effect::Sacrifice` only (not folded into the // Kicker-clause rewrite's gate above): that gate is shared by every // `replace_target_with_parent`-eligible effect kind, and narrowing it there // regressed unrelated cards — see the comment above. - let prior_clause_has_no_target_concept = builder + let nearest_semantic_clause = builder .clauses() - .last() - .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()) + .iter() + .rev() + .find(|clause| !matches!(clause.disposition, ClauseDisposition::Continue { .. })); + let prior_clause_has_no_target_concept = !parent_target_available && !chain_prior_referent_is_created_token(builder.clauses()) - && if_you_do_anchor.is_none(); + && nearest_semantic_clause + .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()); if condition.is_some() && !is_distributed_chunk && prior_clause_has_no_target_concept { if let Effect::Sacrifice { target, .. } = &mut clause.effect { if matches!(target, TargetFilter::ParentTarget) { diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index ca647ced04..f76c015079 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -18747,6 +18747,49 @@ fn populate_conditional_sacrifice_keeps_created_token_not_self_ref() { ); } +/// Issue #6507 review follow-up (HIGH): an `Otherwise` else-branch is parsed +/// as its own recursive `parse_effect_chain_ir` call whose own `clauses` start +/// empty (see `parse_effect_chain_ir`'s "Otherwise" handler) — so a naive +/// "does the immediately-preceding INTERNAL clause expose a target?" check +/// can never see an outer referent established BEFORE the paired conditional +/// (Brilliance Unleashed's class). That outer referent is exactly what +/// `ctx.parent_target_available` carries across the recursive call for. Drive +/// the recursive parser directly with `parent_target_available: true` seeded +/// (mirroring what the "Otherwise" handler seeds from a real outer typed +/// target) to prove the no-antecedent Sacrifice fallback defers to it: a +/// targetless inner clause (`create an emblem` has no `target` field, so it +/// looks exactly like Gemstone Mine's untargeted mana ability in isolation) +/// followed by a conditional "sacrifice it" must still resolve to +/// `ParentTarget` — inheriting the outer referent — not get misrouted to +/// `SelfRef` (the ability's own source). +#[test] +fn conditional_sacrifice_defers_to_seeded_parent_target_available() { + let mut ctx = ParseContext { + parent_target_available: true, + ..Default::default() + }; + let ability = parse_effect_chain_with_context( + "create an emblem. If it wasn't kicked, sacrifice it.", + AbilityKind::Spell, + &mut ctx, + ); + assert!(matches!(&*ability.effect, Effect::Unimplemented { .. })); + let sac = ability + .sub_ability + .as_ref() + .expect("expected the conditional sacrifice chained after the targetless clause"); + let Effect::Sacrifice { target, .. } = &*sac.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sac.effect); + }; + assert_eq!( + *target, + TargetFilter::ParentTarget, + "with parent_target_available seeded (simulating an inherited outer \ + referent from an enclosing Otherwise chain), sacrifice must keep \ + ParentTarget, not fall back to SelfRef" + ); +} + #[test] fn kicker_leading_instead_produces_correct_condition() { // CR 608.2c: "if kicked, instead [effect]" — leading "instead" variant.