From 2e9db496d96a48a0554ad504a345d56d8b5c75a6 Mon Sep 17 00:00:00 2001 From: michiot05 <281539540+michiot05@users.noreply.github.com> Date: Fri, 24 Jul 2026 23:52:49 +0200 Subject: [PATCH 1/4] fix(engine): a player-directed "exile a creature they control" is a choice, not a target (Strategic Betrayal) Strategic Betrayal ("Target opponent exiles a creature they control and their graveyard.") triggered Ward on the opponent's own creature: the "exile a creature they control" clause resolved as a TARGETED exile with the wrong actor, instead of a resolution-time choice the target opponent makes. Per CR 115.10a/b a non-"target" object is not targeted (Ward, CR 702.21a, must not fire) and CR 109.4 makes the target player the actor. Two coupled defects, fixed at their seams: - Parser origin leak: a trailing "and their graveyard" conjunct leaked origin:Graveyard onto the creature leg. Origin is now inferred from the primary clause slice only (compound_exile_origin_scan), applied uniformly across the three compound-exile sites. - Target-vs-choice + actor: the "target player exiles ..." arm now pins the anaphoric "they control" scope to ScopedPlayer (unifying the creature and graveyard legs) and stamps Resolution timing on the battlefield pick, so it is a non-targeted player-directed choice. At resolution, a single Player-target ability with a ScopedPlayer-scoped move binds scoped_player to the target player (sibling of the existing trigger-event stamp), so the target opponent is the chooser and their own graveyard is exiled. Reuses existing mechanisms only (no new enum variant); controller_for_relative_filter, sacrifice.rs, and rebind_owned_scope are untouched. The fix generalizes to Leadership Vacuum ("target player returns each commander they control"). Closes #6505 Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/effects/mod.rs | 38 ++ crates/engine/src/game/stack.rs | 25 + .../src/parser/oracle_effect/imperative.rs | 7 +- .../engine/src/parser/oracle_effect/lower.rs | 5 +- crates/engine/src/parser/oracle_effect/mod.rs | 122 +++- crates/engine/src/parser/oracle_tests.rs | 9 +- crates/engine/tests/integration/main.rs | 1 + .../integration/strategic_betrayal_6505.rs | 634 ++++++++++++++++++ 8 files changed, 823 insertions(+), 18 deletions(-) create mode 100644 crates/engine/tests/integration/strategic_betrayal_6505.rs diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index df0396d9f0..41d7791fc8 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -5925,6 +5925,44 @@ fn filter_uses_relative_controller_scoped(filter: &TargetFilter) -> bool { } } +/// CR 109.4 + CR 115.10a (issue #6505): True when this ability's effect — or any +/// effect in its sub-ability / else-ability chain — carries a `ScopedPlayer`- +/// scoped move-object filter ("a creature they control" / "their graveyard"). +/// Used by `stack::resolve_top` to bind the resolution-time scoped player to the +/// ability's single player target, so a "target opponent exiles a creature they +/// control" chooser is the target opponent, not the caster. +pub(crate) fn ability_uses_relative_controller_scoped(ability: &ResolvedAbility) -> bool { + fn effect_moves_scoped_filter(effect: &Effect) -> bool { + if effect + .target_filter() + .is_some_and(filter_uses_relative_controller_scoped) + { + return true; + } + // `Effect::target_filter()` does not surface `ChangeZoneAll`'s mass + // filter, so check it explicitly (the "their graveyard" leg). + matches!( + effect, + Effect::ChangeZoneAll { target, .. } if filter_uses_relative_controller_scoped(target) + ) + } + let mut node = Some(ability); + while let Some(current) = node { + if effect_moves_scoped_filter(¤t.effect) { + return true; + } + if current + .else_ability + .as_deref() + .is_some_and(ability_uses_relative_controller_scoped) + { + return true; + } + node = current.sub_ability.as_deref(); + } + false +} + pub(crate) fn controller_for_relative_filter( ability: &ResolvedAbility, target_filter: &TargetFilter, diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index 791718071d..d68d302238 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -459,6 +459,31 @@ pub fn resolve_top(state: &mut GameState, events: &mut Vec) { } } + // CR 109.4 + CR 115.10a/b (issue #6505): "Target opponent exiles a creature + // they control and their graveyard" (Strategic Betrayal). The spell targets + // ONLY the opponent (CR 115.1a); that opponent then CHOOSES a creature they + // control and exiles their graveyard — so a `ScopedPlayer`-scoped move-object + // filter must resolve its acting/choosing player against the resolved single + // player target, not the caster. Sibling of the DamageDealt/AttackersDeclared + // scoped-player stamp above: bind `scoped_player` from the ability's lone + // `TargetRef::Player` before the change_zone choosers run at resolution. + if let Some(ability) = ability.as_mut() { + if ability.scoped_player.is_none() { + let single_player_target = ability + .targets + .iter() + .filter(|target| matches!(target, TargetRef::Player(_))) + .count() + == 1; + if single_player_target + && crate::game::effects::ability_uses_relative_controller_scoped(ability) + { + let actor = ability.target_player(); + ability.set_scoped_player_recursive(actor); + } + } + } + // CR 608.2c: Re-stamp ParentTarget anaphora from the stack entry's trigger // event at resolution time (Stationed/VehicleCrewed/Saddled/attack batches). // Push-time seeding in `push_pending_trigger_to_stack_with_event_batch` can diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 36a6fcf6f0..1b6ee07f7c 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -8377,7 +8377,12 @@ pub(super) fn parse_exile_ast( } else { parsed_target }; - let origin = super::infer_origin_zone(rest_lower); + // CR 400.7 (issue #6505): infer the origin zone excluding ONLY a trailing + // compound conjunct ("and their graveyard", Strategic Betrayal) so its + // sibling graveyard leg cannot leak Zone::Graveyard onto the battlefield + // creature leg — while a non-"and" qualifier ("instead of putting it into + // its owner's graveyard") still defines this leg's origin. + let origin = super::infer_origin_zone(&super::compound_exile_origin_scan(rest_text, rem)); Some(ZoneCounterImperativeAst::Exile { origin, target, diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index ce956a53b7..8cf655538f 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -1611,7 +1611,10 @@ fn filter_mentions_exiled_by_source(filter: &TargetFilter) -> bool { /// CR 115.1: True when a `ChangeZone` clause selects from the battlefield /// (explicitly or by permanent-type default) rather than a private/off-BF zone. -fn change_zone_selects_battlefield_permanent(origin: Option, target: &TargetFilter) -> bool { +pub(super) fn change_zone_selects_battlefield_permanent( + origin: Option, + target: &TargetFilter, +) -> bool { if target.is_context_ref() { return false; } diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index a5523736d1..2a45e6e9db 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -15097,8 +15097,10 @@ fn try_parse_verb_and_target<'a>( )); } - // Exile: infer origin zone from the full post-verb text (NOT the remainder, - // since parse_zone_suffix inside parse_type_phrase consumes zone phrases). + // Exile: infer origin zone from the primary target clause the target parser + // consumed (see the `infer_origin_zone` call below) — NOT the bare remainder + // (parse_zone_suffix inside parse_type_phrase strips zone phrases off it) and + // NOT the full post-verb text (a trailing compound conjunct would leak). if let Some((_, rest)) = nom_on_lower(text, lower, |i| { value((), alt((tag("exile all "), tag("exile each ")))).parse(i) }) { @@ -15140,7 +15142,11 @@ fn try_parse_verb_and_target<'a>( } else { parsed_target }; - let origin = infer_origin_zone(rest_lower); + // CR 400.7 (issue #6505): infer the origin excluding ONLY a trailing + // compound conjunct ("and their graveyard") so its sibling zone cannot + // leak onto the primary battlefield leg — a non-"and" qualifier still + // scopes the primary leg's origin. + let origin = infer_origin_zone(&compound_exile_origin_scan(rest, rem)); return Some(( TargetedImperativeAst::ZoneCounterProxy(Box::new(ZoneCounterImperativeAst::Exile { origin, @@ -15162,7 +15168,12 @@ fn try_parse_verb_and_target<'a>( } else { parsed_target }; - let origin = infer_origin_zone(rest_lower); + // CR 400.7 (issue #6505): infer the origin excluding ONLY a trailing + // compound conjunct ("and their graveyard") so it cannot leak + // Zone::Graveyard onto the battlefield leg — a non-"and" qualifier + // ("instead of putting it into its owner's graveyard", No More Lies) + // still defines this leg's origin. + let origin = infer_origin_zone(&compound_exile_origin_scan(rest, rem)); return Some(( TargetedImperativeAst::ZoneCounterProxy(Box::new(ZoneCounterImperativeAst::Exile { origin, @@ -18904,7 +18915,37 @@ fn lower_subject_predicate_ast( enters_under: None, }); } + // CR 115.10a + CR 109.4 (issue #6505): lower the predicate with the + // relative player scope pinned to `ScopedPlayer` so an anaphoric + // "they control" on a moved-object leg ("target opponent exiles a + // creature they control …", Strategic Betrayal) resolves to the + // acting scoped player (oracle_target::parse_controller_suffix reads + // `relative_player_scope.unwrap_or(You)`), unifying with the already + // scope-agnostic "their graveyard" leg (`Owned{ScopedPlayer}`). The + // save/restore is tight around ONLY this lowering call — the scope is + // restored before any predicate-shape handling below runs, so it + // cannot leak into unrelated re-parsing. The runtime binds this + // scoped filter to the real target player at resolution (see the + // single-Player-target `scoped_player` stamp in `stack::resolve_top`). + // + // Guarded tightly to this arm: pin the scope ONLY when (a) no outer + // relative scope is already established — a trigger/vote/antecedent + // context sets its own ("that player", damage-all, chosen-player), + // and clobbering it mis-binds those — AND (b) the subject is an + // explicit player target (the same precondition as the moved-object + // wrapper below). Without these guards ScopedPlayer leaks into every + // subject-predicate imperative-fallback clause. + let pin_scoped_player = ctx.relative_player_scope.is_none() + && subject + .target + .as_ref() + .is_some_and(target_filter_can_target_player); + let saved_relative_scope = ctx.relative_player_scope.clone(); + if pin_scoped_player { + ctx.relative_player_scope = Some(ControllerRef::ScopedPlayer); + } let mut clause = lower_imperative_clause(&text, ctx); + ctx.relative_player_scope = saved_relative_scope; // CR 608.2c + CR 109.4 + CR 115.1: "target 's controller/owner // s it" (Arcum Dagsson, Mercy Killing). `parse_subject_application` // records this possessive shift as `affected = ParentTargetController/ @@ -19019,6 +19060,23 @@ fn lower_subject_predicate_ast( clause.effect, Effect::ChangeZone { .. } | Effect::ChangeZoneAll { .. } ) { + // CR 115.1a + CR 702.21a (issue #6505): distinguish a + // battlefield-wide, NON-targeted resolution pick ("target + // opponent exiles a creature they control", Strategic + // Betrayal) from an off-battlefield or explicitly-targeted + // moved-object leg. Mirror `target_choice_timing_for_clause` + // in lower.rs: a battlefield-selecting ChangeZone with no + // "target " token has its object CHOSEN at resolution, so it + // never becomes a target and Ward on the opponent's own + // creature never fires. + let creature_leg_is_resolution_pick = match &clause.effect { + Effect::ChangeZone { origin, target, .. } + | Effect::ChangeZoneAll { origin, target, .. } => { + lower::change_zone_selects_battlefield_permanent(*origin, target) + && !scan_contains_phrase(&pred_lower, "target ") + } + _ => false, + }; // CR 109.4 (issue #1077): "target player exiles a card // from their graveyard" (Relic of Progenitus, Scrabbling // Claws, Merrow Bonegnawer, Graveyard Shovel, Grave @@ -19026,18 +19084,23 @@ fn lower_subject_predicate_ast( // [zone]" (Memory's Journey, above). The moved-object // filter's possessive "their" parses as // `Owned { controller: ScopedPlayer }` because the - // zone-suffix parser is scope-agnostic — but this branch - // only runs for an explicit "target player" (not an - // anaphoric "that player"), so the filter must be rebound - // to the real declared target before it's cloned into the - // sub-ability, or it stays scoped to the acting player - // (the activator) instead of the player just targeted. - match &mut clause.effect { - Effect::ChangeZone { target, .. } - | Effect::ChangeZoneAll { target, .. } => { - rebind_owned_scope(target, ControllerRef::TargetPlayer); + // zone-suffix parser is scope-agnostic. An off-battlefield + // or targeted leg selects its object on the STACK — before + // the resolution-time `scoped_player` stamp lands — so the + // filter must be rebound to the real declared target now, or + // it stays scoped to the activator instead of the targeted + // player. A battlefield resolution pick (issue #6505) keeps + // `ScopedPlayer` and is bound at resolution instead, so its + // acting/choosing player is the target opponent, not the + // caster (CR 115.10a — being affected is not choosing). + if !creature_leg_is_resolution_pick { + match &mut clause.effect { + Effect::ChangeZone { target, .. } + | Effect::ChangeZoneAll { target, .. } => { + rebind_owned_scope(target, ControllerRef::TargetPlayer); + } + _ => {} } - _ => {} } let mut sub_ability = AbilityDefinition::new(AbilityKind::Spell, clause.effect.clone()); @@ -19049,6 +19112,16 @@ fn lower_subject_predicate_ast( // wrapper. Carry it onto the sub-ability so the cast // surfaces N card slots. sub_ability.multi_target = clause.multi_target; + // CR 115.1a + CR 702.21a (issue #6505): a battlefield + // resolution pick is chosen at resolution and never becomes a + // target — stamp Resolution timing (the default is Stack) so + // no BecomesTarget event fires and Ward on the chosen creature + // stays silent. Off-battlefield/targeted legs keep the Stack + // default so their stack-time selection is unchanged. + if creature_leg_is_resolution_pick { + sub_ability.target_choice_timing = + crate::types::ability::TargetChoiceTiming::Resolution; + } return ParsedEffectClause { effect: Effect::TargetOnly { target: player_target.clone(), @@ -31682,6 +31755,25 @@ fn parse_put_rest_library_position(lower: &str) -> Option { } } +/// CR 400.7 (issue #6505): produce the lowercase text `infer_origin_zone` should +/// scan for a (possibly compound) exile. Exclude ONLY a trailing COMPOUND +/// conjunct (`" and "`) — e.g. "exile a creature they control AND +/// their graveyard", whose sibling graveyard leg must not leak `Zone::Graveyard` +/// onto the primary battlefield leg. A non-"and" remainder legitimately QUALIFIES +/// the primary leg ("exile it … instead of putting it into its owner's graveyard", +/// No More Lies) and stays in scope. `rem` is a suffix of `base` (a +/// `parse_target` remainder), so `base.len() - rem.len()` lands on a valid UTF-8 +/// boundary; scan the ORIGINAL-CASE base then lowercase. +pub(super) fn compound_exile_origin_scan(base: &str, rem: &str) -> String { + let rem_head = rem.trim_start(); + let trailing_and_conjunct = tag::<_, _, OracleError<'_>>("and ").parse(rem_head).is_ok(); + if trailing_and_conjunct { + base[..base.len() - rem.len()].to_ascii_lowercase() + } else { + base.to_ascii_lowercase() + } +} + fn infer_origin_zone(lower: &str) -> Option { // CR 400.7: An object that moves from one zone to another becomes a new // object — the "from" prepositional phrase identifies that origin zone. diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 392e163a60..cdd9da7537 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -3687,7 +3687,14 @@ fn leadership_vacuum_returns_target_players_commanders_to_command_zone() { destination, target: TargetFilter::Typed(TypedFilter { - controller: Some(ControllerRef::You), + // CR 109.4 + CR 115.10a (issue #6505): "each commander THEY + // control" is the TARGET player's commanders, not the + // caster's. The anaphoric "they" now lowers to `ScopedPlayer` + // (bound to the declared target player by the resolution-time + // `scoped_player` stamp), fixing the same class as Strategic + // Betrayal. Previously it wrongly resolved to `You` (the + // caster), contradicting this test's own name. + controller: Some(ControllerRef::ScopedPlayer), properties, .. }), diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index fac0eefaf6..95c24e8161 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -814,6 +814,7 @@ mod std_s07_damage_doublers; mod steadfast_armasaur_lki_toughness; mod steelform_sliver_toughness_anthem; mod stensian_sanguinist_prepare; +mod strategic_betrayal_6505; mod summer_bloom_5979; mod sun_droplet_remove_counter_infeasible_4776; mod support; diff --git a/crates/engine/tests/integration/strategic_betrayal_6505.rs b/crates/engine/tests/integration/strategic_betrayal_6505.rs new file mode 100644 index 0000000000..618c4730e9 --- /dev/null +++ b/crates/engine/tests/integration/strategic_betrayal_6505.rs @@ -0,0 +1,634 @@ +//! Issue #6505: Strategic Betrayal ({1}{B} Sorcery) — verbatim Oracle text: +//! "Target opponent exiles a creature they control and their graveyard." +//! +//! Rules-correct behavior (CR 115.1a + CR 115.10a/b + CR 702.21a + CR 109.4): +//! the spell targets ONLY the opponent (CR 115.1a). That opponent then CHOOSES +//! — does not TARGET — a creature THEY control to exile, and exiles THEIR +//! graveyard. Because the creature is chosen at resolution rather than targeted, +//! no `BecomesTarget` event fires, so Ward on the opponent's OWN creature never +//! triggers (CR 115.10a: being affected by a spell is not being targeted). +//! +//! The bug had two halves: +//! (A) origin-inference leaked `Zone::Graveyard` onto the battlefield-creature +//! leg (a trailing "and their graveyard" conjunct polluted the scan), and +//! (B) the creature leg was emitted as a `Stack` target (Ward) chosen by the +//! wrong actor (the caster instead of the target opponent). +//! +//! These tests are per-mechanism revert-sensitive; each notes which revert flips +//! it (see the file header in the PR for the RED→GREEN matrix). + +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::parser::oracle::parse_oracle_text; +use engine::types::ability::{ + AbilityDefinition, ControllerRef, Effect, TargetChoiceTiming, TargetFilter, +}; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::keywords::{Keyword, WardCost}; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +/// Strategic Betrayal — verbatim Oracle text (used to build every test card). +const SB_ORACLE: &str = "Target opponent exiles a creature they control and their graveyard."; + +// --------------------------------------------------------------------------- +// Shared helpers +// --------------------------------------------------------------------------- + +/// Walk an ability chain (effect + sub_ability + else_ability) for any +/// `Effect::Unimplemented` — the coverage-honesty reach-guard shared by every +/// positive test so a vacuous pass (parse failed to a strict `Unimplemented`) +/// is impossible. +fn chain_has_unimplemented(def: &AbilityDefinition) -> bool { + fn effect_unimplemented(effect: &Effect) -> bool { + matches!(effect, Effect::Unimplemented { .. }) + } + effect_unimplemented(&def.effect) + || def + .sub_ability + .as_deref() + .is_some_and(chain_has_unimplemented) + || def + .else_ability + .as_deref() + .is_some_and(chain_has_unimplemented) +} + +/// The single parsed spell ability for a verbatim sorcery body. +fn parse_sb() -> AbilityDefinition { + let parsed = parse_oracle_text(SB_ORACLE, "Strategic Betrayal", &[], &[], &[]); + assert_eq!( + parsed.abilities.len(), + 1, + "SB parses to exactly one spell ability; got {:#?}", + parsed.abilities + ); + parsed.abilities.into_iter().next().unwrap() +} + +/// Find the first `ChangeZone` (single, the creature leg) in a chain. +fn find_change_zone(def: &AbilityDefinition) -> Option<&AbilityDefinition> { + let mut cursor = Some(def); + while let Some(node) = cursor { + if matches!(*node.effect, Effect::ChangeZone { .. }) { + return Some(node); + } + cursor = node.sub_ability.as_deref(); + } + None +} + +/// Find the first `ChangeZoneAll` (mass, the graveyard leg) in a chain. +fn find_change_zone_all(def: &AbilityDefinition) -> Option<&AbilityDefinition> { + let mut cursor = Some(def); + while let Some(node) = cursor { + if matches!(*node.effect, Effect::ChangeZoneAll { .. }) { + return Some(node); + } + cursor = node.sub_ability.as_deref(); + } + None +} + +fn ward2_creature( + scenario: &mut GameScenario, + controller: engine::types::player::PlayerId, + p: i32, + t: i32, + name: &str, +) -> ObjectId { + scenario + .add_creature(controller, name, p, t) + .with_keyword(Keyword::Ward(WardCost::Mana(ManaCost::generic(2)))) + .id() +} + +fn graveyard_ids(runner: &GameRunner, player: engine::types::player::PlayerId) -> Vec { + runner.state().players[player.0 as usize] + .graveyard + .iter() + .copied() + .collect() +} + +// --------------------------------------------------------------------------- +// 1. END-TO-END — the headline behavior. +// --------------------------------------------------------------------------- + +/// Opponent controls TWO creatures (one warded) plus graveyard cards. Casting +/// SB on the opponent must: (a) raise NO Ward payment/choice; (b) prompt the +/// OPPONENT with an `EffectZoneChoice` offering its two battlefield creatures; +/// (c) exile the chosen creature; (d) exile the opponent's whole graveyard. +/// +/// Revert sensitivity: reverting the timing stamp flips (a) to a Ward +/// `UnlessPayment`; reverting the `scoped_player` stamp flips (b)'s player to +/// the caster; reverting the origin slice flips (b)'s offered set to graveyard. +#[test] +fn end_to_end_opponent_chooses_creature_and_loses_graveyard_no_ward() { + // Reach-guard: the spell parses with zero Unimplemented before we cast it. + let sb = parse_sb(); + assert!( + !chain_has_unimplemented(&sb), + "SB must parse with no strict-failure Unimplemented; got {sb:#?}" + ); + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let plain = scenario.add_creature(P1, "Opp Plain Beast", 2, 2).id(); + let warded = ward2_creature(&mut scenario, P1, 3, 3, "Opp Warded Beast"); + scenario.with_graveyard(P1, &["Grave Relic A", "Grave Relic B"]); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Strategic Betrayal", false, SB_ORACLE) + .id(); + + let mut runner = scenario.build(); + let gy = graveyard_ids(&runner, P1); + assert_eq!(gy.len(), 2, "opponent graveyard seeded with two cards"); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + + // (a) + (b): no Ward, opponent is the chooser, both battlefield creatures + // offered — and NOT the graveyard cards. + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { player, cards, .. } => { + assert_eq!(*player, P1, "the OPPONENT chooses the exiled creature"); + assert!( + cards.contains(&plain) && cards.contains(&warded), + "both of the opponent's battlefield creatures must be offered; got {cards:?}" + ); + for g in &gy { + assert!( + !cards.contains(g), + "graveyard cards must not be offered to the creature leg; got {cards:?}" + ); + } + } + WaitingFor::UnlessPayment { .. } + | WaitingFor::WardSacrificeChoice { .. } + | WaitingFor::WardDiscardChoice { .. } => { + panic!( + "Ward must NOT fire — the creature is chosen at resolution, not targeted; got {:?}", + outcome.final_waiting_for() + ); + } + other => panic!("expected the opponent's creature EffectZoneChoice; got {other:?}"), + } + + // (c): choose the plain creature. + runner + .act(GameAction::SelectCards { cards: vec![plain] }) + .expect("opponent selects a creature to exile"); + + assert_eq!( + runner.state().objects[&plain].zone, + Zone::Exile, + "the chosen creature is exiled" + ); + assert_eq!( + runner.state().objects[&warded].zone, + Zone::Battlefield, + "the unchosen creature stays on the battlefield" + ); + // (d): the opponent's graveyard is exiled wholesale. + for g in &gy { + assert_eq!( + runner.state().objects[g].zone, + Zone::Exile, + "every card in the opponent's graveyard is exiled" + ); + } + assert!( + graveyard_ids(&runner, P1).is_empty(), + "the opponent's graveyard is now empty" + ); +} + +// --------------------------------------------------------------------------- +// 2. B(1)(c) ALONE — Ward isolation via the Resolution timing stamp. +// --------------------------------------------------------------------------- + +/// The creature leg parses as a Resolution-timed, non-targeted pick (SHAPE), and +/// at runtime a lone warded creature raises NO Ward. Reverting ONLY the timing +/// stamp restores `TargetChoiceTiming::Stack`, which re-introduces the +/// `BecomesTarget` → Ward `UnlessPayment` path. +#[test] +fn creature_leg_is_resolution_chosen_so_ward_never_fires() { + // SHAPE: creature leg carries Resolution timing (default is Stack). + let sb = parse_sb(); + let creature = find_change_zone(&sb).expect("SB has a single-creature ChangeZone leg"); + assert_eq!( + creature.target_choice_timing, + TargetChoiceTiming::Resolution, + "the creature leg must be resolution-chosen (non-targeted), not a Stack target" + ); + + // Runtime: a single warded creature (auto-selected once chosen at + // resolution) must not produce a Ward payment window. + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let warded = ward2_creature(&mut scenario, P1, 3, 3, "Lone Warded Beast"); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Strategic Betrayal", false, SB_ORACLE) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + assert!( + !matches!( + outcome.final_waiting_for(), + WaitingFor::UnlessPayment { .. } + | WaitingFor::WardSacrificeChoice { .. } + | WaitingFor::WardDiscardChoice { .. } + ), + "Ward must stay silent on a resolution-chosen creature; got {:?}", + outcome.final_waiting_for() + ); + // Reach-guard: the single warded creature was exiled (we reached resolution, + // proving the no-Ward assertion is not vacuous). + assert_eq!( + runner.state().objects[&warded].zone, + Zone::Exile, + "the lone warded creature is exiled at resolution" + ); +} + +// --------------------------------------------------------------------------- +// 3. B(2) ALONE — chooser isolation via the scoped_player stamp. +// --------------------------------------------------------------------------- + +/// The OPPONENT (not the caster) is the chooser of the exiled creature. +/// Reverting ONLY the `scoped_player` stamp in `stack::resolve_top` flips the +/// `EffectZoneChoice.player` back to the caster. +#[test] +fn opponent_not_caster_is_the_chooser() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + // Two creatures so the choice is a real prompt (not an auto-pick). + scenario.add_creature(P1, "Opp A", 2, 2); + scenario.add_creature(P1, "Opp B", 2, 2); + // The caster also controls creatures — if the chooser wrongly resolved to + // the caster, its own creatures (not the opponent's) would be offered. + let caster_creature = scenario.add_creature(P0, "Caster Own", 4, 4).id(); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Strategic Betrayal", false, SB_ORACLE) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { player, cards, .. } => { + assert_eq!( + *player, P1, + "the target OPPONENT is the chooser, not the caster" + ); + assert!( + !cards.contains(&caster_creature), + "the caster's own creature must never be an eligible pick; got {cards:?}" + ); + } + other => panic!("expected the opponent's EffectZoneChoice; got {other:?}"), + } +} + +// --------------------------------------------------------------------------- +// 4. PART A ALONE — the eligible set is the BATTLEFIELD, not the graveyard. +// --------------------------------------------------------------------------- + +/// With graveyard cards present, the creature leg still offers BATTLEFIELD +/// creatures. Reverting the origin slice (letting the trailing "and their +/// graveyard" leak `Zone::Graveyard` onto the creature leg) would make the +/// eligible set the graveyard cards instead. Bites imperative.rs:8380. +#[test] +fn creature_leg_selects_from_battlefield_not_graveyard() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let bf_a = scenario.add_creature(P1, "BF Creature A", 2, 2).id(); + let bf_b = scenario.add_creature(P1, "BF Creature B", 2, 2).id(); + scenario.with_graveyard(P1, &["GY Card X", "GY Card Y"]); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Strategic Betrayal", false, SB_ORACLE) + .id(); + let mut runner = scenario.build(); + let gy = graveyard_ids(&runner, P1); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { + player, + cards, + zone, + .. + } => { + assert_eq!(*player, P1); + assert_eq!( + *zone, + Zone::Battlefield, + "the creature leg must select from the battlefield" + ); + assert!( + cards.contains(&bf_a) && cards.contains(&bf_b), + "battlefield creatures are the eligible set; got {cards:?}" + ); + for g in &gy { + assert!( + !cards.contains(g), + "graveyard cards must not be eligible for the creature leg; got {cards:?}" + ); + } + } + other => panic!("expected a battlefield EffectZoneChoice; got {other:?}"), + } +} + +// --------------------------------------------------------------------------- +// 5. CASTER-CHOOSER control (the BLOCKING-2 negative). +// --------------------------------------------------------------------------- + +/// A caster-subject "you control" exile keeps the CASTER as chooser — the +/// scope-set and scoped_player stamp key on a single Player TARGET plus a +/// ScopedPlayer filter, neither of which a "you control" imperative produces. +#[test] +fn caster_subject_exile_keeps_caster_as_chooser() { + const CASTER_EXILE: &str = "Exile a creature you control and their graveyard."; + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + // Two caster creatures so the caster's pick is a real prompt. + let own_a = scenario.add_creature(P0, "Own A", 2, 2).id(); + let own_b = scenario.add_creature(P0, "Own B", 2, 2).id(); + // An opponent creature which must NEVER be offered to the caster. + let opp = scenario.add_creature(P1, "Opp Creature", 5, 5).id(); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Betrayal Control", false, CASTER_EXILE) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).resolve(); + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { player, cards, .. } => { + assert_eq!(*player, P0, "the CASTER chooses their own creature"); + assert!( + cards.contains(&own_a) && cards.contains(&own_b), + "the caster's creatures are offered; got {cards:?}" + ); + assert!( + !cards.contains(&opp), + "the opponent's creature must not be offered to a 'you control' exile; got {cards:?}" + ); + } + other => panic!("expected the caster's own EffectZoneChoice; got {other:?}"), + } +} + +// --------------------------------------------------------------------------- +// 6. NEGATIVE — Ward is NOT over-suppressed. +// --------------------------------------------------------------------------- + +/// A plain "Exile target creature." still targets the warded creature, so Ward +/// STILL fires (the timing fix is scoped to the resolution-picked SB shape, not +/// to all exiles). Reach-guard: declining the Ward payment counters the spell, +/// so the creature is NOT exiled. +#[test] +fn plain_targeted_exile_still_triggers_ward() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let warded = ward2_creature(&mut scenario, P1, 3, 3, "Warded Target"); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Murder-Exile", true, "Exile target creature.") + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_object(warded).resolve(); + assert!( + matches!( + outcome.final_waiting_for(), + WaitingFor::UnlessPayment { .. } + ), + "Ward must fire on a genuinely targeted exile; got {:?}", + outcome.final_waiting_for() + ); + + // Reach-guard: decline the Ward cost — the exile spell is countered and the + // creature stays on the battlefield. + runner + .act(GameAction::PayUnlessCost { pay: false }) + .expect("declining Ward payment is legal"); + runner.advance_until_stack_empty(); + assert_eq!( + runner.state().objects[&warded].zone, + Zone::Battlefield, + "unpaid Ward counters the exile; the creature survives" + ); +} + +// --------------------------------------------------------------------------- +// 7. SIBLING regression — Diabolic Edict (sacrifice) is unchanged. +// --------------------------------------------------------------------------- + +/// "Target player sacrifices a creature." routes through the sacrifice path +/// (untouched by this fix). The target player is still the chooser and no Ward +/// fires (sacrifice is not targeting the creature). +#[test] +fn target_player_sacrifice_still_binds_chooser_no_ward() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_creature(P1, "Sac A", 2, 2); + let warded = ward2_creature(&mut scenario, P1, 3, 3, "Warded Sac Target"); + + let spell = scenario + .add_spell_to_hand_from_oracle( + P0, + "Diabolic Edict", + true, + "Target player sacrifices a creature.", + ) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { player, .. } => { + assert_eq!(*player, P1, "the target player chooses the sacrifice"); + } + WaitingFor::UnlessPayment { .. } + | WaitingFor::WardSacrificeChoice { .. } + | WaitingFor::WardDiscardChoice { .. } => panic!( + "sacrifice must not target the warded creature; got {:?}", + outcome.final_waiting_for() + ), + other => panic!("expected the target player's sacrifice choice; got {other:?}"), + } + // Reach-guard: the warded creature is still eligible to be sacrificed (the + // sacrifice path is not target-gated by Ward). + let WaitingFor::EffectZoneChoice { cards, .. } = outcome.final_waiting_for().clone() else { + unreachable!("asserted above"); + }; + assert!( + cards.contains(&warded), + "the warded creature is a legal sacrifice; got {cards:?}" + ); +} + +// --------------------------------------------------------------------------- +// 8. PARSER SHAPE (labeled SHAPE, semantic accessors). +// --------------------------------------------------------------------------- + +/// The full parsed shape: creature leg is a battlefield-origin, ScopedPlayer- +/// scoped, Resolution-timed ChangeZone; graveyard leg is a ChangeZoneAll whose +/// filter carries `Owned{ScopedPlayer}`; zero Unimplemented anywhere. +#[test] +fn shape_creature_scopedplayer_resolution_graveyard_owned_scopedplayer() { + let sb = parse_sb(); + assert!( + !chain_has_unimplemented(&sb), + "zero Unimplemented; got {sb:#?}" + ); + + // Root is the player TargetOnly wrapper. + assert!( + matches!(&*sb.effect, Effect::TargetOnly { target } if target_is_opponent_like(target)), + "root effect targets ONLY the opponent; got {:?}", + sb.effect + ); + + let creature = find_change_zone(&sb).expect("creature ChangeZone leg"); + // Battlefield origin (Part A): origin inference must NOT leak Graveyard. + if let Effect::ChangeZone { + origin, + target, + destination, + .. + } = &*creature.effect + { + assert!( + origin.is_none() || *origin == Some(Zone::Battlefield), + "creature leg origin is the battlefield (not Graveyard); got {origin:?}" + ); + assert_eq!(*destination, Zone::Exile, "creature leg exiles"); + assert!( + filter_controller_is_scoped(target), + "creature leg controller is ScopedPlayer; got {target:?}" + ); + } else { + panic!( + "expected ChangeZone creature leg; got {:?}", + creature.effect + ); + } + assert_eq!( + creature.target_choice_timing, + TargetChoiceTiming::Resolution, + "creature leg is resolution-chosen" + ); + + let graveyard = find_change_zone_all(&sb).expect("graveyard ChangeZoneAll leg"); + if let Effect::ChangeZoneAll { + target, + destination, + .. + } = &*graveyard.effect + { + assert_eq!(*destination, Zone::Exile, "graveyard leg exiles"); + assert!( + filter_owned_scoped(target), + "graveyard leg carries Owned{{ScopedPlayer}}; got {target:?}" + ); + } else { + panic!( + "expected ChangeZoneAll graveyard leg; got {:?}", + graveyard.effect + ); + } +} + +/// Hostile fixture: an explicit "target" on the creature keeps it a stack +/// target (no scope-set, no Resolution stamp) — and still parses cleanly. +#[test] +fn shape_explicit_target_creature_stays_stack_target() { + const EXPLICIT: &str = "Exile target creature and their graveyard."; + let parsed = parse_oracle_text(EXPLICIT, "Explicit Betrayal", &[], &[], &[]); + assert_eq!(parsed.abilities.len(), 1); + let root = &parsed.abilities[0]; + assert!( + !chain_has_unimplemented(root), + "explicit-target compound exile parses cleanly; got {root:#?}" + ); + if let Some(creature) = find_change_zone(root) { + assert_eq!( + creature.target_choice_timing, + TargetChoiceTiming::Stack, + "an explicit 'target creature' stays a Stack target" + ); + } +} + +/// Hostile fixture for Part A site 3 (mod.rs:15143): "Exile all creatures they +/// control and their graveyard." The mass-creature + whole-graveyard COMPOUND is +/// not modelled by the parser (a pre-existing gap unaffected by this fix — the +/// bare imperative never enters `lower_subject_predicate_ast`, and the site-3 +/// origin slice only changes the inferred zone, never whether the arm parses). +/// Coverage honesty (per the plan's fail-closed rule): the WHOLE clause must +/// stay an explicit strict failure carrying the full text — NOT a silently +/// narrowed partial that dropped the "and their graveyard" leg (a swallow bug). +#[test] +fn shape_exile_all_creatures_and_graveyard_stays_fail_closed() { + const MASS: &str = "Exile all creatures they control and their graveyard."; + let parsed = parse_oracle_text(MASS, "Mass Betrayal", &[], &[], &[]); + assert_eq!(parsed.abilities.len(), 1, "got {:#?}", parsed.abilities); + let root = &parsed.abilities[0]; + // The unmodelled compound stays fail-closed: an Unimplemented carrying the + // full sentence, not a partial that silently kept only one leg. + assert!( + matches!(&*root.effect, Effect::Unimplemented { description, .. } + if description.as_deref().is_some_and(|d| d.contains("their graveyard"))), + "the unmodelled mass-creature + graveyard compound must stay a whole-clause \ + strict failure (fail-closed, no silent orphan); got {root:#?}" + ); +} + +// --------------------------------------------------------------------------- +// SHAPE predicates +// --------------------------------------------------------------------------- + +fn target_is_opponent_like(target: &TargetFilter) -> bool { + matches!(target, TargetFilter::Opponent | TargetFilter::Player) + || matches!(target, TargetFilter::Typed(tf) if tf.controller == Some(ControllerRef::Opponent)) +} + +fn filter_controller_is_scoped(target: &TargetFilter) -> bool { + match target { + TargetFilter::Typed(tf) => tf.controller == Some(ControllerRef::ScopedPlayer), + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().any(filter_controller_is_scoped) + } + TargetFilter::Not { filter } => filter_controller_is_scoped(filter), + _ => false, + } +} + +fn filter_owned_scoped(target: &TargetFilter) -> bool { + use engine::types::ability::FilterProp; + match target { + TargetFilter::Typed(tf) => tf.properties.iter().any(|p| { + matches!( + p, + FilterProp::Owned { + controller: ControllerRef::ScopedPlayer + } + ) + }), + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().any(filter_owned_scoped) + } + TargetFilter::Not { filter } => filter_owned_scoped(filter), + _ => false, + } +} From b35b2a86c92186ea8df0f948ba099e25c8828f71 Mon Sep 17 00:00:00 2001 From: michiot05 <281539540+michiot05@users.noreply.github.com> Date: Sat, 25 Jul 2026 05:23:47 +0200 Subject: [PATCH 2/4] fix(engine): narrow the target-player exile scope to the moved-object leg; account for parse blast radius (#6505 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Strategic Betrayal fix pinned `relative_player_scope = ScopedPlayer` around the ENTIRE `lower_imperative_clause` call, so it re-scoped every "they control" predicate — including NON-ChangeZone effects (Sacrifice / PutCounter / PutCounterAll / UntapAll / bounce-repeat) that the runtime `scoped_player` stamp does not cover (`ability_uses_relative_controller_scoped` only matches ChangeZone/ChangeZoneAll). Those parse changes were unintended and latently broken (`ScopedPlayer` with no resolution-time binding). Narrowing (replaces the whole-clause parse-time pin): - Remove the `relative_player_scope = ScopedPlayer` save/restore around the predicate lowering. The predicate now lowers with its natural scope. - Apply the `ScopedPlayer` rebind POST-lowering to the moved-object leg ALONE: when the lowered leg is a battlefield resolution-pick ChangeZone/ChangeZoneAll (`creature_leg_is_resolution_pick`) whose anaphoric "they control" lowered to the default `ControllerRef::You`, rewrite ONLY that leg's `You` controller to `ScopedPlayer` (new `rebind_controller_scope(from, to)` general helper). Gated on the "they control" anaphor so a "you control" caster leg (also `You`) is never mis-rebound. The Resolution-timing stamp stays gated on `creature_leg_is_resolution_pick`, unchanged. `rebind_owned_scope`, `controller_for_relative_filter`, and `sacrifice.rs` are untouched. Effect: the Sacrifice / PutCounter / PutCounterAll / UntapAll / bounce-repeat predicates keep their original scope and DROP OUT of the parse-diff. Verified by a baseline(192897ca5)-vs-head focused parse harness over the full named blast radius: every non-ChangeZone card is `ScopedPlayer=0` on both sides (Consecrate//Consume, Consumed by Greed, Will of the Abzan, Riveteers Charm, Linessa Zephyr Mage, Hunted Nightmare, Shadrix Silverquill, Early Harvest). Accounted parse-diff (all correct, no regressions): - Part B (target-player "exile/move/return a permanent they control" → the moved-object leg is ScopedPlayer + Resolution): Strategic Betrayal, Debt to the Kami (both modes), Doomfall (creature mode), Early Winter (enchantment mode), Azula Cunning Usurper, Leadership Vacuum (mass "each commander they control"). Blot Out / End of the Hunt do NOT change — their "greatest mana value" superlative is a context-ref filter (not a resolution-pick), so they revert to the pre-existing (unchanged) baseline scope. - Part A (origin de-leak, `from: graveyard/library → ∅`): the trailing "and " no longer leaks its zone onto the primary battlefield/self leg. `∅` is a correct, safer parse: for a specific object the resolver keys on the object's actual zone and treats `origin` as an optional guard (`Some(x)` that mismatches → skip), so `∅` = "no constraint" exiles from the real zone — and actually FIXES Silent Gravestone (baseline `origin=Graveyard` would have skipped exiling its own battlefield artifact). Verified correct for Silent Gravestone, Spellweaver Volute, Gilded Ambusher, and (full-pipeline) Once More with Feeling, Shadows' Verdict, Ultimate Nullification. Dragon's Approach stays Unimplemented (no surfaced change). Also in this commit: - `compound_exile_origin_scan`: replace the manual `base[..base.len()-rem.len()]` slice (panics on a non-suffix / mid-UTF-8 `rem`) with a fail-closed `base.strip_suffix(rem).filter(|_| trailing_and_conjunct)`. - strategic_betrayal_6505.rs hostile fixture: make the creature-leg lookup REQUIRED (`expect`) so it can't pass vacuously. - New discriminating tests: a bare "target player exiles a creature they control" sibling runtime test (opponent is chooser, correct scope, no Ward); a Leadership Vacuum mass-leg ScopedPlayer shape test; and an origin-leak class test (Silent Gravestone's "exile this artifact and all cards from all graveyards" — primary leg stays battlefield origin). Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/parser/oracle_effect/mod.rs | 125 +++++++---- .../integration/strategic_betrayal_6505.rs | 196 +++++++++++++++++- 2 files changed, 274 insertions(+), 47 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 2a45e6e9db..9c345810f7 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7550,6 +7550,40 @@ fn rebind_owned_scope(filter: &mut TargetFilter, to: ControllerRef) { } } +/// Rewrite a moved-object filter's controller/owner scope `from` → `to`, recursing +/// through `And`/`Or`/`Not` composites. The general building block for controller- +/// scope rebinding; `rebind_owned_scope` above is the pre-existing +/// `ScopedPlayer → ` specialization kept as-is per the #6505 review. +/// +/// CR 109.4 + CR 115.10a (issue #6505): used to lift a battlefield resolution-pick +/// leg's default `You` scope (the anaphoric "they control" lowered without a +/// parse-time relative-scope pin) to `ScopedPlayer`, so the resolution-time +/// `scoped_player` stamp binds the chooser to the target player, not the caster. +fn rebind_controller_scope(filter: &mut TargetFilter, from: ControllerRef, to: ControllerRef) { + use crate::types::ability::FilterProp; + match filter { + TargetFilter::Typed(tf) => { + if tf.controller.as_ref() == Some(&from) { + tf.controller = Some(to.clone()); + } + for prop in tf.properties.iter_mut() { + if let FilterProp::Owned { controller } = prop { + if *controller == from { + *controller = to.clone(); + } + } + } + } + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + for f in filters.iter_mut() { + rebind_controller_scope(f, from.clone(), to.clone()); + } + } + TargetFilter::Not { filter } => rebind_controller_scope(filter, from, to), + _ => {} + } +} + fn try_parse_player_draws_and_gains_control( text: &str, ctx: &mut ParseContext, @@ -18915,37 +18949,17 @@ fn lower_subject_predicate_ast( enters_under: None, }); } - // CR 115.10a + CR 109.4 (issue #6505): lower the predicate with the - // relative player scope pinned to `ScopedPlayer` so an anaphoric - // "they control" on a moved-object leg ("target opponent exiles a - // creature they control …", Strategic Betrayal) resolves to the - // acting scoped player (oracle_target::parse_controller_suffix reads - // `relative_player_scope.unwrap_or(You)`), unifying with the already - // scope-agnostic "their graveyard" leg (`Owned{ScopedPlayer}`). The - // save/restore is tight around ONLY this lowering call — the scope is - // restored before any predicate-shape handling below runs, so it - // cannot leak into unrelated re-parsing. The runtime binds this - // scoped filter to the real target player at resolution (see the - // single-Player-target `scoped_player` stamp in `stack::resolve_top`). - // - // Guarded tightly to this arm: pin the scope ONLY when (a) no outer - // relative scope is already established — a trigger/vote/antecedent - // context sets its own ("that player", damage-all, chosen-player), - // and clobbering it mis-binds those — AND (b) the subject is an - // explicit player target (the same precondition as the moved-object - // wrapper below). Without these guards ScopedPlayer leaks into every - // subject-predicate imperative-fallback clause. - let pin_scoped_player = ctx.relative_player_scope.is_none() - && subject - .target - .as_ref() - .is_some_and(target_filter_can_target_player); - let saved_relative_scope = ctx.relative_player_scope.clone(); - if pin_scoped_player { - ctx.relative_player_scope = Some(ControllerRef::ScopedPlayer); - } + // NOTE (issue #6505, review follow-up): the predicate is lowered with + // NO parse-time relative-scope pin. An earlier revision pinned + // `relative_player_scope = ScopedPlayer` around this whole call, which + // re-scoped EVERY "they control" predicate (Sacrifice / Bounce / + // PutCounter / UntapAll …) — including non-ChangeZone effects the + // runtime `scoped_player` stamp does not cover, leaving them latently + // broken. The `ScopedPlayer` rebind is now applied post-lowering to the + // moved-object ChangeZone/ChangeZoneAll leg ALONE (see the resolution- + // pick rewrite in the player-target wrapper below), so sibling + // predicates keep their original scope. let mut clause = lower_imperative_clause(&text, ctx); - ctx.relative_player_scope = saved_relative_scope; // CR 608.2c + CR 109.4 + CR 115.1: "target 's controller/owner // s it" (Arcum Dagsson, Mercy Killing). `parse_subject_application` // records this possessive shift as `affected = ParentTargetController/ @@ -19089,11 +19103,38 @@ fn lower_subject_predicate_ast( // the resolution-time `scoped_player` stamp lands — so the // filter must be rebound to the real declared target now, or // it stays scoped to the activator instead of the targeted - // player. A battlefield resolution pick (issue #6505) keeps - // `ScopedPlayer` and is bound at resolution instead, so its + // player. A battlefield resolution pick (issue #6505) is + // instead bound to the scoped player at resolution, so its // acting/choosing player is the target opponent, not the // caster (CR 115.10a — being affected is not choosing). - if !creature_leg_is_resolution_pick { + // + // CR 109.4 + CR 115.10a (issue #6505, review follow-up): the + // battlefield resolution-pick leg's anaphoric "they control" + // lowered to the default `ControllerRef::You` + // (oracle_target::parse_controller_suffix, no scope pinned), + // so rewrite ONLY this moved-object leg's `You` scope to + // `ScopedPlayer` here — the narrow, targeted replacement for + // the removed whole-clause parse-time pin. The runtime + // `scoped_player` stamp (stack::resolve_top) then binds the + // chooser to the target player. Gated on the "they control" + // anaphor so a "you control" caster leg (also `You`) is never + // mis-rebound; the "their graveyard" leg is already + // `Owned{ScopedPlayer}` (scope-agnostic) and needs no rewrite. + let creature_leg_is_they_control_pick = creature_leg_is_resolution_pick + && scan_contains_phrase(&pred_lower, "they control"); + if creature_leg_is_they_control_pick { + match &mut clause.effect { + Effect::ChangeZone { target, .. } + | Effect::ChangeZoneAll { target, .. } => { + rebind_controller_scope( + target, + ControllerRef::You, + ControllerRef::ScopedPlayer, + ); + } + _ => {} + } + } else if !creature_leg_is_resolution_pick { match &mut clause.effect { Effect::ChangeZone { target, .. } | Effect::ChangeZoneAll { target, .. } => { @@ -31761,16 +31802,20 @@ fn parse_put_rest_library_position(lower: &str) -> Option { /// their graveyard", whose sibling graveyard leg must not leak `Zone::Graveyard` /// onto the primary battlefield leg. A non-"and" remainder legitimately QUALIFIES /// the primary leg ("exile it … instead of putting it into its owner's graveyard", -/// No More Lies) and stays in scope. `rem` is a suffix of `base` (a -/// `parse_target` remainder), so `base.len() - rem.len()` lands on a valid UTF-8 -/// boundary; scan the ORIGINAL-CASE base then lowercase. +/// No More Lies) and stays in scope. `rem` is normally a suffix of `base` (a +/// `parse_target` remainder); `strip_suffix` fails CLOSED (scan the whole base) if +/// it is not — never a panicking mid-UTF-8 / out-of-bounds manual slice. Scan the +/// ORIGINAL-CASE base then lowercase. pub(super) fn compound_exile_origin_scan(base: &str, rem: &str) -> String { let rem_head = rem.trim_start(); let trailing_and_conjunct = tag::<_, _, OracleError<'_>>("and ").parse(rem_head).is_ok(); - if trailing_and_conjunct { - base[..base.len() - rem.len()].to_ascii_lowercase() - } else { - base.to_ascii_lowercase() + // Structural fail-closed suffix strip of an ALREADY-parsed `parse_target` + // remainder (`rem`) off `base` — not parsing dispatch; the nom dispatch (the + // trailing-"and " conjunct test) is the `tag(...)` above. + // allow-noncombinator: strip a known parse remainder, not parser dispatch + match base.strip_suffix(rem).filter(|_| trailing_and_conjunct) { + Some(primary) => primary.to_ascii_lowercase(), + None => base.to_ascii_lowercase(), } } diff --git a/crates/engine/tests/integration/strategic_betrayal_6505.rs b/crates/engine/tests/integration/strategic_betrayal_6505.rs index 618c4730e9..501abc7016 100644 --- a/crates/engine/tests/integration/strategic_betrayal_6505.rs +++ b/crates/engine/tests/integration/strategic_betrayal_6505.rs @@ -561,13 +561,15 @@ fn shape_explicit_target_creature_stays_stack_target() { !chain_has_unimplemented(root), "explicit-target compound exile parses cleanly; got {root:#?}" ); - if let Some(creature) = find_change_zone(root) { - assert_eq!( - creature.target_choice_timing, - TargetChoiceTiming::Stack, - "an explicit 'target creature' stays a Stack target" - ); - } + // REQUIRED (not `if let`): the creature leg MUST exist, or the assertion + // below would pass vacuously when no ChangeZone is found. + let creature = find_change_zone(root) + .expect("explicit-target compound exile has a creature ChangeZone leg"); + assert_eq!( + creature.target_choice_timing, + TargetChoiceTiming::Stack, + "an explicit 'target creature' stays a Stack target" + ); } /// Hostile fixture for Part A site 3 (mod.rs:15143): "Exile all creatures they @@ -594,6 +596,186 @@ fn shape_exile_all_creatures_and_graveyard_stays_fail_closed() { ); } +// --------------------------------------------------------------------------- +// 11. SIBLING (review follow-up) — the narrowed rewrite generalizes to a bare +// "target player exiles a creature they control" (no graveyard leg). +// --------------------------------------------------------------------------- + +/// Doomfall / Debt to the Kami class: a bare "Target opponent exiles a creature +/// they control." (the SB creature leg WITHOUT the graveyard conjunct). The +/// narrowed post-lowering `You → ScopedPlayer` rewrite must still fire, so the +/// target OPPONENT (not the caster) chooses from THEIR OWN creatures at +/// resolution, no `BecomesTarget` fires, and Ward stays silent. Reverting the +/// narrowing (dropping the ChangeZone-leg rewrite) flips the chooser/eligible set +/// back to the caster. +#[test] +fn sibling_bare_target_player_exile_they_control_binds_chooser_no_ward() { + const SIBLING: &str = "Target opponent exiles a creature they control."; + + // SHAPE: single ChangeZone leg is ScopedPlayer-scoped + Resolution-timed. + let parsed = parse_oracle_text(SIBLING, "Doomfall Mode", &[], &[], &[]); + assert_eq!(parsed.abilities.len(), 1); + let root = &parsed.abilities[0]; + assert!( + !chain_has_unimplemented(root), + "the bare sibling parses cleanly; got {root:#?}" + ); + let creature = find_change_zone(root).expect("sibling has a creature ChangeZone leg"); + assert!( + filter_controller_is_scoped(match &*creature.effect { + Effect::ChangeZone { target, .. } => target, + other => panic!("expected ChangeZone; got {other:?}"), + }), + "the sibling creature leg is ScopedPlayer-scoped; got {:?}", + creature.effect + ); + assert_eq!( + creature.target_choice_timing, + TargetChoiceTiming::Resolution, + "the sibling creature leg is resolution-chosen" + ); + + // Runtime: opponent controls a warded + a plain creature; the caster also + // controls a creature that must never be offered. + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let plain = scenario.add_creature(P1, "Opp Plain", 2, 2).id(); + let warded = ward2_creature(&mut scenario, P1, 3, 3, "Opp Warded"); + let caster_own = scenario.add_creature(P0, "Caster Own", 4, 4).id(); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Doomfall Mode", false, SIBLING) + .id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_player(P1).resolve(); + match outcome.final_waiting_for() { + WaitingFor::EffectZoneChoice { player, cards, .. } => { + assert_eq!(*player, P1, "the target OPPONENT chooses, not the caster"); + assert!( + cards.contains(&plain) && cards.contains(&warded), + "both of the opponent's creatures are offered; got {cards:?}" + ); + assert!( + !cards.contains(&caster_own), + "the caster's own creature must never be offered; got {cards:?}" + ); + } + WaitingFor::UnlessPayment { .. } + | WaitingFor::WardSacrificeChoice { .. } + | WaitingFor::WardDiscardChoice { .. } => panic!( + "Ward must NOT fire — the creature is chosen at resolution, not targeted; got {:?}", + outcome.final_waiting_for() + ), + other => panic!("expected the opponent's EffectZoneChoice; got {other:?}"), + } + + // Reach-guard: the opponent exiles a chosen creature. + runner + .act(GameAction::SelectCards { + cards: vec![warded], + }) + .expect("opponent selects a creature to exile"); + assert_eq!( + runner.state().objects[&warded].zone, + Zone::Exile, + "the opponent's chosen (warded) creature is exiled without paying Ward" + ); +} + +// --------------------------------------------------------------------------- +// 12. LEADERSHIP VACUUM (review follow-up) — the mass ChangeZoneAll sibling still +// binds ScopedPlayer under the narrowed rewrite (no whole-clause pin). +// --------------------------------------------------------------------------- + +/// "Target player returns each commander they control from the battlefield to the +/// command zone." The mass `ChangeZoneAll` moved-object leg must carry a +/// `ScopedPlayer` controller (bound to the target player at resolution), proving +/// the narrowed rewrite covers the mass leg exactly as the whole-clause pin did. +#[test] +fn leadership_vacuum_mass_leg_is_scopedplayer() { + const LV: &str = + "Target player returns each commander they control from the battlefield to the command zone."; + let parsed = parse_oracle_text(LV, "Leadership Vacuum", &[], &[], &[]); + assert_eq!(parsed.abilities.len(), 1); + let root = &parsed.abilities[0]; + assert!( + !chain_has_unimplemented(root), + "Leadership Vacuum's mass return parses cleanly; got {root:#?}" + ); + // Root wraps the lone player target; the mass leg is the sub-ability. + assert!( + matches!(&*root.effect, Effect::TargetOnly { target } if target_is_opponent_like(target) + || matches!(target, TargetFilter::Player)), + "root targets only the player; got {:?}", + root.effect + ); + let mass = find_change_zone_all(root).expect("Leadership Vacuum has a mass ChangeZoneAll leg"); + let Effect::ChangeZoneAll { target, .. } = &*mass.effect else { + unreachable!("find_change_zone_all guarantees the variant"); + }; + assert!( + filter_controller_is_scoped(target), + "the mass 'each commander they control' leg is ScopedPlayer-scoped; got {target:?}" + ); + assert_eq!( + mass.target_choice_timing, + TargetChoiceTiming::Resolution, + "the mass leg is resolution-bound (scoped_player stamp lands there)" + ); +} + +// --------------------------------------------------------------------------- +// 13. ORIGIN-LEAK CLASS (review follow-up) — a compound "exile X and " whose primary leg is a BATTLEFIELD/self object (Silent +// Gravestone) must not inherit the trailing conjunct's Graveyard origin. +// --------------------------------------------------------------------------- + +/// Silent Gravestone's activated body: "Exile this artifact and all cards from all +/// graveyards." The primary self/battlefield leg must NOT leak `Zone::Graveyard` +/// from the trailing "and all cards from all graveyards" conjunct — its origin +/// stays battlefield/default — while the trailing ChangeZoneAll leg keeps its own +/// `Zone::Graveyard` origin. Reverting `compound_exile_origin_scan` leaks Graveyard +/// onto the primary leg (the Part A defect), a different affected card than SB's +/// "and their graveyard". +#[test] +fn origin_leak_compound_exile_primary_leg_stays_battlefield() { + const CLAUSE: &str = "Exile this artifact and all cards from all graveyards."; + let parsed = parse_oracle_text( + CLAUSE, + "Silent Gravestone", + &["Artifact".to_string()], + &[], + &[], + ); + assert_eq!(parsed.abilities.len(), 1); + let root = &parsed.abilities[0]; + assert!( + !chain_has_unimplemented(root), + "the compound self-exile parses cleanly; got {root:#?}" + ); + + let primary = find_change_zone(root).expect("primary self/battlefield exile leg"); + let Effect::ChangeZone { origin, .. } = &*primary.effect else { + unreachable!("find_change_zone guarantees the variant"); + }; + assert!( + origin.is_none() || *origin == Some(Zone::Battlefield), + "the primary leg's origin must NOT leak Graveyard from the trailing conjunct; got {origin:?}" + ); + + let graveyard = + find_change_zone_all(root).expect("trailing 'all cards from all graveyards' leg"); + let Effect::ChangeZoneAll { origin, .. } = &*graveyard.effect else { + unreachable!("find_change_zone_all guarantees the variant"); + }; + assert_eq!( + *origin, + Some(Zone::Graveyard), + "the trailing conjunct keeps its own Graveyard origin; got {origin:?}" + ); +} + // --------------------------------------------------------------------------- // SHAPE predicates // --------------------------------------------------------------------------- From a323232cfb3fbb8bd909623873651a80178d72cf Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 24 Jul 2026 20:47:28 -0700 Subject: [PATCH 3/4] fix(PR-6607): make controller scope rebinding exhaustive --- crates/engine/src/parser/oracle_effect/mod.rs | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 9c345810f7..d386560a53 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7580,7 +7580,54 @@ fn rebind_controller_scope(filter: &mut TargetFilter, from: ControllerRef, to: C } } TargetFilter::Not { filter } => rebind_controller_scope(filter, from, to), - _ => {} + TargetFilter::None + | TargetFilter::Any + | TargetFilter::Player + | TargetFilter::Controller + | TargetFilter::ControllerAndControlledPermanents { .. } + | TargetFilter::Opponent + | TargetFilter::SelfRef + | TargetFilter::GrantingObject + | TargetFilter::SourceOrPaired + | TargetFilter::StackAbility { .. } + | TargetFilter::StackSpell + | TargetFilter::SpecificObject { .. } + | TargetFilter::SpecificPlayer { .. } + | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::Neighbor { .. } + | TargetFilter::ScopedPlayer + | TargetFilter::AttachedTo + | TargetFilter::LastCreated + | TargetFilter::LastRevealed + | TargetFilter::CostPaidObject + | TargetFilter::ChosenCard + | TargetFilter::TrackedSet { .. } + | TargetFilter::TrackedSetFiltered { .. } + | TargetFilter::ExiledBySource + | TargetFilter::ExiledCardByIndex { .. } + | TargetFilter::TriggeringSpellController + | TargetFilter::TriggeringSpellOwner + | TargetFilter::TriggeringPlayer + | TargetFilter::TriggeringSource + | TargetFilter::EventTarget + | TargetFilter::TriggeringSourceController + | TargetFilter::ParentTarget + | TargetFilter::ParentTargetSlot { .. } + | TargetFilter::ParentTargetController + | TargetFilter::ParentTargetOwner + | TargetFilter::SourceChosenPlayer + | TargetFilter::OriginalController + | TargetFilter::OriginalSource + | TargetFilter::PostReplacementSourceController + | TargetFilter::PostReplacementDamageSource + | TargetFilter::PostReplacementDamageTarget + | TargetFilter::PostReplacementDamageTargetOwner + | TargetFilter::DefendingPlayer + | TargetFilter::HasChosenName + | TargetFilter::ChosenDamageSource { .. } + | TargetFilter::Named { .. } + | TargetFilter::Owner + | TargetFilter::AllPlayers => {} } } From 8f26be98382d5c0e8f02346c13f9d48723233747 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 24 Jul 2026 20:55:07 -0700 Subject: [PATCH 4/4] fix(PR-6607): cover current target filter terminal --- crates/engine/src/parser/oracle_effect/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index f7ab94ca7c..c3fbce6f3b 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7599,6 +7599,7 @@ fn rebind_controller_scope(filter: &mut TargetFilter, from: ControllerRef, to: C | TargetFilter::AttachedTo | TargetFilter::LastCreated | TargetFilter::LastRevealed + | TargetFilter::LastZoneChanged | TargetFilter::CostPaidObject | TargetFilter::ChosenCard | TargetFilter::TrackedSet { .. }