diff --git a/client/src/adapter/types.ts b/client/src/adapter/types.ts index 1eb37bf59d..49d9ae78a2 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 ee7ef04ccf..82aed89213 100644 --- a/crates/engine/src/ai_support/candidates.rs +++ b/crates/engine/src/ai_support/candidates.rs @@ -6327,7 +6327,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 @@ -6364,8 +6364,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); @@ -6398,7 +6398,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/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 73b5bd5f69..07ebbec826 100644 --- a/crates/engine/src/analysis/resource.rs +++ b/crates/engine/src/analysis/resource.rs @@ -4059,6 +4059,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 @@ -4090,7 +4112,6 @@ fn static_mode_references_growing_class(mode: &crate::types::statics::StaticMode | StaticMode::CantLoseLife | StaticMode::PlayerProtection(..) | StaticMode::MustAttack - | StaticMode::MustAttackPlayer { .. } | StaticMode::MustBlock | StaticMode::MustBlockAttacker { .. } | StaticMode::CantDraw { .. } @@ -5366,6 +5387,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/database/encore_tests.rs b/crates/engine/src/database/encore_tests.rs index 6bbae9dac5..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() @@ -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) )), @@ -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 { @@ -308,14 +308,14 @@ 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, }) }) - .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/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 4c33dff067..f7d5abed57 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -2219,6 +2219,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, } } @@ -2570,6 +2573,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, } } @@ -3261,11 +3267,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) } @@ -5479,8 +5488,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); @@ -6620,6 +6632,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(), } } @@ -6645,6 +6660,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 912d6b2e72..4af049cfec 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -1203,12 +1203,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 } @@ -4204,6 +4208,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, } } @@ -4238,6 +4245,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 89f272b282..fc3d7ee4c3 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -3437,7 +3437,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, } } @@ -3985,6 +3994,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 737e51ae5f..089115d0c9 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). @@ -3180,121 +3221,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 @@ -3311,7 +3384,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| { @@ -3319,22 +3392,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, @@ -3362,7 +3466,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 @@ -3376,22 +3480,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 { @@ -3425,10 +3533,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. @@ -3720,40 +3828,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 ≠ @@ -4045,8 +4157,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 { @@ -4090,28 +4209,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 { @@ -4121,17 +4244,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, }); } } @@ -4293,15 +4416,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)), @@ -4801,7 +4931,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 { @@ -5145,14 +5275,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 @@ -5390,7 +5520,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(); @@ -5408,29 +5541,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 @@ -5438,12 +5571,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( @@ -6845,25 +6978,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, @@ -7007,7 +7140,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![], @@ -7017,7 +7150,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. @@ -7027,11 +7160,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, @@ -12661,10 +12794,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 @@ -12682,11 +12815,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), ); @@ -12713,20 +12846,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" @@ -12739,7 +12872,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] @@ -12758,16 +12891,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 @@ -12778,16 +12911,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)), @@ -12795,9 +12928,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); @@ -12825,8 +12961,8 @@ 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` / - /// `MustAttackPlayer` requirement split → `score_single` / `max_no_payment`. + /// `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() { // The exact production most-life filter (no hand-built AST): reuse the parser @@ -12848,8 +12984,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(), }, }) @@ -12858,8 +12994,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), }, }) @@ -12870,8 +13006,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" ); @@ -13026,7 +13165,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![], }, ); @@ -13277,7 +13416,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; @@ -13289,12 +13428,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, @@ -13398,11 +13537,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()); @@ -13418,8 +13557,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 2b4248e04c..557e513fdc 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -153,10 +153,10 @@ 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::MustAttackPlayer { .. } + | StaticMode::MustAttackDefender { .. } // CR 509.1b: CantBeBlockedByMoreThan carries the blocker maximum // (Stalking Tiger). Enforced in combat.rs declare-blockers validation. | StaticMode::CantBeBlockedByMoreThan { .. } @@ -891,6 +891,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!( @@ -1063,6 +1065,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 { @@ -1138,6 +1142,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() } @@ -1268,6 +1274,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)) @@ -2427,7 +2435,6 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { | Effect::Connive { target, .. } | Effect::PhaseOut { target } | Effect::PhaseIn { target } - | Effect::ForceAttack { 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 { @@ -2456,6 +2463,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 { .. } => {} 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..c38e750d3e 100644 --- a/crates/engine/src/game/effects/encore.rs +++ b/crates/engine/src/game/effects/encore.rs @@ -23,10 +23,10 @@ //! - **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_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 @@ -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( @@ -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..686f59c47c 100644 --- a/crates/engine/src/game/effects/force_attack.rs +++ b/crates/engine/src/game/effects/force_attack.rs @@ -1,14 +1,191 @@ 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: which KIND of defender a `required_defender` filter names. +/// +/// 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. +fn snapshot_required_defender( + state: &GameState, + ability: &ResolvedAbility, + filter: &TargetFilter, +) -> Option { + 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), + }) + } + } +} + +/// CR 611.2c + CR 115.1: how a force-attack subject must be installed. +/// +/// 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 +/// 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, +) -> 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 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 SubjectLowering::Unlowerable; + }; + 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 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 }); + } + SubjectLowering::Population(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 +193,76 @@ 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. + match lower_dynamic_affected(ability, target, *scope) { + SubjectLowering::Population(affected) => state.add_transient_continuous_effect( + ability.source_id, + ability.controller, + duration.clone(), + affected, + vec![ContinuousModification::AddStaticMode { + mode: StaticMode::MustAttackDefender { defender }, + }], + None, + ), + 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, + ); + } + 0 + } + // CR 611.2c: install NOTHING rather than a frozen per-object graft. + // See `SubjectLowering::Unlowerable`. + SubjectLowering::Unlowerable => 0, + }; } events.push(GameEvent::EffectResolved { @@ -54,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}; @@ -70,8 +371,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 +415,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 +444,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 +469,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 7da525dd54..9194caaa66 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -5480,7 +5480,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, @@ -5494,7 +5494,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 { @@ -13324,6 +13324,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..f452c772d3 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 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 + // 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 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()` + // 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 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. + /// + /// 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 1c08c07fc3..9d9355dac0 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 2f638e1b70..daa5e36d6b 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1490,6 +1490,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`, or any filter nested anywhere inside it, satisfies `leaf`. @@ -2003,6 +2008,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, } } @@ -3107,6 +3114,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 @@ -4006,6 +4023,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; + } + } } } @@ -5514,6 +5543,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, }; @@ -5573,6 +5607,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 @@ -6322,6 +6359,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. @@ -6689,6 +6729,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, } } @@ -7351,6 +7394,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 e47d22b9c1..7ba081be64 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; } } @@ -6804,8 +6813,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/… @@ -6814,7 +6823,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 @@ -8212,7 +8221,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 @@ -8592,6 +8601,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/players.rs b/crates/engine/src/game/players.rs index f2e4906eaa..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 @@ -478,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; diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index 27360103fc..cce10b5422 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -4717,6 +4717,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(), ) @@ -4783,6 +4786,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, } } @@ -6325,6 +6331,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" + ) + } } } @@ -6423,6 +6434,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 64b0dddb91..b8d41f1855 100644 --- a/crates/engine/src/game/replacement.rs +++ b/crates/engine/src/game/replacement.rs @@ -5550,6 +5550,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, } } @@ -5688,6 +5691,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 { @@ -5738,6 +5745,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 { @@ -5919,6 +5930,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!( @@ -6143,6 +6155,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; @@ -6557,6 +6570,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 c17895c3da..740dea1a33 100644 --- a/crates/engine/src/game/sba.rs +++ b/crates/engine/src/game/sba.rs @@ -648,6 +648,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 ddb67e3acf..4c2a2733fd 100644 --- a/crates/engine/src/game/scenario.rs +++ b/crates/engine/src/game/scenario.rs @@ -703,6 +703,60 @@ 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; + // 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); + + 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 ed2f026d14..0706ca9f66 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. @@ -1995,6 +1995,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 47ec684d6c..e0b3c31592 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 { @@ -1993,10 +1995,38 @@ fn stack_entry_controller_matches( return true; }; let is_you = entry.controller == source_controller; + // 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, - _ => false, + // 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 + // 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 + // 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, } } @@ -2583,6 +2613,74 @@ pub(crate) fn resolve_tracked_set_sentinel( #[cfg(test)] mod tests { + + /// 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 + /// 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; diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index e1c4710543..a6588d06ce 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -2698,6 +2698,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: @@ -6004,12 +6011,12 @@ fn attach_neuter_recipient_resolves_via_subject(ctx: &ParseContext) -> bool { } } +/// 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. 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<'_, ()> { @@ -6207,6 +6214,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 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 +/// `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 — @@ -6230,6 +6247,12 @@ pub(super) fn resolve_prevent_recipient( recipient: TextPair<'_>, parent_target_available: bool, ) -> Option { + // 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) + { + return Some(filter); + } if let Some((filter, _)) = parse_anaphoric_target_ref(recipient.original, parent_target_available) { @@ -12906,11 +12929,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 { @@ -13742,11 +13774,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('.'); @@ -13791,7 +13828,7 @@ pub(super) fn try_parse_attack_if_able(lower: &str) -> Option>> = ( + let targeted: DefenderBoundAttackParse<'_> = ( alt((tag("attacks"), tag("attack"))), preceded( tag(" "), @@ -13804,6 +13841,12 @@ pub(super) fn try_parse_attack_if_able(lower: &str) -> Option Option 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 @@ -19430,10 +19480,10 @@ mod tests { match result.unwrap() { ImperativeFamilyAst::ForceAttack { duration, - required_player, + required_defender, } => { - 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:?}"), } @@ -19449,13 +19499,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:?}"), @@ -19503,18 +19553,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 10c58150fa..cbef032e01 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -26238,7 +26238,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 @@ -26566,7 +26566,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 @@ -26585,7 +26585,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()) @@ -29192,6 +29192,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 3d1f847764..5b7b6eac31 100644 --- a/crates/engine/src/parser/oracle_effect/subject.rs +++ b/crates/engine/src/parser/oracle_effect/subject.rs @@ -17,8 +17,8 @@ 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, ObjectScope, - PlayerFilter, PlayerScope, PtValue, QuantityExpr, QuantityRef, StaticCondition, + ControllerRef, Duration, EachDamageRecipient, Effect, EffectScope, FilterProp, MultiTargetSpec, + ObjectScope, PlayerFilter, PlayerScope, PtValue, QuantityExpr, QuantityRef, StaticCondition, StaticDefinition, TargetFilter, TypedFilter, }; use crate::types::game_state::DayNight; @@ -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) { @@ -2597,7 +2657,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 bc6cfe85d9..ed31e92ebb 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -21918,8 +21918,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 996113964c..6838a0c517 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -673,15 +673,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..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 / @@ -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 0a3caef681..21e159e072 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -9053,8 +9053,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(), }, }, @@ -9082,8 +9082,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(), }, }, @@ -9100,8 +9100,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, }, }, @@ -9172,7 +9172,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 d052b48d4c..a8af054692 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -148,6 +148,62 @@ pub(crate) fn parse_anaphoric_target_ref( matches!(filter, TargetFilter::ParentTarget).then_some((filter, rest)) } +/// 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 +/// 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 6229134fe7..e83c4c068c 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -1703,6 +1703,224 @@ 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, + scope, + .. + } = &*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" + ); + // 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:?}"); + }; + 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.8: Destroy) ------------------------------------------ + 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 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 +/// (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 9b7f4e2fcc..92c9e78203 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. @@ -5677,6 +5699,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 @@ -12626,14 +12670,43 @@ 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. + /// + /// `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`: 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, - #[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, @@ -15413,7 +15486,6 @@ impl Effect { | Effect::PhaseOut { target, .. } | Effect::PhaseIn { target, .. } | Effect::ForceBlock { target, .. } - | Effect::ForceAttack { target, .. } | Effect::BecomePrepared { target, .. } | Effect::BecomeUnprepared { target, .. } | Effect::BecomeSaddled { target, .. } @@ -15637,6 +15709,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 @@ -22642,7 +22731,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 52348a570b..c66be5d05d 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -27137,7 +27137,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 @@ -27160,8 +27160,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)), @@ -27175,8 +27175,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..b8c460d174 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 } => { @@ -4595,22 +4638,32 @@ 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::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, }, }, + // 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(); @@ -4627,8 +4680,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..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 @@ -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 4558fad799..1dcf2b56d7 100644 --- a/crates/engine/tests/integration/deterministic_game_state_serde.rs +++ b/crates/engine/tests/integration/deterministic_game_state_serde.rs @@ -1913,7 +1913,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..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() { @@ -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..888a33324b --- /dev/null +++ b/crates/engine/tests/integration/gideon_jura_forced_attack_planeswalker.rs @@ -0,0 +1,382 @@ +//! 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 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::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()); + 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 fda6125963..4b9c26e3e9 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -260,6 +260,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..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 @@ -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}; @@ -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)`. @@ -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, @@ -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)` @@ -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)" @@ -233,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() { @@ -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..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] @@ -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() }; @@ -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). /// @@ -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/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(), + }), } } diff --git a/crates/phase-ai/src/combat_ai.rs b/crates/phase-ai/src/combat_ai.rs index d096544642..316b989c9e 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..f7c5859ab7 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() }; @@ -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" + ); +}