From ffc9161c53762a20750dec2d3b194f0c936d53d7 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:46:04 -0500 Subject: [PATCH 1/6] Implement Gideon Jura: planeswalker-directed attack requirements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gideon Jura's "+2" was `Effect::Unimplemented`, and its "0" carried a rules bug that also affected Gideon of the Trials. All three loyalty abilities now parse and resolve correctly. CR 506.3 makes "a player, a planeswalker, or a battle" ONE defender category, and the engine already modelled it as one type (`combat::AttackTarget`). The required-defender axis is widened to span that category rather than forking a parallel player-only/permanent-only static pair: * `RequiredDefender::Permanent` snapshots the defender as an `ObjectIncarnationRef` (CR 400.7, mirroring `MustBlockAttacker`), and the `AttackTarget` kind it presents is derived live at each declare-attackers step. `StaticMode::MustAttackPlayer` is renamed `MustAttackDefender` so the name cannot invite player-only code, and the CR 508.1d solver's requirements key on `AttackTarget`. * `Effect::ForceAttack` gains an `EffectScope`, parameterized exactly as on `Transform`/`SetTapState`. Per CR 115.1 the "+2" targets only the opponent, so the `All` scope makes "creatures that player controls" a non-targeting population and no creature slot is built. * `ControllerRef::SpecificPlayer` / `PlayerScope::SpecificPlayer` are resolution-time snapshots, following the lower-at-resolution contract already documented on `RestrictionPlayerScope::SpecificPlayer`. Per CR 611.2c the affected creature SET stays dynamic (the card's ruling: the ability "doesn't lock in what it applies to ... includes creatures that come under that player's control after the ability has resolved"), while the player and the expiry are fixed at resolution. Two fixes are building-block level rather than card level: * "prevent all damage that would be dealt to HIM" fell through to a recipient of `TargetFilter::Any` — a shield with no recipient constraint, i.e. a turn-long Fog over every damage event in the game. Gideon of the Trials shares this; its existing test only asserted the absence of `Unimplemented` and never checked the shield's scope. A gendered pronoun is now recognized as the ungated printed-name self-reference (singular-they stays excluded — it is recipient-anaphoric for player-enchanting Auras). * A bare-plural subject ("creatures that player controls") called the ctx-free `parse_target`, so the "that player" anaphor took the documented `ControllerRef::You` fallback and pointed the requirement at the ACTIVATING player's creatures. It now threads `ParseContext` like its sibling "target " arm already did. Two seams needed narrowing to avoid stealing behavior from existing cards: the "during ... next turn" duration arm accepts only the TARGETED possessives, because the positional wrappers run before clause-level grammars and the pronoun forms are owned by the CR 723.1 control-next-turn grammar (Construct a Cosmic Cube); and the counted attackable-defender sweep keeps its historical `attackable_player_sweeps` counter name, which is a key in phase-ai's persisted perf baseline. Verified: clippy clean; engine 18857 lib + 4819 integration + 30 bin; phase-ai 2135; frontend tsc clean + 30 vitest. Coverage/semantic-audit were NOT run - this worktree has no MTGJSON corpus, so the corpus-wide parse blast radius needs the CI parse-diff report. Co-Authored-By: Claude Opus 5 --- client/src/adapter/types.ts | 5 +- .../board/__tests__/ActionButton.test.tsx | 2 +- .../__tests__/combatRequirements.test.tsx | 6 +- crates/engine/src/ai_support/candidates.rs | 4 +- crates/engine/src/ai_support/filter.rs | 2 +- crates/engine/src/analysis/resource.rs | 2 +- crates/engine/src/database/encore_tests.rs | 8 +- crates/engine/src/game/ability_rw.rs | 24 +- crates/engine/src/game/ability_scan.rs | 14 +- crates/engine/src/game/ability_utils.rs | 13 +- crates/engine/src/game/combat.rs | 513 +++++++++++------- crates/engine/src/game/coverage.rs | 27 +- crates/engine/src/game/effects/copy_spell.rs | 5 +- crates/engine/src/game/effects/encore.rs | 6 +- .../engine/src/game/effects/force_attack.rs | 221 +++++++- crates/engine/src/game/effects/mod.rs | 5 +- .../engine/src/game/effects/prevent_damage.rs | 146 +++++ crates/engine/src/game/effects/sacrifice.rs | 2 + crates/engine/src/game/filter.rs | 46 ++ crates/engine/src/game/layers.rs | 23 +- crates/engine/src/game/players.rs | 5 +- crates/engine/src/game/quantity.rs | 16 + crates/engine/src/game/replacement.rs | 14 + crates/engine/src/game/sba.rs | 2 + crates/engine/src/game/scenario.rs | 47 ++ crates/engine/src/game/static_abilities.rs | 5 + crates/engine/src/game/targeting.rs | 2 + .../src/parser/oracle_effect/imperative.rs | 109 +++- crates/engine/src/parser/oracle_effect/mod.rs | 33 +- .../src/parser/oracle_effect/subject.rs | 78 ++- .../engine/src/parser/oracle_effect/tests.rs | 5 +- crates/engine/src/parser/oracle_ir/ast.rs | 18 +- .../engine/src/parser/oracle_nom/duration.rs | 134 ++++- .../src/parser/oracle_nom/enters_under.rs | 5 +- .../src/parser/oracle_static/evasion.rs | 4 +- .../parser/oracle_static/static_helpers.rs | 2 + .../engine/src/parser/oracle_static/tests.rs | 12 +- crates/engine/src/parser/oracle_target.rs | 49 ++ crates/engine/src/parser/oracle_tests.rs | 208 +++++++ crates/engine/src/types/ability.rs | 98 +++- crates/engine/src/types/game_state.rs | 8 +- crates/engine/src/types/statics.rs | 95 +++- .../bbfu7_attacks_if_able_not_goad.rs | 10 +- .../deterministic_game_state_serde.rs | 2 +- .../galactus_forced_attack_most_life.rs | 6 +- .../gideon_jura_forced_attack_planeswalker.rs | 361 ++++++++++++ .../goaded_creature_under_pacifism_visible.rs | 2 +- crates/engine/tests/integration/main.rs | 1 + .../must_attack_player_attribution.rs | 50 +- .../engine/tests/integration/rules/combat.rs | 16 +- crates/phase-ai/src/combat_ai.rs | 16 +- crates/phase-ai/tests/scenarios.rs | 4 +- 52 files changed, 2112 insertions(+), 379 deletions(-) create mode 100644 crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs diff --git a/client/src/adapter/types.ts b/client/src/adapter/types.ts index 3871dfc3d0..046ebae9a5 100644 --- a/client/src/adapter/types.ts +++ b/client/src/adapter/types.ts @@ -331,7 +331,10 @@ export interface MeldSelection { // engine surfaces on the declare-attackers/blockers waiting payloads for // display-only badges + Confirm gating. `#[serde(tag = "kind")]` in the engine. export type CombatRequirement = - | { kind: "MustAttack"; players: PlayerId[]; sources?: ObjectId[] } + // CR 506.3: `defenders` spans the whole defender category — players, + // planeswalkers, and battles — so a planeswalker-directed lure (Gideon Jura's + // "+2") surfaces the same way a player-directed one does. + | { kind: "MustAttack"; defenders: AttackTarget[]; sources?: ObjectId[] } | { kind: "MustBlock"; sources?: ObjectId[]; attackers?: ObjectId[] } | { kind: "CantAttack"; sources?: ObjectId[] } | { kind: "CantBlock"; sources?: ObjectId[] }; diff --git a/client/src/components/board/__tests__/ActionButton.test.tsx b/client/src/components/board/__tests__/ActionButton.test.tsx index fcfc1d5591..ec389880f3 100644 --- a/client/src/components/board/__tests__/ActionButton.test.tsx +++ b/client/src/components/board/__tests__/ActionButton.test.tsx @@ -303,7 +303,7 @@ describe("ActionButton", () => { valid_attacker_ids: [100], valid_attack_targets: [target], valid_attack_targets_by_attacker: { "100": [target] }, - attacker_constraints: { "100": { kind: "MustAttack", players: [] } }, + attacker_constraints: { "100": { kind: "MustAttack", defenders: [] } }, }, }; useGameStore.setState({ diff --git a/client/src/components/combat/__tests__/combatRequirements.test.tsx b/client/src/components/combat/__tests__/combatRequirements.test.tsx index 4bf20377da..2bb2fb2a53 100644 --- a/client/src/components/combat/__tests__/combatRequirements.test.tsx +++ b/client/src/components/combat/__tests__/combatRequirements.test.tsx @@ -49,7 +49,7 @@ describe("useAttackRequirements", () => { data: { player: 0, valid_attacker_ids: [100], - attacker_constraints: { "100": { kind: "MustAttack", players: [] } }, + attacker_constraints: { "100": { kind: "MustAttack", defenders: [] } }, }, }); @@ -85,7 +85,7 @@ describe("useAttackRequirements", () => { data: { player: 0, valid_attacker_ids: [100], - attacker_constraints: { "100": { kind: "MustAttack", players: [], sources: [200] } }, + attacker_constraints: { "100": { kind: "MustAttack", defenders: [], sources: [200] } }, }, }); const r = renderHook(() => useAttackRequirements()); @@ -221,7 +221,7 @@ describe("AttackRequirementBadges source attribution (display-only)", () => { player: 0, valid_attacker_ids: [objectId], attacker_constraints: { - [String(objectId)]: { kind: "MustAttack", players: [], sources }, + [String(objectId)]: { kind: "MustAttack", defenders: [], sources }, }, }, }); diff --git a/crates/engine/src/ai_support/candidates.rs b/crates/engine/src/ai_support/candidates.rs index df354974da..618be81074 100644 --- a/crates/engine/src/ai_support/candidates.rs +++ b/crates/engine/src/ai_support/candidates.rs @@ -6324,8 +6324,8 @@ mod tests { .unwrap() .static_definitions .push(StaticDefinition::new( - crate::types::statics::StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + crate::types::statics::StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }, )); let goaded = make_creature(&mut state, 2); diff --git a/crates/engine/src/ai_support/filter.rs b/crates/engine/src/ai_support/filter.rs index acefea928b..13c948acc7 100644 --- a/crates/engine/src/ai_support/filter.rs +++ b/crates/engine/src/ai_support/filter.rs @@ -1218,7 +1218,7 @@ impl LegalityPoisonGates { StaticMode::CantAttack | StaticMode::CantAttackOrBlock | StaticMode::MustAttack - | StaticMode::MustAttackPlayer { .. } + | StaticMode::MustAttackDefender { .. } | StaticMode::Goaded | StaticMode::MustAttackAwayFromSource | StaticMode::CanAttackWithDefender diff --git a/crates/engine/src/analysis/resource.rs b/crates/engine/src/analysis/resource.rs index 3197611125..c0312ca219 100644 --- a/crates/engine/src/analysis/resource.rs +++ b/crates/engine/src/analysis/resource.rs @@ -4030,7 +4030,7 @@ fn static_mode_references_growing_class(mode: &crate::types::statics::StaticMode | StaticMode::CantLoseLife | StaticMode::PlayerProtection(..) | StaticMode::MustAttack - | StaticMode::MustAttackPlayer { .. } + | StaticMode::MustAttackDefender { .. } | StaticMode::MustBlock | StaticMode::MustBlockAttacker { .. } | StaticMode::CantDraw { .. } diff --git a/crates/engine/src/database/encore_tests.rs b/crates/engine/src/database/encore_tests.rs index 6bbae9dac5..151ec03a9b 100644 --- a/crates/engine/src/database/encore_tests.rs +++ b/crates/engine/src/database/encore_tests.rs @@ -207,8 +207,8 @@ fn encore_activation_creates_attacking_haste_copy_per_opponent() { must_attack.modifications.iter().any(|m| matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player }, + mode: StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player }, }, } if *player == PlayerId(1) )), @@ -308,8 +308,8 @@ fn encore_three_player_one_token_per_opponent_then_sacrificed_at_end_step() { ce.modifications.iter().find_map(|m| match m { ContinuousModification::AddStaticMode { mode: - StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player }, + StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player }, }, } => Some(*player), _ => None, diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 3d6b8a59b6..7ac9e5110d 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -2215,6 +2215,9 @@ fn legacy_controller_ref(x: &ControllerRef) -> bool { // CR 102.1: the active player is a game-defined role read live, not a // frozen event-context tag. | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: a frozen player id is a plain literal read, not + // an event-context tag. + | ControllerRef::SpecificPlayer { .. } | ControllerRef::EnchantedPlayer => false, } } @@ -2564,6 +2567,9 @@ fn member_bound_controller_ref(x: &ControllerRef) -> bool { // CR 102.1: the active player is a game-defined role read live from // `state.active_player`, not per-source member-bound storage. | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: a frozen player id is a plain literal read, not + // an event-context tag. + | ControllerRef::SpecificPlayer { .. } | ControllerRef::DefendingPlayer => false, } } @@ -3255,11 +3261,14 @@ fn legacy_effect(x: &Effect) -> bool { } Effect::ForceAttack { target, - required_player, + required_defender, duration, + // A static single-vs-mass discriminant (CR 115.1), never a legacy + // target/duration seam of its own. + scope: _, } => { legacy_target_filter(target) - || legacy_target_filter(required_player) + || legacy_target_filter(required_defender) || legacy_duration(duration) } @@ -5456,8 +5465,11 @@ fn rw_effect( | Effect::SolveCase => (ext_write(StateKind::Other), None), Effect::ForceAttack { target, - required_player: _, + required_defender: _, duration, + // A static single-vs-mass discriminant (CR 115.1): reads nothing and + // writes nothing, so it contributes no read/write profile. + scope: _, } => { let mut p = ext_write(StateKind::Other); flag_legacy_write_target(&mut p, target); @@ -6594,6 +6606,9 @@ fn rw_player_scope(x: &PlayerScope) -> RwProfile { | PlayerScope::Opponent { .. } | PlayerScope::RecipientController | PlayerScope::AnyTurn + // CR 611.2 + CR 514.2: duration-timing-only, like `AnyTurn` — never reached + // from a value/quantity/player-selection position. + | PlayerScope::SpecificPlayer { .. } | PlayerScope::DefendingPlayer => RwProfile::empty(), } } @@ -6619,6 +6634,9 @@ fn rw_controller_ref(x: &ControllerRef) -> RwProfile { // CR 102.1: a live read of `state.active_player` — no sibling-mutable // state, empty RW profile (mirrors `DefendingPlayer`). | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: a frozen player id is a plain literal read, not + // an event-context tag. + | ControllerRef::SpecificPlayer { .. } // resolution-local (ResolvedAbility.chosen_players) | ControllerRef::ChosenPlayer { .. } => RwProfile::empty(), } diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 15d91485f9..4f0ec611de 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -1193,12 +1193,16 @@ fn scan_effect(x: &Effect, mode: ScanMode) -> Axes { } Effect::ForceAttack { target, - required_player, + required_defender, duration, + // A static single-vs-mass discriminant (CR 115.1) — no event, sibling, + // or projected-resource axis; the filters it selects between are + // classified below. + scope: _, } => { let mut acc = Axes::NONE; acc = acc.or(scan_target_filter(target, target_ctx, mode)); - acc = acc.or(scan_target_filter(required_player, target_ctx, mode)); + acc = acc.or(scan_target_filter(required_defender, target_ctx, mode)); acc = acc.or(scan_duration(duration, mode)); acc } @@ -4189,6 +4193,9 @@ fn scan_player_scope(x: &PlayerScope) -> Axes { // CR 513.1: turn-agnostic end-step deadline reached via the // `UntilNextStepOf` duration walk — a pure timing referent, no axes. PlayerScope::AnyTurn => Axes::NONE, + // CR 611.2: a frozen literal id — reads no event, sibling, or projected + // resource. + PlayerScope::SpecificPlayer { .. } => Axes::NONE, } } @@ -4223,6 +4230,9 @@ fn scan_controller_ref(x: &ControllerRef) -> Axes { ControllerRef::EnchantedPlayer => Axes::NONE, // CR 102.1: a live read of `state.active_player` — no event/sibling axis. ControllerRef::ActivePlayer => Axes::NONE, + // CR 109.4 + CR 611.2: a frozen literal id — reads no event, sibling, or + // projected resource. + ControllerRef::SpecificPlayer { .. } => Axes::NONE, } } diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index a160856922..ddf9fb8e73 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -3436,7 +3436,16 @@ fn mass_all_target_filter(effect: &Effect) -> Option<&TargetFilter> { target, .. } - | Effect::DoublePTAll { target, .. } => Some(target), + | Effect::DoublePTAll { target, .. } + // CR 508.1d + CR 109.4: the mass forced-attack population (Gideon Jura's + // "creatures that player controls"). Listed here — not just excluded from + // `target_filter()` — so its `ControllerRef::TargetOpponent` still + // surfaces the COMPANION PLAYER slot the ability genuinely targets. + | Effect::ForceAttack { + scope: EffectScope::All, + target, + .. + } => Some(target), _ => None, } } @@ -3984,6 +3993,8 @@ pub(crate) fn collect_player_targets( // CR 102.1 + CR 109.4: the active player, resolvable directly // (unlike the fail-closed DefendingPlayer arm above). Some(ControllerRef::ActivePlayer) => p.id == state.active_player, + // CR 109.4 + CR 611.2: a snapshotted id, resolvable directly. + Some(ControllerRef::SpecificPlayer { id }) => p.id == *id, None => true, }) .map(|p| p.id) diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index 0e43a938f7..b1dd773a40 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -118,16 +118,27 @@ pub fn default_attack_target() -> AttackTarget { #[serde(tag = "kind")] pub enum CombatRequirement { /// CR 508.1d + CR 701.15b: this creature attacks this combat if able. - /// `players` carries the CR 508.1d specific-player requirements - /// (`StaticMode::MustAttackPlayer`) intersected with the currently - /// attackable players; empty for a generic "attacks each combat if able" - /// requirement or for goad with no surviving specific-player constraint. + /// `defenders` carries the CR 508.1d specific-defender requirements + /// (`StaticMode::MustAttackDefender`) intersected with the currently + /// attackable defenders — players, planeswalkers, and battles alike per + /// CR 506.3, so a Gideon Jura lure surfaces exactly like a player lure. + /// Empty for a generic "attacks each combat if able" requirement or for goad + /// with no surviving specific-defender constraint. /// `sources` names the objects imposing the requirement (intrinsic → the /// creature itself; remote → the anthem/`Goaded`-static carrier). EMPTY /// when the only cause is player-level goad (`goaded_by`), which carries no /// object (CR 701.15b). + /// + /// `serde`: pre-widening snapshots wrote this field as `players`, holding + /// bare `PlayerId` integers. Both the NAME and the ELEMENT SHAPE therefore + /// need a compat path — the `alias` covers the name, and + /// [`deserialize_defenders`] widens each legacy integer to + /// `AttackTarget::Player`. The two element shapes are disjoint (number vs. + /// tagged map), so the widening is unambiguous. Mirrors the identical + /// legacy-integer shims on [`RequiredDefender`] and `ObjectIncarnationRef`. MustAttack { - players: Vec, + #[serde(alias = "players", deserialize_with = "deserialize_defenders")] + defenders: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] sources: Vec, }, @@ -157,6 +168,36 @@ pub enum CombatRequirement { }, } +/// CR 506.3: Back-compatible element decoder for +/// [`CombatRequirement::MustAttack`]'s `defenders`. +/// +/// New writes emit tagged `AttackTarget`s (`{"type":"Player","data":0}`). A +/// mid-combat snapshot taken before the field was widened from players to +/// defenders (restore / undo / P2P resume) holds bare `PlayerId` integers under +/// the old `players` name; those decode to `AttackTarget::Player`. The two +/// shapes are number vs. map, so `#[serde(untagged)]` selects between them by +/// shape and the widening can never be ambiguous. +fn deserialize_defenders<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + #[derive(Deserialize)] + #[serde(untagged)] + enum DefenderWire { + /// Current shape: a fully tagged defender of any kind. + Target(AttackTarget), + /// Pre-widening shape: a bare `PlayerId`. + LegacyPlayer(PlayerId), + } + Ok(Vec::::deserialize(deserializer)? + .into_iter() + .map(|wire| match wire { + DefenderWire::Target(target) => target, + DefenderWire::LegacyPlayer(player) => AttackTarget::Player(player), + }) + .collect()) +} + /// CR 702.111b (Menace) + CR 509.1b ("except by N or more"): the minimum-blocker /// COUNT floor for one attacker, with `sources` naming the carriers imposing it /// (the attacker itself for Menace; each `MinBlockers` static's carrier otherwise). @@ -3135,121 +3176,153 @@ fn cant_attack_sources_gated( /// override (CR 702.3b) /// - `has_summoning_sickness(obj)` (CR 302.6) pub fn creature_must_attack(state: &GameState, obj_id: ObjectId) -> bool { - let attackable_players = attackable_player_targets(state); - creature_must_attack_with_attackable_players(state, obj_id, &attackable_players) + let attackable = attackable_defender_targets(state); + creature_must_attack_with_attackable_targets(state, obj_id, &attackable) } -pub fn attackable_player_targets(state: &GameState) -> Vec { +/// CR 506.3: the live defender universe — every player, planeswalker, and battle +/// the active team may currently attack — as the COUNTED sweep that must-attack +/// callers hoist out of their per-creature loops. +/// +/// Identical in value to [`get_valid_attack_targets`]; the difference is the perf +/// counter, which exists so the "sweep once per enumeration, not once per +/// creature" contract is revert-failing (see `attacker_actions` in +/// `ai_support/candidates.rs` and `choose_attackers` in `phase-ai`). Callers that +/// are not the hoisted sweep call `get_valid_attack_targets` directly. +/// +/// The counter keeps its historical `attackable_player_sweeps` name even though +/// the sweep now covers every defender kind: it is a key in the persisted +/// `phase-ai/baselines/perf-baseline.json`, and renaming it would invalidate the +/// baseline for a purely cosmetic gain. +pub fn attackable_defender_targets(state: &GameState) -> Vec { crate::game::perf_counters::record_attackable_player_sweep(); get_valid_attack_targets(state) - .into_iter() - .filter_map(|target| match target { - AttackTarget::Player(pid) => Some(pid), - _ => None, - }) - .collect() } -/// CR 508.1b: The players this creature is required to attack directly via a -/// `StaticMode::MustAttackPlayer` static ("attacks ~ each combat if able" -/// directed at a specific player). Single authority for the requirement; the -/// declare-attackers validator enforces it and the AI candidate generator reuses -/// it to steer a forced-legal assignment toward the required player. -pub(crate) fn must_attack_players_for_creature( +/// CR 506.3 + CR 508.1b: The defenders this creature is required to attack +/// directly via a `StaticMode::MustAttackDefender` static ("attacks ~ each +/// combat if able" directed at a specific player, or Gideon Jura's "attack +/// Gideon Jura if able" directed at a planeswalker). Single authority for the +/// requirement; the declare-attackers validator enforces it and the AI candidate +/// generator reuses it to steer a forced-legal assignment toward the required +/// defender. +pub(crate) fn must_attack_defenders_for_creature( state: &GameState, obj: &GameObject, -) -> Vec { - let mut players: Vec = must_attack_player_directives_for_creature(state, obj) +) -> Vec { + let mut defenders: Vec = must_attack_defender_directives_for_creature(state, obj) .into_iter() .flat_map(|(defender, _)| defender.into_members()) .collect(); - // CR 508.1d: players is a SET — a per-player requirement is obeyed by - // attacking that player once (CR 508.1d counts requirements), so multiple - // directives naming the same player collapse to one entry; otherwise + // CR 508.1d: defenders is a SET — a per-defender requirement is obeyed by + // attacking that defender once (CR 508.1d counts requirements), so multiple + // directives naming the same defender collapse to one entry; otherwise // `score_declaration` would double-count a single requirement and bias // attack selection. Per-directing-source multiplicity lives in `sources`. // This flat union (Fixed singletons + every `Matching` member) drives the - // "is any required player attackable" gate and the display badge; the CR + // "is any required defender attackable" gate and the display badge; the CR // 508.1d SOLVER keeps `Matching` directives as alternative-sets (see the // requirement builder), which this projection deliberately flattens away. - players.sort_unstable_by_key(|p| p.0); - players.dedup(); - players + defenders.sort_unstable(); + defenders.dedup(); + defenders } -/// CR 508.1d + CR 604.1 / CR 611.2c: one resolved `MustAttackPlayer` directive on -/// a creature — the acceptable defending players of a SINGLE static, kept ungrouped -/// from every other directive. Mirrors [`RequiredDefender`] after live resolution: -/// `Fixed` is a resolution-time snapshot (exactly one player); `Matching` is the -/// live player class (every current member, e.g. all opponents tied for the most -/// life). Preserving the directive boundary is load-bearing for CR 508.1d: a -/// `Matching` directive is ONE alternative-set requirement (attack any member), so -/// flattening its members into a shared deduped player set would merge a tied -/// member with a coexisting `Fixed` requirement and let the max-requirement solver -/// wrongly permit a non-fixed tied member. +/// CR 508.1d + CR 604.1 / CR 611.2c: one resolved `MustAttackDefender` directive +/// on a creature — the acceptable defenders of a SINGLE static, kept ungrouped +/// from every other directive. Members are [`AttackTarget`]s, the engine's single +/// CR 506.3 defender type ("a player, a planeswalker, or a battle"), so a +/// player-directed lure and Gideon Jura's planeswalker-directed lure score +/// through one solver path. +/// +/// Mirrors [`RequiredDefender`] after live resolution: `Fixed` is a +/// resolution-time snapshot (exactly one defender — a `Fixed { player }` lure or +/// a `Permanent { permanent }` planeswalker); `Matching` is the live player class +/// (every current member, e.g. all opponents tied for the most life). Preserving +/// the directive boundary is load-bearing for CR 508.1d: a `Matching` directive +/// is ONE alternative-set requirement (attack any member), so flattening its +/// members into a shared deduped defender set would merge a tied member with a +/// coexisting `Fixed` requirement and let the max-requirement solver wrongly +/// permit a non-fixed tied member. pub(crate) enum ResolvedRequiredDefender { - /// CR 611.2: a single snapshotted defending player. - Fixed(PlayerId), + /// CR 611.2: a single snapshotted defender. + Fixed(AttackTarget), /// CR 604.1 + CR 508.1b/d: the live class members — attacking ANY ONE obeys /// the single requirement; the active player picks among tied legal defenders /// (CR 508.1b). - Matching(Vec), + Matching(Vec), } impl ResolvedRequiredDefender { - /// The acceptable defending players (CR 508.1d): a `Fixed` singleton or the - /// live `Matching` class members. Borrows without allocating — both arms are - /// the same `slice::Iter` type. - fn members(&self) -> std::iter::Copied> { + /// The acceptable defenders (CR 508.1d): a `Fixed` singleton or the live + /// `Matching` class members. Borrows without allocating — both arms are the + /// same `slice::Iter` type. + fn members(&self) -> std::iter::Copied> { match self { - Self::Fixed(player) => std::slice::from_ref(player).iter().copied(), - Self::Matching(players) => players.as_slice().iter().copied(), + Self::Fixed(defender) => std::slice::from_ref(defender).iter().copied(), + Self::Matching(defenders) => defenders.as_slice().iter().copied(), } } /// Consuming form of [`members`](Self::members) for the flat-union projection. - fn into_members(self) -> Vec { + fn into_members(self) -> Vec { match self { - Self::Fixed(player) => vec![player], - Self::Matching(players) => players, + Self::Fixed(defender) => vec![defender], + Self::Matching(defenders) => defenders, } } } -/// CR 508.1d + CR 611.2c: the (required player, directing carrier) pairs from -/// every `MustAttackPlayer` static on `obj`. `source_object` names the object +/// CR 508.1d + CR 611.2c: the (required defender, directing carrier) pairs from +/// every `MustAttackDefender` static on `obj`. `source_object` names the object /// that grafted the requirement (ForceAttack / Encore / mass-coerce source); /// `None` for an intrinsic def → the creature itself is the carrier. Retains -/// per-source multiplicity (two sources forcing the same player yield two +/// per-source multiplicity (two sources forcing the same defender yield two /// pairs) so the source collector surfaces every directing id; the -/// `must_attack_players_for_creature` projection dedups. Single authority: the -/// players list and the source collector are both projections of this one scan +/// `must_attack_defenders_for_creature` projection dedups. Single authority: the +/// defenders list and the source collector are both projections of this one scan /// (n6 invariant). -pub(crate) fn must_attack_player_directives_for_creature( +pub(crate) fn must_attack_defender_directives_for_creature( state: &GameState, obj: &GameObject, ) -> Vec<(ResolvedRequiredDefender, Option)> { - // CR 508.1d + CR 611.2 / CR 604.2: MustAttackPlayer directives; the required - // defender may be a resolution-time snapshot (`Fixed`, ForceAttack/Encore) or - // a live static class (`Matching`, Galactus) re-evaluated each - // declare-attackers step. Collect (defender, source_object, source_controller) - // triples first so the `active_static_definitions` borrow is dropped before we - // call `matches_player_scope`, which re-borrows `state.players`. + // CR 508.1d + CR 611.2 / CR 604.2: MustAttackDefender directives; the required + // defender may be a resolution-time snapshot (`Fixed`/`Permanent`, + // ForceAttack/Encore/Gideon Jura) or a live static class (`Matching`, + // Galactus) re-evaluated each declare-attackers step. Collect (defender, + // source_object, source_controller) triples first so the + // `active_static_definitions` borrow is dropped before we call + // `matches_player_scope`, which re-borrows `state.players`. let directives: Vec<(RequiredDefender, Option, Option)> = super::functioning_abilities::active_static_definitions(state, obj) .filter_map(|sd| match &sd.mode { - StaticMode::MustAttackPlayer { player } => { - Some((player.clone(), sd.source_object, sd.source_controller)) + StaticMode::MustAttackDefender { defender } => { + Some((defender.clone(), sd.source_object, sd.source_controller)) } _ => None, }) .collect(); directives .into_iter() - .map(|(defender, src, src_ctrl)| { + .filter_map(|(defender, src, src_ctrl)| { let resolved = match defender { // CR 611.2: a snapshotted id — used verbatim. - RequiredDefender::Fixed { player } => ResolvedRequiredDefender::Fixed(player), + RequiredDefender::Fixed { player } => { + ResolvedRequiredDefender::Fixed(AttackTarget::Player(player)) + } + // CR 506.3 + CR 400.7: a snapshotted PERMANENT defender (Gideon + // Jura). Which defender kind it presents is derived LIVE from the + // permanent's current card types, so a Gideon animated by its own + // third ability is still an attackable planeswalker (CR 306.1). + // An expired pin (the permanent left the battlefield, or left and + // re-entered as a new object) names no defender at all, so the + // directive is DROPPED here rather than resolved to an empty set: + // CR 508.1d then imposes no requirement, matching the official + // ruling that the affected player "may have it attack you, + // another one of your planeswalkers, or nothing at all." + RequiredDefender::Permanent { permanent } => { + ResolvedRequiredDefender::Fixed(permanent_attack_target(state, &permanent)?) + } // CR 604.1 / CR 604.2 + CR 102.2 / CR 102.3: re-evaluate the class // each check. "you"/"your opponents" resolves to the static's // controller (the graft-time snapshot, else the carrier's @@ -3266,7 +3339,7 @@ pub(crate) fn must_attack_player_directives_for_creature( // evaluator is worth the redundant lookup at 2-6 players; a // batch `players_matching_scope` helper is the future extraction // if a hot path ever appears. - let members: Vec = state + let members: Vec = state .players .iter() .filter(|p| { @@ -3274,22 +3347,53 @@ pub(crate) fn must_attack_player_directives_for_creature( state, p.id, &filter, controller, source_id, ) }) - .map(|p| p.id) + .map(|p| AttackTarget::Player(p.id)) .collect(); ResolvedRequiredDefender::Matching(members) } }; - (resolved, src) + Some((resolved, src)) }) .collect() } +/// CR 506.3 + CR 400.7: the [`AttackTarget`] a snapshotted permanent defender +/// currently presents, or `None` when it presents none. +/// +/// `None` covers every way Gideon Jura's "+2" requirement can go unobeyable: +/// the pin is stale (the permanent left the battlefield, or left and re-entered +/// as a new object — CR 400.7), the permanent is phased out (CR 702.26b: treated +/// as though it does not exist), or its live card types are neither planeswalker +/// nor battle. CR 506.3 admits exactly planeswalkers and battles as permanent +/// defenders; a permanent that is ONLY a creature is not attackable, and one +/// that is a creature AND a planeswalker (a self-animated Gideon, CR 306.1) is +/// still attacked as a planeswalker — hence planeswalker is tested first. +fn permanent_attack_target( + state: &GameState, + permanent: &ObjectIncarnationRef, +) -> Option { + if !permanent.is_current(state) { + return None; + } + let obj = state.objects.get(&permanent.object_id)?; + if obj.zone != Zone::Battlefield || obj.is_phased_out() { + return None; + } + if obj.card_types.core_types.contains(&CoreType::Planeswalker) { + return Some(AttackTarget::Planeswalker(obj.id)); + } + if obj.card_types.core_types.contains(&CoreType::Battle) { + return Some(AttackTarget::Battle(obj.id)); + } + None +} + /// CR 508.1d + CR 701.15b/c: sorted, deduped carriers of every must-attack cause /// on `obj`. Called by the producer ONLY when the enforcement bool already /// returned true (all exemptions cleared), so no exemption re-check is needed. /// `attackable_must_player_carriers` is precomputed by the producer (n6: the /// single directives scan feeds both `players` and this) — one entry per -/// attackable `MustAttackPlayer` directive, resolved to its directing object. +/// attackable `MustAttackDefender` directive, resolved to its directing object. /// Direct `goaded_by` designations contribute NO source (CR 701.15b, player-level). fn must_attack_sources_gated( state: &GameState, @@ -3317,7 +3421,7 @@ fn must_attack_sources_gated( crate::game::perf_counters::record_static_full_scan(); sources.extend(goad_static_hits_for_creature(state, obj_id).map(|(_, src)| src)); } - // CR 508.1d + CR 611.2c: MustAttackPlayer statics are grafted onto the + // CR 508.1d + CR 611.2c: MustAttackDefender statics are grafted onto the // creature by a directing object (ForceAttack / Encore / mass-coerce). The // producer resolved each attackable requirement's carrier via // `source_object` (unwrap_or(creature) for an intrinsic def). Attribute the @@ -3331,22 +3435,26 @@ fn must_attack_sources_gated( sources } -pub fn creature_must_attack_with_attackable_players( +/// CR 506.3: `attackable` is the live defender universe +/// ([`get_valid_attack_targets`]) — players, planeswalkers, and battles alike — +/// so a requirement directed at a planeswalker (Gideon Jura) is gated on the +/// same attackability check as a player-directed lure. +pub fn creature_must_attack_with_attackable_targets( state: &GameState, obj_id: ObjectId, - attackable_players: &[PlayerId], + attackable: &[AttackTarget], ) -> bool { // Single-permanent entry: compute the loop-invariant gates once, then // delegate. The single batch caller (`declare_attackers_with_bands`) reuses // its already-hoisted gates via the `_gated` form below. let gates = CombatStaticGates::compute(state); - creature_must_attack_with_attackable_players_gated(state, obj_id, attackable_players, &gates) + creature_must_attack_with_attackable_targets_gated(state, obj_id, attackable, &gates) } -fn creature_must_attack_with_attackable_players_gated( +fn creature_must_attack_with_attackable_targets_gated( state: &GameState, obj_id: ObjectId, - attackable_players: &[PlayerId], + attackable: &[AttackTarget], gates: &CombatStaticGates, ) -> bool { let Some(obj) = state.objects.get(&obj_id) else { @@ -3380,10 +3488,10 @@ fn creature_must_attack_with_attackable_players_gated( // also attacks each combat if able. let must_attack_away = !players_to_attack_away_from_gated(state, obj_id, gates.has_goad).is_empty(); - let has_attackable_must_attack_player = must_attack_players_for_creature(state, obj) + let has_attackable_must_attack_defender = must_attack_defenders_for_creature(state, obj) .iter() - .any(|player| attackable_players.contains(player)); - if !has_must_attack && !must_attack_away && !has_attackable_must_attack_player { + .any(|defender| attackable.contains(defender)); + if !has_must_attack && !must_attack_away && !has_attackable_must_attack_defender { return false; } // Exemptions: tapped, defender (no override), summoning sick. @@ -3675,40 +3783,44 @@ pub fn propagate_banding_block_state(combat: &mut CombatState) { /// CR 508.1d + CR 701.15c: one individual attack requirement the maximum- /// requirement solver scores independently. A single creature can carry several /// (a generic "attacks if able" plus one `Goad` entry per distinct goader plus -/// one `MustAttackPlayer` per specific-player static), and CR 701.15c makes each -/// distinct goader an additional requirement — hence a flat multiset, not a +/// one `MustAttackDefender` per specific-defender static), and CR 701.15c makes +/// each distinct goader an additional requirement — hence a flat multiset, not a /// per-creature aggregate. /// -/// Not `Copy`: `MustAttackAnyOf` carries a `Vec` (a live player class -/// can hold more than one member), so the multiset is moved/borrowed, never -/// bit-copied. +/// Not `Copy`: `MustAttackAnyOf` carries a `Vec` (a live player +/// class can hold more than one member), so the multiset is moved/borrowed, +/// never bit-copied. #[derive(Debug, Clone, PartialEq, Eq)] enum AttackRequirement { /// CR 508.1d + CR 701.15b (first clause): `creature` attacks this combat if /// able. Obeyed iff `creature` attacks any legal target. MustAttackGeneric { creature: ObjectId }, - /// CR 508.1b + CR 508.1d: `creature` must attack `player` directly. Obeyed - /// iff `creature` attacks that player (not a planeswalker/battle they control, - /// per CR 508.5). Only emitted when `player` is currently attackable. This is - /// a `RequiredDefender::Fixed` directive (a resolution-time snapshot, e.g. - /// Alluring Siren / a ForceAttack graft). - MustAttackPlayer { + /// CR 508.1b + CR 508.1d: `creature` must attack `defender` directly. Obeyed + /// iff `creature` attacks exactly that defender — attacking a planeswalker + /// its required PLAYER controls does not obey a player-directed requirement + /// (CR 508.5), and symmetrically, attacking the controller of a required + /// PLANESWALKER does not obey Gideon Jura's. Both readings fall out of + /// comparing the whole [`AttackTarget`], the engine's single CR 506.3 + /// defender type. Only emitted when `defender` is currently attackable. This + /// is a `RequiredDefender::Fixed`/`Permanent` directive (a resolution-time + /// snapshot — Alluring Siren, a ForceAttack graft, Gideon Jura's "+2"). + MustAttackDefender { creature: ObjectId, - player: PlayerId, + defender: AttackTarget, }, /// CR 508.1b + CR 508.1d + CR 604.1: `creature` must attack ANY ONE of - /// `players` — a single `RequiredDefender::Matching` directive whose live + /// `defenders` — a single `RequiredDefender::Matching` directive whose live /// player class currently resolves to these members (e.g. every opponent tied /// for the most life; CR 508.1b lets the active player pick which tied legal /// defender to attack). This is ONE requirement (CR 508.1d counts the /// directive once, NOT once per member), kept distinct from any coexisting - /// `MustAttackPlayer` so a fixed requirement retains its own CR 701.15c - /// multiplicity. `players` is sorted + deduped and holds only currently + /// `MustAttackDefender` so a fixed requirement retains its own CR 701.15c + /// multiplicity. `defenders` is sorted + deduped and holds only currently /// attackable members; the variant is emitted only when non-empty. Obeyed iff - /// `creature` attacks a player in `players`. + /// `creature` attacks a defender in `defenders`. MustAttackAnyOf { creature: ObjectId, - players: Vec, + defenders: Vec, }, /// CR 701.15b (second clause) + CR 701.15c: `creature` attacks a player other /// than `avoided` if able. Obeyed iff `creature` attacks a player ≠ @@ -4000,8 +4112,15 @@ impl AttackDeclarationConstraints { let gates = CombatStaticGates::compute(state); let active_team = active_attacking_team(state); let candidates = team_eligible_attacker_ids(state, &gates); - let all_targets = get_valid_attack_targets(state); - let attackable_players = attackable_player_targets(state); + // CR 506.3: `all_targets` is the whole defender universe (players, + // planeswalkers, battles), so it doubles as the attackability gate for + // every `MustAttackDefender` directive regardless of defender kind. + // + // The COUNTED accessor: this is the one hoisted defender sweep per model + // build, and `attacker_candidates_sweep_attackable_players_once` is + // revert-failing on it staying hoisted (pre-fix, each candidate creature + // re-swept). + let all_targets = attackable_defender_targets(state); let mut legal_targets: HashMap> = HashMap::new(); for &cid in &candidates { @@ -4045,28 +4164,32 @@ impl AttackDeclarationConstraints { if has_generic_must || !avoided.is_empty() { requirements.push(AttackRequirement::MustAttackGeneric { creature: cid }); } - // CR 508.1d + CR 604.1: emit ONE requirement per specific-player + // CR 508.1d + CR 604.1: emit ONE requirement per specific-defender // directive, preserving the directive boundary. `Fixed` directives - // collapse by attackable player (multiple sources naming the same - // player are one requirement — CR 508.1d set semantics); each + // collapse by attackable defender (multiple sources naming the same + // defender are one requirement — CR 508.1d set semantics); each // `Matching` directive stays a single alternative-set requirement // (attack ANY current member), never merged into the fixed set, so a // coexisting fixed requirement keeps its own CR 701.15c multiplicity. - let directives = must_attack_player_directives_for_creature(state, obj); - let mut fixed_players: Vec = directives + let directives = must_attack_defender_directives_for_creature(state, obj); + let mut fixed_defenders: Vec = directives .iter() .filter_map(|(defender, _)| match defender { - ResolvedRequiredDefender::Fixed(player) => Some(*player), + ResolvedRequiredDefender::Fixed(defender) => Some(*defender), ResolvedRequiredDefender::Matching(_) => None, }) - .filter(|player| attackable_players.contains(player)) + // CR 508.1b + CR 506.3: `all_targets` is the whole live defender + // universe (players, planeswalkers, battles), so a Gideon Jura + // requirement is gated on the SAME attackability check as a + // player-directed lure — one path, no defender-kind special case. + .filter(|defender| all_targets.contains(defender)) .collect(); - fixed_players.sort_unstable_by_key(|p| p.0); - fixed_players.dedup(); - for player in fixed_players { - requirements.push(AttackRequirement::MustAttackPlayer { + fixed_defenders.sort_unstable(); + fixed_defenders.dedup(); + for defender in fixed_defenders { + requirements.push(AttackRequirement::MustAttackDefender { creature: cid, - player, + defender, }); } for (defender, _) in &directives { @@ -4076,17 +4199,17 @@ impl AttackDeclarationConstraints { // CR 508.1b: only currently-attackable members can satisfy the // directive; an all-unattackable class contributes no obeyable // requirement (mirrors the `Fixed` attackable gate above). - let mut players: Vec = members + let mut defenders: Vec = members .iter() .copied() - .filter(|player| attackable_players.contains(player)) + .filter(|defender| all_targets.contains(defender)) .collect(); - players.sort_unstable_by_key(|p| p.0); - players.dedup(); - if !players.is_empty() { + defenders.sort_unstable(); + defenders.dedup(); + if !defenders.is_empty() { requirements.push(AttackRequirement::MustAttackAnyOf { creature: cid, - players, + defenders, }); } } @@ -4248,15 +4371,22 @@ fn requirement_obeyed(req: &AttackRequirement, attacks: &[(ObjectId, AttackTarge AttackRequirement::MustAttackGeneric { creature } => { attacks.iter().any(|(c, _)| c == creature) } - AttackRequirement::MustAttackPlayer { creature, player } => attacks - .iter() - .any(|(c, t)| c == creature && matches!(t, AttackTarget::Player(p) if p == player)), + // CR 506.3 + CR 508.5: whole-`AttackTarget` equality — attacking a + // planeswalker controlled by the required player does NOT obey a + // player-directed requirement, and attacking a required planeswalker's + // controller does NOT obey Gideon Jura's. + AttackRequirement::MustAttackDefender { creature, defender } => { + attacks.iter().any(|(c, t)| c == creature && t == defender) + } // CR 508.1b + CR 508.1d: an alternative-set directive is obeyed by // attacking ANY current member of its live class (one requirement, any // member — not one per member). - AttackRequirement::MustAttackAnyOf { creature, players } => attacks.iter().any(|(c, t)| { - c == creature && matches!(t, AttackTarget::Player(p) if players.contains(p)) - }), + AttackRequirement::MustAttackAnyOf { + creature, + defenders, + } => attacks + .iter() + .any(|(c, t)| c == creature && defenders.contains(t)), AttackRequirement::AttackAwayFrom { creature, avoided } => attacks .iter() .any(|(c, t)| c == creature && matches!(t, AttackTarget::Player(p) if p != avoided)), @@ -4756,7 +4886,7 @@ fn validate_declaration_core( // CR 508.1d: maximum-requirement bar. This single comparison replaces the old // per-creature MustAttack loop, the goad-redirect loop, and the universal - // MustAttackPlayer loop — correctly permitting a maximum-score declaration even + // MustAttackDefender loop — correctly permitting a maximum-score declaration even // when individual requirements are mutually incompatible. let score = score_declaration(constraints, attacks); if score < required { @@ -5345,7 +5475,10 @@ pub fn attacker_constraints_for_active_player( // player ∪ teammates), matching the team-aware eligible set. let active_team = active_attacking_team(state); let gates = CombatStaticGates::compute(state); - let attackable = attackable_player_targets(state); + // CR 506.3: the whole live defender universe — players, planeswalkers, and + // battles — so a planeswalker-directed requirement (Gideon Jura) is gated and + // displayed exactly like a player-directed one. + let attackable = attackable_defender_targets(state); let valid: HashSet = valid_attacker_ids.iter().copied().collect(); let mut constraints = HashMap::new(); @@ -5363,29 +5496,29 @@ pub fn attacker_constraints_for_active_player( // must-attack predicate return false for it — so eligible creatures are // the only MustAttack candidates and the complement carries CantAttack. if valid.contains(&obj_id) { - if creature_must_attack_with_attackable_players_gated( + if creature_must_attack_with_attackable_targets_gated( state, obj_id, &attackable, &gates, ) { - // CR 508.1d: specific-player requirements intersected with the - // currently attackable players. n6: this single directives scan - // feeds BOTH the players list (CombatRequirement.players) and the - // source collector's carrier list — no second scan, no drift. - let directives = must_attack_player_directives_for_creature(state, obj); + // CR 508.1d: specific-defender requirements intersected with the + // currently attackable defenders. n6: this single directives scan + // feeds BOTH the defenders list (CombatRequirement.defenders) and + // the source collector's carrier list — no second scan, no drift. + let directives = must_attack_defender_directives_for_creature(state, obj); // Display-only badge (CR 508.1d): the flat union of every // attackable candidate defender across all directives (`Fixed` // singletons + every `Matching` member), deduped. The client only // renders "must attack (one of) these"; the alternative-set // grouping that legality depends on lives in the solver, not here. - let mut players: Vec = directives + let mut defenders: Vec = directives .iter() .flat_map(|(defender, _)| defender.members()) - .filter(|p| attackable.contains(p)) + .filter(|d| attackable.contains(d)) .collect(); - players.sort_unstable_by_key(|p| p.0); - players.dedup(); + defenders.sort_unstable(); + defenders.dedup(); // CR 611.2c: resolve each attackable directive's carrier — the // directing object (`source_object`), or the creature itself for // an intrinsic def. One entry per directive with an attackable @@ -5393,12 +5526,12 @@ pub fn attacker_constraints_for_active_player( // ObjectId. let attackable_carriers: Vec = directives .iter() - .filter(|(defender, _)| defender.members().any(|p| attackable.contains(&p))) + .filter(|(defender, _)| defender.members().any(|d| attackable.contains(&d))) .map(|(_, src)| src.unwrap_or(obj_id)) .collect(); let sources = must_attack_sources_gated(state, obj_id, &gates, &attackable_carriers); - constraints.insert(obj_id, CombatRequirement::MustAttack { players, sources }); + constraints.insert(obj_id, CombatRequirement::MustAttack { defenders, sources }); } } else if creature_cant_attack_gated(state, obj_id, &gates) { constraints.insert( @@ -6800,25 +6933,25 @@ mod tests { #[test] fn best_free_declaration_matches_brute_force_oracle() { use AttackRequirement::{ - AttackAwayFrom, MustAttackAnyOf, MustAttackGeneric, MustAttackPlayer, + AttackAwayFrom, MustAttackAnyOf, MustAttackDefender, MustAttackGeneric, }; let state = GameState::new_two_player(42); let p = |n: u8| AttackTarget::Player(PlayerId(n)); let pid = PlayerId; let cases: Vec<(AttackDeclarationConstraints, &str)> = vec![ - // Incompatible MustAttackPlayer: one creature, two lures → max 1. + // Incompatible MustAttackDefender: one creature, two lures → max 1. ( mk_constraints( vec![(10, vec![p(1), p(2)])], vec![ - MustAttackPlayer { + MustAttackDefender { creature: ObjectId(10), - player: pid(1), + defender: p(1), }, - MustAttackPlayer { + MustAttackDefender { creature: ObjectId(10), - player: pid(2), + defender: p(2), }, ], None, @@ -6962,7 +7095,7 @@ mod tests { vec![(10, vec![p(1), p(2)])], vec![MustAttackAnyOf { creature: ObjectId(10), - players: vec![pid(1), pid(2)], + defenders: vec![p(1), p(2)], }], None, vec![], @@ -6972,7 +7105,7 @@ mod tests { "tied matching alternative-set counts once", ), // CR 508.1d regression (the reviewer's case): a tied `Matching` directive - // {P1,P2} PLUS a fixed `MustAttackPlayer` P1. Attacking P1 obeys BOTH (2); + // {P1,P2} PLUS a fixed `MustAttackDefender` P1. Attacking P1 obeys BOTH (2); // attacking P2 obeys only the alternative-set (1). Max 2 → the solver must // force P1. Had the alternative-set been flattened+deduped into the fixed // player set ({P1,P2}), attacking P2 would tie at 1 and be wrongly legal. @@ -6982,11 +7115,11 @@ mod tests { vec![ MustAttackAnyOf { creature: ObjectId(10), - players: vec![pid(1), pid(2)], + defenders: vec![p(1), p(2)], }, - MustAttackPlayer { + MustAttackDefender { creature: ObjectId(10), - player: pid(1), + defender: p(1), }, ], None, @@ -12616,10 +12749,10 @@ mod tests { #[test] fn attacker_constraints_surface_must_attack_specific_player() { - // CR 508.1d: a creature under a `MustAttackPlayer{P2}` static surfaces as + // CR 508.1d: a creature under a `MustAttackDefender{P2}` static surfaces as // `MustAttack { players: [P2] }` (non-empty) — the specific-player list // intersected with the currently attackable players. REVERT-FAIL: dropping - // the `must_attack_players_for_creature` intersection would emit an empty + // the `must_attack_defenders_for_creature` intersection would emit an empty // `players` list, failing the `vec![P2]` assertion. // // Differential: a sibling creature under a generic `MustAttack` static in @@ -12637,11 +12770,11 @@ mod tests { .unwrap() .static_definitions // MAJOR-1: production-faithful — no static ships `affected: None`. SelfRef - // is inert to `must_attack_players_for_creature` (matches on `sd.mode`), + // is inert to `must_attack_defenders_for_creature` (matches on `sd.mode`), // so it changes no assertion; it upholds the no-`affected:None` invariant. .push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2).into(), + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(2).into(), }) .affected(TargetFilter::SelfRef), ); @@ -12668,20 +12801,20 @@ mod tests { let constraints = attacker_constraints_for_active_player(&state, &valid_attacker_ids); assert_eq!( constraints.get(&lured), - // CR 508.1d: MustAttackPlayer is a local static → carrier = lured. The + // CR 508.1d: MustAttackDefender is a local static → carrier = lured. The // generic sibling's SelfRef MustAttack no longer cross-attributes here. Some(&CombatRequirement::MustAttack { - players: vec![PlayerId(2)], + defenders: vec![AttackTarget::Player(PlayerId(2))], sources: vec![lured], }), - "MustAttackPlayer{{P2}} surfaces the specific attackable player" + "MustAttackDefender{{P2}} surfaces the specific attackable player" ); assert_eq!( constraints.get(&generic), // CR 508.1d: generic's own SelfRef MustAttack matches itself in both the // local push and the remote collect → dedup → carrier = generic. Some(&CombatRequirement::MustAttack { - players: vec![], + defenders: vec![], sources: vec![generic], }), "a generic must-attack creature surfaces an empty specific-player list" @@ -12694,7 +12827,7 @@ mod tests { /// `AttackDeclarationConstraints::build` push at combat.rs:3365) counts the /// requirement once and does not bias attack selection toward the doubly-forced /// player. REVERT-FAIL: dropping the `players.dedup()` in - /// `must_attack_players_for_creature` pushes two identical `MustAttackPlayer` + /// `must_attack_defenders_for_creature` pushes two identical `MustAttackDefender` /// requirements, so `score_single(creature → P1)` returns 2 (not 1) and the /// P1-vs-P2 tie breaks. #[test] @@ -12713,16 +12846,16 @@ mod tests { .unwrap() .static_definitions .push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(9000)), ); assert_eq!( - must_attack_players_for_creature(&state, state.objects.get(&baseline).unwrap()), - vec![PlayerId(1)], - "baseline: a single directive surfaces one player" + must_attack_defenders_for_creature(&state, state.objects.get(&baseline).unwrap()), + vec![AttackTarget::Player(PlayerId(1))], + "baseline: a single directive surfaces one defender" ); // The doubly-forced creature: two distinct sources force P1 (distinct @@ -12733,16 +12866,16 @@ mod tests { let defs = &mut state.objects.get_mut(&creature).unwrap().static_definitions; for src in [ObjectId(9001), ObjectId(9002)] { defs.push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(src), ); } defs.push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2).into(), + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(2).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(9003)), @@ -12750,9 +12883,12 @@ mod tests { // Players projection is a deduped SET: [P1, P2], NOT [P1, P1, P2]. assert_eq!( - must_attack_players_for_creature(&state, state.objects.get(&creature).unwrap()), - vec![PlayerId(1), PlayerId(2)], - "two same-player directives collapse to one entry (CR 508.1d set semantics)" + must_attack_defenders_for_creature(&state, state.objects.get(&creature).unwrap()), + vec![ + AttackTarget::Player(PlayerId(1)), + AttackTarget::Player(PlayerId(2)) + ], + "two same-defender directives collapse to one entry (CR 508.1d set semantics)" ); let constraints = AttackDeclarationConstraints::build(&state); @@ -12781,7 +12917,7 @@ mod tests { /// P2 would each score 1 and tie, wrongly permitting P2. This exercises the real /// production seam: `AttackDeclarationConstraints::build` → /// `must_attack_player_directives_for_creature` → the `MustAttackAnyOf` / - /// `MustAttackPlayer` requirement split → `score_single` / `max_no_payment`. + /// `MustAttackDefender` requirement split → `score_single` / `max_no_payment`. #[test] fn matching_tie_plus_fixed_forces_the_fixed_defender() { // The exact production most-life filter (no hand-built AST): reuse the parser @@ -12803,8 +12939,8 @@ mod tests { let defs = &mut state.objects.get_mut(&creature).unwrap().static_definitions; // Live "attacks an opponent with the most life …" — resolves to {P1, P2}. defs.push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: RequiredDefender::Matching { + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { filter: most_life.clone(), }, }) @@ -12813,8 +12949,8 @@ mod tests { // A coexisting fixed lure onto P1 (distinct source so it does not collapse // into the live directive at the def level). defs.push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player: PlayerId(1), }, }) @@ -12825,8 +12961,11 @@ mod tests { // Reach-guard: the live directive is non-vacuous — BOTH tied opponents // surface in the flat projection. assert_eq!( - must_attack_players_for_creature(&state, state.objects.get(&creature).unwrap()), - vec![PlayerId(1), PlayerId(2)], + must_attack_defenders_for_creature(&state, state.objects.get(&creature).unwrap()), + vec![ + AttackTarget::Player(PlayerId(1)), + AttackTarget::Player(PlayerId(2)) + ], "both tied most-life opponents are candidate defenders" ); @@ -12981,7 +13120,7 @@ mod tests { serde_json::from_str::(r#"{"kind":"MustAttack","players":[1]}"#) .unwrap(), CombatRequirement::MustAttack { - players: vec![PlayerId(1)], + defenders: vec![AttackTarget::Player(PlayerId(1))], sources: vec![], }, ); @@ -13232,7 +13371,7 @@ mod tests { #[test] fn must_attack_player_enforces_specific_player() { - // CR 508.1d: a creature with MustAttackPlayer{P2} must attack P2 when + // CR 508.1d: a creature with MustAttackDefender{P2} must attack P2 when // P2 is a legal target; attacking a different player is illegal. let mut state = GameState::new(crate::types::format::FormatConfig::standard(), 3, 42); state.turn_number = 2; @@ -13244,12 +13383,12 @@ mod tests { .get_mut(&attacker) .unwrap() .static_definitions - .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2).into(), + .push(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(2).into(), })); // Attacking the wrong player (P1) while P2 is a legal target: illegal. New - // contract (CR 508.1d): MustAttackPlayer is scored by the maximum-requirement + // contract (CR 508.1d): MustAttackDefender is scored by the maximum-requirement // bar, so the rejection cites CR 508.1d. let wrong = declare_attackers( &mut state, @@ -13353,11 +13492,11 @@ mod tests { .get_mut(&attacker) .unwrap() .static_definitions - .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + .push(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), })); - // New contract (CR 508.1d): the MustAttackPlayer requirement is scored by the + // New contract (CR 508.1d): the MustAttackDefender requirement is scored by the // maximum-requirement bar, so an omitted required attacker cites CR 508.1d. let result = declare_attackers(&mut state, &[], &mut vec![]); assert!(result.is_err()); @@ -13373,8 +13512,8 @@ mod tests { .get_mut(&attacker) .unwrap() .static_definitions - .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + .push(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), })); let planeswalker = create_planeswalker(&mut state, PlayerId(1), "Required Player's Walker"); diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index 2be6093fb7..b11c0f35d6 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -156,7 +156,7 @@ pub(crate) fn is_data_carrying_static(mode: &StaticMode) -> bool { // CR 508.1d: MustAttackPlayer carries the `PlayerId` that must be // attacked (Alluring Siren). Enforced by direct match in combat.rs // declare-attackers validation. - | StaticMode::MustAttackPlayer { .. } + | StaticMode::MustAttackDefender { .. } // CR 509.1b: CantBeBlockedByMoreThan carries the blocker maximum // (Stalking Tiger). Enforced in combat.rs declare-blockers validation. | StaticMode::CantBeBlockedByMoreThan { .. } @@ -890,6 +890,8 @@ fn fmt_typed_filter(tf: &TypedFilter) -> String { ControllerRef::EnchantedPlayer => "enchanted player's", // CR 102.1: Display label for active-player controller scope. ControllerRef::ActivePlayer => "the active player's", + // CR 109.4 + CR 611.2: snapshotted controller scope. + ControllerRef::SpecificPlayer { .. } => "that player's", }; let zone_str = format!("{zone:?}").to_lowercase(); parts.push(format!( @@ -1062,6 +1064,8 @@ fn fmt_typed_filter(tf: &TypedFilter) -> String { ControllerRef::EnchantedPlayer => "enchanted player", // CR 102.1: Display label for active-player controller scope. ControllerRef::ActivePlayer => "the active player", + // CR 109.4 + CR 611.2: Display label for a snapshotted controller scope. + ControllerRef::SpecificPlayer { .. } => "that player", }; parts.push(label.into()); } else { @@ -1137,6 +1141,8 @@ fn fmt_controller(ctrl: &ControllerRef) -> String { ControllerRef::EnchantedPlayer => "enchanted player controls", // CR 102.1: Display label for active-player controller scope. ControllerRef::ActivePlayer => "the active player controls", + // CR 109.4 + CR 611.2: Display label for a snapshotted controller scope. + ControllerRef::SpecificPlayer { .. } => "that player controls", } .into() } @@ -1267,6 +1273,8 @@ fn fmt_player_scope(scope: &PlayerScope) -> String { PlayerScope::DefendingPlayer => "defending player".to_string(), PlayerScope::SourceChosenPlayer => "the chosen player".to_string(), PlayerScope::AnyTurn => "any turn".to_string(), + // CR 109.4 + CR 611.2: display label for a snapshotted duration scope. + PlayerScope::SpecificPlayer { .. } => "that player".to_string(), PlayerScope::ParentObjectTargetController => "parent target's controller".to_string(), PlayerScope::Opponent { aggregate } => { format!("{} of opponents", fmt_aggregate_function(*aggregate)) @@ -2418,7 +2426,15 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { | Effect::Connive { target, .. } | Effect::PhaseOut { target } | Effect::PhaseIn { target } - | Effect::ForceAttack { target, .. } + // CR 508.1d: single-scope ForceAttack reports its chosen `target` + // ("Target creature attacks you this combat if able"); the mass scope + // (Gideon Jura's "creatures that player controls") reports a `filter` + // below, since CR 115.1 makes that population a non-target. + | Effect::ForceAttack { + scope: EffectScope::Single, + target, + .. + } // CR 701.27a: single-scope Transform reports its `target` like other // single-target effects; mass Transform (scope:All) reports a `filter` below. | Effect::Transform { @@ -2471,6 +2487,13 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { target, .. } + // CR 508.1d + CR 115.1: the mass forced-attack population (Gideon Jura's + // "creatures that player controls") is a `filter`, not a target. + | Effect::ForceAttack { + scope: EffectScope::All, + target, + .. + } | Effect::BounceAll { target, .. } | Effect::CounterAll { target, .. } | Effect::DamageAll { diff --git a/crates/engine/src/game/effects/copy_spell.rs b/crates/engine/src/game/effects/copy_spell.rs index 13451d6f9c..c925d752ad 100644 --- a/crates/engine/src/game/effects/copy_spell.rs +++ b/crates/engine/src/game/effects/copy_spell.rs @@ -497,7 +497,10 @@ fn resolve_copier_player( | ControllerRef::EnchantedPlayer // CR 102.1: no card scopes "the active player copies this spell"; // fail closed (mirrors DefendingPlayer / EnchantedPlayer). - | ControllerRef::ActivePlayer => None, + | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: no card scopes a copier to a snapshotted player; + // the lowering exists only for combat-requirement continuous effects. + | ControllerRef::SpecificPlayer { .. } => None, } } diff --git a/crates/engine/src/game/effects/encore.rs b/crates/engine/src/game/effects/encore.rs index 77cf959715..48d1f7d50c 100644 --- a/crates/engine/src/game/effects/encore.rs +++ b/crates/engine/src/game/effects/encore.rs @@ -26,7 +26,7 @@ //! `MustAttackPlayer { player }` requirement (CR 508.1d) bound to the opponent //! it was created for, lasting until end of turn (CR 702.141a "this turn"). //! Binding the requirement per opponent — instead of via the generic -//! `Effect::ForceAttack`, whose `required_player` context ref has no +//! `Effect::ForceAttack`, whose `required_defender` context ref has no //! "the opponent" resolution and would fall back to the controller — is the //! reason Encore needs a dedicated resolver. //! - **Haste is baked into the copy** via `extra_keywords` (CR 707.2, the @@ -95,8 +95,8 @@ pub fn resolve( TargetFilter::SpecificObject { id: token_id }, vec![ContinuousModification::AddStaticMode { // CR 611.2: snapshot the specific opponent at resolution. - mode: StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player: opponent }, + mode: StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player: opponent }, }, }], None, diff --git a/crates/engine/src/game/effects/force_attack.rs b/crates/engine/src/game/effects/force_attack.rs index 3936c6b95a..d173714e18 100644 --- a/crates/engine/src/game/effects/force_attack.rs +++ b/crates/engine/src/game/effects/force_attack.rs @@ -1,14 +1,138 @@ use super::resolve_player_for_context_ref; use crate::game::targeting::resolved_object_ids_for_filter; use crate::types::ability::{ - ContinuousModification, Effect, EffectError, EffectKind, ResolvedAbility, TargetFilter, + ContinuousModification, ControllerRef, Duration, Effect, EffectError, EffectKind, EffectScope, + PlayerScope, ResolvedAbility, TargetFilter, TargetRef, }; use crate::types::events::GameEvent; use crate::types::game_state::GameState; +use crate::types::identifiers::ObjectIncarnationRef; use crate::types::statics::{RequiredDefender, StaticMode}; -/// CR 508.1d: Force attack — the target creature must attack the required player -/// this turn/combat if able. +/// CR 506.3 + CR 611.2: Classify the `required_defender` filter and snapshot it +/// into the durable [`RequiredDefender`] combat enforcement reads. +/// +/// A filter naming an OBJECT lowers to `Permanent` (pinned by incarnation — CR +/// 400.7, so a defender that leaves and re-enters does not inherit a requirement +/// aimed at the old object); every other filter is a player reference and lowers +/// to `Fixed` via the shared context-ref resolver. `SelfRef` is the only object +/// arm a printed card reaches today (Gideon Jura's "attack Gideon Jura if +/// able"), but the classification is by REFERENT KIND rather than by that one +/// filter, so a future "attacks target planeswalker if able" needs no new +/// branch. +/// +/// Returns `None` when an object referent names no live object, so the caller +/// grafts nothing rather than a requirement aimed at a vanished defender. +fn snapshot_required_defender( + state: &GameState, + ability: &ResolvedAbility, + filter: &TargetFilter, +) -> Option { + if !filter_denotes_object(filter) { + return Some(RequiredDefender::Fixed { + player: resolve_player_for_context_ref(state, ability, filter), + }); + } + let defender_id = resolved_object_ids_for_filter(state, ability, filter) + .into_iter() + .next()?; + let obj = state.objects.get(&defender_id)?; + Some(RequiredDefender::Permanent { + permanent: ObjectIncarnationRef::from_object(obj), + }) +} + +/// CR 506.3: whether `filter` denotes an OBJECT defender rather than a player. +/// An explicit allow-list, never a catch-all: a filter whose referent kind is +/// unclear keeps the pre-existing player reading, which is the conservative +/// direction — every card using this effect before Gideon Jura named a player. +fn filter_denotes_object(filter: &TargetFilter) -> bool { + matches!( + filter, + TargetFilter::SelfRef + | TargetFilter::SpecificObject { .. } + | TargetFilter::ParentTarget + | TargetFilter::ParentTargetSlot { .. } + ) +} + +/// CR 611.2c: For a BROADCAST subject ("creatures that player controls") return +/// the affected filter to install INTACT, with its player reference lowered to a +/// resolution-time snapshot. Returns `None` for a chosen-target subject ("target +/// creature attacks you this combat if able"), which keeps the pre-existing +/// per-object `SpecificObject` graft — CR 611.2c's dynamic-population concern +/// does not arise when the effect names specific objects. +/// +/// Gideon Jura's official ruling is the reason the broadcast form cannot freeze +/// its set: the "+2" "doesn't lock in what it applies to … whatever creatures +/// the targeted opponent controls during the declare attackers step of their +/// next turn must attack Gideon Jura if able. This includes creatures that come +/// under that player's control after the ability has resolved." +/// +/// Only `ControllerRef::TargetPlayer` / `TargetOpponent` need lowering: +/// `ControllerRef::You` / `Opponent` are resolved by `layers.rs` against the +/// continuous effect's own snapshotted `controller` (the Kardur path), and no +/// other controller ref reaches a broadcast force-attack subject today. +fn lower_dynamic_affected( + ability: &ResolvedAbility, + target: &TargetFilter, + scope: EffectScope, +) -> Option { + // CR 115.1: the scope is the authority for which form this is — a `Single` + // subject is a chosen target no matter what filter shape it happens to + // carry, so it must never take the population path. + if scope != EffectScope::All { + return None; + } + let TargetFilter::Typed(typed) = target else { + return None; + }; + let mut typed = typed.clone(); + if matches!( + typed.controller, + Some(ControllerRef::TargetPlayer | ControllerRef::TargetOpponent) + ) { + // CR 109.4 + CR 611.2: "that player" is fixed when the ability resolves. + // `ability.targets` no longer exists when the layer pass re-derives the + // affected set, so bind the id now. + let id = ability.targets.iter().find_map(|t| match t { + TargetRef::Player(pid) => Some(*pid), + TargetRef::Object(_) => None, + })?; + typed.controller = Some(ControllerRef::SpecificPlayer { id }); + } + Some(TargetFilter::Typed(typed)) +} + +/// CR 611.2 + CR 514.2: Lower a target-scoped duration to a resolution-time +/// snapshot, so the installed continuous effect's expiry still names a concrete +/// player after the resolving ability (and its `targets`) is gone. +/// +/// `PlayerScope::Target` is the only scope needing this: `Controller` is already +/// carried by the continuous effect's own `controller` field, which +/// `layers.rs::prune_until_next_turn_effects` reads directly. A duration whose +/// target cannot be resolved is left untouched rather than guessed at — an +/// unarmable expiry is a visible bug, a silently wrong player is not. +fn lower_target_scoped_duration(ability: &ResolvedAbility, duration: Duration) -> Duration { + let Duration::UntilEndOfNextTurnOf { + player: PlayerScope::Target, + } = duration + else { + return duration; + }; + let Some(id) = ability.targets.iter().find_map(|t| match t { + TargetRef::Player(pid) => Some(*pid), + TargetRef::Object(_) => None, + }) else { + return duration; + }; + Duration::UntilEndOfNextTurnOf { + player: PlayerScope::SpecificPlayer { id }, + } +} + +/// CR 508.1d: Force attack — the creatures matching `target` must attack the +/// required defender this turn/combat if able. pub fn resolve( state: &mut GameState, ability: &ResolvedAbility, @@ -16,32 +140,71 @@ pub fn resolve( ) -> Result<(), EffectError> { let Effect::ForceAttack { target, - required_player, + required_defender, duration, + scope, } = &ability.effect else { return Ok(()); }; - let player = resolve_player_for_context_ref(state, ability, required_player); - for obj_id in resolved_object_ids_for_filter(state, ability, target) { - if !state.objects.contains_key(&obj_id) { - continue; - } + // CR 611.2a: "lasts as long as stated by the spell or ability creating it." + // A stated duration written as a leading CLAUSE rather than inside the + // predicate — Gideon Jura's "During target opponent's next turn, creatures + // that player controls attack ~ if able" — is stamped by the parser onto + // `ability.duration`, so it must win over the effect's own field. Same + // precedence the `GenericEffect` arm of `effects/effect.rs::resolve` applies, + // for the same reason. + let duration = ability.duration.clone().unwrap_or_else(|| duration.clone()); - state.add_transient_continuous_effect( - ability.source_id, - ability.controller, - duration.clone(), - TargetFilter::SpecificObject { id: obj_id }, - vec![ContinuousModification::AddStaticMode { - // CR 611.2: the required defender is snapshotted at resolution. - mode: StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player }, - }, - }], - None, - ); + // CR 611.2 + CR 109.4: "during TARGET opponent's next turn" is scoped to the + // player this ability targeted. `PlayerScope::Target` resolves by reading + // `ability.targets`, which no longer exists once the continuous effect is + // installed and the ability is gone — so snapshot it now, exactly as the + // affected filter's controller ref is snapshotted below. + let duration = lower_target_scoped_duration(ability, duration); + + let resolved = snapshot_required_defender(state, ability, required_defender); + + if let Some(defender) = resolved { + // CR 611.2c: a broadcast subject keeps ONE continuous effect carrying the + // live filter, so the affected creature set is re-derived every + // declare-attackers step. `register_transient_effect` routes + // `MustAttackAwayFromSource` grants down the same path for the same + // reason (Kardur, Maximum Carnage); this resolver installs directly, so + // it makes the same call here. + if let Some(affected) = lower_dynamic_affected(ability, target, *scope) { + state.add_transient_continuous_effect( + ability.source_id, + ability.controller, + duration.clone(), + affected, + vec![ContinuousModification::AddStaticMode { + mode: StaticMode::MustAttackDefender { defender }, + }], + None, + ); + } else { + for obj_id in resolved_object_ids_for_filter(state, ability, target) { + if !state.objects.contains_key(&obj_id) { + continue; + } + + state.add_transient_continuous_effect( + ability.source_id, + ability.controller, + duration.clone(), + TargetFilter::SpecificObject { id: obj_id }, + vec![ContinuousModification::AddStaticMode { + // CR 611.2: the required defender is snapshotted at resolution. + mode: StaticMode::MustAttackDefender { + defender: defender.clone(), + }, + }], + None, + ); + } + } } events.push(GameEvent::EffectResolved { @@ -70,8 +233,9 @@ mod tests { ResolvedAbility::new( Effect::ForceAttack { target: TargetFilter::Any, - required_player: TargetFilter::Controller, + required_defender: TargetFilter::Controller, duration, + scope: EffectScope::Single, }, vec![TargetRef::Object(target)], source, @@ -113,8 +277,8 @@ mod tests { matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player }, + mode: StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player }, }, } if *player == PlayerId(0) ) @@ -142,10 +306,11 @@ mod tests { let mut ability = ResolvedAbility::new( Effect::ForceAttack { target: TargetFilter::SelfRef, - required_player: TargetFilter::Typed( + required_defender: TargetFilter::Typed( TypedFilter::default().controller(ControllerRef::ChosenPlayer { index: 0 }), ), duration: Duration::UntilEndOfCombat, + scope: EffectScope::Single, }, vec![], source, @@ -166,8 +331,8 @@ mod tests { matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { player }, + mode: StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player }, }, } if *player == PlayerId(1) ) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 03c7d6aaf8..ede598cfad 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -5163,7 +5163,7 @@ fn affected_objects_from_events( matches!( sd.mode, crate::types::statics::StaticMode::MustAttack - | crate::types::statics::StaticMode::MustAttackPlayer { .. } + | crate::types::statics::StaticMode::MustAttackDefender { .. } | crate::types::statics::StaticMode::Continuous ) }) else { @@ -12785,6 +12785,9 @@ fn resolve_grant_next_spell_ability( | PlayerScope::AllPlayers { .. } | PlayerScope::RecipientController | PlayerScope::AnyTurn + // CR 611.2 + CR 514.2: duration-timing-only, like `AnyTurn` — never reached + // from a value/quantity/player-selection position. + | PlayerScope::SpecificPlayer { .. } | PlayerScope::DefendingPlayer | PlayerScope::ParentObjectTargetController | PlayerScope::SourceChosenPlayer => ability.controller, diff --git a/crates/engine/src/game/effects/prevent_damage.rs b/crates/engine/src/game/effects/prevent_damage.rs index b8f3bb0a1c..53c5173309 100644 --- a/crates/engine/src/game/effects/prevent_damage.rs +++ b/crates/engine/src/game/effects/prevent_damage.rs @@ -201,6 +201,16 @@ fn untargeted_damage_filter( // matching is `typed_recipient_valid_card_filter`'s job, so this // arm must be checked BEFORE the generic `is_context_ref()` catch-all. TargetFilter::TrackedSet { .. } | TargetFilter::TrackedSetFiltered { .. } => None, + // CR 615 + CR 608.2c: the printed-name self-reference ("prevent all + // damage that would be dealt to HIM this turn" — Gideon Jura, Gideon of + // the Trials) names the source OBJECT, not a player. `SelfRef` is in + // `is_context_ref()`, so without this arm the catch-all below would + // lower it to a PLAYER shield on the source's controller — preventing + // all damage to the player instead of to the Gideon. Object matching is + // `typed_recipient_valid_card_filter`'s job, so this arm must precede + // the generic `is_context_ref()` catch-all (same ordering contract as + // the `TrackedSet` carve-out above). + TargetFilter::SelfRef => None, filter if filter.is_context_ref() => Some(player_damage_filter( super::resolve_player_for_context_ref(state, ability, filter), )), @@ -229,6 +239,14 @@ fn typed_recipient_valid_card_filter(target: &TargetFilter) -> Option { Some(filter.clone()) } + // CR 615 + CR 608.2c: the printed-name self-reference IS an object + // recipient — the shield rides on the source permanent (the untargeted + // branch of `resolve`) and `valid_card: SelfRef` scopes it to damage + // dealt to that host. Checked before the generic `is_context_ref()` + // exclusion below, which would otherwise reject it; mirrors the + // `TrackedSet` carve-out and pairs with `untargeted_damage_filter`'s + // matching `SelfRef => None` arm. + filter @ TargetFilter::SelfRef => Some(filter.clone()), filter if filter.is_context_ref() => None, filter => Some(filter.clone()), } @@ -1307,6 +1325,134 @@ mod tests { assert_eq!(state.players[0].life, 20); } + /// CR 615 + CR 608.2c: a `SelfRef` recipient ("prevent all damage that would + /// be dealt to HIM this turn" — Gideon Jura, Gideon of the Trials) scopes the + /// shield to the SOURCE OBJECT. + /// + /// Two revert-failing halves, because the bug had two independent ways to + /// manifest: + /// * `untargeted_damage_filter` must NOT lower `SelfRef` (a context ref) to + /// a PLAYER shield on the source's controller — that would prevent damage + /// to the Gideon's controller instead of to the Gideon. + /// * the shield must carry `valid_card: SelfRef` so it fires only on damage + /// to its host. With the pre-fix `TargetFilter::Any` recipient the shield + /// carried NO constraint at all and Fogged every damage event that turn — + /// which the negative half below catches. + #[test] + fn self_ref_recipient_prevention_scopes_the_shield_to_its_host() { + let mut state = GameState::new_two_player(42); + let gideon = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Gideon Jura".to_string(), + Zone::Battlefield, + ); + let damage_source = create_object( + &mut state, + CardId(2), + PlayerId(1), + "Attacker".to_string(), + Zone::Battlefield, + ); + let bystander = create_object( + &mut state, + CardId(3), + PlayerId(0), + "Bystander".to_string(), + Zone::Battlefield, + ); + for id in [gideon, bystander] { + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.power = Some(6); + obj.toughness = Some(6); + } + + let ability = ResolvedAbility::new( + Effect::PreventDamage { + amount: PreventionAmount::All, + amount_dynamic: None, + target: TargetFilter::SelfRef, + scope: PreventionScope::AllDamage, + damage_source_filter: None, + prevention_duration: None, + }, + vec![], + gideon, + PlayerId(0), + ); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + + // The shield rides on the source permanent, object-scoped — never in the + // global pending registry, which is the un-constrained Fog placement. + assert!( + state.pending_damage_replacements.is_empty(), + "a SelfRef recipient is object-scoped, not a global Fog: {:?}", + state.pending_damage_replacements + ); + let shield = state + .objects + .get(&gideon) + .unwrap() + .replacement_definitions + .last() + .expect("the shield hosts on the source"); + assert_eq!(shield.valid_card, Some(TargetFilter::SelfRef)); + assert_eq!( + shield.damage_target_filter, None, + "SelfRef is an OBJECT recipient — lowering it to a player shield \ + would protect the controller instead of the Gideon" + ); + + let ctx = deal_damage::DamageContext::from_source(&state, damage_source).unwrap(); + // Damage to the host is prevented. + let to_host = deal_damage::apply_damage_to_target( + &mut state, + &ctx, + TargetRef::Object(gideon), + 3, + false, + &mut events, + ) + .unwrap(); + assert!( + matches!(to_host, deal_damage::DamageResult::Applied(0)), + "damage to the shield's host is prevented" + ); + + // Negative half: everything else still takes damage. This is what fails + // on the pre-fix `Any` recipient, which prevented all damage in the game. + let to_bystander = deal_damage::apply_damage_to_target( + &mut state, + &ctx, + TargetRef::Object(bystander), + 3, + false, + &mut events, + ) + .unwrap(); + assert!( + matches!(to_bystander, deal_damage::DamageResult::Applied(3)), + "another permanent is untouched by a host-scoped shield" + ); + let to_controller = deal_damage::apply_damage_to_target( + &mut state, + &ctx, + TargetRef::Player(PlayerId(0)), + 3, + false, + &mut events, + ) + .unwrap(); + assert!( + matches!(to_controller, deal_damage::DamageResult::Applied(3)), + "the source's CONTROLLER is not the recipient" + ); + assert_eq!(state.players[0].life, 17); + } + #[test] fn player_recipient_prevention_uses_damage_target_filter() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/game/effects/sacrifice.rs b/crates/engine/src/game/effects/sacrifice.rs index ce77d9455d..bbf214e5c6 100644 --- a/crates/engine/src/game/effects/sacrifice.rs +++ b/crates/engine/src/game/effects/sacrifice.rs @@ -110,6 +110,8 @@ fn resolve_sacrifice_scope( .unwrap_or_default(), // CR 102.1: the active player, read live. Some(ControllerRef::ActivePlayer) => vec![state.active_player], + // CR 109.4 + CR 611.2: a snapshotted id names exactly one sacrificer. + Some(ControllerRef::SpecificPlayer { id }) => vec![id], } } diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 5d67c3ab9e..5fc90f0d32 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1486,6 +1486,11 @@ pub(crate) fn controller_ref_player( .and_then(|host| host.as_player()), // CR 102.1: the player whose turn it is — read live. ControllerRef::ActivePlayer => Some(state.active_player), + // CR 109.4 + CR 611.2: a resolution-time snapshot; already concrete, so + // it needs neither `ability` nor `state` context. This is what makes it + // the correct lowering for a continuous effect that outlives its + // resolving ability (Gideon Jura's "+2"). + ControllerRef::SpecificPlayer { id } => Some(*id), } } /// Whether `filter` references the resolution-local `last_zone_changed_ids` @@ -1736,6 +1741,8 @@ fn stack_entry_controller_matches( } // CR 102.1: the active player, read live. Some(ControllerRef::ActivePlayer) => state.active_player == entry_controller, + // CR 109.4 + CR 611.2: a resolution-time snapshot — compare directly. + Some(ControllerRef::SpecificPlayer { id }) => *id == entry_controller, } } @@ -2839,6 +2846,16 @@ fn filter_inner_for_object( return false; } } + // CR 109.4 + CR 611.2: "that player controls", already lowered + // to a snapshot id. This is the arm Gideon Jura's "+2" runs + // through at every declare-attackers step: the OBJECT SET is + // re-derived here each time (CR 611.2c), while the player it + // is derived against was frozen when the ability resolved. + ControllerRef::SpecificPlayer { id } => { + if *id != obj_ctrl { + return false; + } + } } } // All source-relative properties share the exact triggered-source @@ -3737,6 +3754,18 @@ pub fn spell_record_matches_filter( // spell-history record (a cast snapshot carries no live // turn context). Fail closed. ControllerRef::ActivePlayer => return false, + // CR 109.4 + CR 611.2: a snapshot id IS resolvable here, but + // spell history is already scoped to `controller`'s casts, so + // the record matches only when the snapshot names that same + // player. No card produces this combination today (the + // lowering exists only for combat-requirement continuous + // effects), but the comparison is exact rather than + // fail-closed because the id needs no missing context. + ControllerRef::SpecificPlayer { id } => { + if *id != controller { + return false; + } + } } } @@ -5243,6 +5272,11 @@ fn matches_filter_prop( (Some(ControllerRef::EnchantedPlayer), Some(pid)) => perm.controller == pid, // CR 102.1: active-player-scoped name match (resolved live). (Some(ControllerRef::ActivePlayer), Some(pid)) => perm.controller == pid, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + (Some(ControllerRef::SpecificPlayer { .. }), Some(pid)) => { + perm.controller == pid + } (Some(_), None) => false, (None, _) => true, }; @@ -5302,6 +5336,9 @@ fn matches_filter_prop( } // CR 102.1: Ownership relative to the active player (read live). ControllerRef::ActivePlayer => state.active_player == obj.owner, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + ControllerRef::SpecificPlayer { id } => *id == obj.owner, }, // CR 303.4 + CR 301.5f: `EnchantedBy` is source-relative when the // source is an Aura ("enchanted creature gets +1/+1"). When the source @@ -6051,6 +6088,9 @@ fn zone_change_record_matches_property( } // CR 102.1: Ownership relative to the active player (read live). ControllerRef::ActivePlayer => state.active_player == record.owner, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + ControllerRef::SpecificPlayer { id } => *id == record.owner, }, // CR 205.3e + CR 205.3m + CR 702.73a: Source's chosen creature type // applied to the snapshot subtypes, including changeling snapshots. @@ -6418,6 +6458,9 @@ fn attachment_controller_matches( } // CR 102.1: attachment controller relative to the active player (live). Some(ControllerRef::ActivePlayer) => state.active_player == attachment_controller, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + Some(ControllerRef::SpecificPlayer { id }) => *id == attachment_controller, } } @@ -7080,6 +7123,9 @@ fn player_matches_target_filter_with( // active-player resolution path runs through `controller_ref_player` // where `state` is in scope. Some(ControllerRef::ActivePlayer) => false, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + Some(ControllerRef::SpecificPlayer { id }) => *id == player_id, None => true, }, // Typed filters with type_filters don't match players diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index d479f090df..7943bdfd6c 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -660,13 +660,22 @@ pub fn prune_until_next_turn_effects(state: &mut GameState, active_player: Playe // turn's own cleanup because that turn's untap step already passed before // the effect was created, so this is the controller's *next* turn. for e in state.transient_continuous_effects.iter_mut() { - if matches!( - e.duration, + // CR 514.2 + CR 109.4: the player whose next turn ends this effect. The + // `Controller` scope reads the effect's own controller ("until the end of + // YOUR next turn"); `SpecificPlayer` is the resolution-time snapshot a + // resolver installs when the window belongs to someone else — Gideon + // Jura's "During target opponent's next turn". Both arm identically once + // that player becomes the active player. + let armed_for = match e.duration { Duration::UntilEndOfNextTurnOf { - player: PlayerScope::Controller - } - ) && e.controller == active_player - { + player: PlayerScope::Controller, + } => Some(e.controller), + Duration::UntilEndOfNextTurnOf { + player: PlayerScope::SpecificPlayer { id }, + } => Some(id), + _ => None, + }; + if armed_for == Some(active_player) { e.duration = Duration::UntilEndOfTurn; } } @@ -6811,7 +6820,7 @@ fn static_mode_needs_source_controller_anchor(mode: &crate::types::statics::Stat /// match arm when a new consumer needs another mode's directing source. fn static_mode_carries_directing_source(mode: &crate::types::statics::StaticMode) -> bool { use crate::types::statics::StaticMode; - matches!(mode, StaticMode::MustAttackPlayer { .. }) + matches!(mode, StaticMode::MustAttackDefender { .. }) } /// CR 109.5: True when a `TargetFilter` constrains the controller of matched diff --git a/crates/engine/src/game/players.rs b/crates/engine/src/game/players.rs index f2e4906eaa..e9ce1c5b6c 100644 --- a/crates/engine/src/game/players.rs +++ b/crates/engine/src/game/players.rs @@ -309,7 +309,10 @@ pub fn apnap_order_from( // CR 303.4b: Enchanted-player scope is not enumerable. Fail closed. | ControllerRef::EnchantedPlayer // CR 102.1: the active player is exactly this default anchor. - | ControllerRef::ActivePlayer, + | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: a snapshotted id is not a role this anchor + // enumerates; fall back to the default anchor. + | ControllerRef::SpecificPlayer { .. }, ) => state.active_player, }; diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index f221faf51e..deb2a32801 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -4620,6 +4620,9 @@ fn resolve_ref( } // CR 102.1: attachment controlled by the active player. Some(ControllerRef::ActivePlayer) => snap.controller == state.active_player, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + Some(ControllerRef::SpecificPlayer { id }) => snap.controller == *id, }) .count(), ) @@ -4686,6 +4689,9 @@ fn damage_source_controller_matches( } // CR 102.1: damage source controlled by the active player (read live). ControllerRef::ActivePlayer => actual == state.active_player, + // CR 109.4 + CR 611.2: a resolution-time snapshot player id — concrete with + // no ability/event context needed, unlike the fail-closed siblings above. + ControllerRef::SpecificPlayer { id } => actual == *id, } } @@ -6183,6 +6189,11 @@ fn resolve_single_player_scope( "PlayerScope::AnyTurn is duration-timing-only; never reached via QuantityRef" ) } + PlayerScope::SpecificPlayer { .. } => { + unreachable!( + "PlayerScope::SpecificPlayer is duration-timing-only; never reached via QuantityRef" + ) + } } } @@ -6281,6 +6292,11 @@ where "PlayerScope::AnyTurn is duration-timing-only; never reached via QuantityRef" ) } + PlayerScope::SpecificPlayer { .. } => { + unreachable!( + "PlayerScope::SpecificPlayer is duration-timing-only; never reached via QuantityRef" + ) + } } } diff --git a/crates/engine/src/game/replacement.rs b/crates/engine/src/game/replacement.rs index 0ff8278dd5..7d7810340d 100644 --- a/crates/engine/src/game/replacement.rs +++ b/crates/engine/src/game/replacement.rs @@ -5539,6 +5539,9 @@ fn replacement_active_player_matches( Some(ControllerRef::TriggeringPlayer) => false, Some(ControllerRef::EnchantedPlayer) => false, Some(ControllerRef::ActivePlayer) => false, + // CR 109.4 + CR 611.2: a snapshot id IS resolvable — the active player + // satisfies the requirement exactly when they are that player. + Some(ControllerRef::SpecificPlayer { id }) => state.active_player == id, None => true, } } @@ -5677,6 +5680,10 @@ fn evaluate_replacement_condition( // controller-relative role (You/Opponent); `ActivePlayer` is not // one, and the parser does not emit it here. Fail closed. Some(ControllerRef::ActivePlayer) => false, + // CR 109.4 + CR 611.2: the turn gate expects a controller-relative + // role (You/Opponent); a snapshot id is not one, and the parser + // never emits it here. Fail closed (mirrors ActivePlayer). + Some(ControllerRef::SpecificPlayer { .. }) => false, None => true, }; if !turn_ok { @@ -5727,6 +5734,10 @@ fn evaluate_replacement_condition( // controller-relative role (You/Opponent); `ActivePlayer` is not // one, and the parser does not emit it here. Fail closed. Some(ControllerRef::ActivePlayer) => false, + // CR 109.4 + CR 611.2: the turn gate expects a controller-relative + // role (You/Opponent); a snapshot id is not one, and the parser + // never emits it here. Fail closed (mirrors ActivePlayer). + Some(ControllerRef::SpecificPlayer { .. }) => false, None => true, }; if !turn_ok { @@ -5908,6 +5919,7 @@ fn evaluate_replacement_condition( // CR 102.1: no replacement condition scopes its event source to the // active player here. Fail closed (mirrors the siblings above). | ControllerRef::ActivePlayer => false, + | ControllerRef::SpecificPlayer { .. } => false, } } ReplacementCondition::EffectCausedDiscard => matches!( @@ -6132,6 +6144,7 @@ fn apply_state_level_gates( | crate::types::ability::ControllerRef::TriggeringPlayer | crate::types::ability::ControllerRef::EnchantedPlayer | crate::types::ability::ControllerRef::ActivePlayer => false, + crate::types::ability::ControllerRef::SpecificPlayer { .. } => false, }; if !matches { return false; @@ -6546,6 +6559,7 @@ fn object_replacement_candidate_applies( // CR 102.1: token-owner scope is not scoped to the active player // here; fail closed (mirrors the siblings above). | crate::types::ability::ControllerRef::ActivePlayer => false, + | crate::types::ability::ControllerRef::SpecificPlayer { .. } => false, }; if !matches { return false; diff --git a/crates/engine/src/game/sba.rs b/crates/engine/src/game/sba.rs index 5033e4b8c9..9ec86d14ca 100644 --- a/crates/engine/src/game/sba.rs +++ b/crates/engine/src/game/sba.rs @@ -630,6 +630,8 @@ fn static_affects_player( // CR 102.1: this matcher has no `GameState` to read // `active_player` from. Fail closed (mirrors the siblings above). Some(ControllerRef::ActivePlayer) => false, + // CR 109.4 + CR 611.2: a snapshotted id IS resolvable here. + Some(ControllerRef::SpecificPlayer { id }) => *id == player_id, None => true, }, Some(TargetFilter::Player) => true, diff --git a/crates/engine/src/game/scenario.rs b/crates/engine/src/game/scenario.rs index 1ee37cc937..9cb8577f66 100644 --- a/crates/engine/src/game/scenario.rs +++ b/crates/engine/src/game/scenario.rs @@ -703,6 +703,53 @@ impl GameScenario { builder } + /// CR 306.1 + CR 306.5b: Add a planeswalker to the battlefield with its + /// loyalty abilities parsed from Oracle text and `loyalty` loyalty counters + /// already on it. + /// + /// Mirrors [`Self::add_enchantment_from_oracle`], plus the two things a + /// planeswalker needs that no other permanent does: the loyalty counters + /// (CR 306.5b — a planeswalker with none is put into its owner's graveyard + /// as a state-based action, so a fixture without them evaporates), and the + /// `Gideon`-style planeswalker subtype the caller supplies. + pub fn add_planeswalker_from_oracle( + &mut self, + player: PlayerId, + name: &str, + subtype: &str, + loyalty: u32, + oracle_text: &str, + ) -> CardBuilder<'_> { + let card_id = CardId(self.state.next_object_id); + let id = create_object( + &mut self.state, + card_id, + player, + name.to_string(), + Zone::Battlefield, + ); + let ts = self.state.next_timestamp(); + let obj = self.state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Planeswalker); + obj.card_types.subtypes.push(subtype.to_string()); + obj.base_card_types = obj.card_types.clone(); + obj.timestamp = ts; + // A pre-existing permanent (entered on a prior turn), matching the + // enchantment/land builders. CR 302.6 gates only creatures, but a + // planeswalker animated by its own ability would otherwise inherit the + // flag. + obj.summoning_sick = false; + obj.counters + .insert(crate::types::counter::CounterType::Loyalty, loyalty); + + let mut builder = CardBuilder { + state: &mut self.state, + id, + }; + builder.from_oracle_text(oracle_text); + builder + } + /// Add a creature to hand with abilities parsed from Oracle text. pub fn add_creature_to_hand_from_oracle( &mut self, diff --git a/crates/engine/src/game/static_abilities.rs b/crates/engine/src/game/static_abilities.rs index 8e9ca7d58c..5f42705ac6 100644 --- a/crates/engine/src/game/static_abilities.rs +++ b/crates/engine/src/game/static_abilities.rs @@ -1983,6 +1983,11 @@ pub(crate) fn static_filter_matches( crate::types::ability::ControllerRef::ActivePlayer => { state.active_player == player_id } + // CR 109.4 + CR 611.2: a snapshotted id, resolvable + // directly (mirrors the ActivePlayer arm above). + crate::types::ability::ControllerRef::SpecificPlayer { id } => { + *id == player_id + } }; } return true; diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index 235ffe83ad..0683b80aa4 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -260,6 +260,8 @@ fn find_legal_targets_with_context( // player and is a valid candidate for an active-player-scoped // target filter (read live). Some(ControllerRef::ActivePlayer) => player.id == state.active_player, + // CR 109.4 + CR 611.2: a snapshotted id, compared directly. + Some(ControllerRef::SpecificPlayer { id }) => player.id == *id, None => true, }; if include { diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 7278021a5d..8a3f733382 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -2697,6 +2697,13 @@ fn parse_zone_word(input: &str) -> nom::IResult<&str, Zone, OracleError<'_>> { /// `clippy::type_complexity`. type MultiZonePlayerExileParse<'a> = (&'a str, (Vec, ControllerRef, Vec)); +/// CR 508.1d + CR 506.3: result of the defender-bound "attack(s) `` +/// [window] if able" parse — the required defender plus the window it states for +/// itself. `None` is the WINDOWLESS form ("attack ~ if able" — Gideon Jura), +/// whose span comes from an enclosing clause instead. +type DefenderBoundAttackParse<'a> = + Result<(&'a str, (TargetFilter, Option)), nom::Err>>; + /// CR 400.3 + CR 404.1 + CR 406.2 + CR 108.2 + CR 205.2a: "exile all `[]` /// cards from `` `` and ``" — mass exile of the cards a /// player owns across a *union* of zones. Two forms: @@ -5999,12 +6006,12 @@ fn attach_neuter_recipient_resolves_via_subject(ctx: &ParseContext) -> bool { } } +/// CR 608.2c + CR 109.5: the attach path's whole-phrase form of the shared +/// source-anaphoric gendered pronoun recognizer. Delegates to the single +/// authority (`oracle_target::parse_source_anaphoric_pronoun`) under +/// `all_consuming` — an attach recipient is the pronoun and nothing else. fn parse_gendered_attach_self_recipient(input: &str) -> OracleResult<'_, ()> { - all_consuming(alt(( - |i| parse_word_bounded(i, "her"), - |i| parse_word_bounded(i, "him"), - ))) - .parse(input) + all_consuming(crate::parser::oracle_target::parse_source_anaphoric_pronoun).parse(input) } fn parse_neuter_attach_self_recipient(input: &str) -> OracleResult<'_, ()> { @@ -6202,6 +6209,16 @@ pub(super) fn parse_prevention_amount(rest: &str) -> PreventionAmount { /// dealt by those creatures") so both prevent-recipient parsers agree. /// /// Tries, in priority order: +/// 0. A source-anaphoric gendered pronoun ("him"/"her"/"himself"/"herself") → +/// `SelfRef`, UNGATED (CR 608.2c + CR 109.5). Magic templating uses a +/// gendered pronoun only as the printed-name self-reference, so unlike the +/// neuter "it" it needs no `parent_target_available` gate. This tier must +/// run FIRST: with the gate closed, tier 1 declines and tier 2's +/// `is_broadcast_population_filter` check rejects `SelfRef`, so without it +/// "prevent all damage that would be dealt to him this turn" (Gideon Jura, +/// Gideon of the Trials) fell through to the `Any` default — a shield with +/// NO recipient constraint, i.e. a turn-long Fog over every damage event in +/// the game rather than a shield on the Gideon. /// 1. A singular chosen-target anaphor ("that creature"/"it"/"the creature"), /// gated on `parent_target_available` (CR 608.2c, issue #1094). /// 2. Any other recipient phrase `parse_target` recognizes as a real filter — @@ -6225,6 +6242,12 @@ pub(super) fn resolve_prevent_recipient( recipient: TextPair<'_>, parent_target_available: bool, ) -> Option { + // CR 608.2c + CR 109.5: tier 0 — the ungated printed-name self-reference. + if let Some((filter, _)) = + crate::parser::oracle_target::parse_source_anaphoric_pronoun_ref(recipient.original) + { + return Some(filter); + } if let Some((filter, _)) = parse_anaphoric_target_ref(recipient.original, parent_target_available) { @@ -12894,11 +12917,20 @@ fn lower_imperative_family_effect(ast: ImperativeFamilyAst) -> Effect { }, ImperativeFamilyAst::ForceAttack { duration, - required_player, + required_defender, } => Effect::ForceAttack { target: TargetFilter::Any, - required_player, - duration, + required_defender, + // CR 115.1: the imperative path is the TARGETED form ("Target + // creature attacks you this combat if able"); subject injection + // fills `target` from the declared target phrase. + scope: EffectScope::Single, + // CR 611.2a: a windowless predicate ("attack ~ if able") states no + // span of its own — its window comes from the enclosing clause, which + // the clause machinery re-stamps onto this effect. The `UntilEndOfTurn` + // fallback is the pre-existing default for a stated-window-free grant + // and is never the value a windowless Gideon-Jura-class clause keeps. + duration: duration.unwrap_or(Duration::UntilEndOfTurn), }, // CR 701.15a: Goad target creature. Subject injection fills target from parsed text. ImperativeFamilyAst::Goad => Effect::Goad { @@ -13729,11 +13761,16 @@ fn try_parse_adapt(lower: &str) -> Option { Some(Effect::Adapt { count }) } -/// CR 508.1d: Parse "attacks/attack [player] this turn/combat if able" requirements. +/// CR 508.1d: Parse "attacks/attack [defender] [window] if able" requirements. /// /// Bare forms ("attacks this turn if able") emit a temporary `MustAttack`. -/// Player-bound "attacks you ..." forms emit `ForceAttack`, whose resolver binds -/// "you" to the resolving ability controller and grants `MustAttackPlayer`. +/// Defender-bound forms ("attacks you …", "attack ~ if able") emit +/// `ForceAttack`, whose resolver binds the referent and grants +/// `MustAttackDefender`. +/// +/// CR 506.3 makes the defender axis one category — "a player, a planeswalker, or +/// a battle" — so the player arms and the source-permanent arm are alternatives +/// on a single `alt()`, not separate parsers. pub(super) fn try_parse_attack_if_able(lower: &str) -> Option { let trimmed = lower.trim_end_matches('.'); @@ -13778,7 +13815,7 @@ pub(super) fn try_parse_attack_if_able(lower: &str) -> Option>> = ( + let targeted: DefenderBoundAttackParse<'_> = ( alt((tag("attacks"), tag("attack"))), preceded( tag(" "), @@ -13791,6 +13828,12 @@ pub(super) fn try_parse_attack_if_able(lower: &str) -> Option Option { - assert_eq!(duration, Duration::UntilEndOfCombat); - assert_eq!(required_player, TargetFilter::Controller); + assert_eq!(duration, Some(Duration::UntilEndOfCombat)); + assert_eq!(required_defender, TargetFilter::Controller); } other => panic!("Expected ForceAttack, got {other:?}"), } @@ -19435,13 +19485,13 @@ mod tests { match result { ImperativeFamilyAst::ForceAttack { duration, - required_player, + required_defender, } => { - assert_eq!(duration, Duration::UntilEndOfCombat); + assert_eq!(duration, Some(Duration::UntilEndOfCombat)); assert_eq!( - required_player.chosen_player_index(), + required_defender.chosen_player_index(), Some(0), - "that player must reference the chosen player at index 0, got {required_player:?}" + "that player must reference the chosen player at index 0, got {required_defender:?}" ); } other => panic!("Expected ForceAttack, got {other:?}"), @@ -19489,18 +19539,23 @@ mod tests { // resolution-scoped chosen opponent (chosen_players[0]). let Effect::ForceAttack { target, - required_player, + required_defender, duration, + scope, } = &*sub.effect else { panic!("sub-ability must be a ForceAttack, got {:?}", sub.effect); }; assert_eq!(*target, TargetFilter::SelfRef); assert_eq!(*duration, Duration::UntilEndOfCombat); + // CR 115.1: `~` names ONE object, so this subject is not a broadcast + // population — the scope must stay `Single` (only Gideon-Jura-class + // "creatures that player controls" subjects reach `All`). + assert_eq!(*scope, EffectScope::Single); assert_eq!( - required_player.chosen_player_index(), + required_defender.chosen_player_index(), Some(0), - "must force attacking the chosen player, got {required_player:?}" + "must force attacking the chosen player, got {required_defender:?}" ); } diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index b56868e58d..30be5d9eaf 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -26557,7 +26557,7 @@ fn is_mass_coerce_static(effect: &Effect) -> bool { static_abilities.iter().any(|static_def| { matches!( static_def.mode, - StaticMode::MustAttack | StaticMode::MustAttackPlayer { .. } + StaticMode::MustAttack | StaticMode::MustAttackDefender { .. } ) && target .as_ref() .or(static_def.affected.as_ref()) @@ -29164,6 +29164,37 @@ pub(crate) fn parse_ability_ir( mode: ChainLoweringMode, ctx: &mut ParseContext, ) -> AbilityIr { + // CR 608.2c + CR 109.4: a leading "During target opponent's next turn, …" + // window DECLARES a player target, and that player is in scope for the whole + // ability body — so a "that player" anaphor anywhere in it refers to the + // window's target (Gideon Jura's "+2": "creatures that player controls"). + // + // Published HERE, at the body entry point, rather than at any one + // leading-duration strip site: the body is re-entered by several recognizers + // (the subject path strips the same leading duration itself), so a per-strip + // hook would bind the anaphor only on whichever path happened to run first. + // Without it, `parse_controller_suffix`'s documented fallback binds "that + // player controls" to `ControllerRef::You` and the requirement lands on the + // ACTIVATING player's creatures instead of the targeted opponent's. + // + // `None` (no such window) leaves whatever scope the caller already supplied. + // + // The `starts_with` guard keeps this off the hot path: `parse_ability_ir` + // runs for every ability of every card, and only a line that literally opens + // with "during " can match, so the lowercase allocation is paid by that + // handful of lines instead of all ~30k cards' worth. + if text + .get(.."during ".len()) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("during ")) + { + if let Some(possessor) = + crate::parser::oracle_nom::duration::leading_next_turn_window_possessor( + text.to_lowercase().as_str(), + ) + { + ctx.relative_player_scope = Some(possessor.controller_ref()); + } + } // The conditional protection recognizer may mutate `ParseContext` before // declining. Preserve the two legacy entry-point semantics exactly: the // standalone bypass context was fresh and discarded on decline, while the diff --git a/crates/engine/src/parser/oracle_effect/subject.rs b/crates/engine/src/parser/oracle_effect/subject.rs index eedbe53139..91ea92dcbb 100644 --- a/crates/engine/src/parser/oracle_effect/subject.rs +++ b/crates/engine/src/parser/oracle_effect/subject.rs @@ -17,7 +17,7 @@ use super::{resolve_it_pronoun, ParseContext}; use crate::parser::oracle_ir::ast::*; use crate::types::ability::{ AbilityDefinition, AbilityKind, ChosenSubtypeKind, ColorChangeMode, ContinuousModification, - ControllerRef, Duration, EachDamageRecipient, Effect, FilterProp, MultiTargetSpec, + ControllerRef, Duration, EachDamageRecipient, Effect, EffectScope, FilterProp, MultiTargetSpec, PlayerFilter, PlayerScope, PtValue, QuantityExpr, QuantityRef, StaticCondition, StaticDefinition, TargetFilter, TypedFilter, }; @@ -1819,10 +1819,70 @@ fn try_parse_subject_restriction_clause( unless_pay: None, }); } + // CR 508.1d + CR 506.3 + CR 611.2c: the defender-bound `ForceAttack` form + // with a BROADCAST subject — "creatures that player controls attack ~ if + // able" (Gideon Jura). + // + // ONLY the broadcast form is captured here. A chosen-target subject + // ("Target creature attacks you this combat if able") keeps its existing + // route through the imperative path's own target injection, which binds + // the declared target rather than the subject filter; capturing it here + // would rewrite that target to `ParentTarget` and change what the + // pre-existing lure cards resolve against. The `else` below re-runs the + // recognizer for the bare `MustAttack` form, exactly as before. + // + // Binding the subject as the effect's `target` filter — rather than + // freezing it to the objects matching it right now — is what keeps the + // affected set dynamic per CR 611.2c; `force_attack::resolve` installs + // the filter intact. Gideon Jura's ruling requires exactly that: the + // "+2" "doesn't lock in what it applies to." + if let Some(ImperativeFamilyAst::ForceAttack { + duration, + required_defender, + }) = imperative::try_parse_attack_if_able(&predicate) + { + // CR 115.1: a genuine broadcast POPULATION is enumerated at + // resolution and never targeted, so `EffectScope::All` keeps + // `collect_target_slots` from building a spurious creature slot — + // which would both over-target the ability and make it fizzle when + // that creature became an illegal target. + // + // A subject that names ONE specific object is NOT this form, whether + // it was declared as a target or is a self/inherited reference + // (`~ attacks that player this combat if able` — Knight Rampager). + // `is_broadcast_population_filter` is the single authority for that + // distinction; re-deriving it as "did a target get declared" would + // misclassify every `SelfRef` subject. + let broadcast = parse_subject_application(subject, ctx).filter(|application| { + application.target.is_none() + && !application.inherits_parent + && super::is_broadcast_population_filter(&static_affected_for_application( + application, + )) + }); + if let Some(application) = broadcast { + return Some(ParsedEffectClause { + effect: Effect::ForceAttack { + target: static_affected_for_application(&application), + required_defender, + scope: EffectScope::All, + // CR 611.2a: a windowless predicate states no span of its + // own; the enclosing clause's duration is applied by + // `with_clause_duration` and arrives on `duration` below. + duration: duration.clone().unwrap_or(Duration::UntilEndOfTurn), + }, + distribute: None, + multi_target: application.multi_target, + duration, + sub_ability: None, + condition: None, + optional: application.is_optional, + unless_pay: None, + }); + } + } // Classify via the existing recognizer. Only the bare GenericEffect form - // (MustAttack) is re-bound here; the player-bound `ForceAttack` form - // ("attacks you/that player …") has its own targeted handling and must - // NOT be captured. + // (MustAttack) is re-bound here. if let Some(ImperativeFamilyAst::GainKeyword(Effect::GenericEffect { duration, .. })) = imperative::try_parse_attack_if_able(&predicate) { @@ -2591,7 +2651,15 @@ pub(super) fn parse_subject_application( .is_err() { let normalized = format!("all {noun_subject}"); - let (filter, rest) = parse_target(&normalized); + // CR 109.4 + CR 608.2c: thread the parse context for the same reason the + // "target " arm above does — controller-suffix resolution inside + // `parse_target` needs the enclosing relative-player scope to bind a + // "that player controls" anaphor. A bare-plural subject takes that + // anaphor just as readily as a targeted one ("creatures that player + // controls attack ~ if able" — Gideon Jura); without `ctx` it silently + // fell back to `ControllerRef::You`, scoping the clause to the WRONG + // player's creatures. + let (filter, rest) = parse_target_with_ctx(&normalized, ctx); if rest.trim().is_empty() { let filter = if had_other { add_another_property(filter) diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 6487731100..25d3b3e095 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -21854,8 +21854,11 @@ fn force_attack_you_this_combat_targets_creature() { e, Effect::ForceAttack { target: TargetFilter::Typed(_), - required_player: TargetFilter::Controller, + required_defender: TargetFilter::Controller, duration: Duration::UntilEndOfCombat, + // CR 115.1: "TARGET creature" declares a target slot, so the + // subject is a single chosen object, not a broadcast population. + scope: EffectScope::Single, } ), "Expected ForceAttack with typed target and controller requirement, got {:?}", diff --git a/crates/engine/src/parser/oracle_ir/ast.rs b/crates/engine/src/parser/oracle_ir/ast.rs index d1b571e0dc..cffea3f7a1 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -661,15 +661,21 @@ pub(crate) enum ImperativeFamilyAst { attacker: Option, duration: Duration, }, - /// CR 508.1d: Attack a required player this turn/combat if able. The - /// `required_player` filter selects whom the forced attacker must attack — - /// `TargetFilter::Controller` for "attacks you", or + /// CR 508.1d + CR 506.3: Attack a required DEFENDER this turn/combat if + /// able. The `required_defender` filter selects whom the forced attacker + /// must attack — `TargetFilter::Controller` for "attacks you", /// `ControllerRef::ChosenPlayer { index }` for "attacks that player" (the /// opponent chosen by a preceding "choose an opponent" instruction in the - /// same resolution, e.g. Ruhan of the Fomori). + /// same resolution, e.g. Ruhan of the Fomori), or `TargetFilter::SelfRef` + /// for a permanent defender ("attack ~ if able" — Gideon Jura, whose + /// required defender is the planeswalker itself). ForceAttack { - duration: Duration, - required_player: TargetFilter, + /// `None` is the WINDOWLESS form ("attack ~ if able" — Gideon Jura), + /// whose span is stated by an enclosing clause ("During target + /// opponent's next turn, …") and applied by the clause machinery. + /// `Some` carries the window the predicate states for itself. + duration: Option, + required_defender: TargetFilter, }, /// CR 701.15a: Goad target creature. Goad, diff --git a/crates/engine/src/parser/oracle_nom/duration.rs b/crates/engine/src/parser/oracle_nom/duration.rs index 7de3eed617..8c55930816 100644 --- a/crates/engine/src/parser/oracle_nom/duration.rs +++ b/crates/engine/src/parser/oracle_nom/duration.rs @@ -8,7 +8,13 @@ //! upkeep", "until its controller's next untap step"), "until ~/this creature //! leaves the battlefield", "until you exile another card with ~/this //! ability", "for the rest of the game", "for as long as [condition]", "this -//! turn", "this/that combat". +//! turn", "this/that combat", and the "during target opponent's/player's next +//! turn" WINDOW (Gideon Jura). +//! +//! A phrase added here is taken away from every clause-level grammar that owned +//! it, because the positional wrappers below run first — see +//! `parse_next_turn_window_possessor` for why the "during …" arm accepts only +//! the targeted possessives. //! //! Positional wrappers (`strip_trailing_duration` / `strip_leading_duration` //! in `oracle_effect/lower.rs`, the clause shell, and the combat-grant @@ -26,7 +32,9 @@ use nom::Parser; use super::condition::{parse_inner_condition, parse_recipient_has_counters}; use super::error::{oracle_err, OracleError, OracleResult}; use super::primitives::scan_contains; -use crate::types::ability::{Duration, ObjectScope, PlayerScope, StaticCondition, TargetFilter}; +use crate::types::ability::{ + ControllerRef, Duration, ObjectScope, PlayerScope, StaticCondition, TargetFilter, +}; use crate::types::phase::Phase; /// Parse a duration phrase from Oracle text. @@ -40,11 +48,133 @@ pub fn parse_duration(input: &str) -> OracleResult<'_, Duration> { alt(( preceded(tag("until "), parse_until_body), preceded(tag("for "), parse_for_body), + preceded(tag("during "), parse_during_body), parse_current_phase_duration, )) .parse(input) } +/// Alternatives after the shared "during " prefix. +/// +/// CR 514.2 + CR 508.1d: "during next turn" names a WINDOW — the +/// whole of that player's next turn — which is exactly the span +/// [`Duration::UntilEndOfNextTurnOf`] already models (armed at that player's +/// untap step, pruned at that turn's cleanup). CR 508.1d's closing sentence +/// makes the whole-turn reading load-bearing rather than incidental: "If a +/// requirement that says a creature attacks if able during a certain turn refers +/// to a turn with multiple combat phases, the creature attacks if able during +/// each declare attackers step in that turn." Gideon Jura's official ruling says +/// the same in card terms — the "+2" "applies during each combat phase of the +/// affected player's next turn (as opposed to applying during the affected +/// player's next combat phase)". +/// +/// The possessor is its own axis (`parse_next_turn_window_possessor`), so +/// "during your next turn" and "during target opponent's next turn" are one +/// production rather than enumerated full-string arms. +fn parse_during_body(input: &str) -> OracleResult<'_, Duration> { + let (rest, possessor) = parse_next_turn_window_possessor(input)?; + let (rest, _) = tag(" next turn").parse(rest)?; + Ok(( + rest, + Duration::UntilEndOfNextTurnOf { + player: possessor.scope(), + }, + )) +} + +/// The possessor of a "during next turn" window, **as written**. +/// +/// Deliberately distinct from the emitted [`PlayerScope`], for the same reason +/// [`StepDeadlinePossessor`] is: two spellings that produce the SAME runtime +/// `PlayerScope` can still differ in what the rest of the parser must do about +/// them. Here, "target player's" and "target opponent's" both emit +/// `PlayerScope::Target` (CR 109.4 — the duration reads the first player target +/// either way), but they declare different companion target SLOTS, and the +/// clause body's "that player" anaphor must inherit the matching +/// [`ControllerRef`] so the slot's legal-target set is right. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum NextTurnWindowPossessor { + /// CR 109.4: "target player's". + TargetPlayer, + /// CR 109.4 + CR 102.2: "target opponent's" (Gideon Jura). Runtime-read + /// identical to `TargetPlayer`; the slot excludes the controller. + TargetOpponent, +} + +impl NextTurnWindowPossessor { + /// The duration's runtime scope. Both spellings collapse here — the legality + /// difference lives in [`Self::controller_ref`], not in the duration. + fn scope(self) -> PlayerScope { + match self { + Self::TargetPlayer | Self::TargetOpponent => PlayerScope::Target, + } + } + + /// CR 608.2c: the `ControllerRef` a "that player" anaphor in the clause body + /// must bind to. + pub(crate) fn controller_ref(self) -> ControllerRef { + match self { + Self::TargetPlayer => ControllerRef::TargetPlayer, + Self::TargetOpponent => ControllerRef::TargetOpponent, + } + } +} + +/// CR 109.4: the possessor axis of a "during next turn" window. +/// +/// **Deliberately TARGETED possessives only** — not +/// [`parse_controller_possessive_pronoun`]'s "your"/"their". This grammar is +/// reached through the POSITIONAL wrappers (`strip_leading_duration` / +/// `strip_trailing_duration`), which peel a duration phrase off ANY clause, so a +/// phrase added here is taken away from every other clause-level grammar that +/// owns it. "during their next turn" is owned by the CR 723.1 control-next-turn +/// grammar (`try_parse_control_next_turn_suffix` — Mindslaver, Construct a +/// Cosmic Cube's "you control target opponent during their next turn"), where +/// the window is part of the effect rather than a separable duration. Accepting +/// the pronoun forms here silently stripped that window and left the +/// control-opponent rider unparsed. +/// +/// "during target opponent's next turn" (Gideon Jura) is owned by no other +/// grammar, so it is safe — and necessary — here. +/// +/// The possessive marker is a shared trailing `alt()` over both apostrophe +/// glyphs and the apostrophe-less spelling, matching the factoring in +/// [`parse_object_controller_possessive`] — the noun and the marker are separate +/// axes, not enumerated pairs. +fn parse_next_turn_window_possessor(input: &str) -> OracleResult<'_, NextTurnWindowPossessor> { + terminated( + preceded( + tag("target "), + alt(( + value(NextTurnWindowPossessor::TargetOpponent, tag("opponent")), + value(NextTurnWindowPossessor::TargetPlayer, tag("player")), + )), + ), + alt((tag("\u{2019}s"), tag("'s"), tag("s"))), + ) + .parse(input) +} + +/// CR 608.2c + CR 109.4: The possessor of a LEADING "during next +/// turn, …" window, for callers that must publish the window's targeted player +/// as the clause body's relative-player scope. +/// +/// Shares the single possessor combinator with [`parse_during_body`], so the +/// duration value and the anaphor scope can never disagree about which spelling +/// was written. Returns `None` when `input` does not open with such a window. +pub(crate) fn leading_next_turn_window_possessor(input: &str) -> Option { + let (rest, possessor) = preceded( + tag::<_, _, OracleError<'_>>("during "), + parse_next_turn_window_possessor, + ) + .parse(input) + .ok()?; + let (_, _) = (tag::<_, _, OracleError<'_>>(" next turn"), tag(", ")) + .parse(rest) + .ok()?; + Some(possessor) +} + /// Alternatives after the shared "until " prefix. fn parse_until_body(input: &str) -> OracleResult<'_, Duration> { alt(( diff --git a/crates/engine/src/parser/oracle_nom/enters_under.rs b/crates/engine/src/parser/oracle_nom/enters_under.rs index 6736029953..b92cd696b9 100644 --- a/crates/engine/src/parser/oracle_nom/enters_under.rs +++ b/crates/engine/src/parser/oracle_nom/enters_under.rs @@ -252,7 +252,10 @@ fn map_relative_player_scope(scope: &ControllerRef) -> Option { | ControllerRef::DefendingPlayer | ControllerRef::SourceChosenPlayer | ControllerRef::EnchantedPlayer - | ControllerRef::ActivePlayer => None, + | ControllerRef::ActivePlayer + // CR 109.4 + CR 611.2: never produced by the parser — this lowering is + // installed by resolvers only, so no enters-under scope maps to it. + | ControllerRef::SpecificPlayer { .. } => None, } } diff --git a/crates/engine/src/parser/oracle_static/evasion.rs b/crates/engine/src/parser/oracle_static/evasion.rs index a6af94b70c..98cafd7ef6 100644 --- a/crates/engine/src/parser/oracle_static/evasion.rs +++ b/crates/engine/src/parser/oracle_static/evasion.rs @@ -2529,8 +2529,8 @@ fn parse_forced_attack_defender_static_body(text: &str) -> Option TargetFilter::Typed(TypedFilter::card()), + Some(ControllerRef::SpecificPlayer { .. }) => TargetFilter::Typed(TypedFilter::card()), None => TargetFilter::Typed(TypedFilter::card()), } }; @@ -884,6 +885,7 @@ pub(crate) fn try_parse_cost_modification( // CR 102.1: active-player scope is not emitted for cost statics; // fall back to an untyped card filter (same as TriggeringPlayer). Some(ControllerRef::ActivePlayer) => TargetFilter::Typed(TypedFilter::card()), + Some(ControllerRef::SpecificPlayer { .. }) => TargetFilter::Typed(TypedFilter::card()), None => TargetFilter::Typed(TypedFilter::card()), } }; diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 4e7da4ccfc..553f5eaa73 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -9024,8 +9024,8 @@ fn galactus_forced_attack_static_parses_with_flavor_label() { .expect("Galactus forced-attack static must parse"); assert_eq!( def.mode, - StaticMode::MustAttackPlayer { - player: RequiredDefender::Matching { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { filter: expected_most_life_defender(), }, }, @@ -9053,8 +9053,8 @@ fn forced_attack_defender_static_flavor_label_is_optional() { .expect("unlabeled forced-attack static must parse"); assert_eq!( def.mode, - StaticMode::MustAttackPlayer { - player: RequiredDefender::Matching { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { filter: expected_most_life_defender(), }, }, @@ -9071,8 +9071,8 @@ fn forced_attack_defender_static_bare_opponent_selector() { .expect("bare-opponent forced-attack static must parse"); assert_eq!( def.mode, - StaticMode::MustAttackPlayer { - player: RequiredDefender::Matching { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { filter: PlayerFilter::Opponent, }, }, diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index d052b48d4c..8ca29a9b91 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -148,6 +148,55 @@ pub(crate) fn parse_anaphoric_target_ref( matches!(filter, TargetFilter::ParentTarget).then_some((filter, rest)) } +/// CR 608.2c + CR 109.5: Recognize a leading **source-anaphoric gendered +/// pronoun** ("him" / "himself" / "her" / "herself") and bind it to +/// [`TargetFilter::SelfRef`] — the ability's own source object. +/// +/// Unlike the neuter "it" (which may anaphor an earlier clause's chosen target +/// and therefore needs the `parent_target_available` gate in +/// [`parse_anaphoric_target_ref`]), a gendered pronoun on a Magic card is +/// UNAMBIGUOUSLY the printed-name self-reference: the templating uses it only +/// where the card's own name would otherwise repeat (Gideon Jura, "Prevent all +/// damage that would be dealt to **him** this turn"; Gideon of the Trials; +/// Winter Soldier, "Equipment attached to **him**"). No printed card uses a +/// gendered pronoun for a chosen target, so the binding needs no gate. +/// +/// The singular-they "them" is DELIBERATELY excluded: it is recipient-anaphoric +/// for player-enchanting Auras (Curse of Thirst's "Curses attached to them" = +/// the enchanted player, not the Aura source), so accepting it here would bind +/// the wrong object. This mirrors the identical carve-out documented on +/// `oracle_nom::quantity`'s `AttachedToSource` arm. +/// +/// Returns the bound filter and the remainder of the ORIGINAL-case `text` +/// following the matched pronoun, so callers can keep parsing trailing +/// duration/qualifier phrases. +pub(crate) fn parse_source_anaphoric_pronoun_ref(text: &str) -> Option<(TargetFilter, &str)> { + let trimmed = text.trim_start(); + let lower = trimmed.to_ascii_lowercase(); + let (rest, ()) = parse_source_anaphoric_pronoun(lower.as_str()).ok()?; + // `parse_word_bounded` never splits a char boundary (ASCII pronouns only), + // so the consumed byte count maps 1:1 onto the original-case slice. + Some(( + TargetFilter::SelfRef, + &trimmed[trimmed.len() - rest.len()..], + )) +} + +/// The raw combinator behind [`parse_source_anaphoric_pronoun_ref`], for callers +/// that need to compose it (e.g. under `all_consuming` when the pronoun must be +/// the WHOLE phrase). Input must already be lowercase. Reflexive forms are tried +/// before their bare stems so "himself" is never truncated to "him" plus a +/// dangling "self". +pub(crate) fn parse_source_anaphoric_pronoun(input: &str) -> OracleResult<'_, ()> { + alt(( + |i| parse_word_bounded(i, "himself"), + |i| parse_word_bounded(i, "herself"), + |i| parse_word_bounded(i, "him"), + |i| parse_word_bounded(i, "her"), + )) + .parse(input) +} + /// Parse a word with a word boundary check: the next char after the word must be /// non-alphanumeric (whitespace, comma, period, etc.) or end-of-input. /// Prevents "it" from matching "item", "you" from matching "your", etc. diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 7c6c1a3154..9487636ff9 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -1661,6 +1661,214 @@ fn parse( parse_oracle_text(text, name, &keyword_names, &types, &subtypes) } +/// CR 506.3 + CR 508.1d + CR 611.2c + CR 615: Gideon Jura (verbatim MTGJSON +/// Oracle text) parses all three loyalty abilities with zero residual +/// `Unimplemented`, and each lands on the exact shape its rules text requires. +/// +/// The three seams this pins, each of which was independently wrong before: +/// +/// 1. **+2 — the defender is the PLANESWALKER, not a player.** CR 506.3 makes +/// "a player, a planeswalker, or a battle" one defender category, so the +/// requirement rides `Effect::ForceAttack` with `required_defender: SelfRef`. +/// Before, the whole line was `Effect::Unimplemented { name: "during" }`. +/// 2. **+2 — the affected creatures belong to the TARGETED OPPONENT.** "that +/// player controls" is an anaphor to the player the leading "During target +/// opponent's next turn," window declared. Without the scope publication, the +/// documented `parse_controller_suffix` fallback bound it to +/// `ControllerRef::You` — pointing the requirement at the ACTIVATING player's +/// own creatures, i.e. the exact opposite of the card. +/// 3. **0 — the damage shield is scoped to Gideon.** "dealt to him" is the +/// printed-name self-reference; it must reach `TargetFilter::SelfRef`. As +/// `TargetFilter::Any` (the old fallback) the shield carried NO recipient +/// constraint, making a turn-long Fog over every damage event in the game. +/// +/// Reverting any of the three fails this test. +#[test] +fn gideon_jura_full_parse() { + let r = parse( + "+2: During target opponent's next turn, creatures that player controls attack Gideon Jura if able.\n\u{2212}2: Destroy target tapped creature.\n0: Until end of turn, Gideon Jura becomes a 6/6 Human Soldier creature that's still a planeswalker. Prevent all damage that would be dealt to him this turn.", + "Gideon Jura", + &[], + &["Planeswalker"], + &["Gideon"], + ); + assert_eq!( + r.abilities.len(), + 3, + "three loyalty abilities, got {:#?}", + r.abilities + ); + // Positive reach guard for every assertion below: nothing fell back to a + // residual, so each shape asserted here was genuinely produced. + for def in &r.abilities { + assert!( + !has_unimplemented(def), + "no residual Unimplemented node, got {def:#?}" + ); + } + + // --- +2 (CR 508.1d + CR 506.3 + CR 611.2c) --------------------------- + let Effect::ForceAttack { + target, + required_defender, + .. + } = &*r.abilities[0].effect + else { + panic!( + "the +2 is a forced-attack requirement, got {:?}", + r.abilities[0].effect + ); + }; + assert_eq!( + required_defender, + &TargetFilter::SelfRef, + "CR 506.3: the required defender is Gideon Jura itself, not a player" + ); + let TargetFilter::Typed(typed) = target else { + panic!("the affected subject is a typed creature population, got {target:?}"); + }; + assert!( + typed.type_filters.contains(&TypeFilter::Creature), + "the requirement affects creatures: {typed:?}" + ); + assert_eq!( + typed.controller, + Some(ControllerRef::TargetOpponent), + "CR 608.2c: \"that player\" is the targeted opponent, NOT the activator" + ); + // CR 508.1d (final sentence) + the card's own ruling: the window is the + // whole of that player's next turn, so it must survive to every declare- + // attackers step in it — `UntilNextTurnOf` (which expires at the BEGINNING + // of that turn) would make the requirement inert. + assert_eq!( + r.abilities[0].duration, + Some(Duration::UntilEndOfNextTurnOf { + player: PlayerScope::Target + }), + "the window spans the targeted opponent's entire next turn" + ); + + // --- −2 (CR 701.7) --------------------------------------------------- + let Effect::Destroy { target, .. } = &*r.abilities[1].effect else { + panic!("the −2 destroys, got {:?}", r.abilities[1].effect); + }; + let TargetFilter::Typed(typed) = target else { + panic!("expected a typed destroy target, got {target:?}"); + }; + assert!( + typed.type_filters.contains(&TypeFilter::Creature) + && typed.properties.contains(&FilterProp::Tapped), + "the −2 targets a TAPPED creature: {typed:?}" + ); + + // --- 0 (CR 306.1 + CR 615) ------------------------------------------- + let Effect::GenericEffect { + static_abilities, .. + } = &*r.abilities[2].effect + else { + panic!("the 0 animates, got {:?}", r.abilities[2].effect); + }; + let mods = &static_abilities[0].modifications; + assert!( + mods.contains(&ContinuousModification::SetPower { value: 6 }) + && mods.contains(&ContinuousModification::SetToughness { value: 6 }) + && mods.contains(&ContinuousModification::AddType { + core_type: crate::types::card_type::CoreType::Creature + }), + "the 0 makes Gideon a 6/6 creature: {mods:?}" + ); + // CR 306.1: "that's still a planeswalker" — the animation ADDS the creature + // type and never removes Planeswalker, so no RemoveType is emitted. + assert!( + !mods + .iter() + .any(|m| matches!(m, ContinuousModification::RemoveType { .. })), + "Gideon stays a planeswalker — nothing removes a card type: {mods:?}" + ); + let sub = r.abilities[2] + .sub_ability + .as_ref() + .expect("the 0 carries the damage-prevention rider"); + let Effect::PreventDamage { target, amount, .. } = &*sub.effect else { + panic!("the rider prevents damage, got {:?}", sub.effect); + }; + assert_eq!( + target, + &TargetFilter::SelfRef, + "CR 615: \"dealt to him\" scopes the shield to Gideon — an `Any` \ + recipient would Fog the whole turn" + ); + assert_eq!(amount, &PreventionAmount::All, "\"prevent ALL damage\""); +} + +/// CR 608.2c + CR 109.5: the building block behind Gideon Jura's shield fix — +/// a source-anaphoric GENDERED pronoun recipient binds to the source with NO +/// `parent_target_available` gate, because Magic templating uses "him"/"her" +/// only as the printed-name self-reference. Tested at the building-block level +/// (both genders, reflexive and bare, gate open and closed) rather than through +/// one card, so any prevent-damage clause in the class is covered. +/// +/// The singular-they "them" is asserted to be EXCLUDED: it is +/// recipient-anaphoric for player-enchanting Auras, so binding it to the source +/// would name the wrong object. +#[test] +fn prevent_damage_gendered_self_anaphor_recipient_binds_to_source() { + for pronoun in ["him", "her", "himself", "herself"] { + let effect = parse_effect_chain( + &format!("Prevent all damage that would be dealt to {pronoun} this turn."), + AbilityKind::Spell, + ); + let Effect::PreventDamage { target, .. } = &*effect.effect else { + panic!( + "expected PreventDamage for {pronoun:?}, got {:?}", + effect.effect + ); + }; + assert_eq!( + target, + &TargetFilter::SelfRef, + "{pronoun:?} must bind to the ability's source" + ); + } + + // The `parent_target_available` gate is genuinely OPEN here: the leading + // clause declares a chosen target, so the neuter-anaphor tier would bind + // `ParentTarget`. A gendered pronoun must still reach `SelfRef` — being + // ungated is the whole point of tier 0, and it is what Gideon Jura's "0" + // depends on, since its animate clause precedes the shield. + let chained = parse_effect_chain( + "Destroy target creature. Prevent all damage that would be dealt to him this turn.", + AbilityKind::Spell, + ); + let shield = chained + .sub_ability + .as_ref() + .expect("the prevent clause chains as a sub-ability"); + let Effect::PreventDamage { target, .. } = &*shield.effect else { + panic!("expected a PreventDamage rider, got {:?}", shield.effect); + }; + assert_eq!( + target, + &TargetFilter::SelfRef, + "a gendered pronoun outranks the chosen-target anaphor even with the gate open" + ); + + // Reach guard: the recognizer is not simply returning `SelfRef` for every + // recipient. "them" is deliberately outside the gendered family. + let effect = parse_effect_chain( + "Prevent all damage that would be dealt to them this turn.", + AbilityKind::Spell, + ); + let Effect::PreventDamage { target, .. } = &*effect.effect else { + panic!("expected PreventDamage, got {:?}", effect.effect); + }; + assert_ne!( + target, + &TargetFilter::SelfRef, + "the singular-they \"them\" is recipient-anaphoric, not source-anaphoric" + ); +} + /// Cluster 97 (CR 603.7a + CR 311.2 + CR 701.31): The Doctor's Childhood Barn — /// a Planechase plane — parses its "Whenever chaos ensues …" trigger chain with /// ZERO `Effect::Unimplemented` and ZERO `StaticCondition::Unrecognized`, and diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index db49f3f1ff..35472684dd 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -4052,6 +4052,28 @@ pub enum ControllerRef { /// player (Siren's Call, Maddening Imp), cast/activated only during an /// opponent's turn. ActivePlayer, + /// CR 109.4 + CR 611.2c: a player id SNAPSHOTTED at resolution — the lowered + /// form the dynamic siblings above collapse to once the resolving ability is + /// gone. Never produced by the parser; produced only by resolvers that + /// install a durable continuous effect whose *object set* must stay dynamic + /// while its *player reference* must not. + /// + /// Gideon Jura's "+2: During target opponent's next turn, creatures that + /// player controls attack Gideon Jura if able" is the canonical member. Per + /// CR 611.2c the requirement modifies no characteristics and changes no + /// controller, so the affected creature set is re-derived every + /// declare-attackers step (official ruling: the ability "doesn't lock in what + /// it applies to … includes creatures that come under that player's control + /// after the ability has resolved"). The *player*, by contrast, is fixed when + /// the ability resolves — and `ControllerRef::TargetPlayer` resolves by + /// reading `ability.targets`, which no longer exists at layer-evaluation + /// time, so `force_attack::resolve` lowers it to this arm on install. + /// + /// Mirrors the identical lower-at-resolution contract already documented on + /// [`RestrictionPlayerScope::SpecificPlayer`] and [`TargetFilter::SpecificPlayer`]. + SpecificPlayer { + id: PlayerId, + }, } /// CR 301 / CR 303: Kinds of attachments to permanents. @@ -5667,6 +5689,28 @@ pub enum PlayerScope { /// `Duration::UntilNextStepOf` — never from a value/quantity/player-selection /// position. AnyTurn, + /// CR 109.4 + CR 611.2 + CR 514.2: a player id SNAPSHOTTED at resolution — + /// the player-scalar-axis analogue of [`ControllerRef::SpecificPlayer`] and + /// [`RestrictionPlayerScope::SpecificPlayer`], and the lowered form the + /// dynamic siblings above collapse to once the resolving ability is gone. + /// + /// DURATION-TIMING-ONLY, like [`AnyTurn`](Self::AnyTurn): never produced by + /// the parser and never read from a value/quantity/player-selection + /// position. It is constructed solely by resolvers that install a durational + /// continuous effect whose expiry keys on a player OTHER than the effect's + /// controller. + /// + /// Gideon Jura's "+2: During target opponent's next turn, …" is the + /// canonical member: the parser emits + /// `UntilEndOfNextTurnOf { player: PlayerScope::Target }`, and + /// `force_attack::resolve` lowers `Target` to this arm. Without the + /// lowering, the prune in `layers.rs::prune_until_next_turn_effects` — which + /// arms `UntilEndOfNextTurnOf` by comparing the ACTIVE player against the + /// effect's own `controller` — could never see the targeted opponent, and + /// the requirement would never arm nor expire. Overloading the effect's + /// `controller` field with the target instead would break CR 109.5's meaning + /// of "you" for every other consumer of that field. + SpecificPlayer { id: PlayerId }, } /// Scope selector for object-axis quantities (Round Π-5). Picks WHICH object @@ -12565,14 +12609,44 @@ pub enum Effect { #[serde(default = "default_duration_until_end_of_turn")] duration: Duration, }, - /// CR 508.1d: Target creature must attack the required player this turn/combat if able. + /// CR 508.1d + CR 506.3: The creatures matching `target` must attack the + /// required defender this turn/combat if able. + /// + /// `required_defender` is a `TargetFilter` because CR 506.3's defender + /// category ("a player, a planeswalker, or a battle") is already spanned by + /// that type — no second reference vocabulary is introduced. A filter that + /// denotes a PLAYER (`Controller`, a `ChosenPlayer` ref) grafts + /// `RequiredDefender::Fixed`; one that denotes an OBJECT (`SelfRef` — Gideon + /// Jura's "attack Gideon Jura if able") grafts `RequiredDefender::Permanent`. + /// `force_attack::resolve` is the single place that classifies it. + /// + /// `serde(alias)`: pre-widening payloads named this field `required_defender`. + /// `scope` is the single-vs-mass axis, exactly as on [`Effect::Transform`] + /// and [`Effect::SetTapState`] — parameterized rather than split into a + /// sibling `ForceAttackAll`. `Single` (the default, and every pre-Gideon-Jura + /// card) makes `target` a SELECTABLE target filter that surfaces a slot + /// ("Target creature attacks you this combat if able"). `All` makes it a + /// non-targeting POPULATION filter enumerated at resolution — Gideon Jura's + /// "creatures that player controls", which per CR 115.1 targets only the + /// opponent and never the creatures. `target_filter()` is `None` under `All`, + /// so no creature slot is built; the companion PLAYER slot still surfaces via + /// `mass_all_target_filter`. + /// + /// `serde(alias)`: pre-widening payloads named `required_defender` + /// `required_player`; `scope` defaults to `Single`, which is what every such + /// payload meant. ForceAttack { #[serde(default = "default_target_filter_any")] target: TargetFilter, - #[serde(default = "default_target_filter_controller")] - required_player: TargetFilter, + #[serde( + default = "default_target_filter_controller", + alias = "required_player" + )] + required_defender: TargetFilter, #[serde(default = "default_duration_until_end_of_turn")] duration: Duration, + #[serde(default = "default_effect_scope_single")] + scope: EffectScope, }, /// CR 719.2: Solve the source Case — it becomes solved. SolveCase, @@ -15340,7 +15414,6 @@ impl Effect { | Effect::PhaseOut { target, .. } | Effect::PhaseIn { target, .. } | Effect::ForceBlock { target, .. } - | Effect::ForceAttack { target, .. } | Effect::BecomePrepared { target, .. } | Effect::BecomeUnprepared { target, .. } | Effect::BecomeSaddled { target, .. } @@ -15564,6 +15637,23 @@ impl Effect { .. } => None, + // CR 508.1d + CR 115.1: `ForceAttack` exposes its target only for the + // single-creature scope ("Target creature attacks you this combat if + // able"). The `All` scope is a non-targeting population enumerated at + // resolution — Gideon Jura's "creatures that player controls", whose + // only target is the opponent — so, like `Transform`/`SetTapState` + // above, its `target_filter()` is `None` and no creature slot or + // prompt is built. + Effect::ForceAttack { + scope: EffectScope::Single, + target, + .. + } => Some(target), + Effect::ForceAttack { + scope: EffectScope::All, + .. + } => None, + // CR 701.60a: `Suspect`/`Unsuspect` expose a target slot only for the // single-permanent scope (targeted/anaphoric "suspect target // creature" / "it's no longer suspected"). The `All` scope ("all diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index 5db8d5e62a..007ae98634 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -26419,8 +26419,8 @@ mod tests { Zone::Battlefield, ); object.static_definitions.push( - StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(800)), @@ -26434,8 +26434,8 @@ mod tests { b.objects .get_mut(&ObjectId(500)) .unwrap() - .static_definitions = vec![StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + .static_definitions = vec![StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(801))] diff --git a/crates/engine/src/types/statics.rs b/crates/engine/src/types/statics.rs index baacecf9d3..13ecfbb058 100644 --- a/crates/engine/src/types/statics.rs +++ b/crates/engine/src/types/statics.rs @@ -807,7 +807,7 @@ pub enum AttackDefenderScope { } /// CR 508.1d + CR 611.2 / CR 604.2: how the required defending player of a -/// [`StaticMode::MustAttackPlayer`] requirement is determined. Struct variants +/// [`StaticMode::MustAttackDefender`] requirement is determined. Struct variants /// (NOT tuple/newtype) so internal `#[serde(tag = "type")]` tagging stays valid /// — this mirrors [`super::ability::QuantityExpr`] (`Fixed { value }` | /// `Ref { qty }`), which uses struct variants for the same serde reason: a @@ -829,6 +829,31 @@ pub enum RequiredDefender { /// effect is continuously applied; the requirement is re-checked each /// declare-attackers step. Resolved via `game::effects::matches_player_scope`. Matching { filter: PlayerFilter }, + /// CR 506.3 + CR 508.1 + CR 611.2: a specific PERMANENT defender. CR 506.3 + /// makes "a player, a planeswalker, or a battle" one defender category, and + /// the engine already models that category as one type + /// (`combat::AttackTarget`), so the required-defender axis spans it rather + /// than forking a parallel player-only/permanent-only static pair. Gideon + /// Jura: "+2: During target opponent's next turn, creatures that player + /// controls attack Gideon Jura if able." + /// + /// The permanent is snapshotted at resolution (CR 611.2) as an + /// [`ObjectIncarnationRef`], NOT a bare `ObjectId` — CR 400.7: a permanent + /// that leaves and re-enters the battlefield is a new object, and the engine + /// reuses `ObjectId` as storage identity, so a bare id would let a + /// re-entered Gideon inherit a requirement aimed at the old one. Mirrors + /// [`StaticMode::MustBlockAttacker`]'s identical pin. + /// + /// Which `AttackTarget` kind the permanent presents is derived LIVE at each + /// declare-attackers step from its current card types, never pre-committed + /// here: a Gideon that animated itself is still a planeswalker (CR 306.1) + /// and still attackable, while one that has left the battlefield is not + /// attackable at all. CR 508.1d then simply drops the unobeyable + /// requirement — matching the official ruling: "If a creature controlled by + /// the affected player can't attack Gideon Jura (because he's no longer on + /// the battlefield, for example), that player may have it attack you, + /// another one of your planeswalkers, or nothing at all." + Permanent { permanent: ObjectIncarnationRef }, } impl From for RequiredDefender { @@ -873,12 +898,14 @@ impl<'de> Deserialize<'de> for RequiredDefender { enum Tagged { Fixed { player: PlayerId }, Matching { filter: PlayerFilter }, + Permanent { permanent: ObjectIncarnationRef }, } let tagged: Tagged = serde_json::from_value(value).map_err(serde::de::Error::custom)?; Ok(match tagged { Tagged::Fixed { player } => RequiredDefender::Fixed { player }, Tagged::Matching { filter } => RequiredDefender::Matching { filter }, + Tagged::Permanent { permanent } => RequiredDefender::Permanent { permanent }, }) } _ => Err(serde::de::Error::custom( @@ -1227,22 +1254,34 @@ pub enum StaticMode { /// runtime-implemented; other arms are inert. PlayerProtection(super::keywords::ProtectionTarget), MustAttack, - /// CR 508.1d: This creature must attack a *specific* player (or a live player - /// CLASS) if able. The required defender is a [`RequiredDefender`]: + /// CR 508.1d: This creature must attack a *specific* defender if able. Per + /// CR 506.3 a defender is "a player, a planeswalker, or a battle"; the + /// required one is a [`RequiredDefender`]: /// - `Fixed { player }` — a resolution-time snapshot id (Alluring Siren, /// Dulcet Sirens, Encore; grafted via `Effect::ForceAttack`), CR 611.2. /// - `Matching { filter }` — a printed static's live player class re-evaluated /// each declare-attackers step (Galactus, "an opponent with the most life /// among your opponents"), CR 604.1 / CR 604.2. + /// - `Permanent { permanent }` — a snapshotted planeswalker/battle (Gideon + /// Jura's "+2: … creatures that player controls attack Gideon Jura if + /// able"), CR 611.2 + CR 400.7. /// /// Unlike the generic [`MustAttack`] (attack any defender), this narrows the - /// requirement to specific players. Data-carrying variant — not + /// requirement to specific defenders. Data-carrying variant — not /// registry-registered (see `coverage::is_data_carrying_static`); enforced by /// direct pattern-match in `combat.rs` declare-attackers validation (the - /// resolver at `must_attack_player_directives_for_creature` resolves the - /// `RequiredDefender` to concrete `PlayerId`s). Mirrors [`MustBlockAttacker`]. - MustAttackPlayer { - player: RequiredDefender, + /// resolver at `must_attack_defender_directives_for_creature` resolves the + /// `RequiredDefender` to concrete `AttackTarget`s). + /// Mirrors [`MustBlockAttacker`]. + /// + /// `serde(alias)`: pre-widening snapshots (game-state saves, P2P resume, + /// undo journals) wrote this variant as `MustAttackPlayer` with a `player` + /// field, so both the variant name and the field keep a read alias. New + /// writes emit the canonical names. + #[serde(alias = "MustAttackPlayer")] + MustAttackDefender { + #[serde(alias = "player")] + defender: RequiredDefender, }, MustBlock, /// CR 702.39a / CR 509.1c: This creature must block a *specific* attacker if @@ -2164,7 +2203,7 @@ pub enum StaticModeKind { CantLoseLife, PlayerProtection, MustAttack, - MustAttackPlayer, + MustAttackDefender, MustBlock, MustBlockAttacker, CantDraw, @@ -2302,7 +2341,7 @@ impl StaticMode { StaticMode::CantLoseLife => StaticModeKind::CantLoseLife, StaticMode::PlayerProtection(..) => StaticModeKind::PlayerProtection, StaticMode::MustAttack => StaticModeKind::MustAttack, - StaticMode::MustAttackPlayer { .. } => StaticModeKind::MustAttackPlayer, + StaticMode::MustAttackDefender { .. } => StaticModeKind::MustAttackDefender, StaticMode::MustBlock => StaticModeKind::MustBlock, StaticMode::MustBlockAttacker { .. } => StaticModeKind::MustBlockAttacker, StaticMode::CantDraw { .. } => StaticModeKind::CantDraw, @@ -2479,13 +2518,17 @@ impl Hash for StaticMode { StaticMode::ExtraBlockers { count } => count.hash(state), StaticMode::MustBlockAttacker { attacker } => attacker.hash(state), // CR 508.1d: `RequiredDefender::Matching` wraps a non-Hash - // `PlayerFilter`; hash the discriminant for both arms and the - // concrete id only for `Fixed` (precedent: the non-Hash TargetFilter - // arms below). Equal values still hash equal. - StaticMode::MustAttackPlayer { player } => { - std::mem::discriminant(player).hash(state); - if let RequiredDefender::Fixed { player: p } = player { - p.hash(state); + // `PlayerFilter`; hash the discriminant for every arm and the + // concrete id only for the hashable ones (precedent: the non-Hash + // TargetFilter arms below). Equal values still hash equal. + StaticMode::MustAttackDefender { defender } => { + std::mem::discriminant(defender).hash(state); + match defender { + RequiredDefender::Fixed { player } => player.hash(state), + // CR 400.7: the incarnation pin is fully hashable, so a + // permanent defender contributes its exact identity. + RequiredDefender::Permanent { permanent } => permanent.hash(state), + RequiredDefender::Matching { .. } => {} } } StaticMode::MaxAttackersEachCombat { max, defender } => { @@ -2686,7 +2729,7 @@ impl StaticMode { | StaticMode::CantLoseLife | StaticMode::PlayerProtection(_) | StaticMode::MustAttack - | StaticMode::MustAttackPlayer { .. } + | StaticMode::MustAttackDefender { .. } | StaticMode::MustBlock | StaticMode::MustBlockAttacker { .. } | StaticMode::CantDraw { .. } @@ -2883,8 +2926,8 @@ impl fmt::Display for StaticMode { write!(f, "PlayerProtection({target:?})") } StaticMode::MustAttack => write!(f, "MustAttack"), - StaticMode::MustAttackPlayer { player } => { - write!(f, "MustAttackPlayer({player:?})") + StaticMode::MustAttackDefender { defender } => { + write!(f, "MustAttackDefender({defender:?})") } StaticMode::MustBlock => write!(f, "MustBlock"), StaticMode::MustBlockAttacker { attacker } => { @@ -4601,13 +4644,13 @@ mod tests { #[test] fn must_attack_player_round_trips_through_static_mode() { for mode in [ - StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player: PlayerId(1), }, }, - StaticMode::MustAttackPlayer { - player: RequiredDefender::Matching { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { filter: PlayerFilter::Opponent, }, }, @@ -4627,8 +4670,8 @@ mod tests { let mode: StaticMode = serde_json::from_str(legacy).unwrap(); assert_eq!( mode, - StaticMode::MustAttackPlayer { - player: RequiredDefender::Fixed { + StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { player: PlayerId(1) }, } diff --git a/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs b/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs index c76b1e337b..82a0a83954 100644 --- a/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs +++ b/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs @@ -845,7 +845,7 @@ fn grafted_must_attack_half_reaches_the_combat_authority() { ); let constraints = attacker_constraints_for_active_player(runner.state(), &valid); - let Some(CombatRequirement::MustAttack { players, sources }) = constraints.get(&ox) else { + let Some(CombatRequirement::MustAttack { defenders, sources }) = constraints.get(&ox) else { panic!( "expected a MustAttack requirement for the bound creature, got {:?}", constraints.get(&ox) @@ -854,11 +854,11 @@ fn grafted_must_attack_half_reaches_the_combat_authority() { // Exclude the other two `sources` contributors, each by the thing that // actually feeds it. // - // (a) `MustAttackPlayer`: `players` and the carrier list are built from the - // same filtered directive iterator, so empty players ⟺ no carriers. + // (a) `MustAttackDefender`: `defenders` and the carrier list are built from + // the same filtered directive iterator, so empty defenders ⟺ no carriers. assert!( - players.is_empty(), - "no MustAttackPlayer directive exists in this fixture, got {players:?}" + defenders.is_empty(), + "no MustAttackDefender directive exists in this fixture, got {defenders:?}" ); // (b) `StaticMode::Goaded`: fed by `goad_static_hits_for_creature`, which // supplies the CARRIER's id — so it could only yield `ox` if `ox` itself diff --git a/crates/engine/tests/integration/deterministic_game_state_serde.rs b/crates/engine/tests/integration/deterministic_game_state_serde.rs index 9cd729f0a5..d9a97962eb 100644 --- a/crates/engine/tests/integration/deterministic_game_state_serde.rs +++ b/crates/engine/tests/integration/deterministic_game_state_serde.rs @@ -1912,7 +1912,7 @@ fn declare_attackers_numeric_maps_round_trip_through_value_bare_raw_and_trusted( ( ObjectId(2), CombatRequirement::MustAttack { - players: vec![PlayerId(1)], + defenders: vec![AttackTarget::Player(PlayerId(1))], sources: vec![ObjectId(22)], }, ), diff --git a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs index b7b2f78661..3fdb17b5d2 100644 --- a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs +++ b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs @@ -95,15 +95,15 @@ fn galactus_requirement_surfaces_most_life_opponent() { "reach-guard: Galactus is a valid attacker (the requirement is non-vacuous)" ); let constraints = attacker_constraints_for_active_player(runner.state(), &valid); - let Some(CombatRequirement::MustAttack { players, .. }) = constraints.get(&galactus) else { + let Some(CombatRequirement::MustAttack { defenders, .. }) = constraints.get(&galactus) else { panic!( "expected a MustAttack requirement for Galactus, got {:?}", constraints.get(&galactus) ); }; assert_eq!( - players, - &vec![P1], + defenders, + &vec![AttackTarget::Player(P1)], "the live-evaluated required defender is the single most-life opponent" ); } diff --git a/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs b/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs new file mode 100644 index 0000000000..27c2d06aa0 --- /dev/null +++ b/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs @@ -0,0 +1,361 @@ +//! CR 506.3 + CR 508.1d + CR 611.2c: Gideon Jura's "+2: During target +//! opponent's next turn, creatures that player controls attack Gideon Jura if +//! able." — a forced-attack requirement whose required defender is a +//! PLANESWALKER rather than a player. +//! +//! These drive the real pipeline end-to-end: verbatim Oracle text → parser +//! (`Effect::ForceAttack { required_defender: SelfRef }`) → +//! `force_attack::resolve` (which snapshots the defender as +//! `RequiredDefender::Permanent` and installs ONE continuous effect carrying the +//! live affected filter) → `must_attack_defender_directives_for_creature` → +//! `attacker_constraints_for_active_player` (the DeclareAttackers payload +//! authority) AND the `declare_attackers` legality validator via the production +//! `GameAction::DeclareAttackers` route. +//! +//! The rulings these pin, verbatim from Gatherer: +//! * "Gideon Jura's first ability doesn't lock in what it applies to. … This +//! includes creatures that come under that player's control after the +//! ability has resolved." +//! * "If a creature controlled by the affected player can't attack Gideon Jura +//! (because he's no longer on the battlefield, for example), that player may +//! have it attack you, another one of your planeswalkers, or nothing at all." + +use engine::game::combat::{ + attacker_constraints_for_active_player, get_valid_attacker_ids, AttackTarget, CombatRequirement, +}; +use engine::game::layers::evaluate_layers; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::ContinuousModification; +use engine::types::ability::{Duration, PlayerScope}; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::phase::Phase; +use engine::types::statics::{RequiredDefender, StaticMode}; +use engine::types::zones::Zone; + +/// Gideon Jura — verbatim Oracle text (Scryfall). Only the "+2" matters here; +/// the other two abilities ride along so the fixture parses the real card rather +/// than an excerpt. +const GIDEON_JURA_ORACLE: &str = concat!( + "+2: During target opponent's next turn, creatures that player controls ", + "attack Gideon Jura if able.\n", + "\u{2212}2: Destroy target tapped creature.\n", + "0: Until end of turn, Gideon Jura becomes a 6/6 Human Soldier creature ", + "that's still a planeswalker. Prevent all damage that would be dealt to him ", + "this turn.", +); + +/// P0 controls Gideon Jura; P1 controls `p1_creatures` vanilla bears. Returns +/// the runner (parked in P0's main phase with the statics live), Gideon's id, +/// and P1's creature ids. +fn setup(p1_creatures: usize) -> (GameRunner, ObjectId, Vec) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let gideon = scenario + .add_planeswalker_from_oracle(P0, "Gideon Jura", "Gideon", 6, GIDEON_JURA_ORACLE) + .id(); + let bears: Vec = (0..p1_creatures) + .map(|i| scenario.add_creature(P1, &format!("Bear {i}"), 2, 2).id()) + .collect(); + let mut runner = scenario.build(); + { + let state = runner.state_mut(); + state.active_player = P0; + state.priority_player = P0; + state.phase = Phase::PreCombatMain; + state.turn_number = 2; + state.layers_dirty.mark_full(); + } + evaluate_layers(runner.state_mut()); + (runner, gideon, bears) +} + +/// Activate the "+2" (loyalty ability index 0) targeting P1, then resolve it. +/// +/// CR 601.2c: the companion player slot is the one `ControllerRef::TargetOpponent` +/// surfaces for "creatures that player controls" — if the parser bound that +/// anaphor to `You` instead, there would be no slot to fill and this panics. +fn activate_plus_two(runner: &mut GameRunner, gideon: ObjectId) { + runner.activate(gideon, 0).target_player(P1).resolve(); +} + +/// Hand the turn to P1 and park at their declare-attackers step with the layer +/// pass fresh, so the requirement is evaluated exactly as it is in a real turn. +fn hand_turn_to_p1(runner: &mut GameRunner) { + { + let state = runner.state_mut(); + state.active_player = P1; + state.priority_player = P1; + state.phase = Phase::DeclareAttackers; + state.turn_number = 3; + // CR 302.6: the bears have been under P1's control since before this + // turn began, so they are able to attack. + for id in state.battlefield.clone() { + if let Some(obj) = state.objects.get_mut(&id) { + obj.summoning_sick = false; + } + } + state.layers_dirty.mark_full(); + } + evaluate_layers(runner.state_mut()); + let valid = get_valid_attacker_ids(runner.state()); + runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { + player: P1, + valid_attacker_ids: valid, + valid_attack_targets: vec![], + valid_attack_targets_by_attacker: None, + attacker_constraints: Default::default(), + }; +} + +fn declare(runner: &mut GameRunner, attacks: Vec<(ObjectId, AttackTarget)>) -> Result<(), String> { + runner + .act(GameAction::DeclareAttackers { + attacks, + bands: vec![], + }) + .map(|_| ()) + .map_err(|e| format!("{e:?}")) +} + +/// CR 611.2: the resolved "+2" installs ONE continuous effect whose modification +/// is a `MustAttackDefender` bound to a `RequiredDefender::Permanent` — the +/// planeswalker itself — and whose expiry is the TARGETED player's next turn, +/// lowered to a concrete snapshot. +/// +/// REVERT-FAIL on three separate seams: a player-only `RequiredDefender` cannot +/// express the defender; an un-lowered `PlayerScope::Target` expiry would never +/// arm (`prune_until_next_turn_effects` compares against a concrete player); and +/// a frozen `SpecificObject` affected set would replace the live filter. +#[test] +fn plus_two_installs_permanent_defender_requirement_scoped_to_target() { + let (mut runner, gideon, _bears) = setup(1); + activate_plus_two(&mut runner, gideon); + + let effect = runner + .state() + .transient_continuous_effects + .iter() + .find(|ce| { + ce.modifications.iter().any(|m| { + matches!( + m, + ContinuousModification::AddStaticMode { + mode: StaticMode::MustAttackDefender { .. } + } + ) + }) + }) + .expect("the +2 installs a must-attack requirement"); + + let ContinuousModification::AddStaticMode { + mode: StaticMode::MustAttackDefender { defender }, + } = &effect.modifications[0] + else { + panic!( + "expected a MustAttackDefender grant, got {:?}", + effect.modifications + ); + }; + let RequiredDefender::Permanent { permanent } = defender else { + panic!("CR 506.3: the required defender is the PLANESWALKER, got {defender:?}"); + }; + assert_eq!( + permanent.object_id, gideon, + "the snapshotted defender is Gideon Jura itself" + ); + + // CR 508.1d (final sentence): the window is that player's whole next turn. + assert_eq!( + effect.duration, + Duration::UntilEndOfNextTurnOf { + player: PlayerScope::SpecificPlayer { id: P1 } + }, + "the expiry is lowered to the TARGETED player, not the controller" + ); + + // CR 611.2c: the affected set stays a live FILTER, never a frozen id list — + // the ruling requires creatures that arrive later to be caught too. + assert!( + !matches!( + effect.affected, + engine::types::ability::TargetFilter::SpecificObject { .. } + ), + "the affected population must stay dynamic, got {:?}", + effect.affected + ); +} + +/// CR 115.1: the "+2" targets exactly ONE thing — the opponent. "creatures that +/// player controls" is a population, not a target, so no creature slot may be +/// declared. +/// +/// This is what `EffectScope::All` buys: before it, the broadcast subject filter +/// was read as a selectable target and the ability went on the stack with TWO +/// targets (the player AND an arbitrary creature). That over-targets the ability +/// — it would wrongly fizzle when that creature became an illegal target, and it +/// would illegally "target" a creature with hexproof. +#[test] +fn plus_two_targets_only_the_opponent() { + let (mut runner, gideon, bears) = setup(2); + runner + .act(GameAction::ActivateAbility { + source_id: gideon, + ability_index: 0, + }) + .expect("activation is legal"); + let stacked = runner + .state() + .stack + .last() + .and_then(|entry| entry.ability()) + .expect("the ability is on the stack awaiting resolution"); + assert_eq!( + stacked.targets.len(), + 1, + "CR 115.1: exactly one target — the opponent, got {:?}", + stacked.targets + ); + assert!( + matches!( + stacked.targets[0], + engine::types::ability::TargetRef::Player(P1) + ), + "the sole target is the opponent, got {:?}", + stacked.targets + ); + for bear in &bears { + assert!( + !stacked + .targets + .iter() + .any(|t| matches!(t, engine::types::ability::TargetRef::Object(id) if id == bear)), + "no creature is targeted: {:?}", + stacked.targets + ); + } +} + +/// CR 508.1d enforcement, through the production `GameAction::DeclareAttackers` +/// route: P1's creature must attack Gideon Jura. Attacking P0 instead is +/// rejected, attacking Gideon commits, and declining entirely is rejected. +#[test] +fn plus_two_forces_the_targeted_opponents_creature_onto_gideon() { + // Attacking the PLAYER leaves the requirement unmet. + let (mut wrong, gideon, bears) = setup(1); + activate_plus_two(&mut wrong, gideon); + hand_turn_to_p1(&mut wrong); + assert!( + declare(&mut wrong, vec![(bears[0], AttackTarget::Player(P0))]).is_err(), + "attacking Gideon's controller does not obey a requirement naming the planeswalker" + ); + + // Attacking the PLANESWALKER satisfies it. + let (mut right, gideon, bears) = setup(1); + activate_plus_two(&mut right, gideon); + hand_turn_to_p1(&mut right); + declare( + &mut right, + vec![(bears[0], AttackTarget::Planeswalker(gideon))], + ) + .expect("attacking Gideon Jura satisfies CR 508.1d"); + assert!( + right.state().combat.is_some(), + "the satisfying declaration commits" + ); + + // Declining entirely leaves it unmet — the requirement genuinely binds. + let (mut none, gideon, _bears) = setup(1); + activate_plus_two(&mut none, gideon); + hand_turn_to_p1(&mut none); + assert!( + declare(&mut none, vec![]).is_err(), + "declaring no attacker leaves the requirement unmet" + ); +} + +/// CR 611.2c + the card's own ruling — "doesn't lock in what it applies to … +/// includes creatures that come under that player's control after the ability +/// has resolved." +/// +/// Resolve the "+2" while P1 controls NOTHING, then give them a creature, then +/// declare. The late arrival is still forced onto Gideon. A resolution-time +/// snapshot of the affected set would leave it free, so this fails on any +/// freeze-the-population regression. +#[test] +fn plus_two_affected_set_is_not_locked_in_at_resolution() { + let (mut runner, gideon, _bears) = setup(0); + activate_plus_two(&mut runner, gideon); + + // The creature arrives AFTER the ability resolved. + let latecomer = { + let state = runner.state_mut(); + let id = engine::game::zones::create_object( + state, + engine::types::identifiers::CardId(9001), + P1, + "Latecomer Bear".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types + .core_types + .push(engine::types::card_type::CoreType::Creature); + obj.base_card_types = obj.card_types.clone(); + obj.power = Some(2); + obj.toughness = Some(2); + id + }; + + hand_turn_to_p1(&mut runner); + let valid = get_valid_attacker_ids(runner.state()); + assert!( + valid.contains(&latecomer), + "reach-guard: the late arrival is an eligible attacker" + ); + let constraints = attacker_constraints_for_active_player(runner.state(), &valid); + let Some(CombatRequirement::MustAttack { defenders, .. }) = constraints.get(&latecomer) else { + panic!( + "the late arrival must carry the requirement, got {:?}", + constraints.get(&latecomer) + ); + }; + assert_eq!( + defenders, + &vec![AttackTarget::Planeswalker(gideon)], + "CR 611.2c: the population is re-derived at declare-attackers" + ); +} + +/// The card's ruling: "If a creature controlled by the affected player can't +/// attack Gideon Jura (because he's no longer on the battlefield, for example), +/// that player may have it attack you … or nothing at all." +/// +/// CR 508.1d drops a requirement that cannot be obeyed, so with Gideon gone the +/// creature is free — both to attack the player and to decline. This is the +/// vacuity guard for the enforcement test above: without it, an implementation +/// that simply never enforced the requirement would also pass "declining works". +#[test] +fn requirement_lapses_when_gideon_leaves_the_battlefield() { + let (mut runner, gideon, bears) = setup(1); + activate_plus_two(&mut runner, gideon); + + // CR 400.7: Gideon leaves; the snapshotted incarnation pin no longer names a + // live defender. + let mut events = Vec::new(); + engine::game::zones::move_to_zone(runner.state_mut(), gideon, Zone::Graveyard, &mut events); + hand_turn_to_p1(&mut runner); + + let valid = get_valid_attacker_ids(runner.state()); + let constraints = attacker_constraints_for_active_player(runner.state(), &valid); + assert!( + !matches!( + constraints.get(&bears[0]), + Some(CombatRequirement::MustAttack { .. }) + ), + "with Gideon gone the requirement is unobeyable and must not surface: {:?}", + constraints.get(&bears[0]) + ); + declare(&mut runner, vec![]).expect("the creature may attack nothing at all"); +} diff --git a/crates/engine/tests/integration/goaded_creature_under_pacifism_visible.rs b/crates/engine/tests/integration/goaded_creature_under_pacifism_visible.rs index bb891b73af..4aa3381ae8 100644 --- a/crates/engine/tests/integration/goaded_creature_under_pacifism_visible.rs +++ b/crates/engine/tests/integration/goaded_creature_under_pacifism_visible.rs @@ -141,7 +141,7 @@ fn goaded_creature_under_pacifism_is_visible_as_cant_attack_not_must_attack() { // CR 701.15b: direct player-goad (`goaded_by`) carries no object source → // EMPTY sources. This is the documented player-level goad row. Some(&CombatRequirement::MustAttack { - players: vec![], + defenders: vec![], sources: vec![] }), "an unencumbered goaded creature must surface as MustAttack with no specific-player constraint" diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 98b53a2153..708d23b261 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -248,6 +248,7 @@ mod gemstone_mine_depletion_sacrifice_6507; mod gev_scaled_scorch_enter_counters; mod giada_angel_counters; mod giant_ox_crew_toughness; +mod gideon_jura_forced_attack_planeswalker; mod gideon_trials_emblem; mod gift_delivery_draw_sequence_migration; mod gift_recipient_phased_out_opponent; diff --git a/crates/engine/tests/integration/must_attack_player_attribution.rs b/crates/engine/tests/integration/must_attack_player_attribution.rs index 2273c7648f..484299d2d1 100644 --- a/crates/engine/tests/integration/must_attack_player_attribution.rs +++ b/crates/engine/tests/integration/must_attack_player_attribution.rs @@ -20,7 +20,7 @@ use std::sync::Arc; use engine::game::combat::{ - attacker_constraints_for_active_player, get_valid_attacker_ids, CombatRequirement, + attacker_constraints_for_active_player, get_valid_attacker_ids, AttackTarget, CombatRequirement, }; use engine::game::layers::evaluate_layers; use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; @@ -47,8 +47,8 @@ fn graft_must_attack_player( Duration::UntilEndOfCombat, TargetFilter::SpecificObject { id: creature }, vec![ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { - player: player.into(), + mode: StaticMode::MustAttackDefender { + defender: player.into(), }, }], None, @@ -92,14 +92,18 @@ fn grafted_must_attack_player_attributes_directing_source() { ); let constraints = attacker_constraints_for_active_player(runner.state(), &valid); - let Some(CombatRequirement::MustAttack { players, sources }) = constraints.get(&creature) + let Some(CombatRequirement::MustAttack { defenders, sources }) = constraints.get(&creature) else { panic!( "expected a MustAttack requirement for the forced creature, got {:?}", constraints.get(&creature) ); }; - assert_eq!(players, &vec![P1], "the required defending player surfaces"); + assert_eq!( + defenders, + &vec![AttackTarget::Player(P1)], + "the required defending player surfaces" + ); assert!( sources.contains(&source), "the directing object is attributed as the requirement source" @@ -108,11 +112,11 @@ fn grafted_must_attack_player_attributes_directing_source() { !sources.contains(&creature), "the creature itself is NOT the source — the stamp fired (this is the whole change)" ); - // 7.4 drift pin: for the single attackable directive, its player is in - // `players` iff its carrier is in `sources` (one scan feeds both). + // 7.4 drift pin: for the single attackable directive, its defender is in + // `defenders` iff its carrier is in `sources` (one scan feeds both). assert!( - players.contains(&P1) == sources.contains(&source), - "players and sources derive from one scan — no drift" + defenders.contains(&AttackTarget::Player(P1)) == sources.contains(&source), + "defenders and sources derive from one scan — no drift" ); } @@ -140,7 +144,7 @@ fn generic_must_attack_attributes_the_creature_itself() { assert_eq!( constraints.get(&creature), Some(&CombatRequirement::MustAttack { - players: vec![], + defenders: vec![], sources: vec![creature], }), "a generic must-attack creature is its own source (carrier fallback)" @@ -149,7 +153,7 @@ fn generic_must_attack_attributes_the_creature_itself() { /// 7.1c — two distinct directing sources forcing the SAME creature to attack the /// SAME player retain BOTH ids in `sources` (full-def dedup keeps both because -/// their `source_object` differs), while `players` deduplicates to one entry. +/// their `source_object` differs), while `defenders` deduplicates to one entry. /// REVERT-FAIL: without per-source `source_object`, the two grafts collapse to /// one def and `sources` carries a single (creature-fallback) id. #[test] @@ -168,7 +172,7 @@ fn two_sources_forcing_same_player_surface_both_and_dedup_players() { let valid = get_valid_attacker_ids(runner.state()); let constraints = attacker_constraints_for_active_player(runner.state(), &valid); - let Some(CombatRequirement::MustAttack { players, sources }) = constraints.get(&creature) + let Some(CombatRequirement::MustAttack { defenders, sources }) = constraints.get(&creature) else { panic!("expected MustAttack, got {:?}", constraints.get(&creature)); }; @@ -177,15 +181,15 @@ fn two_sources_forcing_same_player_surface_both_and_dedup_players() { "both directing sources are attributed ({sources:?})" ); assert_eq!( - players, - &vec![P1], - "the multi-source multiplicity lives in `sources`, not `players` (deduped set)" + defenders, + &vec![AttackTarget::Player(P1)], + "the multi-source multiplicity lives in `sources`, not `defenders` (deduped set)" ); // 7.4 drift pin for the multi-source case. for src in [s1, s2] { assert!( - players.contains(&P1) == sources.contains(&src), - "each attackable directive's player∈players iff its carrier∈sources" + defenders.contains(&AttackTarget::Player(P1)) == sources.contains(&src), + "each attackable directive's defender∈defenders iff its carrier∈sources" ); } } @@ -213,14 +217,18 @@ fn departed_directing_source_id_is_surfaced_without_panic() { let valid = get_valid_attacker_ids(runner.state()); let constraints = attacker_constraints_for_active_player(runner.state(), &valid); - let Some(CombatRequirement::MustAttack { players, sources }) = constraints.get(&creature) + let Some(CombatRequirement::MustAttack { defenders, sources }) = constraints.get(&creature) else { panic!( "the requirement must persist after the source departs (CR 611.2c), got {:?}", constraints.get(&creature) ); }; - assert_eq!(players, &vec![P1], "the requirement persists"); + assert_eq!( + defenders, + &vec![AttackTarget::Player(P1)], + "the requirement persists" + ); assert!( sources.contains(&source), "the departed directing id is still surfaced (no panic, no silent drop)" @@ -285,8 +293,8 @@ fn static_definition_source_object_serde_default() { assert_eq!(decoded.source_object, None); // A stamped value round-trips. - let stamped = StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1).into(), + let stamped = StaticDefinition::new(StaticMode::MustAttackDefender { + defender: PlayerId(1).into(), }) .source_object(ObjectId(7)); let round: StaticDefinition = diff --git a/crates/engine/tests/integration/rules/combat.rs b/crates/engine/tests/integration/rules/combat.rs index d349e74cb2..b5d548b07e 100644 --- a/crates/engine/tests/integration/rules/combat.rs +++ b/crates/engine/tests/integration/rules/combat.rs @@ -1788,11 +1788,11 @@ fn incompatible_must_attack_player_accepts_max_score_declaration() { let mut scenario = GameScenario::new_n_player(3, 42); let attacker = { let mut b = scenario.add_creature(P0, "Doubly Lured Bear", 2, 2); - b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1.into(), + b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: P1.into(), })); - b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P2.into(), + b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: P2.into(), })); b.id() }; @@ -2281,8 +2281,8 @@ fn must_attack_whose_only_target_is_taxed_does_not_force_payment() { // and it is taxed, so no FREE declaration satisfies the lure. let attacker = { let mut b = scenario.add_creature(P0, "Lured Bear", 2, 2); - b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1.into(), + b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: P1.into(), })); b.id() }; @@ -2486,8 +2486,8 @@ fn shared_scaled_tax_taxes_each_attacker_independently() { let _sphere = add_sphere_of_safety(&mut scenario, P1); let lure = |scenario: &mut GameScenario, name: &str| { let mut b = scenario.add_creature(P0, name, 2, 2); - b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1.into(), + b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: P1.into(), })); b.id() }; diff --git a/crates/phase-ai/src/combat_ai.rs b/crates/phase-ai/src/combat_ai.rs index 4798c7022a..e6810bbeb8 100644 --- a/crates/phase-ai/src/combat_ai.rs +++ b/crates/phase-ai/src/combat_ai.rs @@ -225,15 +225,23 @@ pub fn choose_attackers_with_targets_with_profile( // attackers or the engine rejects the whole declaration. Partition them out // and union them back unconditionally — value heuristics only apply to the // free choices. `creature_must_attack` is the engine's single authority. - // Loop-invariant hoist: `attackable_player_targets` depends only on `state` + // Loop-invariant hoist: the attackable-defender set depends only on `state` // (immutable during this filter), so compute it once instead of per creature - // inside `creature_must_attack`. - let attackable = engine::game::combat::attackable_player_targets(state); + // inside `creature_must_attack`. `attackable_defender_targets` is the COUNTED + // form; `attacker_choice_sweeps_attackable_players_independent_of_goaded_count` + // is revert-failing on this hoist. + // + // CR 506.3: the whole defender universe — players, planeswalkers, and + // battles — so a requirement pointed at a planeswalker (Gideon Jura's "+2") + // is recognized as mandatory here exactly like a player-directed lure. + // Passing only the player subset would have made the AI omit a creature the + // engine then rejects the declaration for. + let attackable = engine::game::combat::attackable_defender_targets(state); let mandatory: Vec = candidates .iter() .copied() .filter(|&id| { - engine::game::combat::creature_must_attack_with_attackable_players( + engine::game::combat::creature_must_attack_with_attackable_targets( state, id, &attackable, diff --git a/crates/phase-ai/tests/scenarios.rs b/crates/phase-ai/tests/scenarios.rs index 579772867b..228f64be11 100644 --- a/crates/phase-ai/tests/scenarios.rs +++ b/crates/phase-ai/tests/scenarios.rs @@ -1626,8 +1626,8 @@ fn ai_declare_attackers_completion_returns_apply_accepted_legal_action() { let mut scenario = GameScenario::new(); let attacker = { let mut b = scenario.add_creature(P0, "Lured Bear", 2, 2); - b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1.into(), + b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackDefender { + defender: P1.into(), })); b.id() }; From 41a0df1a3e0e2a36e6a5e294a81b51e8a4d8d6a2 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:20:01 -0500 Subject: [PATCH 2/6] Cover ControllerRef::SpecificPlayer in the mtgish-import scope converter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `controller_to_scope` matches `ControllerRef` exhaustively, so widening the engine enum broke `mtgish-import` even though the engine crate itself compiled. Adding a variant to a `pub` engine enum is a downstream API change; verifying with `cargo clippy -p phase-engine` instead of the workspace hid the whole class. The new arm strict-fails with `EnginePrerequisiteMissing`, matching every other non-broadcast sibling: `ProhibitionScope` is a broadcast scope (Controller/Opponents/AllPlayers) and a snapshotted player id has no broadcast equivalent, so mapping it would silently over-broaden the prohibition. Unreachable in practice — the variant is produced only by engine resolvers installing durational continuous effects, never by a converter or the Oracle parser. Verified with the exact CI command: `cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings` (locally also excluding `probe-pin`, which is Unix-only and cannot compile on Windows — pre-existing, unrelated to this PR). Co-Authored-By: Claude Opus 5 --- crates/mtgish-import/src/convert/player_effect.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/mtgish-import/src/convert/player_effect.rs b/crates/mtgish-import/src/convert/player_effect.rs index 8953e011cd..53ee2d62dd 100644 --- a/crates/mtgish-import/src/convert/player_effect.rs +++ b/crates/mtgish-import/src/convert/player_effect.rs @@ -348,6 +348,16 @@ fn controller_to_scope(c: &ControllerRef) -> ConvResult { engine_type: "ProhibitionScope", needed_variant: "TargetOpponent".into(), }), + // CR 109.4 + CR 611.2: a resolution-time snapshotted player id. Like the + // single-/targeted-player siblings above it has no broadcast + // `ProhibitionScope` equivalent, so mapping it would over-broaden the + // prohibition — strict-fail. Unreachable in practice: this variant is + // produced only by engine resolvers installing durational continuous + // effects, never by a converter or the Oracle parser. + ControllerRef::SpecificPlayer { .. } => Err(ConversionGap::EnginePrerequisiteMissing { + engine_type: "ProhibitionScope", + needed_variant: "SpecificPlayer".into(), + }), } } From 3770dceb98994f3105b918cfa4435044fb58b279 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:19:55 -0500 Subject: [PATCH 3/6] Address review: AI defender coverage, lowering outcomes, CR citations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer + CodeRabbit review of head 8a5a8321. AI blocker — the diagnosis holds, the predicted consequence does not. The mandatory-attacker sweep does discard the required AttackTarget, and the raw policy really does propose the defending player: RAW [(bear, Player(P0))] COMPLETED [(bear, Planeswalker(gideon))] But both production consumers (search.rs:3902, :3967 — the only two, and the policies merely score pre-built actions) wrap their proposal in `validated_declare_attackers` -> `combat::complete_attacker_proposal`, the single CR 508.1d authority, which replaces an under-max declaration with the maximum-requirement witness before the reducer sees it. Adds the requested regression driving the real `choose_action` seam and asserting reducer acceptance, with the raw-policy result pinned inside it so the test proves the completion is load-bearing instead of passing vacuously. Required coverage: * `force_attack::lower_dynamic_affected` returned `None` for BOTH a chosen-target subject and a broadcast population it could not lower, and the caller grafted per object either way — silently freezing a CR 611.2c population in the second case. Replaced with a three-way `SubjectLowering`; `Unlowerable` now installs nothing. * Pin `EffectScope::All` in the Gideon parser test, so a regression to `Single` (which both surfaces a spurious creature slot and takes the freezing path) fails loudly. * New `prune_until_next_turn_effects` test driving the arming function directly, with controller != snapshotted player so it cannot pass on a same-player fixture — the integration tests only set `active_player` by hand and asserted the installed duration. * The departure test now uses the production CR 704.5i zero-loyalty state-based action instead of poking the zone. * Serde round-trip now covers `RequiredDefender::Permanent`, the arm with hand-rolled `Deserialize` on both sides. * `ForceAttack` gets a dedicated coverage arm emitting the required defender, so a player-directed and a planeswalker-directed attack no longer collapse to one signature in the coverage/parse-diff artifact. CR citations: CR 608.2c is instruction ordering, not self-reference. CR 201.5 ("text that refers to the object it's on by name means just that particular object") is the rule a printed-name pronoun falls under, and is already what the rest of the codebase cites for this. Corrected, and the stale `MustAttackPlayer` / `must_attack_player_directives_for_creature` doc references left by the rename are updated. Two defects this review surfaced that were not in either report: * `add_planeswalker_from_oracle` seeded `counters[Loyalty]` but not the `loyalty` field. The activation gate reads the counters (CR 606.3) and the zero-loyalty SBA reads the field (CR 704.5i), so the helper built planeswalkers that could be activated but could never die. Found only by switching the departure test onto the production SBA path. * An earlier bulk rename had corrupted a `serde(alias)` doc line into a self-referential claim contradicting the paragraph below it. Verified with the exact CI command: `cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings` (locally also excluding `probe-pin`, Unix-only and uncompilable on Windows — pre-existing, unrelated). Co-Authored-By: Claude Opus 5 --- crates/engine/src/ai_support/candidates.rs | 4 +- crates/engine/src/database/encore_tests.rs | 8 +- crates/engine/src/game/combat.rs | 6 +- crates/engine/src/game/coverage.rs | 45 ++++--- crates/engine/src/game/effects/encore.rs | 4 +- .../engine/src/game/effects/force_attack.rs | 108 +++++++++------ crates/engine/src/game/effects/mod.rs | 2 +- .../engine/src/game/effects/prevent_damage.rs | 6 +- crates/engine/src/game/layers.rs | 69 +++++++++- crates/engine/src/game/scenario.rs | 7 + crates/engine/src/game/static_abilities.rs | 2 +- .../src/parser/oracle_effect/imperative.rs | 8 +- crates/engine/src/parser/oracle_effect/mod.rs | 4 +- .../src/parser/oracle_static/evasion.rs | 2 +- .../engine/src/parser/oracle_static/tests.rs | 2 +- crates/engine/src/parser/oracle_target.rs | 9 +- crates/engine/src/parser/oracle_tests.rs | 12 +- crates/engine/src/types/ability.rs | 9 +- crates/engine/src/types/game_state.rs | 2 +- crates/engine/src/types/statics.rs | 18 ++- .../bbfu7_attacks_if_able_not_goad.rs | 4 +- .../galactus_forced_attack_most_life.rs | 6 +- .../gideon_jura_forced_attack_planeswalker.rs | 27 +++- .../must_attack_player_attribution.rs | 10 +- .../engine/tests/integration/rules/combat.rs | 6 +- crates/phase-ai/tests/scenarios.rs | 126 ++++++++++++++++++ 26 files changed, 395 insertions(+), 111 deletions(-) diff --git a/crates/engine/src/ai_support/candidates.rs b/crates/engine/src/ai_support/candidates.rs index 618be81074..5dbadaeb2b 100644 --- a/crates/engine/src/ai_support/candidates.rs +++ b/crates/engine/src/ai_support/candidates.rs @@ -6287,7 +6287,7 @@ mod tests { } /// Regression (multi-requirement residual): a creature directed by - /// `MustAttackPlayer` (CR 508.1b) alongside a goaded creature (CR 701.15b) + /// `MustAttackDefender` (CR 508.1b) alongside a goaded creature (CR 701.15b) /// also forces a mixed-target declaration. The greedy forced-legal candidate /// must steer the directed creature onto its *required* player regardless of /// `valid_attack_targets` ordering. A goad-only target pick would land the @@ -6358,7 +6358,7 @@ mod tests { }); assert!( has_legal, - "forced assignment must direct the MustAttackPlayer creature to its required player" + "forced assignment must direct the MustAttackDefender creature to its required player" ); } diff --git a/crates/engine/src/database/encore_tests.rs b/crates/engine/src/database/encore_tests.rs index 151ec03a9b..b6ec4d1eca 100644 --- a/crates/engine/src/database/encore_tests.rs +++ b/crates/engine/src/database/encore_tests.rs @@ -197,7 +197,7 @@ fn encore_activation_creates_attacking_haste_copy_per_opponent() { ); // CR 702.141a + CR 508.1d: token must attack the opponent (PlayerId(1)) this - // turn — a transient MustAttackPlayer requirement bound to the token. + // turn — a transient MustAttackDefender requirement bound to the token. let must_attack = state .transient_continuous_effects .iter() @@ -241,7 +241,7 @@ fn dt_targets_contain(dt: &crate::types::game_state::DelayedTrigger, id: ObjectI /// CR 702.141a end-to-end (THREE players — the per-opponent case the dedicated /// resolver exists for): activating Encore creates one haste-bearing copy token -/// PER opponent, each bound via `MustAttackPlayer` to the *distinct* opponent it +/// PER opponent, each bound via `MustAttackDefender` to the *distinct* opponent it /// was created for, all collected into one next-end-step sacrifice — and that /// sacrifice, when the end step arrives, actually removes both tokens. #[test] @@ -289,7 +289,7 @@ fn encore_three_player_one_token_per_opponent_then_sacrificed_at_end_step() { ); // Each token is a haste-bearing copy bound to a DISTINCT opponent's - // `MustAttackPlayer` requirement (the whole reason the dedicated resolver + // `MustAttackDefender` requirement (the whole reason the dedicated resolver // exists — generic ForceAttack can't bind "that opponent"). let mut bound_opponents = BTreeSet::new(); for &token in &tokens { @@ -315,7 +315,7 @@ fn encore_three_player_one_token_per_opponent_then_sacrificed_at_end_step() { _ => None, }) }) - .expect("token must carry a MustAttackPlayer requirement"); + .expect("token must carry a MustAttackDefender requirement"); bound_opponents.insert(player); } let expected: BTreeSet = [PlayerId(1), PlayerId(2)].into_iter().collect(); diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index b1dd773a40..2aed01cb98 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -5230,14 +5230,14 @@ pub(crate) fn goading_players_for_creature_gated( /// /// No `CombatStaticGates` field: this is a PER-OBJECT scan of /// `active_static_definitions`, exactly like -/// `must_attack_player_directives_for_creature` — not a battlefield sweep. +/// `must_attack_defender_directives_for_creature` — not a battlefield sweep. /// /// Source attribution for the frontend badge is deliberately NOT extended here /// (see `must_attack_sources_gated`): this mode does not carry `source_object`, /// so the badge renders bare, which the client already handles. /// /// The scan below deliberately ignores `sd.affected`, mirroring the adjacent -/// `must_attack_player_directives_for_creature` sibling. This is NOT the #6296 +/// `must_attack_defender_directives_for_creature` sibling. This is NOT the #6296 /// (`has_local_must_attack`) case where a remote-scoped carrier forced ITSELF to /// attack: that bug needs a PRINTED remote-scoped def, which only generic /// `MustAttack` has (Fumiko the Lowblood). `MustAttackAwayFromSource` is @@ -12916,7 +12916,7 @@ mod tests { /// shared fixed player set ({P1, P2}, as the pre-fix code did), attacking P1 and /// P2 would each score 1 and tie, wrongly permitting P2. This exercises the real /// production seam: `AttackDeclarationConstraints::build` → - /// `must_attack_player_directives_for_creature` → the `MustAttackAnyOf` / + /// `must_attack_defender_directives_for_creature` → the `MustAttackAnyOf` / /// `MustAttackDefender` requirement split → `score_single` / `max_no_payment`. #[test] fn matching_tie_plus_fixed_forces_the_fixed_defender() { diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index b11c0f35d6..8ffa038380 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -153,7 +153,7 @@ pub(crate) fn is_data_carrying_static(mode: &StaticMode) -> bool { // the attacker that must be blocked (Provoke). Enforced by direct // match in combat.rs declare-blockers validation. | StaticMode::MustBlockAttacker { .. } - // CR 508.1d: MustAttackPlayer carries the `PlayerId` that must be + // CR 508.1d: MustAttackDefender carries the `PlayerId` that must be // attacked (Alluring Siren). Enforced by direct match in combat.rs // declare-attackers validation. | StaticMode::MustAttackDefender { .. } @@ -2426,15 +2426,6 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { | Effect::Connive { target, .. } | Effect::PhaseOut { target } | Effect::PhaseIn { target } - // CR 508.1d: single-scope ForceAttack reports its chosen `target` - // ("Target creature attacks you this combat if able"); the mass scope - // (Gideon Jura's "creatures that player controls") reports a `filter` - // below, since CR 115.1 makes that population a non-target. - | Effect::ForceAttack { - scope: EffectScope::Single, - target, - .. - } // CR 701.27a: single-scope Transform reports its `target` like other // single-target effects; mass Transform (scope:All) reports a `filter` below. | Effect::Transform { @@ -2463,6 +2454,33 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { d.push(("duration".into(), format!("{duration:?}"))); } } + // CR 508.1d + CR 506.3: ForceAttack reports the SUBJECT under the key its + // scope earns — `target` for a chosen creature (CR 115.1), `filter` for a + // non-targeting population (Gideon Jura's "creatures that player + // controls") — plus the REQUIRED DEFENDER, which is the axis that + // distinguishes an attack pointed at a player from one pointed at a + // planeswalker. Without the defender in the signature those two collapse + // to one entry and the coverage/parse-diff artifact cannot tell a + // Gideon-Jura-class card from an Alluring-Siren-class one. + // + // Modelled on the `ForceBlock` arm above, including its non-default + // duration rule. + Effect::ForceAttack { + target, + required_defender, + duration, + scope, + } => { + let subject_key = match scope { + EffectScope::Single => "target", + EffectScope::All => "filter", + }; + d.push((subject_key.into(), fmt_target(target))); + d.push(("defender".into(), fmt_target(required_defender))); + if *duration != Duration::UntilEndOfTurn { + d.push(("duration".into(), format!("{duration:?}"))); + } + } // CR 702.50a: EpicCopy's parameters live in its snapshotted ability. Effect::EpicCopy { .. } => {} Effect::Intensify { .. } => {} @@ -2487,13 +2505,6 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { target, .. } - // CR 508.1d + CR 115.1: the mass forced-attack population (Gideon Jura's - // "creatures that player controls") is a `filter`, not a target. - | Effect::ForceAttack { - scope: EffectScope::All, - target, - .. - } | Effect::BounceAll { target, .. } | Effect::CounterAll { target, .. } | Effect::DamageAll { diff --git a/crates/engine/src/game/effects/encore.rs b/crates/engine/src/game/effects/encore.rs index 48d1f7d50c..c38e750d3e 100644 --- a/crates/engine/src/game/effects/encore.rs +++ b/crates/engine/src/game/effects/encore.rs @@ -23,7 +23,7 @@ //! - **Per-opponent must-attack, not enters-attacking.** Encore is activated at //! sorcery speed (typically in a main phase, outside combat), so the tokens //! are *not* created already attacking. Instead each token gains a -//! `MustAttackPlayer { player }` requirement (CR 508.1d) bound to the opponent +//! `MustAttackDefender { defender }` requirement (CR 508.1d) bound to the opponent //! it was created for, lasting until end of turn (CR 702.141a "this turn"). //! Binding the requirement per opponent — instead of via the generic //! `Effect::ForceAttack`, whose `required_defender` context ref has no @@ -85,7 +85,7 @@ pub fn resolve( crate::game::effects::token_copy::resolve(state, ©_ability, events)?; // CR 702.141a + CR 508.1d: each token created for this opponent "attacks - // that opponent this turn if able." Bind a `MustAttackPlayer` requirement + // that opponent this turn if able." Bind a `MustAttackDefender` requirement // to the freshly-created token(s) for the rest of the turn. for token_id in state.last_created_token_ids.clone() { state.add_transient_continuous_effect( diff --git a/crates/engine/src/game/effects/force_attack.rs b/crates/engine/src/game/effects/force_attack.rs index d173714e18..9d12d4eacf 100644 --- a/crates/engine/src/game/effects/force_attack.rs +++ b/crates/engine/src/game/effects/force_attack.rs @@ -56,18 +56,39 @@ fn filter_denotes_object(filter: &TargetFilter) -> bool { ) } -/// CR 611.2c: For a BROADCAST subject ("creatures that player controls") return -/// the affected filter to install INTACT, with its player reference lowered to a -/// resolution-time snapshot. Returns `None` for a chosen-target subject ("target -/// creature attacks you this combat if able"), which keeps the pre-existing -/// per-object `SpecificObject` graft — CR 611.2c's dynamic-population concern -/// does not arise when the effect names specific objects. +/// CR 611.2c + CR 115.1: how a force-attack subject must be installed. /// -/// Gideon Jura's official ruling is the reason the broadcast form cannot freeze -/// its set: the "+2" "doesn't lock in what it applies to … whatever creatures -/// the targeted opponent controls during the declare attackers step of their -/// next turn must attack Gideon Jura if able. This includes creatures that come -/// under that player's control after the ability has resolved." +/// Three OUTCOMES, deliberately distinct rather than collapsed into an +/// `Option`. "Chosen target" and "broadcast population that could not be +/// lowered" both mean "no population filter to install", but they call for +/// opposite handling: the first is correctly grafted per object, while the +/// second must install NOTHING. Grafting an unlowerable population per object +/// would freeze it at resolution — exactly the CR 611.2c violation the Gideon +/// Jura ruling forbids — and would do so silently. +enum SubjectLowering { + /// CR 115.1: a chosen-target subject ("target creature attacks you this + /// combat if able"). Per-object `SpecificObject` grafting is correct; + /// CR 611.2c's dynamic-population concern does not arise when the effect + /// names specific objects. + ChosenTarget, + /// CR 611.2c: a broadcast population, lowered and ready to install INTACT so + /// the layer pass re-derives its members every declare-attackers step. + Population(TargetFilter), + /// CR 611.2c: a broadcast population whose player reference could not be + /// resolved (no player target to bind, or a filter shape this lowering does + /// not understand). Unreachable for every printed card today; if it is ever + /// reached, installing nothing is the honest failure — a frozen set would + /// look like it worked while quietly disobeying the ruling. + Unlowerable, +} + +/// CR 611.2c: Classify a force-attack subject for installation. +/// +/// Gideon Jura's official ruling is why the broadcast form cannot freeze its +/// set: the "+2" "doesn't lock in what it applies to … whatever creatures the +/// targeted opponent controls during the declare attackers step of their next +/// turn must attack Gideon Jura if able. This includes creatures that come under +/// that player's control after the ability has resolved." /// /// Only `ControllerRef::TargetPlayer` / `TargetOpponent` need lowering: /// `ControllerRef::You` / `Opponent` are resolved by `layers.rs` against the @@ -77,15 +98,17 @@ fn lower_dynamic_affected( ability: &ResolvedAbility, target: &TargetFilter, scope: EffectScope, -) -> Option { +) -> SubjectLowering { // CR 115.1: the scope is the authority for which form this is — a `Single` // subject is a chosen target no matter what filter shape it happens to // carry, so it must never take the population path. if scope != EffectScope::All { - return None; + return SubjectLowering::ChosenTarget; } + // An `All` scope IS a population by construction, so every failure below is + // `Unlowerable`, never `ChosenTarget`. let TargetFilter::Typed(typed) = target else { - return None; + return SubjectLowering::Unlowerable; }; let mut typed = typed.clone(); if matches!( @@ -95,13 +118,15 @@ fn lower_dynamic_affected( // CR 109.4 + CR 611.2: "that player" is fixed when the ability resolves. // `ability.targets` no longer exists when the layer pass re-derives the // affected set, so bind the id now. - let id = ability.targets.iter().find_map(|t| match t { + let Some(id) = ability.targets.iter().find_map(|t| match t { TargetRef::Player(pid) => Some(*pid), TargetRef::Object(_) => None, - })?; + }) else { + return SubjectLowering::Unlowerable; + }; typed.controller = Some(ControllerRef::SpecificPlayer { id }); } - Some(TargetFilter::Typed(typed)) + SubjectLowering::Population(TargetFilter::Typed(typed)) } /// CR 611.2 + CR 514.2: Lower a target-scoped duration to a resolution-time @@ -173,8 +198,8 @@ pub fn resolve( // `MustAttackAwayFromSource` grants down the same path for the same // reason (Kardur, Maximum Carnage); this resolver installs directly, so // it makes the same call here. - if let Some(affected) = lower_dynamic_affected(ability, target, *scope) { - state.add_transient_continuous_effect( + match lower_dynamic_affected(ability, target, *scope) { + SubjectLowering::Population(affected) => state.add_transient_continuous_effect( ability.source_id, ability.controller, duration.clone(), @@ -183,28 +208,33 @@ pub fn resolve( mode: StaticMode::MustAttackDefender { defender }, }], None, - ); - } else { - for obj_id in resolved_object_ids_for_filter(state, ability, target) { - if !state.objects.contains_key(&obj_id) { - continue; - } + ), + SubjectLowering::ChosenTarget => { + for obj_id in resolved_object_ids_for_filter(state, ability, target) { + if !state.objects.contains_key(&obj_id) { + continue; + } - state.add_transient_continuous_effect( - ability.source_id, - ability.controller, - duration.clone(), - TargetFilter::SpecificObject { id: obj_id }, - vec![ContinuousModification::AddStaticMode { - // CR 611.2: the required defender is snapshotted at resolution. - mode: StaticMode::MustAttackDefender { - defender: defender.clone(), - }, - }], - None, - ); + state.add_transient_continuous_effect( + ability.source_id, + ability.controller, + duration.clone(), + TargetFilter::SpecificObject { id: obj_id }, + vec![ContinuousModification::AddStaticMode { + // CR 611.2: the required defender is snapshotted at resolution. + mode: StaticMode::MustAttackDefender { + defender: defender.clone(), + }, + }], + None, + ); + } + 0 } - } + // CR 611.2c: install NOTHING rather than a frozen per-object graft. + // See `SubjectLowering::Unlowerable`. + SubjectLowering::Unlowerable => 0, + }; } events.push(GameEvent::EffectResolved { diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index b3c5d45b19..2807a89b19 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -5149,7 +5149,7 @@ fn affected_objects_from_events( // Mirrors the broadcast-static binding in `effect.rs` (`Some(filter)` // arm). Broader than the parser-side `is_mass_coerce_static` // (oracle_effect/mod.rs), which still gates only the MustAttack/ - // MustAttackPlayer coercion pair for its own (unrelated) + // MustAttackDefender coercion pair for its own (unrelated) // ParentTarget-rewrite purpose. Effect::GenericEffect { static_abilities, diff --git a/crates/engine/src/game/effects/prevent_damage.rs b/crates/engine/src/game/effects/prevent_damage.rs index 53c5173309..f452c772d3 100644 --- a/crates/engine/src/game/effects/prevent_damage.rs +++ b/crates/engine/src/game/effects/prevent_damage.rs @@ -201,7 +201,7 @@ fn untargeted_damage_filter( // matching is `typed_recipient_valid_card_filter`'s job, so this // arm must be checked BEFORE the generic `is_context_ref()` catch-all. TargetFilter::TrackedSet { .. } | TargetFilter::TrackedSetFiltered { .. } => None, - // CR 615 + CR 608.2c: the printed-name self-reference ("prevent all + // CR 615 + CR 201.5: the printed-name self-reference ("prevent all // damage that would be dealt to HIM this turn" — Gideon Jura, Gideon of // the Trials) names the source OBJECT, not a player. `SelfRef` is in // `is_context_ref()`, so without this arm the catch-all below would @@ -239,7 +239,7 @@ fn typed_recipient_valid_card_filter(target: &TargetFilter) -> Option { Some(filter.clone()) } - // CR 615 + CR 608.2c: the printed-name self-reference IS an object + // CR 615 + CR 201.5: the printed-name self-reference IS an object // recipient — the shield rides on the source permanent (the untargeted // branch of `resolve`) and `valid_card: SelfRef` scopes it to damage // dealt to that host. Checked before the generic `is_context_ref()` @@ -1325,7 +1325,7 @@ mod tests { assert_eq!(state.players[0].life, 20); } - /// CR 615 + CR 608.2c: a `SelfRef` recipient ("prevent all damage that would + /// CR 615 + CR 201.5: a `SelfRef` recipient ("prevent all damage that would /// be dealt to HIM this turn" — Gideon Jura, Gideon of the Trials) scopes the /// shield to the SOURCE OBJECT. /// diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 7943bdfd6c..55c50e251b 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -6810,8 +6810,8 @@ fn static_mode_needs_source_controller_anchor(mode: &crate::types::statics::Stat /// CR 508.1d + CR 611.2c: True when a granted static mode belongs to the /// directing-source attribution class — modes whose consumers need to name -/// the object that grafted the requirement (currently `MustAttackPlayer`, -/// consumed by `combat::must_attack_player_directives_for_creature`). This +/// the object that grafted the requirement (currently `MustAttackDefender`, +/// consumed by `combat::must_attack_defender_directives_for_creature`). This /// gates the `source_object` stamp so ONLY these modes split into distinct /// defs per directing source; every other `AddStaticMode` mode keeps /// `source_object == None` and dedups unchanged (crew/keyword/evasion/… @@ -8218,7 +8218,7 @@ fn apply_continuous_effect_filtered( // CR 611.2c: stamp the directing object so combat / future // attribution consumers can name the object that grafted this // static (the ForceAttack / Encore / mass-coerce source for a - // MustAttackPlayer requirement). Gated on the attribution-class + // MustAttackDefender requirement). Gated on the attribution-class // predicate so only those modes split per source; every other // mode stays None and dedups unchanged (see the census in the // plan / the crew-delta scoped-stamp guard test). Mirrors the @@ -8598,6 +8598,69 @@ pub(crate) fn compute_current_copiable_values( #[cfg(test)] mod tests { + + /// CR 514.2 + CR 109.4: `prune_until_next_turn_effects` arms an + /// `UntilEndOfNextTurnOf { SpecificPlayer }` effect on the SNAPSHOTTED + /// player's turn — not its controller's. + /// + /// This drives the arming function directly, which the Gideon integration + /// tests do not: they set `active_player` by hand and assert the INSTALLED + /// duration, so they would still pass if the prune never recognized the new + /// scope and the requirement silently never expired. + /// + /// The controller (P0) and the snapshotted player (P1) are deliberately + /// different — that difference is the whole reason the variant exists, and a + /// prune that keyed on `e.controller` would arm on the wrong turn and pass a + /// same-player fixture. + #[test] + fn until_end_of_next_turn_of_specific_player_arms_on_that_players_turn() { + fn armed_after_pruning(active: PlayerId) -> Duration { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(1), + P0, + "Gideon Jura".to_string(), + Zone::Battlefield, + ); + // Controller P0; the window belongs to P1. + state.add_transient_continuous_effect( + source, + P0, + Duration::UntilEndOfNextTurnOf { + player: PlayerScope::SpecificPlayer { id: P1 }, + }, + TargetFilter::Typed( + TypedFilter::default().controller(ControllerRef::SpecificPlayer { id: P1 }), + ), + vec![ContinuousModification::AddStaticMode { + mode: crate::types::statics::StaticMode::MustAttackDefender { + defender: crate::types::statics::RequiredDefender::Fixed { player: P0 }, + }, + }], + None, + ); + prune_until_next_turn_effects(&mut state, active); + state.transient_continuous_effects[0].duration.clone() + } + + // The CONTROLLER's turn must not arm it — the window is not theirs. + assert_eq!( + armed_after_pruning(P0), + Duration::UntilEndOfNextTurnOf { + player: PlayerScope::SpecificPlayer { id: P1 } + }, + "the controller's untap step leaves a SpecificPlayer window un-armed" + ); + + // The SNAPSHOTTED player's turn arms it, so the existing cleanup-step + // prune ends it at that turn's cleanup (CR 514.2). + assert_eq!( + armed_after_pruning(P1), + Duration::UntilEndOfTurn, + "the snapshotted player's untap step arms the window" + ); + } use super::*; use crate::game::elimination::eliminate_player; use crate::game::scenario::{GameScenario, P0, P1}; diff --git a/crates/engine/src/game/scenario.rs b/crates/engine/src/game/scenario.rs index 9cb8577f66..c7bccfb10d 100644 --- a/crates/engine/src/game/scenario.rs +++ b/crates/engine/src/game/scenario.rs @@ -739,6 +739,13 @@ impl GameScenario { // planeswalker animated by its own ability would otherwise inherit the // flag. obj.summoning_sick = false; + // CR 306.5b: a planeswalker's loyalty IS the count of loyalty counters on + // it, but the engine keeps a `loyalty` field alongside the counter map — + // the activation gate reads the counters (CR 606.3) while the + // zero-loyalty state-based action reads the field (CR 704.5i). Seed BOTH + // so they start in sync; seeding only one produces a planeswalker that + // can be activated but can never die. + obj.loyalty = Some(loyalty); obj.counters .insert(crate::types::counter::CounterType::Loyalty, loyalty); diff --git a/crates/engine/src/game/static_abilities.rs b/crates/engine/src/game/static_abilities.rs index 5f42705ac6..956cea2ee6 100644 --- a/crates/engine/src/game/static_abilities.rs +++ b/crates/engine/src/game/static_abilities.rs @@ -197,7 +197,7 @@ pub fn build_static_registry() -> HashMap { // CR 508.1d + CR 701.15b: MustAttackAwayFromSource — the goad requirement // pair without the designation (Kardur, Doomscourge; Maximum Carnage I). // Nullary, so it is registry-keyable (unlike the data-carrying - // `MustAttackPlayer`). Runtime enforcement lives in combat.rs. The registry + // `MustAttackDefender`). Runtime enforcement lives in combat.rs. The registry // key is ALSO what keeps `coverage::unimplemented_mechanics` quiet for every // creature this grafts onto — it is load-bearing for the client, not // decoration. diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 943ce012dc..902fd4c861 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -6007,7 +6007,7 @@ fn attach_neuter_recipient_resolves_via_subject(ctx: &ParseContext) -> bool { } } -/// CR 608.2c + CR 109.5: the attach path's whole-phrase form of the shared +/// CR 201.5 + CR 109.5: the attach path's whole-phrase form of the shared /// source-anaphoric gendered pronoun recognizer. Delegates to the single /// authority (`oracle_target::parse_source_anaphoric_pronoun`) under /// `all_consuming` — an attach recipient is the pronoun and nothing else. @@ -6211,7 +6211,7 @@ pub(super) fn parse_prevention_amount(rest: &str) -> PreventionAmount { /// /// Tries, in priority order: /// 0. A source-anaphoric gendered pronoun ("him"/"her"/"himself"/"herself") → -/// `SelfRef`, UNGATED (CR 608.2c + CR 109.5). Magic templating uses a +/// `SelfRef`, UNGATED (CR 201.5 + CR 109.5). Magic templating uses a /// gendered pronoun only as the printed-name self-reference, so unlike the /// neuter "it" it needs no `parent_target_available` gate. This tier must /// run FIRST: with the gate closed, tier 1 declines and tier 2's @@ -6243,7 +6243,7 @@ pub(super) fn resolve_prevent_recipient( recipient: TextPair<'_>, parent_target_available: bool, ) -> Option { - // CR 608.2c + CR 109.5: tier 0 — the ungated printed-name self-reference. + // CR 201.5 + CR 109.5: tier 0 — the ungated printed-name self-reference. if let Some((filter, _)) = crate::parser::oracle_target::parse_source_anaphoric_pronoun_ref(recipient.original) { @@ -14001,7 +14001,7 @@ pub(super) fn must_attack_away_static_definition() -> StaticDefinition { use crate::types::statics::StaticMode; // DELIBERATE GAP (plan §5.9): keying `mode` on the away-from requirement also // excludes this def from `is_mass_coerce_static` (oracle_effect/mod.rs), which - // publishes the chain tracked set only for `MustAttack`/`MustAttackPlayer`. A + // publishes the chain tracked set only for `MustAttack`/`MustAttackDefender`. A // future card printing this compound followed by a "those creatures" anaphor // (CR 608.2c) would therefore not publish the set, unlike the plain-`MustAttack` // sibling above. Unreachable today and not a regression: neither card-data key diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 30be5d9eaf..c4f66c6ac8 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -26210,7 +26210,7 @@ fn clause_ir_mass_population(clause: &ClauseIr) -> Option { /// onto. Mutational Advantage's official ruling: "The set of permanents /// affected by Mutational Advantage is determined at the time Mutational /// Advantage resolves." Mirrors `is_mass_coerce_static`'s governing-filter -/// selection but is not restricted to the MustAttack/MustAttackPlayer +/// selection but is not restricted to the MustAttack/MustAttackDefender /// coercion statics that function targets — any Continuous static grant over /// a broadcast population qualifies. Only a single static definition is /// accepted: a `GenericEffect` carrying two or more static defs has no single @@ -26538,7 +26538,7 @@ fn rebind_tracked_aggregate_expr(expr: &mut QuantityExpr) { /// CR 508.1a + CR 508.1d + CR 608.2c: A mass "attack this turn if able" coercion — /// a `GenericEffect` carrying a `MustAttack` (CR 508.1a, attack-if-able) or -/// `MustAttackPlayer` (CR 508.1d, directed attack) static over a BROADCAST +/// `MustAttackDefender` (CR 508.1d, directed attack) static over a BROADCAST /// population (not `SelfRef` and not an inherited-target reference) — identifies /// "those creatures" at resolution. When a following "those creatures" clause /// reads that frozen population (Maddening Imp), the coerce is the producer that diff --git a/crates/engine/src/parser/oracle_static/evasion.rs b/crates/engine/src/parser/oracle_static/evasion.rs index 98cafd7ef6..d45d228b26 100644 --- a/crates/engine/src/parser/oracle_static/evasion.rs +++ b/crates/engine/src/parser/oracle_static/evasion.rs @@ -2503,7 +2503,7 @@ fn parse_attacks_required_defender_nom(input: &str) -> OracleResult<'_, PlayerFi /// (Galactus: "an opponent with the most life among your opponents"; CR 102.2 / /// CR 102.3 scope "opponent", CR 508.1b covers the active player's choice among /// tied legal defenders). Emits -/// `MustAttackPlayer { RequiredDefender::Matching { filter } }`, re-evaluated each +/// `MustAttackDefender { RequiredDefender::Matching { filter } }`, re-evaluated each /// declare-attackers step by the combat resolver. /// /// The dispatcher receives the self-ref-normalized line WITHOUT the CR 207.2c / diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 553f5eaa73..f314fa5b3c 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -9143,7 +9143,7 @@ fn flavor_labeled_non_forced_attack_line_is_not_hijacked() { assert!( super::evasion::parse_forced_attack_defender_static("Insatiable Hunger — ~ gets +1/+1.") .is_none(), - "a flavor-labeled non-forced-attack line must not become a MustAttackPlayer static", + "a flavor-labeled non-forced-attack line must not become a MustAttackDefender static", ); } diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index 8ca29a9b91..a8af054692 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -148,10 +148,17 @@ pub(crate) fn parse_anaphoric_target_ref( matches!(filter, TargetFilter::ParentTarget).then_some((filter, rest)) } -/// CR 608.2c + CR 109.5: Recognize a leading **source-anaphoric gendered +/// CR 201.5 + CR 109.5: Recognize a leading **source-anaphoric gendered /// pronoun** ("him" / "himself" / "her" / "herself") and bind it to /// [`TargetFilter::SelfRef`] — the ability's own source object. /// +/// CR 201.5 is the governing rule: "Text that refers to the object it's on by +/// name means just that particular object." Magic's templating substitutes a +/// gendered pronoun for the printed name on cards with a personified character +/// (Gideon Jura's "dealt to him" is "dealt to Gideon Jura"), so the pronoun is +/// that same self-reference rather than a CR 608.2c anaphor to something named +/// earlier in the instruction — which is why it needs no anaphor gate below. +/// /// Unlike the neuter "it" (which may anaphor an earlier clause's chosen target /// and therefore needs the `parent_target_available` gate in /// [`parse_anaphoric_target_ref`]), a gendered pronoun on a Magic card is diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index f3f509ec15..ad9c28b20a 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -1711,6 +1711,7 @@ fn gideon_jura_full_parse() { let Effect::ForceAttack { target, required_defender, + scope, .. } = &*r.abilities[0].effect else { @@ -1724,6 +1725,15 @@ fn gideon_jura_full_parse() { &TargetFilter::SelfRef, "CR 506.3: the required defender is Gideon Jura itself, not a player" ); + // CR 611.2c + CR 115.1: the subject is a live POPULATION, not a chosen + // target. `Single` here would both surface a spurious creature target slot + // and send `force_attack::resolve` down the per-object graft path, freezing + // the affected set at resolution against the card's own ruling. + assert_eq!( + scope, + &EffectScope::All, + "the +2's subject is a broadcast population" + ); let TargetFilter::Typed(typed) = target else { panic!("the affected subject is a typed creature population, got {target:?}"); }; @@ -1801,7 +1811,7 @@ fn gideon_jura_full_parse() { assert_eq!(amount, &PreventionAmount::All, "\"prevent ALL damage\""); } -/// CR 608.2c + CR 109.5: the building block behind Gideon Jura's shield fix — +/// CR 201.5 + CR 109.5: the building block behind Gideon Jura's shield fix — /// a source-anaphoric GENDERED pronoun recipient binds to the source with NO /// `parent_target_available` gate, because Magic templating uses "him"/"her" /// only as the printed-name self-reference. Tested at the building-block level diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index eeb5d952f3..bc621943c2 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -12642,7 +12642,6 @@ pub enum Effect { /// Jura's "attack Gideon Jura if able") grafts `RequiredDefender::Permanent`. /// `force_attack::resolve` is the single place that classifies it. /// - /// `serde(alias)`: pre-widening payloads named this field `required_defender`. /// `scope` is the single-vs-mass axis, exactly as on [`Effect::Transform`] /// and [`Effect::SetTapState`] — parameterized rather than split into a /// sibling `ForceAttackAll`. `Single` (the default, and every pre-Gideon-Jura @@ -12654,9 +12653,9 @@ pub enum Effect { /// so no creature slot is built; the companion PLAYER slot still surfaces via /// `mass_all_target_filter`. /// - /// `serde(alias)`: pre-widening payloads named `required_defender` - /// `required_player`; `scope` defaults to `Single`, which is what every such - /// payload meant. + /// `serde`: pre-widening payloads named this field `required_player`, which + /// the alias below accepts; `scope` is absent from them and defaults to + /// `Single`, which is what every such payload meant. ForceAttack { #[serde(default = "default_target_filter_any")] target: TargetFilter, @@ -22647,7 +22646,7 @@ pub struct StaticDefinition { pub source_controller: Option, /// CR 508.1d + CR 611.2c: The object that grafted this static onto its /// carrier (the ForceAttack/Encore/mass-coerce source for a - /// `MustAttackPlayer` requirement). Stamped at materialization from the + /// `MustAttackDefender` requirement). Stamped at materialization from the /// resolving continuous effect's `source_id`, but ONLY for static modes in /// the directing-source attribution class (see /// `static_mode_carries_directing_source` in game/layers.rs) — mirrors the diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index 1ec2843093..669476f799 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -26406,7 +26406,7 @@ mod tests { ); } - /// CR 104.4b + CR 611.2c + CR 400.7: a materialized `MustAttackPlayer` + /// CR 104.4b + CR 611.2c + CR 400.7: a materialized `MustAttackDefender` /// static's `source_object` provenance is a layer-DERIVED characteristic that /// `object_content_eq` deliberately omits (like the whole `static_definitions` /// vec). Two states differing ONLY in a grafted requirement's directing-source diff --git a/crates/engine/src/types/statics.rs b/crates/engine/src/types/statics.rs index 13ecfbb058..b8c460d174 100644 --- a/crates/engine/src/types/statics.rs +++ b/crates/engine/src/types/statics.rs @@ -4638,11 +4638,15 @@ mod tests { assert!(serde_json::from_str::(r#"{"type":"Bogus"}"#).is_err()); } - /// The production serde path: `MustAttackPlayer` carries a `RequiredDefender`, - /// and BOTH forms must round-trip through the derived `StaticMode` - /// (de)serialization (card-data export + game-state snapshots). + /// The production serde path: `MustAttackDefender` carries a + /// `RequiredDefender`, and EVERY form must round-trip through the derived + /// `StaticMode` (de)serialization (card-data export + game-state snapshots). + /// + /// `Permanent` is the one with a hand-rolled `Deserialize` on both sides — + /// `RequiredDefender`'s custom impl plus `ObjectIncarnationRef`'s + /// legacy-integer shim — so a missed arm in either would surface only here. #[test] - fn must_attack_player_round_trips_through_static_mode() { + fn must_attack_defender_round_trips_through_static_mode() { for mode in [ StaticMode::MustAttackDefender { defender: RequiredDefender::Fixed { @@ -4654,6 +4658,12 @@ mod tests { filter: PlayerFilter::Opponent, }, }, + // CR 506.3 + CR 400.7: the permanent defender, pinned by incarnation. + StaticMode::MustAttackDefender { + defender: RequiredDefender::Permanent { + permanent: ObjectIncarnationRef::of(crate::types::identifiers::ObjectId(7), 3), + }, + }, ] { let json = serde_json::to_string(&mode).unwrap(); let back: StaticMode = serde_json::from_str(&json).unwrap(); diff --git a/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs b/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs index 82a0a83954..99cd1caf8e 100644 --- a/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs +++ b/crates/engine/tests/integration/bbfu7_attacks_if_able_not_goad.rs @@ -758,7 +758,7 @@ fn bound_creature_reports_no_unimplemented_mechanics() { /// REVERT PROBE: delete the unconditional `MustAttackGeneric` push in /// `AttackDeclarationConstraints::build` ⇒ `max_no_payment == 0` ⇒ declaring no /// attackers becomes legal ⇒ leg 1 fails. (Gating the away-from push on -/// `attackable_players`, like the `MustAttackPlayer` push, would NOT flip leg 1 — +/// `attackable_players`, like the `MustAttackDefender` push, would NOT flip leg 1 — /// `MustAttackGeneric` still carries it — which is why that is not the probe.) #[test] fn only_avoided_player_attackable_still_forces_the_attack() { @@ -816,7 +816,7 @@ fn only_avoided_player_attackable_still_forces_the_attack() { /// `CombatRequirement::MustAttack::sources` is the term-specific observable. /// `must_attack_sources_gated` unions three contributors: gated /// `check_static_ability_sources(MustAttack)`, `StaticMode::Goaded` carriers, and -/// attackable `MustAttackPlayer` carriers. This fixture has neither of the latter +/// attackable `MustAttackDefender` carriers. This fixture has neither of the latter /// two (asserted below), and `MustAttackAwayFromSource` deliberately contributes /// NO source (see `players_to_attack_away_from_gated`), so a non-empty `sources` /// can only come from the `MustAttack` graft — via the same gate + check pair the diff --git a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs index 3fdb17b5d2..d27d88240c 100644 --- a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs +++ b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs @@ -5,8 +5,8 @@ //! //! These drive the REAL pipeline end-to-end: verbatim Oracle text → //! `normalize_self_refs_for_static` → parser -//! (`parse_forced_attack_defender_static`) → `MustAttackPlayer { Matching }` → -//! `must_attack_player_directives_for_creature` (the changed runtime seam, +//! (`parse_forced_attack_defender_static`) → `MustAttackDefender { Matching }` → +//! `must_attack_defender_directives_for_creature` (the changed runtime seam, //! re-evaluated each declare-attackers step) → `attacker_constraints_for_active_player` //! (the DeclareAttackers waiting payload authority) AND the `declare_attackers` //! legality validator via the `GameAction::DeclareAttackers` route. @@ -84,7 +84,7 @@ fn declare(runner: &mut GameRunner, galactus: ObjectId, defender: PlayerId) -> R /// The changed seam surfaces through the production requirement authority: the /// required defender resolves to the most-life opponent (CR 604.1 live class). /// REVERT-FAIL: if the `RequiredDefender::Matching` arm of -/// `must_attack_player_directives_for_creature` returned nothing, `players` would +/// `must_attack_defender_directives_for_creature` returned nothing, `players` would /// be empty and this equality fails. #[test] fn galactus_requirement_surfaces_most_life_opponent() { diff --git a/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs b/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs index 27c2d06aa0..888a33324b 100644 --- a/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs +++ b/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs @@ -341,10 +341,31 @@ fn requirement_lapses_when_gideon_leaves_the_battlefield() { let (mut runner, gideon, bears) = setup(1); activate_plus_two(&mut runner, gideon); - // CR 400.7: Gideon leaves; the snapshotted incarnation pin no longer names a - // live defender. + // CR 704.5i + CR 400.7: Gideon leaves the battlefield the way he actually + // would — loyalty hits 0 and the state-based action puts him into his + // owner's graveyard. Driving the production SBA rather than poking the zone + // directly is what makes this a real departure: the object's incarnation is + // bumped by the same pipeline a game would use, so the snapshotted pin in + // `RequiredDefender::Permanent` goes stale exactly as it does in play. + { + let obj = runner + .state_mut() + .objects + .get_mut(&gideon) + .expect("Gideon is on the battlefield"); + // CR 306.5b: keep field and counter map in sync, as the engine does. + obj.loyalty = Some(0); + obj.counters + .insert(engine::types::counter::CounterType::Loyalty, 0); + } let mut events = Vec::new(); - engine::game::zones::move_to_zone(runner.state_mut(), gideon, Zone::Graveyard, &mut events); + engine::game::sba::check_state_based_actions(runner.state_mut(), &mut events); + assert_eq!( + runner.state().objects.get(&gideon).map(|obj| obj.zone), + Some(Zone::Graveyard), + "reach-guard: the SBA actually moved Gideon to the graveyard" + ); + hand_turn_to_p1(&mut runner); let valid = get_valid_attacker_ids(runner.state()); diff --git a/crates/engine/tests/integration/must_attack_player_attribution.rs b/crates/engine/tests/integration/must_attack_player_attribution.rs index 484299d2d1..76b6d0431e 100644 --- a/crates/engine/tests/integration/must_attack_player_attribution.rs +++ b/crates/engine/tests/integration/must_attack_player_attribution.rs @@ -1,7 +1,7 @@ //! CR 508.1d + CR 611.2c: true directing-carrier attribution for grafted -//! `MustAttackPlayer` combat requirements. +//! `MustAttackDefender` combat requirements. //! -//! A `MustAttackPlayer` requirement is never an intrinsic printed static — it is +//! A `MustAttackDefender` requirement is never an intrinsic printed static — it is //! always grafted onto its carrier creature by a directing object (an //! `Effect::ForceAttack` / `Encore` / mass-coerce source) through an //! `AddStaticMode` transient continuous effect. Before this change the combat @@ -31,7 +31,7 @@ use engine::types::phase::Phase; use engine::types::player::PlayerId; use engine::types::statics::{CrewAction, CrewContributionKind, StaticMode}; -/// Graft a `MustAttackPlayer { player }` requirement onto `creature` from the +/// Graft a `MustAttackDefender { player }` requirement onto `creature` from the /// directing object `source`, exactly as `Effect::ForceAttack` resolves it. The /// stamp reads `effect.source_id` (= `source`), so the materialized static gains /// `source_object == Some(source)`. @@ -68,7 +68,7 @@ fn refresh(runner: &mut GameRunner) { evaluate_layers(runner.state_mut()); } -/// 7.1a — a grafted `MustAttackPlayer` requirement attributes the DIRECTING +/// 7.1a — a grafted `MustAttackDefender` requirement attributes the DIRECTING /// object, not the creature. REVERT-FAIL: without the `source_object` stamp in /// `layers.rs` (or the carrier threading in `combat.rs`), the carrier falls back /// to the creature, so `sources == [creature]` and both the `contains(source)` @@ -241,7 +241,7 @@ fn departed_directing_source_id_is_surfaced_without_panic() { /// crew math is byte-identical to today (`base + 1`, NOT `base + 2`). REVERT-FAIL: /// an UNCONDITIONAL stamp (dropping the `static_mode_carries_directing_source` /// gate) splits the two crew grafts and yields `base + 2`. Discriminating -/// positive: the same two-source pattern with `MustAttackPlayer` (7.1c) DOES +/// positive: the same two-source pattern with `MustAttackDefender` (7.1c) DOES /// split — together they prove the gate splits attribution modes and only those. #[test] fn source_object_stamp_scoped_to_attribution_modes() { diff --git a/crates/engine/tests/integration/rules/combat.rs b/crates/engine/tests/integration/rules/combat.rs index b5d548b07e..7a1e2c54bd 100644 --- a/crates/engine/tests/integration/rules/combat.rs +++ b/crates/engine/tests/integration/rules/combat.rs @@ -1773,13 +1773,13 @@ fn tap_all_p0_lands(runner: &mut GameRunner) { } } -/// CR 508.1d flagship: two INCOMPATIBLE `MustAttackPlayer` requirements on one +/// CR 508.1d flagship: two INCOMPATIBLE `MustAttackDefender` requirements on one /// creature (it can attack only one player). The maximum attainable requirement /// count is 1, so a declaration attacking one required player is legal even /// though the other requirement is unmet — this is exactly the incompatible- /// requirements bug the CR 508.1d solver fixes. /// -/// Revert guard: the old validator required EVERY `MustAttackPlayer` target +/// Revert guard: the old validator required EVERY `MustAttackDefender` target /// individually, so attacking only P1 was rejected (P2's lure unmet). Reverting /// `score >= max_no_payment` flips `attack_one_is_legal` back to an error. #[test] @@ -2263,7 +2263,7 @@ fn legacy_object_incarnation_ref_deserializes_to_sentinel() { /// CR 508.1d (final sentence): "If a creature can't attack unless a player pays a /// cost, that player is not required to pay that cost." A creature whose only way -/// to satisfy its `MustAttackPlayer` lure is a TAXED attack does not raise the +/// to satisfy its `MustAttackDefender` lure is a TAXED attack does not raise the /// no-payment maximum — so declaring NO attackers is legal (the player is never /// forced to pay the tax to satisfy the requirement). /// diff --git a/crates/phase-ai/tests/scenarios.rs b/crates/phase-ai/tests/scenarios.rs index 228f64be11..f7c5859ab7 100644 --- a/crates/phase-ai/tests/scenarios.rs +++ b/crates/phase-ai/tests/scenarios.rs @@ -1689,3 +1689,129 @@ fn ai_declare_attackers_completion_returns_apply_accepted_legal_action() { "the host AI loop must take at least one action for the declare step" ); } + +/// CR 506.3 + CR 508.1d: the AI must obey a forced-attack requirement whose +/// required defender is a PLANESWALKER, not just one naming a player. +/// +/// The mandatory-attacker sweep in `combat_ai` records only `ObjectId`s, so the +/// required `AttackTarget` is not carried into target assignment. That is safe +/// only because every production path routes its heuristic proposal through +/// `validated_declare_attackers` -> `combat::complete_attacker_proposal`, the +/// engine's single CR 508.1d authority, which replaces an under-max declaration +/// with the deterministic maximum-requirement witness. This test is the evidence +/// for that claim rather than an argument for it: it drives the real +/// `choose_action` seam on Gideon Jura's "+2" and asserts BOTH that the chosen +/// action attacks the planeswalker and that the reducer accepts it. +/// +/// Sibling of `ai_declare_attackers_completion_returns_apply_accepted_legal_action` +/// (the player-directed lure). If the AI ever returned its raw heuristic +/// assignment instead of the completed proposal, it would attack P0 here and the +/// reducer would reject the declaration — both halves fail. +#[test] +fn ai_obeys_planeswalker_directed_attack_requirement() { + const GIDEON_JURA_ORACLE: &str = concat!( + "+2: During target opponent's next turn, creatures that player controls ", + "attack Gideon Jura if able.\n", + "\u{2212}2: Destroy target tapped creature.\n", + "0: Until end of turn, Gideon Jura becomes a 6/6 Human Soldier creature ", + "that's still a planeswalker. Prevent all damage that would be dealt to ", + "him this turn.", + ); + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let gideon = scenario + .add_planeswalker_from_oracle(P0, "Gideon Jura", "Gideon", 6, GIDEON_JURA_ORACLE) + .id(); + let bear = scenario.add_creature(P1, "Bear", 2, 2).id(); + let mut runner = scenario.build(); + { + let state = runner.state_mut(); + state.active_player = P0; + state.priority_player = P0; + state.turn_number = 2; + state.layers_dirty.mark_full(); + } + engine::game::layers::evaluate_layers(runner.state_mut()); + + // Resolve the "+2" targeting P1 through the production activation path. + runner.activate(gideon, 0).target_player(P1).resolve(); + + // Hand the turn to P1 and park at their declare-attackers step. + { + let state = runner.state_mut(); + state.active_player = P1; + state.priority_player = P1; + state.phase = Phase::DeclareAttackers; + state.turn_number = 3; + // CR 302.6: everything has been under its controller's control since + // before this turn began. + for id in state.battlefield.clone() { + if let Some(obj) = state.objects.get_mut(&id) { + obj.summoning_sick = false; + } + } + state.layers_dirty.mark_full(); + } + engine::game::layers::evaluate_layers(runner.state_mut()); + + let valid = engine::game::combat::get_valid_attacker_ids(runner.state()); + assert!( + valid.contains(&bear), + "reach-guard: P1's creature is an eligible attacker" + ); + let targets = engine::game::combat::get_valid_attack_targets(runner.state()); + assert!( + targets.contains(&AttackTarget::Planeswalker(gideon)), + "reach-guard: the engine offers Gideon as an attackable defender: {targets:?}" + ); + runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { + player: P1, + valid_attacker_ids: valid, + valid_attack_targets: targets, + valid_attack_targets_by_attacker: None, + attacker_constraints: Default::default(), + }; + + // NON-VACUITY PIN: the raw heuristic genuinely gets this wrong. It records + // the creature as mandatory but discards the required `AttackTarget`, so it + // proposes the defending PLAYER. This assertion is what makes the + // `choose_action` check below meaningful — without it, the test would still + // pass if the heuristic happened to pick Gideon for value reasons, and would + // prove nothing about the completion seam. + // + // If a future change teaches the policy to carry the defender itself, this + // assertion flips to the planeswalker and should simply be updated — the + // seam below is the invariant, not the heuristic's raw answer. + let raw = phase_ai::combat_ai::choose_attackers_with_targets(runner.state(), P1); + assert_eq!( + raw, + vec![(bear, AttackTarget::Player(P0))], + "the raw policy proposes the defending player — the engine completion is \ + what repairs it, and that is exactly what this test guards" + ); + + let config = create_config(AiDifficulty::VeryHard, Platform::Native); + let mut rng = SmallRng::seed_from_u64(7); + let action = choose_action(runner.state(), P1, &config, &mut rng) + .expect("AI must choose a declare-attackers action"); + let GameAction::DeclareAttackers { attacks, .. } = &action else { + panic!("expected DeclareAttackers, got {action:?}"); + }; + assert_eq!( + attacks, + &vec![(bear, AttackTarget::Planeswalker(gideon))], + "CR 508.1d: the only maximum-requirement declaration attacks the planeswalker" + ); + + runner + .act(action) + .expect("the AI's declaration must be reducer-legal (apply-accepted)"); + assert!( + runner.state().combat.as_ref().is_some_and(|c| c + .attackers + .iter() + .any(|a| a.object_id == bear && a.attack_target == AttackTarget::Planeswalker(gideon))), + "combat commits with the creature attacking Gideon" + ); +} From 561f1e80722f746c73e9f7c894129134f0d1a840 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:17:33 -0500 Subject: [PATCH 4/6] Address round-2 review: growing-class read, APNAP anchor, defender referent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three findings from the current-head review, each with a regression. [HIGH] `MustAttackDefender::Matching` was grouped with the read-free modes in `static_mode_references_growing_class`, but `combat::must_attack_defender_directives_for_creature` re-evaluates its `PlayerFilter` against live player state at every declare-attackers step (Galactus), so a cached analysis result can go stale. Now splits: `Fixed`/`Permanent` are frozen ids and stay read-free; `Matching` fails closed on ANY filter rather than enumerating `PlayerFilter`'s variants — the doctrine the `activator_filter` site in this same file states at length, and for the same reason. This predates the PR (the entry was `MustAttackPlayer { .. }` in that list and `Matching` shipped with Galactus), but it is inside this change's blast radius, so it is fixed here rather than deferred. [MED] `ControllerRef::SpecificPlayer` anchored APNAP ordering at the active player instead of its stored id. The prior arm deliberately folded it into the fallback on the grounds that it is unreachable there — which is the wrong trade: unreachable AND wrong-if-reached is strictly worse than simply correct, and there is nothing to resolve or fail closed about in a frozen id. Now anchors at the id. [MED] `snapshot_required_defender` classified the defender by filter VARIANT while its own doc claimed it classified by referent kind. `SelfRef`/`SpecificObject` are objects by construction, but `ParentTarget` and `ParentTargetSlot` name whatever the parent clause targeted — which may be a player. Those were routed down the object path, where `resolved_object_ids_for_filter` finds nothing and the requirement is dropped entirely. Split out `defender_referent`, which resolves the inherited target first and branches on whether it is a player or an object, so the code now does what the comment claimed. Regressions: the growing-class scan asserts read-free for both frozen arms and fail-closed for the live class (the false arms are what make the true arm meaningful); the APNAP test uses a NON-ACTIVE snapshotted player, the only configuration in which the old fallback is visible, plus a default-anchor guard; the referent test covers a player-valued and an object-valued parent target in one pair. Verified with the exact CI command: `cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings` clean; engine 18987 lib + 4902 integration + 30 bin; phase-ai 2110; zero failures. Co-Authored-By: Claude Opus 5 --- crates/engine/src/analysis/resource.rs | 68 ++++++- .../engine/src/game/effects/force_attack.rs | 176 ++++++++++++++---- crates/engine/src/game/players.rs | 46 ++++- 3 files changed, 251 insertions(+), 39 deletions(-) diff --git a/crates/engine/src/analysis/resource.rs b/crates/engine/src/analysis/resource.rs index c0312ca219..3d38f6866c 100644 --- a/crates/engine/src/analysis/resource.rs +++ b/crates/engine/src/analysis/resource.rs @@ -3999,6 +3999,28 @@ fn static_mode_references_growing_class(mode: &crate::types::statics::StaticMode // analogue of `modification_grants_growing_cost_keyword`). StaticMode::CastWithKeyword { keyword } => kw_reads(keyword), + // CR 508.1d + CR 604.1: the required defender splits by whether it is a + // resolution-time SNAPSHOT or a LIVE class. + // + // `Fixed`/`Permanent` are frozen ids (a `PlayerId`, an + // `ObjectIncarnationRef`) — nothing is re-derived from the board, so they + // are genuinely read-free. + // + // `Matching` is not. `combat::must_attack_defender_directives_for_creature` + // re-evaluates its `PlayerFilter` against live player state at EVERY + // declare-attackers step (Galactus's "opponent with the most life among + // your opponents"), so the class it names can change as the board does and + // a cached analysis result can go stale. Fail closed on ANY filter rather + // than enumerating `PlayerFilter`'s variants — the same doctrine the + // `activator_filter` site above states at length, and for the same reason: + // enumerating here would silently assert something about every future + // variant. + StaticMode::MustAttackDefender { defender } => match defender { + crate::types::statics::RequiredDefender::Fixed { .. } + | crate::types::statics::RequiredDefender::Permanent { .. } => false, + crate::types::statics::RequiredDefender::Matching { .. } => true, + }, + // Non-cost (or fixed-cost) variants — read-free, listed exhaustively (NO `_`). // `ReduceActionCost`/`DefilerCostReduction` carry only a fixed generic // reduction; `CantPayCost` is a payment PROHIBITION, not a payable cost; the @@ -4030,7 +4052,6 @@ fn static_mode_references_growing_class(mode: &crate::types::statics::StaticMode | StaticMode::CantLoseLife | StaticMode::PlayerProtection(..) | StaticMode::MustAttack - | StaticMode::MustAttackDefender { .. } | StaticMode::MustBlock | StaticMode::MustBlockAttacker { .. } | StaticMode::CantDraw { .. } @@ -5286,6 +5307,51 @@ fn ability_has_per_game_activation_gate(state: &GameState, key: &(ObjectId, usiz #[cfg(test)] mod tests { + + /// CR 508.1d + CR 604.1: only a LIVE required-defender class is a growing-class + /// read. `Fixed`/`Permanent` are frozen ids and stay read-free; `Matching` + /// carries a `PlayerFilter` that + /// `combat::must_attack_defender_directives_for_creature` re-evaluates against + /// live player state at every declare-attackers step (Galactus), so a cached + /// analysis result can go stale and the scan must fail closed. + /// + /// The two halves are paired deliberately: the `false` arms are what make the + /// `true` arm meaningful, since a blanket `=> true` would also pass it. + #[test] + fn must_attack_defender_reads_growing_class_only_for_a_live_class() { + use crate::types::ability::PlayerFilter; + use crate::types::identifiers::{ObjectId, ObjectIncarnationRef}; + use crate::types::player::PlayerId; + use crate::types::statics::{RequiredDefender, StaticMode}; + + // Frozen snapshots — nothing is re-derived from the board. + assert!( + !static_mode_references_growing_class(&StaticMode::MustAttackDefender { + defender: RequiredDefender::Fixed { + player: PlayerId(1) + }, + }), + "a snapshotted player id reads nothing" + ); + assert!( + !static_mode_references_growing_class(&StaticMode::MustAttackDefender { + defender: RequiredDefender::Permanent { + permanent: ObjectIncarnationRef::of(ObjectId(7), 1), + }, + }), + "a snapshotted permanent pin reads nothing" + ); + + // A live class — re-evaluated against player state, so fail closed. + assert!( + static_mode_references_growing_class(&StaticMode::MustAttackDefender { + defender: RequiredDefender::Matching { + filter: PlayerFilter::Opponent, + }, + }), + "a live defender CLASS is re-evaluated against the board and must fail closed" + ); + } use super::*; use crate::game::game_object::GameObject; use crate::types::ability::TriggerDefinitionRef; diff --git a/crates/engine/src/game/effects/force_attack.rs b/crates/engine/src/game/effects/force_attack.rs index 9d12d4eacf..686f59c47c 100644 --- a/crates/engine/src/game/effects/force_attack.rs +++ b/crates/engine/src/game/effects/force_attack.rs @@ -9,17 +9,57 @@ use crate::types::game_state::GameState; use crate::types::identifiers::ObjectIncarnationRef; use crate::types::statics::{RequiredDefender, StaticMode}; -/// CR 506.3 + CR 611.2: Classify the `required_defender` filter and snapshot it -/// into the durable [`RequiredDefender`] combat enforcement reads. +/// CR 506.3: which KIND of defender a `required_defender` filter names. /// -/// A filter naming an OBJECT lowers to `Permanent` (pinned by incarnation — CR -/// 400.7, so a defender that leaves and re-enters does not inherit a requirement -/// aimed at the old object); every other filter is a player reference and lowers -/// to `Fixed` via the shared context-ref resolver. `SelfRef` is the only object -/// arm a printed card reaches today (Gideon Jura's "attack Gideon Jura if -/// able"), but the classification is by REFERENT KIND rather than by that one -/// filter, so a future "attacks target planeswalker if able" needs no new -/// branch. +/// CR 506.3's category is "a player, a planeswalker, or a battle", so this is the +/// discriminator the whole seam turns on. +enum DefenderReferent { + /// A permanent — lowers to `RequiredDefender::Permanent`. + Object, + /// A player — lowers to `RequiredDefender::Fixed`. + Player, +} + +/// CR 506.3: Classify a `required_defender` filter by its RESOLVED referent. +/// +/// Two filters are unconditionally objects by construction (`SelfRef` is the +/// ability's own source; `SpecificObject` names one). The inherited-target forms +/// are NOT: `ParentTarget` / `ParentTargetSlot` name whatever the parent clause +/// targeted, which may be a player — so they must be resolved before they are +/// classified. Deciding by filter VARIANT instead routed a player-valued parent +/// target down the object path, where `resolved_object_ids_for_filter` finds +/// nothing and the whole requirement is silently dropped. +/// +/// Everything else is a player reference, which is the conservative default: +/// every card using this effect before Gideon Jura named a player. +fn defender_referent(ability: &ResolvedAbility, filter: &TargetFilter) -> DefenderReferent { + let inherited = match filter { + TargetFilter::SelfRef | TargetFilter::SpecificObject { .. } => { + return DefenderReferent::Object + } + // CR 608.2c: the parent's chosen target — first slot, or the named one. + TargetFilter::ParentTarget => ability.targets.first(), + TargetFilter::ParentTargetSlot { index } => ability.targets.get(*index), + _ => return DefenderReferent::Player, + }; + match inherited { + Some(TargetRef::Object(_)) => DefenderReferent::Object, + // A player-valued parent target, or no target to inherit at all — read as + // a player, which `resolve_player_for_context_ref` handles. + Some(TargetRef::Player(_)) | None => DefenderReferent::Player, + } +} + +/// CR 506.3 + CR 611.2: Snapshot the `required_defender` filter into the durable +/// [`RequiredDefender`] combat enforcement reads. +/// +/// An OBJECT referent lowers to `Permanent`, pinned by incarnation (CR 400.7, so +/// a defender that leaves and re-enters does not inherit a requirement aimed at +/// the old object); a PLAYER referent lowers to `Fixed` via the shared +/// context-ref resolver. `SelfRef` is the only object form a printed card reaches +/// today (Gideon Jura's "attack Gideon Jura if able"), but the classification is +/// genuinely by referent kind — see [`defender_referent`] — so a future "attacks +/// target planeswalker if able" needs no new branch. /// /// Returns `None` when an object referent names no live object, so the caller /// grafts nothing rather than a requirement aimed at a vanished defender. @@ -28,32 +68,20 @@ fn snapshot_required_defender( ability: &ResolvedAbility, filter: &TargetFilter, ) -> Option { - if !filter_denotes_object(filter) { - return Some(RequiredDefender::Fixed { + match defender_referent(ability, filter) { + DefenderReferent::Player => Some(RequiredDefender::Fixed { player: resolve_player_for_context_ref(state, ability, filter), - }); + }), + DefenderReferent::Object => { + let defender_id = resolved_object_ids_for_filter(state, ability, filter) + .into_iter() + .next()?; + let obj = state.objects.get(&defender_id)?; + Some(RequiredDefender::Permanent { + permanent: ObjectIncarnationRef::from_object(obj), + }) + } } - let defender_id = resolved_object_ids_for_filter(state, ability, filter) - .into_iter() - .next()?; - let obj = state.objects.get(&defender_id)?; - Some(RequiredDefender::Permanent { - permanent: ObjectIncarnationRef::from_object(obj), - }) -} - -/// CR 506.3: whether `filter` denotes an OBJECT defender rather than a player. -/// An explicit allow-list, never a catch-all: a filter whose referent kind is -/// unclear keeps the pre-existing player reading, which is the conservative -/// direction — every card using this effect before Gideon Jura named a player. -fn filter_denotes_object(filter: &TargetFilter) -> bool { - matches!( - filter, - TargetFilter::SelfRef - | TargetFilter::SpecificObject { .. } - | TargetFilter::ParentTarget - | TargetFilter::ParentTargetSlot { .. } - ) } /// CR 611.2c + CR 115.1: how a force-attack subject must be installed. @@ -247,6 +275,86 @@ pub fn resolve( #[cfg(test)] mod tests { + + /// CR 506.3 + CR 608.2c: an inherited-target defender is classified by its + /// RESOLVED referent, not by the filter variant. + /// + /// `ParentTarget` names whatever the parent clause targeted. When that is a + /// PLAYER it must reach `RequiredDefender::Fixed`; classifying by variant sent + /// it down the object path, where `resolved_object_ids_for_filter` finds + /// nothing and the requirement is silently dropped entirely. + /// + /// The object half is the paired guard: it proves the object path still works + /// and that the player half is not passing merely because everything became a + /// player. + #[test] + fn parent_target_defender_is_classified_by_its_resolved_referent() { + fn snapshot_for(target: TargetRef) -> Option { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Coercer".to_string(), + Zone::Battlefield, + ); + let ability = ResolvedAbility::new( + Effect::ForceAttack { + target: TargetFilter::Any, + required_defender: TargetFilter::ParentTarget, + duration: Duration::UntilEndOfCombat, + scope: EffectScope::Single, + }, + vec![target], + source, + PlayerId(0), + ); + snapshot_required_defender(&state, &ability, &TargetFilter::ParentTarget) + } + + // A PLAYER-valued parent target lowers to `Fixed`. + assert_eq!( + snapshot_for(TargetRef::Player(PlayerId(1))), + Some(RequiredDefender::Fixed { + player: PlayerId(1) + }), + "a player-valued parent target is a PLAYER defender" + ); + + // An OBJECT-valued parent target lowers to `Permanent`, pinned by + // incarnation (CR 400.7). + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Coercer".to_string(), + Zone::Battlefield, + ); + let walker = create_object( + &mut state, + CardId(2), + PlayerId(1), + "Some Planeswalker".to_string(), + Zone::Battlefield, + ); + let ability = ResolvedAbility::new( + Effect::ForceAttack { + target: TargetFilter::Any, + required_defender: TargetFilter::ParentTarget, + duration: Duration::UntilEndOfCombat, + scope: EffectScope::Single, + }, + vec![TargetRef::Object(walker)], + source, + PlayerId(0), + ); + let snapshot = snapshot_required_defender(&state, &ability, &TargetFilter::ParentTarget); + let Some(RequiredDefender::Permanent { permanent }) = snapshot else { + panic!("an object-valued parent target is a PERMANENT defender, got {snapshot:?}"); + }; + assert_eq!(permanent.object_id, walker); + } use super::*; use crate::game::zones::create_object; use crate::types::ability::{ControllerRef, Duration, TargetRef, TypedFilter}; diff --git a/crates/engine/src/game/players.rs b/crates/engine/src/game/players.rs index e9ce1c5b6c..0f89d9ecb1 100644 --- a/crates/engine/src/game/players.rs +++ b/crates/engine/src/game/players.rs @@ -294,6 +294,14 @@ pub fn apnap_order_from( // compile error here rather than a silent fall-back to APNAP. let start_player = match starting_with { Some(ControllerRef::You) => controller, + // CR 101.4 + CR 109.4: a resolution-time snapshot names the anchor + // outright, so it anchors AT that id. Folding it into the default arm + // below would silently order from the active player whenever the + // snapshotted player is not the active one — the exact case the variant + // exists to represent. Unlike its dynamic siblings there is nothing to + // resolve and no context to be missing, so there is no reason to fail + // closed here. + Some(ControllerRef::SpecificPlayer { id }) => id, None | Some( ControllerRef::Opponent @@ -309,10 +317,7 @@ pub fn apnap_order_from( // CR 303.4b: Enchanted-player scope is not enumerable. Fail closed. | ControllerRef::EnchantedPlayer // CR 102.1: the active player is exactly this default anchor. - | ControllerRef::ActivePlayer - // CR 109.4 + CR 611.2: a snapshotted id is not a role this anchor - // enumerates; fall back to the default anchor. - | ControllerRef::SpecificPlayer { .. }, + | ControllerRef::ActivePlayer, ) => state.active_player, }; @@ -481,6 +486,39 @@ pub fn team_poison_total(state: &GameState, player: PlayerId) -> u32 { #[cfg(test)] mod tests { + + /// CR 101.4 + CR 109.4: a snapshotted anchor orders from ITS OWN player, not + /// from the active player. + /// + /// The fixture deliberately makes the snapshotted player NON-ACTIVE — that is + /// the only configuration in which the bug is visible, since folding + /// `SpecificPlayer` into the default arm returns the active player and a + /// same-player fixture would pass either way. + #[test] + fn specific_player_anchor_orders_from_the_snapshotted_player() { + let mut state = make_state(3, FormatConfig::free_for_all()); + state.active_player = PlayerId(0); + + let anchored = apnap_order_from( + &state, + Some(ControllerRef::SpecificPlayer { id: PlayerId(2) }), + PlayerId(0), + ); + assert_eq!( + anchored.first(), + Some(&PlayerId(2)), + "the snapshotted (non-active) player anchors the order" + ); + + // Paired guard: the default anchor really is the active player, so the + // assertion above is not passing for an unrelated reason. + let default_anchored = apnap_order_from(&state, None, PlayerId(0)); + assert_eq!( + default_anchored.first(), + Some(&PlayerId(0)), + "with no anchor the order still starts at the active player" + ); + } use super::*; use crate::types::format::FormatConfig; From 5cea596a296efe1778affaa2b325797fb4bab59d Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:49:09 -0500 Subject: [PATCH 5/6] Match SpecificPlayer when targeting a stack ability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stack_entry_controller_matches` admitted only `You`/`Opponent` and sent every other `ControllerRef` through a `_ => false` wildcard. Adding `SpecificPlayer` therefore gave any stack-ability filter carrying a resolution-time snapshot NO legal target at all — a supported controller scope silently failing target legality for the whole class. Three changes, per the review: * `SpecificPlayer` compares the snapshotted id directly. It is already concrete, so it needs none of the ability/event context this function lacks. * The match is now EXHAUSTIVE with no `_`, so a future `ControllerRef` variant fails to compile here instead of silently joining the fail-closed tail. The remaining scopes stay fail-closed but are named individually, making the claim per-variant rather than a blanket wildcard — the wildcard is precisely what let this through. * Regression covering a matching snapshot, a NON-matching snapshot (so the match is an id comparison rather than a blanket true), and an `Opponent` control proving the pre-existing scopes are unaffected. Noting for the record that CodeRabbit raised this same site in the first round and it was skipped as a speculative behavior change pending a test. That was wrong: the wildcard turned a new variant into a live bug immediately, and the test was cheap. Verified with the exact CI command: `cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings` clean; engine 18988 lib + 4902 integration + 30 bin; phase-ai 2110; zero failures. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/targeting.rs | 94 ++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index 0683b80aa4..12b0720b57 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -1971,10 +1971,35 @@ fn stack_entry_controller_matches( return true; }; let is_you = entry.controller == source_controller; + // CR 109.4: EXHAUSTIVE, no `_` — a new `ControllerRef` variant must fail to + // compile here rather than silently join the fail-closed tail. The prior + // wildcard swallowed every variant beyond the two below, which is how + // `SpecificPlayer` came to make a supported controller scope match NOTHING + // for the stack-ability class. match controller { ControllerRef::You => is_you, ControllerRef::Opponent => !is_you, - _ => false, + // CR 109.4 + CR 611.2: a resolution-time snapshot is already a concrete + // id, so it needs none of the ability/event context this function lacks — + // compare it directly. + ControllerRef::SpecificPlayer { id } => entry.controller == *id, + // Every remaining scope needs context this function does not receive (no + // `GameState`, no resolving `ResolvedAbility`, no triggering event), so + // they stay fail-closed — but named, so the claim is per-variant rather + // than a blanket wildcard. + ControllerRef::ScopedPlayer + | ControllerRef::TargetPlayer + | ControllerRef::TargetOpponent + | ControllerRef::ParentTargetController + | ControllerRef::ParentTargetOwner + | ControllerRef::DefendingPlayer + | ControllerRef::SourceChosenPlayer + | ControllerRef::ChosenPlayer { .. } + | ControllerRef::TriggeringPlayer + | ControllerRef::EnchantedPlayer + // CR 102.1: resolvable in principle, but only from `GameState`, which is + // not threaded here. + | ControllerRef::ActivePlayer => false, } } @@ -2561,6 +2586,73 @@ pub(crate) fn resolve_tracked_set_sentinel( #[cfg(test)] mod tests { + + /// CR 109.4 + CR 611.2 + CR 115.1: a `SpecificPlayer` controller scope matches + /// a stack ability by comparing the snapshotted id directly. + /// + /// `stack_entry_controller_matches` previously admitted only `You`/`Opponent` + /// and swallowed everything else through a `_ => false` wildcard, so a filter + /// carrying a resolution-time snapshot silently had NO legal target for the + /// whole stack-ability class. + /// + /// Three arms deliberately: the snapshot that matches, the snapshot that does + /// not, and a `You` control — the negative is what proves the match is an id + /// comparison rather than a blanket `true`, and the `You` arm proves the + /// pre-existing scopes still work. + #[test] + fn stack_ability_controller_matches_a_specific_player_snapshot() { + let controller = PlayerId(1); + let other = PlayerId(0); + let entry = StackEntry { + id: ObjectId(9), + source_id: ObjectId(9), + controller, + kind: StackEntryKind::ActivatedAbility { + source_id: ObjectId(9), + ability: Box::new(ResolvedAbility::new( + crate::types::ability::Effect::Draw { + target: TargetFilter::Controller, + count: QuantityExpr::Fixed { value: 1 }, + }, + vec![], + ObjectId(9), + controller, + )), + }, + }; + + let filter = |id: PlayerId| TargetFilter::StackAbility { + controller: Some(ControllerRef::SpecificPlayer { id }), + tag: None, + kind: None, + }; + + // The snapshot naming this entry's controller matches... + assert!( + stack_ability_matches_filter(&entry, &filter(controller), other), + "a snapshot naming the entry's controller matches" + ); + // ...and one naming anybody else does not. + assert!( + !stack_ability_matches_filter(&entry, &filter(other), other), + "a snapshot naming a different player does not match" + ); + // Reach guard: the pre-existing scopes are unaffected. The entry is + // controlled by P1 while the source controller is P0, so this is + // "an opponent controls it". + assert!( + stack_ability_matches_filter( + &entry, + &TargetFilter::StackAbility { + controller: Some(ControllerRef::Opponent), + tag: None, + kind: None, + }, + other, + ), + "the Opponent scope still matches" + ); + } use super::*; use crate::game::game_object::AttachTarget; use crate::game::zones::create_object; From 85a26c116f4b64cbfff9e24149f5f2a5fe2c72ae Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:25:44 -0500 Subject: [PATCH 6/6] Remove unsupported CR annotations; fix Destroy citation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CR numbers added to `stack_entry_controller_matches` did not support the claims attached to them. CR 109.4 defines which objects have a controller; CR 611.2 covers continuous effects generated by resolution; CR 115.1 defines targets. None of them mandates exhaustive Rust matching or establishes a stored-ID comparison contract, so citing them asserted a Comprehensive Rules verification that had not happened. Both are engine contracts rather than rules behavior, and CLAUDE.md is explicit that only code implementing a game rule should carry an annotation — a wrong number is worse than none because it manufactures false confidence. They now read as plain `ENGINE CONTRACT` notes: `SpecificPlayer` already carries the stored player id, and this predicate compares it with the stack entry's stored controller. The exhaustive match and its regression are unchanged. CR 102.1 is kept on the `ActivePlayer` arm — it genuinely defines "the active player is the player whose turn it is" — but rephrased so the citation identifies the CONCEPT rather than appearing to justify the fail-closed behavior, which is an engine constraint (no `GameState` here). Auditing every CR number this PR adds, rather than only the flagged two, turned up one more: the `-2` clause cited CR 701.7, which is Create. Destroy is CR 701.8. Corrected — exactly the 701.x hazard CLAUDE.md warns about, since those numbers are arbitrary sequential assignments. The remainder verified against docs/MagicCompRules.txt and left as-is: CR 101.4 is the APNAP rule, CR 601.2c is target announcement, CR 704.5i is zero loyalty, CR 201.5 is the printed-name self-reference, CR 506.3 is the defender category. CR 104.4b nearby is pre-existing loop-detection text in which only the renamed variant changed. Comment-only; no executable line altered. Verified with the exact CI command: `cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings` clean; engine 18988 lib + 4902 integration + 30 bin; phase-ai 2110; zero failures. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/targeting.rs | 28 ++++++++++++++---------- crates/engine/src/parser/oracle_tests.rs | 2 +- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index 12b0720b57..d1fa0f96d9 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -1971,17 +1971,18 @@ fn stack_entry_controller_matches( return true; }; let is_you = entry.controller == source_controller; - // CR 109.4: EXHAUSTIVE, no `_` — a new `ControllerRef` variant must fail to - // compile here rather than silently join the fail-closed tail. The prior - // wildcard swallowed every variant beyond the two below, which is how - // `SpecificPlayer` came to make a supported controller scope match NOTHING - // for the stack-ability class. + // ENGINE CONTRACT (not a rules requirement): EXHAUSTIVE, no `_`, so a new + // `ControllerRef` variant fails to compile here rather than silently joining + // the fail-closed tail. The prior wildcard swallowed every variant beyond the + // two below, which is how `SpecificPlayer` came to make a supported + // controller scope match NOTHING for the stack-ability class. match controller { ControllerRef::You => is_you, ControllerRef::Opponent => !is_you, - // CR 109.4 + CR 611.2: a resolution-time snapshot is already a concrete - // id, so it needs none of the ability/event context this function lacks — - // compare it directly. + // ENGINE CONTRACT: `SpecificPlayer` already carries the stored player id, + // so this predicate compares it with the stack entry's stored controller. + // No rules lookup is involved — unlike its siblings it needs none of the + // ability/event context this function lacks. ControllerRef::SpecificPlayer { id } => entry.controller == *id, // Every remaining scope needs context this function does not receive (no // `GameState`, no resolving `ResolvedAbility`, no triggering event), so @@ -1997,8 +1998,10 @@ fn stack_entry_controller_matches( | ControllerRef::ChosenPlayer { .. } | ControllerRef::TriggeringPlayer | ControllerRef::EnchantedPlayer - // CR 102.1: resolvable in principle, but only from `GameState`, which is - // not threaded here. + // The active player (CR 102.1: "the player whose turn it is") is + // resolvable in principle, but only from `GameState`, which this function + // is not given — so it fails closed with the rest for that engine reason, + // not a rules one. | ControllerRef::ActivePlayer => false, } } @@ -2587,8 +2590,9 @@ pub(crate) fn resolve_tracked_set_sentinel( #[cfg(test)] mod tests { - /// CR 109.4 + CR 611.2 + CR 115.1: a `SpecificPlayer` controller scope matches - /// a stack ability by comparing the snapshotted id directly. + /// A `SpecificPlayer` controller scope matches a stack ability by comparing + /// the stored player id with the stack entry's stored controller. This is an + /// engine contract, not a rules behavior, so it carries no CR annotation. /// /// `stack_entry_controller_matches` previously admitted only `You`/`Opponent` /// and swallowed everything else through a `_ => false` wildcard, so a filter diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index f4e3655279..2ab90536c0 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -1758,7 +1758,7 @@ fn gideon_jura_full_parse() { "the window spans the targeted opponent's entire next turn" ); - // --- −2 (CR 701.7) --------------------------------------------------- + // --- −2 (CR 701.8: Destroy) ------------------------------------------ let Effect::Destroy { target, .. } = &*r.abilities[1].effect else { panic!("the −2 destroys, got {:?}", r.abilities[1].effect); };