diff --git a/crates/engine/src/analysis/ability_graph.rs b/crates/engine/src/analysis/ability_graph.rs index 6c1f8eceaa..097c3f3b69 100644 --- a/crates/engine/src/analysis/ability_graph.rs +++ b/crates/engine/src/analysis/ability_graph.rs @@ -868,7 +868,7 @@ fn effect_projection(effect: &Effect) -> Projection { | Effect::ExploreAll { .. } | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Populate | Effect::Clash diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 80f74be192..30f52edddf 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -1862,7 +1862,10 @@ fn legacy_trigger_condition(x: &TriggerCondition) -> bool { | TriggerCondition::AttackedThisTurn | TriggerCondition::FirstCombatPhaseOfTurn | TriggerCondition::HasMaxSpeed - | TriggerCondition::IsMonarch + // CR 725.1: no `legacy_player_scope` classifier exists, and both scopes + // the parser can emit (`Controller`, `ScopedPlayer`) have non-legacy + // `ControllerRef` analogues, so the monarch subject axis stays here. + | TriggerCondition::IsMonarch { .. } | TriggerCondition::IsInitiative | TriggerCondition::NoMonarch | TriggerCondition::HasCityBlessing @@ -1998,7 +2001,9 @@ fn legacy_static_condition(x: &StaticCondition) -> bool { | StaticCondition::DayNightIs { .. } | StaticCondition::CastVariantPaid { .. } | StaticCondition::ClassLevelGE { .. } - | StaticCondition::IsMonarch + // CR 725.1: no `legacy_player_scope` classifier exists; see the + // `legacy_trigger_condition` sibling arm for the same reasoning. + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -2901,6 +2906,8 @@ fn legacy_effect(x: &Effect) -> bool { | Effect::ReassembleContraptionOnSprocket { target, .. } | Effect::ApplySticker { target, .. } | Effect::RememberCard { target } + // CR 725.1 + CR 115.1: "target opponent becomes the monarch". + | Effect::BecomeMonarch { target } | Effect::GrantCastingPermission { target, .. } | Effect::AddTargetReplacement { target, .. } | Effect::DiscardCard { target, .. } @@ -3440,7 +3447,6 @@ fn legacy_effect(x: &Effect) -> bool { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch | Effect::NoOp | Effect::Proliferate | Effect::Populate @@ -5614,12 +5620,19 @@ fn rw_effect( min: _, max: _, } - | Effect::BecomeMonarch | Effect::RingTemptsYou | Effect::TimeTravel | Effect::Planeswalk | Effect::VentureIntoDungeon | Effect::SolveCase => (ext_write(StateKind::Other), None), + // CR 725.1 + CR 725.3: the designation write, plus the chosen-target + // write axis — "target opponent becomes the monarch" writes to a player + // named by a CR 115.1 target slot, exactly like `Effect::ExtraTurn`. + Effect::BecomeMonarch { target } => { + let mut p = ext_write(StateKind::Other); + flag_legacy_write_target(&mut p, target); + (p, None) + } Effect::ForceAttack { target, required_defender: _, @@ -6373,6 +6386,10 @@ fn rw_ability_condition(x: &AbilityCondition) -> RwProfile { fn rw_trigger_condition(x: &TriggerCondition) -> RwProfile { match x { + // CR 725.1: the monarch designation itself is global state with no + // member/event binding; the read profile is entirely determined by the + // subject scope, classified through the shared `PlayerScope` walker. + TriggerCondition::IsMonarch { player } => rw_player_scope(player), TriggerCondition::GainedLife { minimum: _ } | TriggerCondition::LostLife | TriggerCondition::LostLifeLastTurn => reads_player_of(StateKind::JournalLife), @@ -6473,7 +6490,6 @@ fn rw_trigger_condition(x: &TriggerCondition) -> RwProfile { | TriggerCondition::AttackedThisTurn | TriggerCondition::FirstCombatPhaseOfTurn | TriggerCondition::HasMaxSpeed - | TriggerCondition::IsMonarch | TriggerCondition::IsInitiative | TriggerCondition::NoMonarch | TriggerCondition::HasCityBlessing @@ -6495,6 +6511,9 @@ fn rw_trigger_condition(x: &TriggerCondition) -> RwProfile { fn rw_static_condition(x: &StaticCondition) -> RwProfile { match x { + // CR 725.1: see the `rw_trigger_condition` sibling — the read profile is + // entirely determined by the monarch subject scope. + StaticCondition::IsMonarch { player } => rw_player_scope(player), StaticCondition::DevotionGE { .. } | StaticCondition::SharesColorWithMostCommonColorAmongPermanents => reads_zone_membership(), StaticCondition::IsPresent { filter } => match filter { @@ -6574,7 +6593,6 @@ fn rw_static_condition(x: &StaticCondition) -> RwProfile { | StaticCondition::DayNightIs { .. } | StaticCondition::CastVariantPaid { .. } | StaticCondition::ClassLevelGE { .. } - | StaticCondition::IsMonarch | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 6c83730be9..02c3f37e3b 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -814,7 +814,10 @@ fn scan_effect(x: &Effect, mode: ScanMode) -> Axes { Effect::Investigate => Axes::NONE, Effect::Tribute { count: _ } => Axes::NONE, Effect::TimeTravel => Axes::NONE, - Effect::BecomeMonarch => Axes::NONE, + // CR 725.1 + CR 115.1: the designation subject is a target filter, + // walked through the same single authority every other targeted effect + // uses. + Effect::BecomeMonarch { target } => scan_target_filter(target, target_ctx, mode), Effect::NoOp => Axes::NONE, // Captured at activation time; no resolution-time dynamic read. Effect::NoteManaSpent => Axes::NONE, @@ -3355,7 +3358,11 @@ fn scan_trigger_condition(x: &TriggerCondition, mode: ScanMode) -> Axes { acc } TriggerCondition::HasMaxSpeed => Axes::NONE, - TriggerCondition::IsMonarch => Axes::NONE, + // CR 725.1: the monarch predicate itself reads no axis; its subject + // scope is classified per-axis by the shared `PlayerScope` classifier, + // mirroring `WasStartingPlayer { controller }`'s delegation to + // `scan_controller_ref`. + TriggerCondition::IsMonarch { player } => scan_player_scope(player), TriggerCondition::IsInitiative => Axes::NONE, TriggerCondition::NoMonarch => Axes::NONE, TriggerCondition::WasStartingPlayer { controller, .. } => { @@ -3659,7 +3666,9 @@ fn scan_static_condition(x: &StaticCondition, mode: ScanMode) -> Axes { StaticCondition::SourceIsAttacking => Axes::NONE, StaticCondition::SourceIsBlocking => Axes::NONE, StaticCondition::SourceIsBlocked => Axes::NONE, - StaticCondition::IsMonarch => Axes::NONE, + // CR 725.1: see the `TriggerCondition::IsMonarch` arm above — the + // subject scope is classified through the shared `PlayerScope` walker. + StaticCondition::IsMonarch { player } => scan_player_scope(player), StaticCondition::IsInitiative => Axes::NONE, StaticCondition::NoMonarch => Axes::NONE, StaticCondition::HasCityBlessing => Axes::NONE, @@ -5505,7 +5514,7 @@ fn effect_target_ctx(e: &Effect, mode: ScanMode) -> FilterReadContext { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::NoteManaSpent | Effect::Proliferate @@ -5913,7 +5922,7 @@ fn effect_census_role(e: &Effect) -> CensusRole { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::NoteManaSpent | Effect::Proliferate @@ -6150,7 +6159,7 @@ pub(crate) fn effect_is_randomness_bearing(e: &Effect) -> bool { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::NoteManaSpent | Effect::Proliferate diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index 9209854094..36e5e22e8c 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -1249,25 +1249,52 @@ fn per_defender_caps(state: &GameState) -> Vec<(PlayerId, u32)> { .collect() } -/// CR 508.5 + CR 310.9d: Resolve the defending player for an `AttackTarget` — -/// the player for a direct attack, a planeswalker's controller, or a battle's -/// protector. -fn defending_player_for_target(state: &GameState, target: AttackTarget) -> PlayerId { +/// CR 508.5 + CR 310.8d: Resolve the defending player for an `AttackTarget` — +/// the player for a direct attack, the CONTROLLER of the planeswalker being +/// attacked, or the PROTECTOR of the battle being attacked. CR 310.8d is +/// explicit that when a battle's protector differs from its controller, every +/// rule and effect referring to the "defending player" relative to that battle +/// means the protector. +/// +/// `fallback` answers only when the target object is missing from `state` +/// (destroyed planeswalker, battle with no protector). Each caller supplies the +/// value it would otherwise have used. +/// +/// Single authority: this replaces the former +/// `trigger_matchers::attack_target_defending_player`, which was the same +/// `match` with a caller-supplied fallback. One `AttackTarget` → player rule, +/// one home, next to `AttackTarget` itself. +/// +/// (This corrects a pre-existing citation on this function and at +/// `apply_attack_declarations`, both of which pointed at a `310.9d` subrule +/// that does not exist: CR 310.9 is the battle-attachment state-based action +/// and has no lettered subrules. Verified absent from `docs/MagicCompRules.txt`.) +pub(crate) fn defending_player_for_target_or( + state: &GameState, + target: AttackTarget, + fallback: PlayerId, +) -> PlayerId { match target { AttackTarget::Player(pid) => pid, AttackTarget::Planeswalker(pw_id) => state .objects .get(&pw_id) .map(|pw| pw.controller) - .unwrap_or(PlayerId(0)), + .unwrap_or(fallback), AttackTarget::Battle(battle_id) => state .objects .get(&battle_id) .and_then(|b| b.protector()) - .unwrap_or(PlayerId(0)), + .unwrap_or(fallback), } } +/// CR 508.5 + CR 310.8d: [`defending_player_for_target_or`] with the historical +/// `PlayerId(0)` fallback used by attack-declaration bookkeeping. +fn defending_player_for_target(state: &GameState, target: AttackTarget) -> PlayerId { + defending_player_for_target_or(state, target, PlayerId(0)) +} + /// Iterate every battlefield `StaticDefinition` whose mode is a block-restriction /// (`CantBeBlocked`, `CantBeBlockedExceptBy`, or `CantBeBlockedBy`) AND whose /// `affected` filter matches `attacker_id`. Yields `(source, def)` pairs so @@ -3920,7 +3947,7 @@ fn attacker_can_attack_target( gates: &CombatStaticGates, active_team: &[PlayerId], ) -> bool { - // CR 508.1b + CR 310.5/310.9b: target validity + active-team exclusion. + // CR 508.1b + CR 310.5/310.8b: target validity + active-team exclusion. match target { AttackTarget::Player(pid) => { if !state.players.iter().any(|p| p.id == pid) @@ -5005,7 +5032,7 @@ pub(super) fn commit_attack_declaration( let mut attackers: Vec = attacks .iter() .map(|(object_id, target)| { - // CR 508.5 + CR 310.9d: Defending player for a battle = its protector, + // CR 508.5 + CR 310.8d: Defending player for a battle = its protector, // not its controller. For planeswalkers, defending player = controller. let defending_player = defending_player_for_target(state, *target); AttackerInfo::new(*object_id, *target, defending_player) @@ -6346,6 +6373,195 @@ pub fn resolve_defending_player(state: &GameState, source_id: ObjectId) -> Optio }) } +/// Which attack event, if any, a "defending player" reference is BOUND to. +/// +/// Constructed ONLY by [`defending_player_cr508_5`] — no caller builds one. +/// That is deliberate: when each door selected its own binding, the quantity +/// door and the filter door disagreed in exactly the state this authority +/// exists to make coherent (one anaphor, two players). +enum DefenderBinding<'a> { + /// The bound attack event's per-attacker entries and its declared global + /// defending player (CR 508.1b). + TriggerEvent { + entries: &'a [(ObjectId, AttackTarget)], + global: PlayerId, + }, + None, +} + +/// Destructure an `AttackersDeclared` into its per-attacker entries and its +/// declared global defending player. `None` for any other event. +fn attack_entries(event: &GameEvent) -> Option<(&[(ObjectId, AttackTarget)], PlayerId)> { + match event { + GameEvent::AttackersDeclared { + defending_player, + attacks, + .. + } => Some((attacks.as_slice(), *defending_player)), + _ => None, + } +} + +/// CR 508.5 first clause: the ASKER's own entry in the bound event. +/// +/// Uses `find` (not `find_map`) deliberately: the pre-existing `find_map` +/// closure returned `None` from INSIDE the map for planeswalker and battle +/// targets, which `find_map` cannot distinguish from "this entry is not the +/// asker" — so it skipped past the asker's own entry and fell through to the +/// coarse global field. Resolving the matched entry through +/// [`defending_player_for_target_or`] answers with the planeswalker's +/// controller or the battle's protector (CR 310.8d) instead. +fn entry_defender( + state: &GameState, + entries: &[(ObjectId, AttackTarget)], + global: PlayerId, + entry_id: ObjectId, +) -> Option { + entries + .iter() + .find(|(attacker_id, _)| *attacker_id == entry_id) + .map(|(_, target)| defending_player_for_target_or(state, *target, global)) +} + +/// CR 508.5 second clause: when the bound event names exactly ONE attacking +/// creature, that creature is the "attacking creature" the ability refers to, +/// so its defender answers even when the asker is attacking someone else. +fn sole_attacker_defender( + state: &GameState, + entries: &[(ObjectId, AttackTarget)], + global: PlayerId, +) -> Option { + match entries { + [(_, target)] => Some(defending_player_for_target_or(state, *target, global)), + _ => None, + } +} + +/// CR 508.5 (+ CR 508.5a / CR 802.2a in multiplayer): THE authority that decides +/// which attack answers a "defending player" reference. +/// +/// Every door — the `PlayerScope::DefendingPlayer` quantity door +/// (`quantity::defending_player_for_quantity_context`), the quantity-context +/// controller-ref door (`quantity::source_defending_player_for_context`), and +/// the `TargetFilter` controller-ref door (`filter::source_defending_player`) — +/// calls THIS FUNCTION WITH THESE THREE ARGUMENTS AND NOTHING ELSE, so one +/// anaphor can never bind two different players. +/// +/// # The one binding rule (stated once, applied here only) +/// +/// An attack event binds a "defending player" reference **if and only if** the +/// reference is being evaluated inside the scope of a triggered ability — i.e. +/// `trigger_source.is_some()`. CR 603.4: a triggered ability is bound to the +/// event that fired it, and that event is authoritative for its anaphors. A +/// layer/static read, an activated ability, or any filter evaluated outside a +/// triggered ability's scope is bound to NOTHING and must answer from the +/// asker's own combat facts only — otherwise an unrelated in-flight +/// `AttackersDeclared` leaks its attacker into a continuous effect's filter. +/// +/// When bound, the event is the explicit DETECTION event (the +/// `DETECTION_TRIGGER_EVENT` TLS, set by `resolve_quantity_for_trigger_check` +/// whenever an explicit `event` is supplied) if one is present, else +/// `state.current_trigger_event`. Same precedence, and same reason, as the +/// `scoped_player` derivation in `resolve_quantity_for_trigger_check`: +/// `current_trigger_event` may still hold a stale event from an unrelated +/// in-flight resolution in the same step (issue #1323). Reading both here, +/// rather than in the doors, is what makes the rule unforgeable. +/// +/// # Arguments +/// +/// * `asker_id` — the object whose ability is asking, as the caller knows it. +/// * `trigger_source` — the triggered ability's source context, or `None`. Both +/// the LATCH (`combat_status.defending_player`, captured by +/// `zones::capture_combat_status` — the CR 508.5 LAST clause / CR 608.2h last +/// known information, `None` when the asker is not itself an attacker) and +/// the binding decision are derived from this ONE input. A captured `None` +/// means "no answer here", not "no defender" (issue #6678): an +/// Equipment/Aura source is absent from `combat.attackers`, so the chain must +/// fall through rather than collapse to a spurious `Some(None)`. +/// +/// Two ids are derived internally and must not be conflated: the ENTRY-LOOKUP +/// id is `trigger_source`'s LKI reference id when present, else `asker_id` +/// (matching the pre-change quantity `Some` branch); the LIVE-COMBAT id is +/// always `asker_id` (matching the pre-change filter door and quantity `None` +/// branch). +/// +/// # Precedence +/// +/// 1. The asker's OWN entry in the bound event's `attacks` list. CR 508.5 first +/// clause — "an ability of an attacking creature refers to a defending +/// player". Resolved through [`defending_player_for_target_or`], so a +/// planeswalker target answers with its controller and a battle target with +/// its protector (CR 310.8d) instead of being skipped. +/// 2. The bound event's SOLE attacker, when the event names exactly one. +/// +/// **THIS STEP EXISTS TO OUTRANK THE LATCH (step 3), NOT TO RESOLVE THE +/// ATTACK TARGET. DO NOT COLLAPSE IT INTO STEP 4.** +/// +/// CR 508.5 second clause — "a spell or ability refers to both an attacking +/// creature and a defending player": on an observer trigger, the attacking +/// creature the ability refers to is the one the event names, EVEN IF the +/// asker is itself attacking someone else. The latch (step 3) is the CR +/// 508.5 LAST-clause LKI snapshot of the ASKER's own attack; the second +/// clause beats it whenever the event names the referred-to attacker. +/// +/// Note that `trigger_matchers::matching_attack_events` already writes the +/// per-target-RESOLVED defender into each synthesized singleton's global +/// `defending_player` field, so step 2 and step 4 return the SAME `PlayerId` +/// on the production path. The only thing step 2 adds is its POSITION — +/// ahead of the latch. Merging it into step 4 restores latch-before-event +/// and re-breaks the observer-trigger anaphor; the M'Baku integration test +/// `mbaku_buffs_only_the_creature_attacking_the_monarch` and the unit test +/// `event_sole_attacker_outranks_source_latch_cr_508_5` are what fail. +/// 3. The latch. Reached when the bound event names neither the asker nor a +/// sole attacker (a raw multi-attacker batch), or when there is no bound +/// event at all. Not dead code — `raw_batch_without_asker_falls_back_to_latch` +/// pins it. +/// 4. The bound event's declared global `defending_player` (CR 508.1b). The +/// coarsest answer; reachable only for a raw batch, and only inside a +/// triggered ability's scope (an unbound reference never reaches here). +/// 5. [`resolve_defending_player`] — live combat, keyed on `asker_id`. The +/// pre-existing tail; note it retains its OWN +/// `triggering_event_source_object` fallback, which this change does not +/// touch. +/// +/// # Behavior deltas +/// +/// This CHANGES precedence at four of the six (door × trigger-source state) +/// combinations; it is NOT a pure `.or_else` extension of any caller and no +/// parity invariant is claimed. In particular, with no bound event the result +/// is now `None` rather than an unrelated in-flight combat's global defender — +/// that leak removal is deliberate, and on the trigger side the designation +/// boundary gate turns the resulting `None` into a non-firing condition in both +/// polarities. +pub(crate) fn defending_player_cr508_5( + state: &GameState, + asker_id: ObjectId, + trigger_source: Option<&crate::types::game_state::TriggerSourceContext>, +) -> Option { + // The ONE binding rule, evaluated in the ONE place it may be evaluated. + let detection = crate::game::quantity::detection_trigger_event(); + let binding = trigger_source + .and_then(|_| detection.as_ref().or(state.current_trigger_event.as_ref())) + .and_then(attack_entries) + .map_or(DefenderBinding::None, |(entries, global)| { + DefenderBinding::TriggerEvent { entries, global } + }); + + let latch = trigger_source.and_then(|source| source.combat_status.defending_player); + let entry_id = trigger_source.map_or(asker_id, |source| source.identity.reference.object_id); + + match binding { + DefenderBinding::TriggerEvent { entries, global } => { + entry_defender(state, entries, global, entry_id) + .or_else(|| sole_attacker_defender(state, entries, global)) + .or(latch) + .or(Some(global)) + } + DefenderBinding::None => latch, + } + .or_else(|| resolve_defending_player(state, asker_id)) +} + /// Return the next defending player who still needs to declare blockers. pub fn next_defending_player_to_declare_blockers(state: &GameState) -> Option { let declared: HashSet = state @@ -6461,11 +6677,12 @@ pub fn get_valid_attack_targets(state: &GameState) -> Vec { } } - // CR 310.9b + CR 506.2: A battle can be attacked by any attacking player for whom + // CR 310.8b + CR 506.2: A battle can be attacked by any attacking player for whom // its protector is a defending player. Notably a Siege can be attacked by its own - // controller if the protector is a different player (CR 310.9b "Siege battle can - // be attacked by its own controller"). The only player who cannot attack is the - // battle's protector. + // controller if the protector is a different player (CR 310.8b "Notably, a Siege + // battle can be attacked by its own controller"). The only player who cannot + // attack is the battle's protector (CR 310.8b: "A battle's protector can never + // attack it"). for &id in &state.battlefield { if let Some(obj) = state.objects.get(&id) { if !obj @@ -6696,6 +6913,370 @@ mod tests { use crate::types::format::FormatConfig; use crate::types::identifiers::CardId; + // --------------------------------------------------------------------- + // CR 508.5 defending-player anchor — `defending_player_cr508_5` + // --------------------------------------------------------------------- + + /// Three-player state with a battlefield source. `source` is the asker. + fn anchor_state() -> (GameState, ObjectId) { + let mut state = GameState::new(FormatConfig::commander(), 3, 42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Anchor source".to_string(), + Zone::Battlefield, + ); + (state, source) + } + + fn spawn(state: &mut GameState, card: u64, name: &str) -> ObjectId { + create_object( + state, + CardId(card), + PlayerId(0), + name.to_string(), + Zone::Battlefield, + ) + } + + fn latch_context( + state: &GameState, + source: ObjectId, + ) -> crate::types::game_state::TriggerSourceContext { + let object = state.objects.get(&source).expect("source must exist"); + crate::game::triggers::trigger_source_context_for_latch(state, object) + } + + /// Declare `attacks` as the live combat so `capture_combat_status` fills the + /// asker's CR 508.5-last-clause latch. + fn declare_combat(state: &mut GameState, attacks: &[(ObjectId, PlayerId)]) { + state.combat = Some(CombatState { + attackers: attacks + .iter() + .map(|(id, defender)| { + AttackerInfo::new(*id, AttackTarget::Player(*defender), *defender) + }) + .collect(), + ..CombatState::default() + }); + } + + fn singleton_event(attacker: ObjectId, target: AttackTarget, global: PlayerId) -> GameEvent { + GameEvent::AttackersDeclared { + attacker_ids: vec![attacker], + defending_player: global, + attacks: vec![(attacker, target)], + } + } + + /// **The live defect.** CR 508.5 second clause: when an observer ability + /// "refers to both an attacking creature and a defending player", the + /// attacking creature is the one the EVENT names — even though the ability's + /// own source is simultaneously attacking someone else. + /// + /// Revert-failing: with the latch consulted first (the pre-change order in + /// all three doors) this returns P1, the SOURCE's defender, and M'Baku's + /// buff lands on the wrong creature. + #[test] + fn event_sole_attacker_outranks_source_latch_cr_508_5() { + let (mut state, source) = anchor_state(); + let other = spawn(&mut state, 2, "Other attacker"); + // The source itself attacks P1 → its latch is Some(P1). + declare_combat(&mut state, &[(source, PlayerId(1)), (other, PlayerId(2))]); + let ctx = latch_context(&state, source); + assert_eq!( + ctx.combat_status.defending_player, + Some(PlayerId(1)), + "precondition: the source's own latch must be populated" + ); + + // The trigger fired on the OTHER creature attacking P2. + state.current_trigger_event = Some(singleton_event( + other, + AttackTarget::Player(PlayerId(2)), + PlayerId(2), + )); + + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(2)), + "the referred-to attacker's defender wins over the source's own latch" + ); + } + + /// CR 508.5 first clause: when the source IS the event's attacker (Dethrone + /// / Goblin Guide shape — 17 of the 19 corpus `PlayerScope::DefendingPlayer` + /// cards), the answer is unchanged from the pre-change latch read. Both + /// derive from the same target-resolved `AttackerInfo.defending_player`. + #[test] + fn source_is_the_event_attacker_is_unchanged_cr_508_5() { + let (mut state, source) = anchor_state(); + declare_combat(&mut state, &[(source, PlayerId(1))]); + let ctx = latch_context(&state, source); + state.current_trigger_event = Some(singleton_event( + source, + AttackTarget::Player(PlayerId(1)), + PlayerId(1), + )); + + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(1)) + ); + } + + /// CR 508.5 + CR 310.8d hardening: the asker's own entry resolves a + /// PLANESWALKER target to its controller and a BATTLE target to its + /// PROTECTOR, instead of being skipped. + /// + /// Synthetic: `trigger_matchers::matching_attack_events` writes the + /// per-target-resolved defender into every synthesized singleton's global + /// field, so no corpus card can currently produce an event whose global + /// field disagrees with its own `attacks` entry. This fixture guards the + /// raw/hand-built event path and the `find_map` → `find` correction. + #[test] + fn entry_defender_resolves_planeswalker_and_battle_targets_cr_310_8d() { + let (mut state, source) = anchor_state(); + let planeswalker = create_object( + &mut state, + CardId(7), + PlayerId(2), + "Planeswalker".to_string(), + Zone::Battlefield, + ); + let ctx = latch_context(&state, source); + assert_eq!( + ctx.combat_status.defending_player, None, + "precondition: no latch, so step 1 is what answers" + ); + + // Deliberately inconsistent global field (P1) vs the entry (PW@P2). + state.current_trigger_event = Some(singleton_event( + source, + AttackTarget::Planeswalker(planeswalker), + PlayerId(1), + )); + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(2)), + "CR 508.5: the planeswalker's CONTROLLER is the defending player" + ); + + let battle = create_object( + &mut state, + CardId(8), + PlayerId(0), + "Battle".to_string(), + Zone::Battlefield, + ); + // CR 310.8d: the protector is the durable `ChosenAttribute::Player` + // persisted by the Siege's "as ~ enters" replacement, and it is + // deliberately DIFFERENT from the battle's controller (P0) here. + { + let battle_obj = state.objects.get_mut(&battle).unwrap(); + battle_obj.card_types.core_types = vec![CoreType::Battle]; + battle_obj + .chosen_attributes + .push(ChosenAttribute::Player(PlayerId(2))); + } + state.current_trigger_event = Some(singleton_event( + source, + AttackTarget::Battle(battle), + PlayerId(1), + )); + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(2)), + "CR 310.8d: a battle's PROTECTOR is the defending player, not its controller" + ); + } + + /// The latch is NOT dead code: a raw multi-attacker batch that names neither + /// the asker nor a sole attacker falls through steps 1 and 2 to step 3. + /// + /// Revert-failing against a design that promotes the event's global field + /// wholesale, which would answer P3. + #[test] + fn raw_batch_without_asker_falls_back_to_latch_cr_608_2h() { + let (mut state, source) = anchor_state(); + let a = spawn(&mut state, 2, "A"); + let b = spawn(&mut state, 3, "B"); + declare_combat(&mut state, &[(source, PlayerId(1))]); + let ctx = latch_context(&state, source); + + state.current_trigger_event = Some(GameEvent::AttackersDeclared { + attacker_ids: vec![a, b], + defending_player: PlayerId(2), + attacks: vec![ + (a, AttackTarget::Player(PlayerId(2))), + (b, AttackTarget::Player(PlayerId(1))), + ], + }); + + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(1)), + "the asker's own CR 608.2h combat snapshot answers a batch it is not in" + ); + } + + /// CR 508.5 last clause / CR 608.2h: with no attack event bound, the latch + /// is the answer. + #[test] + fn latch_answers_when_no_event_is_bound_cr_508_5() { + let (mut state, source) = anchor_state(); + declare_combat(&mut state, &[(source, PlayerId(2))]); + let ctx = latch_context(&state, source); + state.current_trigger_event = None; + + assert_eq!( + defending_player_cr508_5(&state, source, Some(&ctx)), + Some(PlayerId(2)) + ); + } + + /// The binding rule: OUTSIDE a triggered ability's scope + /// (`trigger_source == None`) no event binds, so an unrelated in-flight + /// `AttackersDeclared` cannot leak its defender into a layer/static read. + /// + /// This is what keeps the `filter.rs` door byte-identical for continuous + /// effects. Paired reach-guard below proves step 5 is still reached. + #[test] + fn no_trigger_source_never_binds_an_unrelated_event_cr_508_5() { + let (mut state, source) = anchor_state(); + let stranger = spawn(&mut state, 9, "Unrelated attacker"); + state.current_trigger_event = Some(singleton_event( + stranger, + AttackTarget::Player(PlayerId(2)), + PlayerId(2), + )); + + // The asker is not in combat at all → genuinely unanswerable. + assert_eq!( + defending_player_cr508_5(&state, source, None), + None, + "an unrelated combat must not answer a reference bound to nothing" + ); + + // Reach-guard: once the asker IS a live attacker, step 5 answers. + declare_combat(&mut state, &[(source, PlayerId(1))]); + assert_eq!( + defending_player_cr508_5(&state, source, None), + Some(PlayerId(1)) + ); + } + + /// CR 603.4: all three doors ask the SAME question with the SAME arguments, + /// so they cannot bind two different players from one anaphor. The binding + /// selection lives inside the authority precisely so reintroducing + /// caller-side selection is a test failure rather than a silent divergence. + #[test] + fn every_door_agrees_on_the_same_anchor_cr_508_5() { + let (mut state, source) = anchor_state(); + let other = spawn(&mut state, 2, "Other attacker"); + declare_combat(&mut state, &[(source, PlayerId(1)), (other, PlayerId(2))]); + let ctx = latch_context(&state, source); + state.current_trigger_event = Some(singleton_event( + other, + AttackTarget::Player(PlayerId(2)), + PlayerId(2), + )); + + let quantity_door = crate::game::quantity::defending_player_for_quantity_context_for_test( + &state, + source, + Some(&ctx), + ); + let filter_door = + crate::game::filter::source_defending_player_for_test(&state, source, Some(&ctx)); + assert_eq!(quantity_door, filter_door); + assert_eq!(quantity_door, Some(PlayerId(2))); + + // Same agreement in the unbound state, where the answer is the asker's + // own combat fact rather than the event's. + let unbound_quantity = + crate::game::quantity::defending_player_for_quantity_context_for_test( + &state, source, None, + ); + let unbound_filter = + crate::game::filter::source_defending_player_for_test(&state, source, None); + assert_eq!(unbound_quantity, unbound_filter); + assert_eq!(unbound_filter, Some(PlayerId(1))); + } + + /// CR 508.5: the `ControllerRef::DefendingPlayer` quantity-context door (the + /// attachment-controller and damage-source-controller comparisons) has the + /// LARGEST behaviour delta in this consolidation: before it, a + /// `trigger_source` whose combat latch was empty answered `None` + /// unconditionally, so every comparison against it was silently false and + /// the attachment/damage filter never matched. + #[test] + fn controller_ref_quantity_door_gains_the_shared_fallbacks_cr_508_5() { + let (mut state, source) = anchor_state(); + let other = spawn(&mut state, 2, "Other attacker"); + + // (a) Latch populated, but the event names a different attacker: the + // event wins, exactly like the other two doors. + declare_combat(&mut state, &[(source, PlayerId(1)), (other, PlayerId(2))]); + let latched = latch_context(&state, source); + state.current_trigger_event = Some(singleton_event( + other, + AttackTarget::Player(PlayerId(2)), + PlayerId(2), + )); + assert_eq!( + crate::game::quantity::source_defending_player_for_context_for_test( + &state, + source, + Some(&latched) + ), + Some(PlayerId(2)), + "event outranks the latch here too" + ); + + // (b) Equipment/Aura shape: the source is NOT an attacker, so the latch + // is empty. Previously this returned `None` outright. + let mut equip_state = GameState::new(FormatConfig::commander(), 3, 42); + let equipment = create_object( + &mut equip_state, + CardId(5), + PlayerId(0), + "Equipment".to_string(), + Zone::Battlefield, + ); + let carrier = spawn(&mut equip_state, 6, "Equipped creature"); + declare_combat(&mut equip_state, &[(carrier, PlayerId(2))]); + let equip_ctx = latch_context(&equip_state, equipment); + assert_eq!( + equip_ctx.combat_status.defending_player, None, + "precondition: an attachment source is never in combat.attackers" + ); + equip_state.current_trigger_event = Some(singleton_event( + carrier, + AttackTarget::Player(PlayerId(2)), + PlayerId(2), + )); + assert_eq!( + crate::game::quantity::source_defending_player_for_context_for_test( + &equip_state, + equipment, + Some(&equip_ctx) + ), + Some(PlayerId(2)), + "issue #6678 shape: a captured `None` means 'no answer here', not \ + 'no defender' — the equipped creature's defender must answer" + ); + + // (c) No trigger source: byte-identical to the pre-change behaviour. + assert_eq!( + crate::game::quantity::source_defending_player_for_context_for_test( + &state, source, None + ), + resolve_defending_player(&state, source) + ); + } + fn exact_choice_source( state: &GameState, object_id: ObjectId, diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index bbff1b8122..92e0f8e531 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -3800,7 +3800,7 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { Effect::Unimplemented { .. } | Effect::Explore | Effect::Investigate - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } @@ -4243,7 +4243,13 @@ fn fmt_trigger_condition(cond: &crate::types::ability::TriggerCondition) -> Stri fmt_quantity(rhs) ), TC::HasMaxSpeed => "has max speed".into(), - TC::IsMonarch => "is monarch".into(), + // CR 725.1 + CR 109.5: keep the controller-scoped description byte-stable + // so existing gap strings do not churn; a scoped subject reads + // differently and gets its own phrase. + TC::IsMonarch { + player: PlayerScope::Controller, + } => "is monarch".into(), + TC::IsMonarch { .. } => "that player is monarch".into(), TC::IsInitiative => "has the initiative".into(), TC::NoMonarch => "no monarch".into(), TC::WasStartingPlayer { .. } => "was the starting player".into(), @@ -4434,7 +4440,11 @@ fn fmt_static_condition(cond: &StaticCondition) -> String { SC::SourceIsAttacking => "source is attacking".into(), SC::SourceIsBlocking => "source is blocking".into(), SC::SourceIsBlocked => "source is blocked".into(), - SC::IsMonarch => "is monarch".into(), + // CR 725.1 + CR 109.5: see the `TC::IsMonarch` arm above. + SC::IsMonarch { + player: PlayerScope::Controller, + } => "is monarch".into(), + SC::IsMonarch { .. } => "that player is monarch".into(), SC::IsInitiative => "has the initiative".into(), SC::NoMonarch => "no monarch".into(), SC::HasCityBlessing => "has the city's blessing".into(), @@ -6574,7 +6584,7 @@ fn visit_direct_effect_ability_payloads<'a>( | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } @@ -7766,8 +7776,21 @@ fn extract_static_condition_features( extract_static_condition_features(sub, features); } } + // `Not` is a boolean COMBINATOR exactly like `And` / `Or` — + // `layers::evaluate_condition` negates its operand's own evaluation and + // has no independent semantics of its own. Letting it fall into the + // catch-all below emitted only `static_condition:Not` (classified + // `Handled`, correctly, because negation itself is implemented) and + // SWALLOWED the operand, so an unhandled leaf under a negation was + // reported as supported. That is a fail-open in the direction coverage + // must never fail: `Not(IsMonarch { ScopedPlayer })` — the "unless that + // player is the monarch" shape the `layers` entry gate hard-rejects to + // `false` — would advertise a restriction that silently never applies. + StaticCondition::Not { condition } => { + extract_static_condition_features(condition, features); + } _ => { - // All other variants (including `Not`) emit a single tag. The + // Every remaining variant is a LEAF and emits a single tag. The // classifier carries compiler-enforced handled/unhandled status. let (name, support) = static_condition_feature(cond); features.insert(format!("static_condition:{name}"), support); @@ -8379,9 +8402,14 @@ fn static_condition_feature(cond: &StaticCondition) -> (&'static str, FeatureSup // Variants below are parsed but not classified as handled by the prior registry. StaticCondition::HasMaxSpeed => ("HasMaxSpeed", Unhandled), StaticCondition::SpeedGE { .. } => ("SpeedGE", Unhandled), - // CR 608.2c: Compound conditions — resolved recursively by + // Compound conditions — resolved recursively by // `layers::evaluate_condition`, which short-circuits And/Or and // negates Not. Verified at layers.rs ~line 263. + // + // All three arms are UNREACHABLE from `extract_static_condition_features`: + // that walker recurses every combinator and only classifies leaves, so a + // combinator never contributes a tag of its own. They exist for + // exhaustiveness and for the direct unit-test callers below. StaticCondition::And { .. } => ("And", Handled), StaticCondition::Or { .. } => ("Or", Handled), StaticCondition::Not { .. } => ("Not", Handled), @@ -8392,7 +8420,14 @@ fn static_condition_feature(cond: &StaticCondition) -> (&'static str, FeatureSup StaticCondition::SourceIsAttacking => ("SourceIsAttacking", Handled), StaticCondition::SourceIsBlocking => ("SourceIsBlocking", Handled), StaticCondition::SourceIsBlocked => ("SourceIsBlocked", Handled), - StaticCondition::IsMonarch => ("IsMonarch", Handled), + // CR 725.1: only the controller subject has a static-side evaluator. + // `layers::evaluate_condition{,_with_recipient}` rejects every other + // scope at its entry boundary (no trigger event, no combat anchor), so + // coverage must report those `Unhandled` rather than claim support. + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + } => ("IsMonarch", Handled), + StaticCondition::IsMonarch { .. } => ("IsMonarch", Unhandled), StaticCondition::IsInitiative => ("IsInitiative", Handled), StaticCondition::NoMonarch => ("NoMonarch", Handled), StaticCondition::HasCityBlessing => ("HasCityBlessing", Handled), @@ -15389,6 +15424,86 @@ mod tests { } } + /// `extract_static_condition_features` must recurse + /// `StaticCondition::Not` exactly as it recurses `And` / `Or`. Negation is a + /// combinator with no semantics of its own, so swallowing its operand + /// reports an UNHANDLED leaf as supported — the fail-open direction coverage + /// must never take. + /// + /// Revert-failing: restore the `_ =>` catch-all for `Not` and the first + /// assertion fails — the map holds only `static_condition:Not` (Handled) and + /// the `IsMonarch` leaf disappears, so + /// `Not(IsMonarch { player: ScopedPlayer })` — the "unless that player is + /// the monarch" shape `layers`' entry gate hard-rejects to `false` — would + /// be advertised as fully supported. + #[test] + fn static_condition_not_recurses_into_its_operand() { + let feature_map = |cond: &StaticCondition| { + let mut features = HashMap::new(); + extract_static_condition_features(cond, &mut features); + features + }; + + let negated_scoped_monarch = StaticCondition::Not { + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer, + }), + }; + let features = feature_map(&negated_scoped_monarch); + assert_eq!( + features.get("static_condition:IsMonarch"), + Some(&FeatureSupport::Unhandled), + "the operand under `Not` must reach the classifier" + ); + assert!( + !features.contains_key("static_condition:Not"), + "`Not` is a combinator and contributes no tag of its own, exactly \ + like `And` / `Or`" + ); + + // Discrimination guard: recursion reports the operand's OWN class — it + // does not blanket-downgrade everything under a negation. + assert_eq!( + feature_map(&StaticCondition::Not { + condition: Box::new(StaticCondition::SourceIsTapped), + }) + .get("static_condition:SourceIsTapped"), + Some(&FeatureSupport::Handled), + ); + + // Nesting guard: `Not(Or(..))` is a real corpus shape; both operands + // must surface, not just the first. + let nested = feature_map(&StaticCondition::Not { + condition: Box::new(StaticCondition::Or { + conditions: vec![ + StaticCondition::SourceIsTapped, + StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer, + }, + ], + }), + }); + assert_eq!( + nested.get("static_condition:SourceIsTapped"), + Some(&FeatureSupport::Handled) + ); + assert_eq!( + nested.get("static_condition:IsMonarch"), + Some(&FeatureSupport::Unhandled) + ); + + // Reach-guard for the affirmative shape: the printed default subject is + // still `Handled`, so the rows above are about the SCOPE, not about + // `IsMonarch` having become unsupported wholesale. + assert_eq!( + feature_map(&StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }) + .get("static_condition:IsMonarch"), + Some(&FeatureSupport::Handled) + ); + } + /// CR 614.1b + CR 614.10: `SkipStep { step: Draw }` must be recognised by /// `is_data_carrying_static` so that cards like Necropotence and /// Yawgmoth's Bargain are marked as supported. diff --git a/crates/engine/src/game/effects/become_monarch.rs b/crates/engine/src/game/effects/become_monarch.rs index d74c9f23e3..2a33c35261 100644 --- a/crates/engine/src/game/effects/become_monarch.rs +++ b/crates/engine/src/game/effects/become_monarch.rs @@ -1,4 +1,4 @@ -use crate::types::ability::{EffectError, ResolvedAbility}; +use crate::types::ability::{EffectError, ResolvedAbility, TargetFilter}; use crate::types::events::GameEvent; use crate::types::game_state::GameState; @@ -6,13 +6,27 @@ use crate::types::game_state::GameState; /// /// CR 725.3: Only one player can be the monarch at a time. As a player becomes /// the monarch, the current monarch ceases to be the monarch. +/// +/// `target` is the CR 109.5 subject printed on the clause, resolved through +/// [`super::resolve_player_for_context_ref`] — the same single authority +/// `Effect::Draw`, `Effect::Mill` and every other "target player does X" effect +/// uses. A context-ref filter (`Controller`, "you become the monarch") answers +/// the controller; a real target filter ("target opponent becomes the monarch" +/// — M'Baku, Jabari Chieftain; Garland, Royal Kidnapper; Jared Carthalion, True +/// Heir; Éomer, King of Rohan; Denethor, Stone Seer) reads the player chosen +/// into `ability.targets` at announcement (CR 115.1). +/// +/// Before the subject axis existed this read `ability.controller` +/// unconditionally, so every targeted printing crowned its own controller — the +/// one player those clauses exist to deny. pub fn resolve( state: &mut GameState, ability: &ResolvedAbility, + target: &TargetFilter, events: &mut Vec, ) -> Result<(), EffectError> { // CR 725.1: The monarch is a designation a player can have. - let player_id = ability.controller; + let player_id = super::resolve_player_for_context_ref(state, ability, target); state.monarch = Some(player_id); events.push(GameEvent::MonarchChanged { player_id }); Ok(()) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index ada38a1a35..58bc01f49b 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -4719,7 +4719,7 @@ pub fn resolve_effect( // CR 701.56a: Time travel — interactive counter manipulation on suspended/time-countered permanents. // Currently a no-op; full interactive implementation requires WaitingFor infrastructure. Effect::TimeTravel => time_travel::resolve(state, ability, events), - Effect::BecomeMonarch => become_monarch::resolve(state, ability, events), + Effect::BecomeMonarch { target } => become_monarch::resolve(state, ability, target, events), // CR 101.3 + CR 608.2: An instruction with no game action. Emit // `EffectResolved` so the chain continues, and do nothing else. Effect::NoOp => { @@ -13707,7 +13707,9 @@ mod tests { ); let ability = ResolvedAbility::new( - Effect::BecomeMonarch, + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, Vec::new(), ObjectId(999), PlayerId(0), diff --git a/crates/engine/src/game/effects/vote.rs b/crates/engine/src/game/effects/vote.rs index a5457009f7..0bcb8d2ee9 100644 --- a/crates/engine/src/game/effects/vote.rs +++ b/crates/engine/src/game/effects/vote.rs @@ -2021,7 +2021,9 @@ mod tests { Box::new(AbilityDefinition::new(AbilityKind::Spell, Effect::NoOp)), Box::new(AbilityDefinition::new( AbilityKind::Spell, - Effect::BecomeMonarch, + Effect::BecomeMonarch { + target: crate::types::ability::TargetFilter::Controller, + }, )), ]; let options = vec!["innocent".to_string(), "guilty".to_string()]; @@ -2067,7 +2069,9 @@ mod tests { Box::new(AbilityDefinition::new(AbilityKind::Spell, Effect::NoOp)), Box::new(AbilityDefinition::new( AbilityKind::Spell, - Effect::BecomeMonarch, + Effect::BecomeMonarch { + target: crate::types::ability::TargetFilter::Controller, + }, )), ]; let options = vec!["innocent".to_string(), "guilty".to_string()]; diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 98772acb1f..4885ca4fb3 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -4891,27 +4891,41 @@ struct SourceContext<'a> { recipient_id: Option, } -/// CR 508.5 + CR 508.5a: Source-relative "defending player" resolution. Prefer -/// the triggered source's captured combat facts — an attacking creature's own -/// attack trigger snapshots its defending player, and that captured fact must -/// answer even after the source changes zones (a recycled storage id must never -/// answer a different ability's filter). +/// CR 508.5 + CR 508.5a: `ControllerRef::DefendingPlayer` door for +/// `TargetFilter` evaluation. /// -/// But an attachment/anthem source (Equipment, Aura) is NOT itself the attacker: -/// `capture_combat_status` finds it absent from `combat.attackers` and records -/// `defending_player: None`. "Whenever equipped creature attacks, ... defending -/// player controls" (Captain America's Shield, Greatsword of Tyr, and the rest -/// of that class) must then resolve the defender of the *attacking creature*, -/// carried by the triggering event. So a captured `None` is "no answer here", -/// not "no defender" — fall through to `resolve_defending_player`, which reads -/// the triggering event's attacker. Using `.map().unwrap_or_else()` collapsed -/// that captured `None` into a spurious `Some(None)` and suppressed the -/// fallback, silently fizzling the ability (issue #6678). +/// Identical call, identical arguments, identical rule as the two quantity +/// doors. The binding decision is NOT made here — see +/// `combat::defending_player_cr508_5`, which owns it so one anaphor read once +/// as a `PlayerScope` and once as a `ControllerRef` cannot bind two different +/// players. +/// +/// The issue-#6678 distinction still governs the latch and now lives on the +/// authority's `trigger_source` parameter: an attachment/anthem source +/// (Equipment, Aura) is not itself the attacker, so `capture_combat_status` +/// records `defending_player: None`, and that captured `None` means "no answer +/// here", not "no defender". +/// +/// When `source.trigger_source` is `None` the authority binds no event, so this +/// door remains byte-identical to its previous `resolve_defending_player` +/// behaviour and no unrelated in-flight combat can leak into continuous-effect +/// filter evaluation. fn source_defending_player(state: &GameState, source: &SourceContext<'_>) -> Option { - source - .trigger_source - .and_then(|context| context.combat_status.defending_player) - .or_else(|| crate::game::combat::resolve_defending_player(state, source.id)) + crate::game::combat::defending_player_cr508_5(state, source.id, source.trigger_source) +} + +/// Drive the production `ControllerRef::DefendingPlayer` door from the +/// cross-door agreement fixture in `combat.rs`. Mirrors +/// `quantity::defending_player_for_quantity_context_for_test` so the fixture +/// compares two PRODUCTION doors rather than two hand-built approximations. +#[cfg(test)] +pub(crate) fn source_defending_player_for_test( + state: &GameState, + source_id: ObjectId, + trigger_source: Option<&TriggerSourceContext>, +) -> Option { + let context = source_context_from_filter(state, source_id, None, None, trigger_source, None); + source_defending_player(state, &context) } fn source_enchanted_player(source: &SourceContext<'_>) -> Option { diff --git a/crates/engine/src/game/functioning_abilities.rs b/crates/engine/src/game/functioning_abilities.rs index 781e9cd811..884e893160 100644 --- a/crates/engine/src/game/functioning_abilities.rs +++ b/crates/engine/src/game/functioning_abilities.rs @@ -485,7 +485,8 @@ pub fn active_replacements( mod tests { use super::*; use crate::types::ability::{ - ReplacementDefinition, StaticCondition, StaticDefinition, TriggerDefinition, TypedFilter, + PlayerScope, ReplacementDefinition, StaticCondition, StaticDefinition, TriggerDefinition, + TypedFilter, }; use crate::types::format::FormatConfig; use crate::types::game_state::GameState; @@ -826,9 +827,11 @@ mod tests { let state = new_state(); assert!(state.monarch.is_none()); let mut obj = make_obj(1, Zone::Battlefield); - obj.static_definitions = vec![ - StaticDefinition::new(StaticMode::Continuous).condition(StaticCondition::IsMonarch) - ] + obj.static_definitions = vec![StaticDefinition::new(StaticMode::Continuous).condition( + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }, + )] .into(); assert_eq!(active_static_definitions(&state, &obj).count(), 0); } @@ -838,9 +841,11 @@ mod tests { let mut state = new_state(); state.monarch = Some(PlayerId(0)); let mut obj = make_obj(1, Zone::Battlefield); - obj.static_definitions = vec![ - StaticDefinition::new(StaticMode::Continuous).condition(StaticCondition::IsMonarch) - ] + obj.static_definitions = vec![StaticDefinition::new(StaticMode::Continuous).condition( + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }, + )] .into(); assert_eq!(active_static_definitions(&state, &obj).count(), 1); } @@ -853,7 +858,9 @@ mod tests { let state = new_state(); let mut obj = make_obj(1, Zone::Battlefield); let trig = TriggerDefinition { - condition: Some(crate::types::ability::TriggerCondition::IsMonarch), + condition: Some(crate::types::ability::TriggerCondition::IsMonarch { + player: PlayerScope::Controller, + }), ..TriggerDefinition::new(TriggerMode::ChangesZone) }; obj.trigger_definitions = vec![trig].into(); @@ -966,9 +973,11 @@ mod tests { let mut state = new_state(); assert!(state.monarch.is_none()); let mut obj = make_obj(1, Zone::Battlefield); - obj.static_definitions = vec![ - StaticDefinition::new(StaticMode::Continuous).condition(StaticCondition::IsMonarch) - ] + obj.static_definitions = vec![StaticDefinition::new(StaticMode::Continuous).condition( + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }, + )] .into(); put_on_battlefield(&mut state, obj); @@ -1047,9 +1056,11 @@ mod tests { let state = new_state(); assert!(state.monarch.is_none()); let mut obj = make_obj(1, Zone::Battlefield); - obj.static_definitions = vec![ - StaticDefinition::new(StaticMode::Continuous).condition(StaticCondition::IsMonarch) - ] + obj.static_definitions = vec![StaticDefinition::new(StaticMode::Continuous).condition( + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }, + )] .into(); assert_eq!( active_static_definitions(&state, &obj).count(), diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 3ea30f13e9..de41a8dcf7 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -919,6 +919,9 @@ pub(crate) fn evaluate_condition( controller: PlayerId, source_id: ObjectId, ) -> bool { + if static_condition_has_unresolvable_designation_anchor(condition) { + return false; + } evaluate_condition_with_context(state, condition, controller, source_id, None) } @@ -929,9 +932,44 @@ pub(crate) fn evaluate_condition_with_recipient( source_id: ObjectId, recipient_id: ObjectId, ) -> bool { + if static_condition_has_unresolvable_designation_anchor(condition) { + return false; + } evaluate_condition_with_context(state, condition, controller, source_id, Some(recipient_id)) } +/// CR 109.4 + CR 725.5 (static analogue of the trigger-side CR 603.4 gate): +/// layer evaluation has no triggering event and no combat anchor, so it cannot +/// resolve any [`PlayerScope`] other than `Controller`. A scoped designation +/// leaf is therefore unanswerable here. +/// +/// Reject the whole condition at the entry boundary — returning `false` from the +/// leaf would let [`StaticCondition::Not`] invert it into an APPLIED +/// restriction, which is exactly the printed "unless that player is the +/// monarch" shape. CR 725.5 independently prescribes "the effect does nothing" +/// for the analogous vacant-monarch case, so `false` here is the +/// rules-prescribed outcome rather than an invented default. +/// +/// Purely structural (no `GameState` needed), mirroring the shape of +/// `condition_uses_recipient_context` and `static_condition_uses_object_population` +/// in this module. The `_ => false` leaf arm is safe because the leaf question +/// is delegated to the compiler-forced +/// [`StaticCondition::designation_player_anchor`] accessor. +fn static_condition_has_unresolvable_designation_anchor(condition: &StaticCondition) -> bool { + if let Some(scope) = condition.designation_player_anchor() { + return !matches!(scope, PlayerScope::Controller); + } + match condition { + StaticCondition::And { conditions } | StaticCondition::Or { conditions } => conditions + .iter() + .any(static_condition_has_unresolvable_designation_anchor), + StaticCondition::Not { condition } => { + static_condition_has_unresolvable_designation_anchor(condition) + } + _ => false, + } +} + /// Selects the controller that supplies "you" for an active effect's /// condition. Printed and granted static abilities read their source's current /// controller; resolution-created transient continuous effects retain the @@ -1098,7 +1136,7 @@ fn static_condition_uses_object_population(condition: &StaticCondition) -> bool | StaticCondition::RecipientAttackingOwnerTarget { .. } | StaticCondition::SourceIsBlocking | StaticCondition::SourceIsBlocked - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -1256,7 +1294,7 @@ fn static_condition_characteristic_reads_at( | StaticCondition::RecipientAttackingOwnerTarget { .. } | StaticCondition::SourceIsBlocking | StaticCondition::SourceIsBlocked - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -1381,7 +1419,7 @@ fn entered_object_perturbs_static_condition( | StaticCondition::RecipientAttackingOwnerTarget { .. } | StaticCondition::SourceIsBlocking | StaticCondition::SourceIsBlocked - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -1839,8 +1877,22 @@ fn evaluate_condition_with_context( .find(|a| a.object_id == source_id) .is_some_and(|a| a.blocked) }), - // CR 725.1: True when the controller is the monarch. - StaticCondition::IsMonarch => eval_is_monarch(state, controller), + // CR 725.1 + CR 109.5: a static ability's "you" is the object's current + // controller. Layer evaluation has no trigger event and no combat + // anchor, so no other scope can EVER resolve here. The scoped form never + // reaches this arm — `evaluate_condition{,_with_recipient}` has already + // rejected the condition at its entry boundary — and + // `coverage::static_condition_feature` reports those scopes `Unhandled`, + // so coverage does not claim support. + // + // CR 725.5: while there is no monarch, a monarch-dependent continuous + // effect does nothing, and begins to apply once a player becomes the + // monarch. `eval_is_monarch` returns false for a vacant designation and + // layers re-evaluate on the monarch change, which is exactly that. + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + } => eval_is_monarch(state, controller), + StaticCondition::IsMonarch { .. } => false, // CR 726.3: True when the controller has the initiative. StaticCondition::IsInitiative => eval_is_initiative(state, controller), // CR 725.1: True when no player holds the monarch designation. @@ -3505,7 +3557,7 @@ fn static_condition_reads_life(condition: &StaticCondition) -> bool { | StaticCondition::SourceIsAttacking | StaticCondition::SourceIsBlocking | StaticCondition::SourceIsBlocked - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -16883,7 +16935,9 @@ mod tests { obj.timestamp = anthem_ts; obj.static_definitions.push( StaticDefinition::continuous() - .condition(StaticCondition::IsMonarch) + .condition(StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }) .affected(TargetFilter::Typed( TypedFilter::creature().controller(ControllerRef::You), )) @@ -16920,6 +16974,77 @@ mod tests { assert_eq!(bear_obj.toughness, Some(3)); } + /// CR 109.4 + CR 725.5: layer evaluation has no triggering event and no + /// combat anchor, so a SCOPED monarch subject is unanswerable there. It must + /// be false in BOTH polarities. + /// + /// The negated case is the revert-failing one: without the entry-boundary + /// gate in `evaluate_condition{,_with_recipient}` the leaf's `false` inverts + /// under `StaticCondition::Not` and the anthem applies UNCONDITIONALLY — + /// which is exactly the printed "unless that player is the monarch" + /// (Fall from Favor) restriction shape, applied when the engine cannot + /// identify the player at all. + #[test] + fn scoped_monarch_static_condition_is_false_in_both_polarities_cr_725_5() { + for (label, condition) in [ + ( + "affirmative", + StaticCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }, + ), + ( + "negated", + StaticCondition::Not { + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }), + }, + ), + ] { + let mut state = setup(); + // Even with the controller AS the monarch, an unresolvable subject + // must not let the effect apply. + state.monarch = Some(PlayerId(0)); + + let anthem = create_object( + &mut state, + CardId(0), + PlayerId(0), + "Scoped Monarch Anthem".to_string(), + Zone::Battlefield, + ); + let anthem_ts = state.next_timestamp(); + { + let obj = state.objects.get_mut(&anthem).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + obj.timestamp = anthem_ts; + obj.static_definitions.push( + StaticDefinition::continuous() + .condition(condition) + .affected(TargetFilter::Typed( + TypedFilter::creature().controller(ControllerRef::You), + )) + .modifications(vec![ + ContinuousModification::AddPower { value: 1 }, + ContinuousModification::AddToughness { value: 1 }, + ]), + ); + } + let bear = make_creature(&mut state, "Bear", 2, 2, PlayerId(0)); + + evaluate_layers(&mut state); + + let bear_obj = state.objects.get(&bear).unwrap(); + assert_eq!( + bear_obj.power, + Some(2), + "{label}: an unanswerable monarch subject must not apply" + ); + assert_eq!(bear_obj.toughness, Some(2), "{label}: toughness unchanged"); + } + } + /// CR 702.94a + CR 400.3: A continuous static ability whose `affected` /// filter carries `InZone { zone: Hand }` applies to hand objects rather /// than battlefield objects. Verifies `apply_continuous_effect` dispatches diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index 835de95169..f4497f2719 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -377,14 +377,43 @@ fn source_controller_for_context(state: &GameState, ctx: &QuantityContext) -> Op source_lki_for_context(state, ctx).map(|lki| lki.controller) } +/// CR 508.5: `ControllerRef::DefendingPlayer` door, quantity-context flavour. +/// +/// Identical call, identical arguments, identical rule as the +/// `PlayerScope::DefendingPlayer` door and the `filter.rs` door. Previously +/// this answered `None` outright whenever the trigger source's combat latch was +/// empty (an Equipment/Aura source is never in `combat.attackers`), silently +/// making every comparison against it false; the shared authority supplies the +/// event and live-combat fallbacks the other doors already had. fn source_defending_player_for_context( state: &GameState, ctx: &QuantityContext, ) -> Option { - match ctx.trigger_source.as_ref() { - Some(source) => source.combat_status.defending_player, - None => crate::game::combat::resolve_defending_player(state, ctx.source), - } + crate::game::combat::defending_player_cr508_5(state, ctx.source, ctx.trigger_source.as_ref()) +} + +/// Drive the production `ControllerRef::DefendingPlayer` quantity-context door +/// (the `attachment-controller` and `damage-source-controller` comparisons) from +/// the cross-door fixtures in `combat.rs`. This door had the largest behaviour +/// delta in the CR 508.5 consolidation — before it, a trigger source with an +/// empty combat latch answered `None` unconditionally. +#[cfg(test)] +pub(crate) fn source_defending_player_for_context_for_test( + state: &GameState, + source: ObjectId, + trigger_source: Option<&TriggerSourceContext>, +) -> Option { + source_defending_player_for_context( + state, + &QuantityContext { + entering: None, + source, + trigger_source: trigger_source.cloned(), + recipient: None, + scoped_player: None, + damage_source: None, + }, + ) } fn source_enchanted_player_for_context( @@ -1111,7 +1140,7 @@ pub(crate) fn static_condition_uses_unspent_mana(condition: &StaticCondition) -> | StaticCondition::SourceIsAttacking | StaticCondition::SourceIsBlocking | StaticCondition::SourceIsBlocked - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::HasCityBlessing @@ -2060,11 +2089,103 @@ pub(crate) fn resolve_quantity_for_trigger_check( resolve_quantity_with_ctx(state, expr, controller, ctx) } +/// CR 109.4 + CR 603.4: Resolve a `PlayerScope` in TRIGGER-CONDITION context. +/// +/// The player-axis sibling of [`resolve_quantity_for_trigger_check`]: it builds +/// the identical `QuantityContext` from the same four inputs and delegates to +/// `resolve_single_player_scope`, the existing single authority for +/// `PlayerScope` → `PlayerId`. No per-scope logic is re-implemented here, and +/// `PlayerScope::DefendingPlayer` therefore reaches +/// `combat::defending_player_cr508_5` on the same path as every other door. +/// +/// Trigger conditions are checked before targets exist at fire time (CR 603.4), +/// so `targets` is empty and `ability` is `None`; scopes that need either +/// (`Target`, `ParentObjectTargetController`) resolve to `None` and the entry +/// boundary rejects the condition rather than substituting the controller. +/// +/// Duration-timing-only scopes are rejected BEFORE delegating: because +/// `IsMonarch { player }` is serde-constructible from `card-data.json` and from +/// mtgish input, `PlayerScope::AnyTurn` / `SpecificPlayer` can reach this +/// function from a malformed row, and `resolve_single_player_scope` answers +/// those with `unreachable!()`. Returning `None` here makes a bad row fail +/// closed instead of panicking the engine inside a trigger check. (Validating +/// at the serde boundary was considered and rejected: it would need a custom +/// deserializer on every variant that carries a `PlayerScope`.) +/// +/// [`PlayerScope::ScopedPlayer`] is rejected the same way when the triggering +/// event names no player. `resolve_single_player_scope` answers that scope with +/// `ctx.scoped_player.unwrap_or(controller)` — always `Some`. That fallback is +/// right in a VALUE context (an unanchored "that player's life total" degrading +/// to the controller's is a wrong number, not a wrong control-flow decision), +/// but it fails OPEN here: this function is the anchor authority for the +/// designation boundary gates in `game::triggers` / `game::layers`, whose whole +/// contract is that an unresolvable anchor is UNANSWERABLE rather than false. +/// Inheriting the controller instead answers a "that player is the monarch" +/// intervening-if about the ABILITY CONTROLLER — silently the wrong player, and +/// with no gate rejection to catch it. This is reachable for any +/// `ScopedPlayer` anchor the parser's attack-trigger rebind does not convert to +/// [`PlayerScope::DefendingPlayer`] (a non-`Attacks` mode, or an `Attacks` +/// trigger whose attacked noun is not a player). +pub(crate) fn resolve_player_scope_for_trigger_check( + state: &GameState, + scope: &PlayerScope, + controller: PlayerId, + source_context: Option<&TriggerSourceContext>, + event: Option<&crate::types::events::GameEvent>, +) -> Option { + if scope.duration_timing_only() { + return None; + } + + // CR 603.4: the explicit `event` wins over `current_trigger_event`, which + // may still hold a stale event from an unrelated in-flight resolution in + // the same step (issue #1323). Same precedence as the `scoped_player` + // derivation in `resolve_quantity_for_trigger_check`. + let resolution_event = event.or(state.current_trigger_event.as_ref()); + let scoped_player = + resolution_event.and_then(|e| crate::game::targeting::extract_player_from_event(e, state)); + + // CR 603.4 + CR 109.4: "that player" is an ANAPHOR — it denotes nobody when + // the triggering event names nobody. Fail closed here rather than let + // `resolve_single_player_scope`'s value-context `unwrap_or(controller)` + // fallback hand the boundary gate the ability controller. See the doc + // comment above for why the two contexts want opposite answers. + if matches!(scope, PlayerScope::ScopedPlayer) && scoped_player.is_none() { + return None; + } + + let ctx = QuantityContext { + entering: None, + source: source_context + .map(|source| source.identity.reference.object_id) + .unwrap_or(ObjectId(0)), + trigger_source: source_context.cloned(), + recipient: None, + scoped_player, + damage_source: None, + }; + + match event { + // CR 603.4: make the triggering event visible to the CR 508.5 anchor + // authority for detection-time checks, exactly as the quantity sibling + // does for `ObjectCount`. + Some(event) => with_detection_trigger_event(event, || { + resolve_single_player_scope(state, scope, controller, ctx.clone(), &[], None) + }), + None => resolve_single_player_scope(state, scope, controller, ctx, &[], None), + } +} + std::thread_local! { - /// Detection-time trigger event override. Populated only inside - /// `resolve_quantity_for_trigger_check` when `state.current_trigger_event` - /// is `None`. Consumed by `ObjectCount` evaluation (see `resolve_ref`) to - /// implement `FilterProp::OtherThanTriggerObject` semantics. + /// Detection-time trigger event override. Populated by + /// `resolve_quantity_for_trigger_check` whenever an EXPLICIT `event` is + /// supplied — including when `state.current_trigger_event` is also set, in + /// which case the explicit event is authoritative (CR 603.4; see the + /// `event.is_none() && …` fast-path guard at the top of that function, and + /// the same precedence applied to `scoped_player` just above it). + /// Consumed by `ObjectCount` evaluation (see `resolve_ref`) to implement + /// `FilterProp::OtherThanTriggerObject` semantics, and by + /// `combat::defending_player_cr508_5` for the CR 508.5 anchor binding. static DETECTION_TRIGGER_EVENT: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; } @@ -6766,60 +6887,39 @@ where } } +/// CR 508.5: `PlayerScope::DefendingPlayer` door. +/// +/// The single authority `combat::defending_player_cr508_5` owns BOTH the +/// binding rule and the precedence. Do NOT read `state.current_trigger_event`, +/// the detection TLS, or `combat_status.defending_player` here — one anaphor +/// must not be able to bind two different players across doors. fn defending_player_for_quantity_context( state: &GameState, ctx: QuantityContext, ) -> Option { - // CR 508.5: prefer the single authority, which resolves the defending player of - // the source's own attack or — for an Equipment/Aura whose source is not the - // attacker — the attacker carried by the triggering event (CR 508.5a, per-attacker). - if let Some(source) = ctx.trigger_source.as_ref() { - let source_id = source.identity.reference.object_id; - return source - .combat_status - .defending_player - // Event-global read: the event's attacker/defender relation, keyed - // by the exact source identity, is authoritative for this trigger. - .or_else(|| { - defending_player_from_event(state.current_trigger_event.as_ref(), source_id) - }) - .or_else(|| { - defending_player_from_event(detection_trigger_event().as_ref(), source_id) - }); - } - crate::game::combat::resolve_defending_player(state, ctx.source) - // CR 508.5a 1v1 fallback: a batched multi-attacker trigger event has no single - // attacking object to resolve individually, so use the event's defending player. - .or_else(|| defending_player_from_event(state.current_trigger_event.as_ref(), ctx.source)) - .or_else(|| defending_player_from_event(detection_trigger_event().as_ref(), ctx.source)) + crate::game::combat::defending_player_cr508_5(state, ctx.source, ctx.trigger_source.as_ref()) } -fn defending_player_from_event( - event: Option<&crate::types::events::GameEvent>, - source_id: ObjectId, +/// Drive the production `PlayerScope::DefendingPlayer` door from the cross-door +/// agreement fixture in `combat.rs` without reconstructing a `QuantityContext` +/// by hand (which would let the two doors diverge in the test itself). +#[cfg(test)] +pub(crate) fn defending_player_for_quantity_context_for_test( + state: &GameState, + source: ObjectId, + trigger_source: Option<&TriggerSourceContext>, ) -> Option { - let crate::types::events::GameEvent::AttackersDeclared { - defending_player, - attacks, - .. - } = event? - else { - return None; - }; - attacks - .iter() - .find_map(|(attacker_id, target)| { - if *attacker_id == source_id { - match target { - crate::game::combat::AttackTarget::Player(pid) => Some(*pid), - crate::game::combat::AttackTarget::Planeswalker(_) - | crate::game::combat::AttackTarget::Battle(_) => None, - } - } else { - None - } - }) - .or(Some(*defending_player)) + defending_player_for_quantity_context( + state, + QuantityContext { + entering: None, + source, + trigger_source: trigger_source.cloned(), + recipient: None, + scoped_player: None, + damage_source: None, + }, + ) } /// CR 810.9a + CR 810.9d: Resolve an aggregate (multi-player) `LifeTotal` diff --git a/crates/engine/src/game/resolution_prompt.rs b/crates/engine/src/game/resolution_prompt.rs index 59479b8f54..f2322ffed8 100644 --- a/crates/engine/src/game/resolution_prompt.rs +++ b/crates/engine/src/game/resolution_prompt.rs @@ -323,7 +323,7 @@ fn effect_offers_choice(e: &Effect) -> bool { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } diff --git a/crates/engine/src/game/trigger_matchers.rs b/crates/engine/src/game/trigger_matchers.rs index 0cee94f65a..dbbeaf8c05 100644 --- a/crates/engine/src/game/trigger_matchers.rs +++ b/crates/engine/src/game/trigger_matchers.rs @@ -786,18 +786,6 @@ fn damage_recipient_filter_can_match_player(filter: &TargetFilter) -> bool { } } -fn is_player_scope_attack_filter(filter: &TargetFilter) -> bool { - match filter { - TargetFilter::Player | TargetFilter::Controller | TargetFilter::AllPlayers => true, - TargetFilter::Typed(TypedFilter { - type_filters, - controller: Some(_), - properties, - }) => type_filters.is_empty() && properties.is_empty(), - _ => false, - } -} - /// Basic runtime matching of a TargetFilter against a game object. /// Handles the common filter patterns used in triggers. pub(super) fn target_filter_matches_object( @@ -1798,7 +1786,7 @@ pub(super) fn matching_attack_events( if let Some(filter) = trigger .valid_source .as_ref() - .filter(|filter| is_player_scope_attack_filter(filter)) + .filter(|filter| filter.is_player_scope()) { // CR 508.3d + CR 508.5a: "[player] attacks [opponent]" triggers // once per attacked defending player, not once per attacking @@ -1830,7 +1818,11 @@ pub(super) fn matching_attack_events( return None; } let event_defending_player = - attack_target_defending_player(state, target, *defending_player); + crate::game::combat::defending_player_for_target_or( + state, + target, + *defending_player, + ); if seen_defending_players.contains(&event_defending_player) { return None; } @@ -1884,8 +1876,11 @@ pub(super) fn matching_attack_events( { return None; } - let event_defending_player = - attack_target_defending_player(state, target, *defending_player); + let event_defending_player = crate::game::combat::defending_player_for_target_or( + state, + target, + *defending_player, + ); if dedup_by_player { if seen_defending_players.contains(&event_defending_player) { return None; @@ -1922,8 +1917,11 @@ fn attack_target_matches( // player is the monarch (CR 725.1), the trigger does not fire (The Spear // of Bashenga). if matches!(filter, crate::types::triggers::AttackTargetFilter::Monarch) { - let defending_player = - attack_target_defending_player(state, target, fallback_defending_player); + let defending_player = crate::game::combat::defending_player_for_target_or( + state, + target, + fallback_defending_player, + ); if state.monarch != Some(defending_player) { return false; } @@ -1931,8 +1929,11 @@ fn attack_target_matches( } if trigger.valid_target.is_some() { - let defending_player = - attack_target_defending_player(state, target, fallback_defending_player); + let defending_player = crate::game::combat::defending_player_for_target_or( + state, + target, + fallback_defending_player, + ); valid_player_matches(trigger, state, defending_player, source_context) } else { true @@ -1968,26 +1969,6 @@ pub(super) fn attack_target_type_matches( ) } -pub(super) fn attack_target_defending_player( - state: &GameState, - target: crate::game::combat::AttackTarget, - fallback_defending_player: PlayerId, -) -> PlayerId { - match target { - crate::game::combat::AttackTarget::Player(player) => player, - crate::game::combat::AttackTarget::Planeswalker(object_id) => state - .objects - .get(&object_id) - .map(|object| object.controller) - .unwrap_or(fallback_defending_player), - crate::game::combat::AttackTarget::Battle(object_id) => state - .objects - .get(&object_id) - .and_then(|object| object.protector()) - .unwrap_or(fallback_defending_player), - } -} - /// Compound matcher for "Whenever ~ enters or attacks" — fires on either /// a ZoneChanged-to-Battlefield event or an AttackersDeclared event for the source. pub(super) fn match_enters_or_attacks( @@ -4162,7 +4143,7 @@ pub(super) fn matching_you_attack_unblocked_pairs( return None; } if trigger.valid_target.is_some() { - let defending_player = attack_target_defending_player( + let defending_player = crate::game::combat::defending_player_for_target_or( state, attacker.attack_target, attacker.defending_player, diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 8e6fe1523a..b6f9e5f61f 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -1254,9 +1254,7 @@ fn contextual_batched_trigger_event( let defending_player = matching .first() .map(|(_, target)| { - super::trigger_matchers::attack_target_defending_player( - state, *target, fallback, - ) + super::combat::defending_player_for_target_or(state, *target, fallback) }) .unwrap_or(fallback); (defending_player, matching) @@ -5188,7 +5186,12 @@ fn collect_pending_triggers_with_collection( if let Some(attacker) = state.objects.get(source_id) { let new_monarch = attacker.controller; if new_monarch != *target_player { - let become_effect = Effect::BecomeMonarch; + // CR 725.2: the synthetic trigger's controller IS the + // new monarch, so the printed-default subject axis is + // exactly right here. + let become_effect = Effect::BecomeMonarch { + target: TargetFilter::Controller, + }; let source_context = trigger_source_context_for_latch(state, attacker); let mut become_ability = ResolvedAbility::new( become_effect, @@ -10513,6 +10516,27 @@ pub(crate) fn check_trigger_condition_with_source( return false; } + // CR 603.4 + CR 109.4: polarity-safe fail-closed for designation leaves. + // + // A leaf whose PLAYER ANCHOR cannot be resolved is UNANSWERABLE, not false. + // Returning `false` from inside the recursion inverts to `true` under + // `TriggerCondition::Not` — the shape every "unless" grammar and the + // "if you're not the monarch" bridge produce — firing the trigger precisely + // when the engine cannot identify the player. Reject here, at the same outer + // boundary that already rejects incoherent zone-change provenance directly + // above, so the boolean combinators in + // `evaluate_trigger_condition_with_source` can never reinterpret it as an + // ordinary false operand. + if !trigger_condition_designation_anchors_resolvable( + state, + condition, + controller, + source_context, + trigger_event, + ) { + return false; + } + evaluate_trigger_condition_with_source( state, condition, @@ -10522,6 +10546,62 @@ pub(crate) fn check_trigger_condition_with_source( ) } +/// CR 603.4 + CR 109.4: boundary predicate — does every designation leaf in this +/// tree have a resolvable player anchor? +/// +/// NOT a second evaluator: it recurses only the boolean combinators and +/// delegates every anchor question to +/// `quantity::resolve_player_scope_for_trigger_check`, the same single authority +/// the leaves use, via the compiler-forced +/// [`TriggerCondition::designation_player_anchor`] accessor. The `_ => true` +/// leaf arm is safe precisely because that accessor is exhaustive: a future +/// anchored leaf is a compile error there, not a silent fail-open here. +/// +/// Deliberately conservative: an unresolvable anchor anywhere rejects the whole +/// condition, INCLUDING inside an `Or` whose other operand is true. No corpus +/// card places a designation leaf under `Or`; the choice is pinned by +/// `unresolvable_designation_anchor_absorbs_or_cr_603_4` so a future card +/// needing the looser reading has a failing test to point at. +fn trigger_condition_designation_anchors_resolvable( + state: &GameState, + condition: &TriggerCondition, + controller: PlayerId, + source_context: Option<&TriggerSourceContext>, + trigger_event: Option<&GameEvent>, +) -> bool { + if let Some(scope) = condition.designation_player_anchor() { + return crate::game::quantity::resolve_player_scope_for_trigger_check( + state, + scope, + controller, + source_context, + trigger_event, + ) + .is_some(); + } + match condition { + TriggerCondition::And { conditions } | TriggerCondition::Or { conditions } => { + conditions.iter().all(|inner| { + trigger_condition_designation_anchors_resolvable( + state, + inner, + controller, + source_context, + trigger_event, + ) + }) + } + TriggerCondition::Not { condition } => trigger_condition_designation_anchors_resolvable( + state, + condition, + controller, + source_context, + trigger_event, + ), + _ => true, + } +} + /// Evaluates a condition after the outer event-provenance boundary has accepted /// its input. Boolean combinators recurse here so invalid provenance cannot be /// reinterpreted as an ordinary false operand. @@ -11316,8 +11396,29 @@ fn evaluate_trigger_condition_with_source( TriggerCondition::SpellCastWithVariantThisTurn { variant } => { crate::game::restrictions::spell_cast_with_variant_this_turn(state, variant) } - // CR 725.1: True when the controller is the monarch. - TriggerCondition::IsMonarch => eval_is_monarch(state, controller), + // CR 725.1 + CR 603.4: the monarch check is evaluated against the player + // the condition names. `PlayerScope::Controller` is CR 109.5's "you"; + // every other scope is an event/combat anchor resolved from the SAME + // explicit `trigger_event` this function threads everywhere else, so the + // fire-time check and the CR 603.4 resolution-time recheck read the same + // player. + // + // An unresolvable scope cannot reach this arm: the entry boundary in + // `check_trigger_condition_with_source` has already rejected the whole + // condition (see `trigger_condition_designation_anchors_resolvable`). + // The `is_some_and` below is therefore a total-function formality, not + // the fail-closed mechanism — putting the rejection here instead would + // fail OPEN under `TriggerCondition::Not`. + TriggerCondition::IsMonarch { player } => { + crate::game::quantity::resolve_player_scope_for_trigger_check( + state, + player, + controller, + source_context, + trigger_event, + ) + .is_some_and(|pid| eval_is_monarch(state, pid)) + } // CR 726.3: True when the controller has the initiative. TriggerCondition::IsInitiative => eval_is_initiative(state, controller), // CR 725.1: True when no player holds the monarch designation. @@ -19052,8 +19153,14 @@ pub mod tests { } let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); // The intervening-`if` as the parser lowers it onto the delayed BODY. ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, @@ -19176,8 +19283,14 @@ pub mod tests { .is_commander = true; } - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -19265,8 +19378,14 @@ pub mod tests { let mut trigger_def = TriggerDefinition::new(TriggerMode::LandPlayed); trigger_def.valid_target = Some(TargetFilter::Controller); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -19419,8 +19538,14 @@ pub mod tests { let mut trigger_def = TriggerDefinition::new(TriggerMode::LandPlayed); trigger_def.valid_target = Some(TargetFilter::Controller); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -19546,7 +19671,14 @@ pub mod tests { // Gate is FALSE (no commander anywhere), so the Otherwise branch is the // one that must run. - let mut ability = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -19640,8 +19772,14 @@ pub mod tests { } let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -19759,8 +19897,14 @@ pub mod tests { ); let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(condition); state.delayed_triggers.push(DelayedTrigger { condition: DelayedTriggerCondition::WhenDies { @@ -19907,8 +20051,14 @@ pub mod tests { } let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::QuantityCheck { lhs: QuantityExpr::Ref { qty: QuantityRef::ObjectCount { @@ -20020,8 +20170,14 @@ pub mod tests { ); let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::QuantityCheck { lhs: QuantityExpr::Ref { qty }, comparator: crate::types::ability::Comparator::GE, @@ -20099,8 +20255,14 @@ pub mod tests { /// conditional continuation and whose GRANDCHILD carries an /// `else_ability` (life gain) that only runs if the chain gets that far. fn body(source: ObjectId, controller: PlayerId) -> ResolvedAbility { - let mut grandchild = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut grandchild = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); grandchild.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -20114,7 +20276,14 @@ pub mod tests { controller, ))); - let mut sub = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut sub = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); // NOT one of the surviving classes: an ordinary game-state gate on a // continuation link, so `resolve_chain_body` stops here when the // parent gate is false. @@ -20122,8 +20291,14 @@ pub mod tests { sub.sub_link = crate::types::ability::SubAbilityLink::ContinuationStep; sub.sub_ability = Some(Box::new(grandchild)); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -20280,8 +20455,14 @@ pub mod tests { ); let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); // Gate FALSE: no commander is staged anywhere. ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, @@ -20382,8 +20563,14 @@ pub mod tests { } let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = - ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::Not { condition: Box::new(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, @@ -20453,7 +20640,14 @@ pub mod tests { ); let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); - let mut ability = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + let mut ability = ResolvedAbility::new( + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); ability.condition = Some(AbilityCondition::ControlsCommander { ownership: CommanderOwnership::Own, }); @@ -24948,6 +25142,391 @@ pub mod tests { )); } + /// Three-player state with a battlefield source controlled by P0, used by + /// the monarch-subject fixtures below. + fn monarch_setup() -> (GameState, ObjectId) { + let mut state = GameState::new(crate::types::format::FormatConfig::commander(), 3, 42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Monarch subject source".to_string(), + Zone::Battlefield, + ); + (state, source) + } + + fn attackers_declared(attacker: ObjectId, defender: PlayerId) -> GameEvent { + GameEvent::AttackersDeclared { + attacker_ids: vec![attacker], + defending_player: defender, + attacks: vec![( + attacker, + crate::game::combat::AttackTarget::Player(defender), + )], + } + } + + /// CR 725.1 + CR 109.5: the controller subject is CR 109.5's "you". + #[test] + fn is_monarch_controller_subject_reads_the_ability_controller() { + let (mut state, source) = monarch_setup(); + let condition = TriggerCondition::IsMonarch { + player: PlayerScope::Controller, + }; + + state.monarch = Some(PlayerId(2)); + assert!(!check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + None + )); + + state.monarch = Some(PlayerId(0)); + assert!(check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + None + )); + } + + /// CR 725.1 + CR 508.5 + CR 603.4: the defending-player subject reads the + /// player the TRIGGERING creature is attacking, not the ability controller. + /// + /// Revert-failing: an arm that ignores `player` and calls + /// `eval_is_monarch(state, controller)` answers `false` for the positive + /// case (P0 is not the monarch) and `true` for the negative one. + #[test] + fn is_monarch_defending_player_subject_reads_the_attacked_player_cr_508_5() { + let (mut state, source) = monarch_setup(); + let attacker = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Attacker".to_string(), + Zone::Battlefield, + ); + let condition = TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }; + + state.monarch = Some(PlayerId(2)); + assert!( + check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&attackers_declared(attacker, PlayerId(2))), + ), + "the attacked player (P2) is the monarch" + ); + assert!( + !check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&attackers_declared(attacker, PlayerId(1))), + ), + "the attacked player (P1) is not the monarch" + ); + } + + /// CR 603.4 + CR 109.4: an unresolvable anchor makes the condition + /// UNANSWERABLE. It must be false in BOTH polarities — the whole point of + /// rejecting at the entry boundary instead of inside the recursion. + /// + /// Revert-failing on the negated case: without the boundary gate the leaf + /// returns `false` and `Not` inverts it to `true`, firing the trigger + /// precisely when the engine cannot identify the player. + #[test] + fn unresolvable_designation_anchor_is_false_in_both_polarities_cr_603_4() { + let (mut state, source) = monarch_setup(); + state.monarch = Some(PlayerId(2)); + + for scope in [ + PlayerScope::DefendingPlayer, + PlayerScope::Target, + // CR 611.2a: serde-reachable duration-timing-only scope; must fail + // closed rather than hit `resolve_single_player_scope`'s panic. + PlayerScope::AnyTurn, + // CR 603.4: the scope `parse_monarch_identity_subject` actually + // EMITS for "that player is the monarch". Every anchor that the + // attack-trigger rebind does not convert to `DefendingPlayer` + // arrives here as `ScopedPlayer`, so this is the one row that must + // not inherit `resolve_single_player_scope`'s value-context + // `unwrap_or(controller)` fallback. + PlayerScope::ScopedPlayer, + ] { + let affirmative = TriggerCondition::IsMonarch { + player: scope.clone(), + }; + assert!( + !check_trigger_condition(&state, &affirmative, PlayerId(0), Some(source), None), + "{scope:?}: affirmative must be false with no attack in scope" + ); + let negated = TriggerCondition::Not { + condition: Box::new(affirmative), + }; + assert!( + !check_trigger_condition(&state, &negated, PlayerId(0), Some(source), None), + "{scope:?}: NEGATED must also be false — `Not` must not invert an \ + unanswerable anchor into a firing trigger" + ); + } + + // Reach-guard: the arm IS reached — a resolvable subject still answers. + state.monarch = Some(PlayerId(0)); + assert!(check_trigger_condition( + &state, + &TriggerCondition::IsMonarch { + player: PlayerScope::Controller + }, + PlayerId(0), + Some(source), + None + )); + } + + /// CR 603.4 + CR 109.4: `PlayerScope::ScopedPlayer` — the scope + /// `parse_monarch_identity_subject` emits for "that player is the monarch" + /// — must read the player NAMED BY THE TRIGGERING EVENT, and must be + /// unanswerable when no event names one. It must never inherit + /// `resolve_single_player_scope`'s value-context `unwrap_or(controller)` + /// fallback. + /// + /// Revert-failing: drop the `ScopedPlayer`/`scoped_player.is_none()` guard + /// in `quantity::resolve_player_scope_for_trigger_check` and the + /// no-event affirmative below answers `true` — the ability CONTROLLER (P0) + /// is the monarch in that arm, which is precisely the wrong player — while + /// the negated case answers `false` for the wrong reason. + #[test] + fn is_monarch_scoped_player_subject_needs_an_event_anchor_cr_603_4() { + let (mut state, source) = monarch_setup(); + let condition = TriggerCondition::IsMonarch { + player: PlayerScope::ScopedPlayer, + }; + let life_changed = |player_id: PlayerId| GameEvent::LifeChanged { + player_id, + amount: -1, + }; + + // Reach-guard: with an event that NAMES a player, the arm resolves and + // discriminates between the named player and everyone else. + state.monarch = Some(PlayerId(2)); + assert!( + check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&life_changed(PlayerId(2))), + ), + "the event's player (P2) is the monarch" + ); + assert!( + !check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&life_changed(PlayerId(1))), + ), + "the event's player (P1) is not the monarch" + ); + + // The controller IS the monarch, and no event names a player. Both + // polarities must be false: the anchor is missing, so the question is + // unanswerable — not silently re-pointed at the controller. + state.monarch = Some(PlayerId(0)); + assert!( + !check_trigger_condition(&state, &condition, PlayerId(0), Some(source), None), + "an unanchored `that player` must not fall back to the controller" + ); + assert!( + !check_trigger_condition( + &state, + &TriggerCondition::Not { + condition: Box::new(condition.clone()), + }, + PlayerId(0), + Some(source), + None + ), + "`Not` must not invert the missing anchor into a firing trigger" + ); + + // Reach-guard for the negative rows: the same anchored event that + // resolves above still resolves with the controller as monarch, so the + // rows above are about the MISSING ANCHOR, not about a dead arm. + assert!(check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&life_changed(PlayerId(0))), + )); + } + + /// The boundary gate is deliberately conservative: an unresolvable anchor + /// rejects the whole condition even inside an `Or` whose other operand is + /// true. No corpus card places a designation leaf under `Or`; this pins the + /// choice so a future card needing the looser reading has a failing test to + /// point at rather than a silent behaviour change. + #[test] + fn unresolvable_designation_anchor_absorbs_or_cr_603_4() { + let (mut state, source) = monarch_setup(); + state.monarch = Some(PlayerId(0)); + + let true_operand = TriggerCondition::IsMonarch { + player: PlayerScope::Controller, + }; + // Reach-guard: on its own the true operand fires. + assert!(check_trigger_condition( + &state, + &true_operand, + PlayerId(0), + Some(source), + None + )); + + let disjunction = TriggerCondition::Or { + conditions: vec![ + TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }, + true_operand, + ], + }; + assert!( + !check_trigger_condition(&state, &disjunction, PlayerId(0), Some(source), None), + "documented conservative choice, not an accident" + ); + } + + /// CR 725.1: vacancy and identity stay distinct predicates after the + /// subject axis is added. + #[test] + fn monarch_vacancy_and_identity_remain_distinct_cr_725_1() { + let (mut state, source) = monarch_setup(); + state.monarch = None; + + assert!(!check_trigger_condition( + &state, + &TriggerCondition::IsMonarch { + player: PlayerScope::Controller + }, + PlayerId(0), + Some(source), + None + )); + assert!(check_trigger_condition( + &state, + &TriggerCondition::NoMonarch, + PlayerId(0), + Some(source), + None + )); + } + + /// CR 508.5a / CR 802.2a: `DefendingPlayerControlsNone` quantifies over + /// EVERY live defender rather than the one defending player CR 508.5a + /// specifies. That is a real gap on Siege Dragon / Spectral Force / + /// Spectral Bears / Fear of the Dark, but it is an all-defenders + /// QUANTIFIER bug, not an anchor-resolution bug: it shares no code with the + /// three `defending_player_cr508_5` doors and fixing it would change those + /// four cards' firing behaviour. + /// + /// This test pins TODAY's behaviour so a later change that routes the arm + /// through the CR 508.5 authority fails here and forces an explicit + /// decision instead of a silent behaviour swap. + #[test] + fn defending_player_controls_none_quantifies_all_defenders_cr_508_5a_gap() { + let (mut state, source) = monarch_setup(); + // Two attackers, two DIFFERENT defenders. Only P2 controls a Wall. + let attacker_a = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Attacker A".to_string(), + Zone::Battlefield, + ); + let attacker_b = create_object( + &mut state, + CardId(3), + PlayerId(0), + "Attacker B".to_string(), + Zone::Battlefield, + ); + let wall = create_object( + &mut state, + CardId(4), + PlayerId(2), + "Wall".to_string(), + Zone::Battlefield, + ); + state.objects.get_mut(&wall).unwrap().card_types.core_types = vec![CoreType::Creature]; + + let mut combat = crate::game::combat::CombatState::default(); + combat.attackers = vec![ + crate::game::combat::AttackerInfo::new( + attacker_a, + crate::game::combat::AttackTarget::Player(PlayerId(1)), + PlayerId(1), + ), + crate::game::combat::AttackerInfo::new( + attacker_b, + crate::game::combat::AttackTarget::Player(PlayerId(2)), + PlayerId(2), + ), + ]; + state.combat = Some(combat); + + let condition = TriggerCondition::DefendingPlayerControlsNone { + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: None, + properties: vec![], + }), + }; + + // TODAY: P1 controls no creature but P2 does, and the `all` quantifier + // over BOTH defenders makes the condition false. Under CR 508.5a the + // per-attacker defending player would be determined individually, so + // the P1 attacker's copy of this ability should see "controls none". + assert!( + !check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&attackers_declared(attacker_a, PlayerId(1))), + ), + "pins the all-defenders quantifier; if this now passes, the arm was \ + routed through combat::defending_player_cr508_5 — re-derive Siege \ + Dragon, Spectral Force, Spectral Bears and Fear of the Dark \ + explicitly rather than letting their firing behaviour change silently" + ); + + // Reach-guard: with the only creature removed, the arm reports true, so + // the assertion above is about the quantifier and not about an + // unreachable arm. + state.battlefield.retain(|id| *id != wall); + state.objects.remove(&wall); + assert!(check_trigger_condition( + &state, + &condition, + PlayerId(0), + Some(source), + Some(&attackers_declared(attacker_a, PlayerId(1))), + )); + } + /// CR 603.4 + CR 810.9a: "if you have N or more life" reads the /// controller's TEAM total in a 2HG game. Team A = 30 + 25 = 55 satisfies a /// minimum of 50, even though neither individual reaches 50. Reverting Site diff --git a/crates/engine/src/parser/oracle_condition.rs b/crates/engine/src/parser/oracle_condition.rs index be63709410..4ae7184683 100644 --- a/crates/engine/src/parser/oracle_condition.rs +++ b/crates/engine/src/parser/oracle_condition.rs @@ -377,7 +377,7 @@ fn static_condition_to_restriction_condition( | StaticCondition::DefendingPlayerControls { .. } | StaticCondition::SourceAttackingAlone | StaticCondition::SourceIsBlocking - | StaticCondition::IsMonarch + | StaticCondition::IsMonarch { .. } | StaticCondition::IsInitiative | StaticCondition::NoMonarch | StaticCondition::CompletedADungeon diff --git a/crates/engine/src/parser/oracle_effect/conditions.rs b/crates/engine/src/parser/oracle_effect/conditions.rs index 072ac7b7eb..f244ad491f 100644 --- a/crates/engine/src/parser/oracle_effect/conditions.rs +++ b/crates/engine/src/parser/oracle_effect/conditions.rs @@ -4565,7 +4565,16 @@ pub(crate) fn static_condition_to_ability_condition( variant: *variant, }) } - StaticCondition::IsMonarch => Some(AbilityCondition::IsMonarch), + // CR 725.1 + CR 109.5: only the controller-scoped monarch gate has an + // `AbilityCondition` counterpart. `AbilityCondition` has no player axis, + // so a scoped monarch gate must NOT be lowered — dropping the scope here + // would silently rebind "that player is the monarch" to the ability's + // controller. Returning `None` leaves the clause unrepresented and + // keeps coverage honest. + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + } => Some(AbilityCondition::IsMonarch), + StaticCondition::IsMonarch { .. } => None, StaticCondition::IsInitiative => Some(AbilityCondition::IsInitiative), StaticCondition::HasCityBlessing => Some(AbilityCondition::HasCityBlessing), // CR 702.195b: The enduring story designation is available to effects. diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 63ddda1401..0041ac8bc2 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -13045,7 +13045,13 @@ fn lower_imperative_family_effect(ast: ImperativeFamilyAst) -> Effect { profile, multi_target: _, } => Effect::TurnFaceDown { target, profile }, - ImperativeFamilyAst::BecomeMonarch => Effect::BecomeMonarch, + // CR 109.5: a bare "become the monarch" imperative has no printed + // subject, so the subject is "you". The targeted form + // ("target opponent becomes the monarch") carries an explicit subject + // phrase and is lowered by `subject::build_become_clause` instead. + ImperativeFamilyAst::BecomeMonarch => Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, ImperativeFamilyAst::VentureIntoDungeon => Effect::VentureIntoDungeon, ImperativeFamilyAst::VentureIntoUndercity => Effect::VentureInto { dungeon: crate::game::dungeon::DungeonId::Undercity, diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 7635905241..5c9cebd197 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -6409,7 +6409,7 @@ pub(super) fn clause_is_dig_lookback_transparent(effect: &Effect) -> bool { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } diff --git a/crates/engine/src/parser/oracle_effect/subject.rs b/crates/engine/src/parser/oracle_effect/subject.rs index c0076df043..db06e0c023 100644 --- a/crates/engine/src/parser/oracle_effect/subject.rs +++ b/crates/engine/src/parser/oracle_effect/subject.rs @@ -4463,6 +4463,28 @@ fn try_parse_become_basic_land_type_modifications( }) } +/// CR 725.1 + CR 109.5: map the parsed subject of "`` become[s] the +/// monarch" onto [`Effect::BecomeMonarch`]'s `target` axis. +/// +/// - a TARGETED PLAYER subject keeps its own parsed filter (CR 115.1) — that is +/// what makes `collect_target_slots` declare a target slot whose legality is +/// the printed restriction, so "target OPPONENT becomes the monarch" cannot be +/// answered with the controller's own seat +/// - an untargeted subject is [`TargetFilter::Controller`], CR 109.5's "you". +/// Deliberately permissive: this is the pre-axis behaviour for every +/// already-shipping "you become the monarch" card, so a stricter +/// `affected == Controller` test would regress them for no gain. `Controller` +/// is a context ref, so it surfaces no target slot. +/// - a targeted NON-player subject has no reading at all under CR 725.3 (only a +/// player can hold the designation), so it declines and the caller emits an +/// honest gap +fn monarch_subject_target(application: &SubjectApplication) -> Option { + match &application.target { + Some(filter) => filter.is_player_scope().then(|| filter.clone()), + None => Some(TargetFilter::Controller), + } +} + fn build_become_clause( application: SubjectApplication, predicate: &str, @@ -4478,7 +4500,20 @@ fn build_become_clause( let consumed = predicate_lower.len() - become_rest.len(); let become_text = predicate[consumed..].trim(); if become_text.eq_ignore_ascii_case("the monarch") { - return Some(super::parsed_clause(Effect::BecomeMonarch)); + // CR 725.1 + CR 109.5: the designation's SUBJECT is the parsed subject + // phrase, not the ability's controller. Dropping it made every + // "target opponent becomes the monarch" card (M'Baku, Jabari Chieftain; + // Garland, Royal Kidnapper; Jared Carthalion, True Heir) crown its own + // controller — the exact player the clause was written to deny. + return Some(match monarch_subject_target(&application) { + Some(target) => super::parsed_clause(Effect::BecomeMonarch { target }), + // A subject the axis cannot express must stay a visible gap rather + // than silently default to the controller. + None => super::parsed_clause(Effect::unimplemented( + "become_monarch_subject", + predicate.trim(), + )), + }); } // CR 611.2b: "Becomes" effects without explicit duration are permanent let duration = duration.or(Some(Duration::Permanent)); diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index f658de4b6a..8b9101cab3 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -52484,7 +52484,9 @@ fn delayed_trigger_intervening_if_retains_the_commander_control_gate() { ); assert_eq!( *delayed_body.effect, - Effect::BecomeMonarch, + Effect::BecomeMonarch { + target: crate::types::ability::TargetFilter::Controller + }, "the delayed body must still be the monarch effect" ); // CR 903.3 + CR 109.5: the regression assertion. Reverting the bridge arm in diff --git a/crates/engine/src/parser/oracle_ir/doc.rs b/crates/engine/src/parser/oracle_ir/doc.rs index b8748a354d..15c34e0ceb 100644 --- a/crates/engine/src/parser/oracle_ir/doc.rs +++ b/crates/engine/src/parser/oracle_ir/doc.rs @@ -1475,7 +1475,7 @@ fn stamp_effect_printed_slot(effect: &mut Effect, slot: usize, kind: PrintedItem Effect::Investigate => {} Effect::Tribute { .. } => {} Effect::TimeTravel => {} - Effect::BecomeMonarch => {} + Effect::BecomeMonarch { .. } => {} Effect::NoOp => {} Effect::Proliferate => {} Effect::ProliferateTarget { .. } => {} diff --git a/crates/engine/src/parser/oracle_nom/condition.rs b/crates/engine/src/parser/oracle_nom/condition.rs index 9d2e1d3dfc..d36bd36d1d 100644 --- a/crates/engine/src/parser/oracle_nom/condition.rs +++ b/crates/engine/src/parser/oracle_nom/condition.rs @@ -1245,10 +1245,12 @@ fn parse_turn_conditions(input: &str) -> OracleResult<'_, StaticCondition> { /// Handles "you're the monarch", "you have the initiative", and "you have the city's blessing". fn parse_player_state_conditions(input: &str) -> OracleResult<'_, StaticCondition> { alt(( - // CR 725.1: Monarch status - value( - StaticCondition::IsMonarch, - alt((tag("you're the monarch"), tag("you are the monarch"))), + // CR 725.1 + CR 109.5: monarch identity, decomposed into + // subject × predicate. The subject is one `alt()`; the predicate tag is + // matched once. Do NOT enumerate (subject × predicate) full-phrase tags. + map( + terminated(parse_monarch_identity_subject, tag("the monarch")), + |player| StaticCondition::IsMonarch { player }, ), // CR 725.1: "if an opponent is the monarch" — a monarch exists and it // is not the controller. Distinct from `Not(IsMonarch)` (also true when @@ -1257,7 +1259,9 @@ fn parse_player_state_conditions(input: &str) -> OracleResult<'_, StaticConditio StaticCondition::And { conditions: vec![ StaticCondition::Not { - condition: Box::new(StaticCondition::IsMonarch), + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }), }, StaticCondition::Not { condition: Box::new(StaticCondition::NoMonarch), @@ -1341,6 +1345,32 @@ fn parse_player_state_conditions(input: &str) -> OracleResult<'_, StaticConditio .parse(input) } +/// CR 109.5: subject axis of the monarch-identity predicate. +/// +/// "you're"/"you are" is the ability's controller (CR 109.5). "that +/// player"/"that opponent" is the anaphoric event-scoped player; this +/// combinator cannot see the owning trigger, so it emits the generic +/// [`PlayerScope::ScopedPlayer`] anchor, which an attack trigger rebinds to +/// [`PlayerScope::DefendingPlayer`] when its clause supplies a more specific +/// antecedent (CR 508.5 — see +/// `oracle_trigger::rebind_attack_anaphor_to_defending_player`). +/// +/// Mirrors `parse_that_player_has_conditions` in this module, which already +/// maps the same two anaphors to [`PlayerScope::ScopedPlayer`]. +fn parse_monarch_identity_subject(input: &str) -> OracleResult<'_, PlayerScope> { + alt(( + value( + PlayerScope::Controller, + alt((tag("you're "), tag("you are "))), + ), + value( + PlayerScope::ScopedPlayer, + alt((tag("that player is "), tag("that opponent is "))), + ), + )) + .parse(input) +} + fn parse_speed_threshold_condition(input: &str) -> OracleResult<'_, StaticCondition> { let (rest, _) = tag("your speed is ").parse(input)?; let (rest, threshold) = parse_number(rest)?; @@ -14454,14 +14484,24 @@ mod tests { fn test_youre_the_monarch() { let (rest, c) = parse_inner_condition("you're the monarch").unwrap(); assert_eq!(rest, ""); - assert_eq!(c, StaticCondition::IsMonarch); + assert_eq!( + c, + StaticCondition::IsMonarch { + player: PlayerScope::Controller + } + ); } #[test] fn test_you_are_the_monarch() { let (rest, c) = parse_inner_condition("you are the monarch").unwrap(); assert_eq!(rest, ""); - assert_eq!(c, StaticCondition::IsMonarch); + assert_eq!( + c, + StaticCondition::IsMonarch { + player: PlayerScope::Controller + } + ); } #[test] @@ -14473,7 +14513,9 @@ mod tests { StaticCondition::And { conditions: vec![ StaticCondition::Not { - condition: Box::new(StaticCondition::IsMonarch), + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::Controller + }), }, StaticCondition::Not { condition: Box::new(StaticCondition::NoMonarch), @@ -14483,6 +14525,56 @@ mod tests { ); } + /// CR 725.1 + CR 109.5: the anaphoric subject "that player"/"that opponent" + /// parses to the generic `ScopedPlayer` anchor. Revert-failing: all three + /// inputs return `Err` without the subject axis, which is what dropped + /// M'Baku's intervening-if entirely. + #[test] + fn test_that_player_is_the_monarch() { + let (rest, c) = parse_inner_condition("that player is the monarch").unwrap(); + assert_eq!(rest, ""); + assert_eq!( + c, + StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + } + ); + } + + #[test] + fn test_that_opponent_is_the_monarch() { + let (rest, c) = parse_inner_condition("that opponent is the monarch").unwrap(); + assert_eq!(rest, ""); + assert_eq!( + c, + StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + } + ); + } + + /// CR 603.4: the clause-boundary contract `try_extract_intervening` depends + /// on — the condition stops at the comma and hands the effect body back. + #[test] + fn test_that_player_is_the_monarch_stops_at_clause_boundary() { + let (rest, c) = + parse_inner_condition("that player is the monarch, that creature gets +1/+1").unwrap(); + assert_eq!(rest, ", that creature gets +1/+1"); + assert_eq!( + c, + StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + } + ); + } + + /// The `tag("the monarch")` predicate guard: a bare subject is not a + /// monarch-identity condition. + #[test] + fn test_that_player_is_the_without_monarch_predicate_is_rejected() { + assert!(parse_inner_condition("that player is the").is_err()); + } + #[test] fn test_you_have_the_initiative() { let (rest, c) = parse_inner_condition("you have the initiative").unwrap(); diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 154b4e2503..2561f39980 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -2185,7 +2185,9 @@ fn extra_blockers_static_gated_on_trailing_as_long_as_condition() { assert_eq!(def.affected, Some(TargetFilter::SelfRef)); assert_eq!( def.condition, - Some(StaticCondition::IsMonarch), + Some(StaticCondition::IsMonarch { + player: PlayerScope::Controller + }), "the 'as long as you're the monarch' rider must gate the extra-block grant, got {:?}", def.condition ); diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index a31f17e996..d6156f6cb5 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -10,6 +10,7 @@ use crate::types::ability::{ SpellStackToGraveyardReplacement, }; use crate::types::counter::{CounterMatch, CounterType}; +use crate::types::triggers::AttackTargetFilter; #[test] fn unsupported_ability_ir_lowering_preserves_generic_and_structural_payloads() { @@ -13056,8 +13057,13 @@ fn become_the_monarch_imperative() { use crate::parser::oracle_effect::parse_effect; let effect = parse_effect("become the monarch"); assert!( - matches!(effect, Effect::BecomeMonarch), - "expected BecomeMonarch, got {:?}", + matches!( + effect, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ), + "expected BecomeMonarch{{Controller}}, got {:?}", effect, ); } @@ -13067,8 +13073,13 @@ fn you_become_the_monarch_subject() { use crate::parser::oracle_effect::parse_effect; let effect = parse_effect("you become the monarch"); assert!( - matches!(effect, Effect::BecomeMonarch), - "expected BecomeMonarch, got {:?}", + matches!( + effect, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ), + "expected BecomeMonarch{{Controller}}, got {:?}", effect, ); } @@ -13168,8 +13179,13 @@ fn heart_shaped_herb_activated_ability_grants_monarch_as_continuation() { .as_ref() .expect("the 'and you become the monarch' conjunct must be recovered"); assert!( - matches!(*monarch.effect, Effect::BecomeMonarch), - "expected BecomeMonarch, got {:?}", + matches!( + *monarch.effect, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ), + "expected BecomeMonarch{{Controller}}, got {:?}", monarch.effect, ); // CR 608.2c: a ContinuationStep under the gated return is skipped when the @@ -13225,8 +13241,13 @@ fn fall_from_favor_trigger_body_grants_monarch_not_unimplemented() { .as_ref() .expect("the 'and you become the monarch' conjunct must be recovered"); assert!( - matches!(*monarch.effect, Effect::BecomeMonarch), - "expected BecomeMonarch, got {:?}", + matches!( + *monarch.effect, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ), + "expected BecomeMonarch{{Controller}}, got {:?}", monarch.effect, ); assert!( @@ -13253,8 +13274,13 @@ fn you_become_monarch_sub_link_tracks_boundary_not_verb() { .as_ref() .expect("sentence-boundary monarch clause must be present"); assert!( - matches!(*monarch.effect, Effect::BecomeMonarch), - "expected BecomeMonarch, got {:?}", + matches!( + *monarch.effect, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ), + "expected BecomeMonarch{{Controller}}, got {:?}", monarch.effect, ); assert_eq!( @@ -20153,6 +20179,418 @@ fn eomer_of_the_riddermark_attack_gate_parses_as_trigger_condition() { ); } +/// M'Baku, Jabari Chieftain — verbatim Scryfall Oracle text. +const MBAKU_ORACLE: &str = "At the beginning of your end step, if there is no monarch, target opponent becomes the monarch.\nWhenever a creature attacks one of your opponents, if that player is the monarch, that creature gets +1/+1 and gains trample until end of turn."; + +fn parse_mbaku() -> ParsedAbilities { + parse( + MBAKU_ORACLE, + "M'Baku, Jabari Chieftain", + &[], + &["Creature"], + &["Human", "Noble", "Warrior"], + ) +} + +/// CR 603.4 + CR 508.5 + CR 725.1: M'Baku's second trigger must retain its +/// intervening-if, bound to the ATTACKED player (CR 508.5), not dropped as a +/// swallowed clause and not left anchored to the attacking player. +/// +/// Revert-failing three ways: without the parser subject axis the condition is +/// `None`; without the `PlayerScope` parameterization the bridge cannot carry a +/// subject at all; without the attack anaphor rebind the condition is +/// `IsMonarch { ScopedPlayer }`, which resolves to the ATTACKING player. +#[test] +fn mbaku_attack_trigger_keeps_monarch_intervening_if_bound_to_defending_player() { + let result = parse_mbaku(); + + assert!( + !parsed_has_unimplemented(&result), + "M'Baku must parse with zero Unimplemented effects: {result:#?}" + ); + assert_eq!(result.triggers.len(), 2, "triggers={:?}", result.triggers); + + let end_step = &result.triggers[0]; + assert_eq!(end_step.mode, TriggerMode::Phase); + assert_eq!(end_step.condition, Some(TriggerCondition::NoMonarch)); + + let attack = &result.triggers[1]; + assert_eq!(attack.mode, TriggerMode::Attacks); + assert_eq!( + attack.condition, + Some(TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }), + "intervening-if must bind the attacked player (CR 508.5), got {:?}", + attack.condition + ); + + // The event clause and the effect body must be unchanged by the rebind. + assert_eq!( + attack.attack_target_filter, + Some(AttackTargetFilter::Player) + ); + assert!(attack.valid_card.is_some(), "attacker filter must survive"); + assert!( + attack.valid_target.is_some(), + "attacked-player filter must survive" + ); + let execute = attack + .execute + .as_ref() + .expect("attack trigger should have an execute body"); + let Effect::GenericEffect { + static_abilities, .. + } = execute.effect.as_ref() + else { + panic!( + "expected the +1/+1 and trample grant, got {:?}", + execute.effect + ); + }; + let static_def = static_abilities + .first() + .expect("M'Baku's grant must contain a static ability"); + assert!( + static_def + .modifications + .contains(&ContinuousModification::AddPower { value: 1 }) + && static_def + .modifications + .contains(&ContinuousModification::AddToughness { value: 1 }) + && static_def + .modifications + .contains(&ContinuousModification::AddKeyword { + keyword: Keyword::Trample, + }), + "expected the +1/+1 and trample grant, got {static_def:?}" + ); + + // CR L4: the Condition_If swallow warning must be cleared. + assert!( + result.parse_warnings.iter().all(|w| !matches!( + w, + OracleDiagnostic::SwallowedClause { detector, .. } if detector == "Condition_If" + )), + "unexpected Condition_If SwallowedClause: {:?}", + result.parse_warnings + ); +} + +/// CR 115.1: the `ControllerRef` a player-scoped `TargetFilter` restricts to, if +/// any. `None` for an unrestricted `TargetFilter::Player`. +fn player_filter_controller_ref(filter: &TargetFilter) -> Option<&ControllerRef> { + match filter { + TargetFilter::Typed(TypedFilter { controller, .. }) => controller.as_ref(), + _ => None, + } +} + +/// CR 115.1 + CR 725.1 + CR 109.5: M'Baku's FIRST trigger must carry the printed +/// subject of "target opponent becomes the monarch" onto +/// `Effect::BecomeMonarch`'s `target` axis, with the OPPONENT restriction +/// intact. +/// +/// Revert-failing: with the pre-axis unit variant the effect equality below +/// fails outright; with a bare `TargetFilter::Player` (or `Controller`) the +/// controller becomes a legal target and the resolver can crown the ability's +/// own controller — the shipped bug. +/// +/// Sibling coverage: the same clause on Garland, Royal Kidnapper and Jared +/// Carthalion, True Heir is asserted by +/// `targeted_become_monarch_binds_the_opponent_filter_across_the_class`; the +/// runtime half is `mbaku_end_step_crowns_the_targeted_opponent_not_its_controller_cr_115_1` +/// in `tests/integration/mbaku_attacked_monarch_intervening_if.rs`. +#[test] +fn mbaku_end_step_trigger_binds_target_opponent_onto_become_monarch_cr_115_1() { + let result = parse_mbaku(); + let end_step = &result.triggers[0]; + let execute = end_step + .execute + .as_ref() + .expect("end-step trigger should have an execute body"); + + let Effect::BecomeMonarch { target } = &*execute.effect else { + panic!("expected BecomeMonarch, got {:?}", execute.effect); + }; + assert!( + target.is_player_scope(), + "the subject must be a player filter, got {target:?}" + ); + assert!( + !target.is_context_ref(), + "`target opponent` is a DECLARED target, not a context ref — a context \ + ref surfaces no target slot and resolves to the controller: {target:?}" + ); + assert_eq!( + player_filter_controller_ref(target), + Some(&ControllerRef::Opponent), + "CR 115.1: the opponent restriction must survive onto the effect, \ + got {target:?}" + ); +} + +/// CR 115.1 + CR 725.1: the same "target opponent becomes the monarch" clause on +/// every printing that has it. Build-for-the-class guard — the fix is in +/// `build_become_clause`'s subject mapping, not in anything M'Baku-specific, so +/// the siblings must lower identically. +#[test] +fn targeted_become_monarch_binds_the_opponent_filter_across_the_class() { + let cards: [(&str, &str); 2] = [ + ( + "Garland, Royal Kidnapper", + "When Garland enters, target opponent becomes the monarch.", + ), + ( + "Jared Carthalion, True Heir", + "When Jared Carthalion enters, target opponent becomes the monarch.", + ), + ]; + + for (name, oracle) in cards { + let parsed = parse(oracle, name, &[], &["Creature"], &["Human"]); + let trigger = parsed + .triggers + .first() + .unwrap_or_else(|| panic!("{name}: enters trigger must parse")); + let execute = trigger + .execute + .as_ref() + .unwrap_or_else(|| panic!("{name}: trigger must have an execute body")); + let Effect::BecomeMonarch { target } = &*execute.effect else { + panic!("{name}: expected BecomeMonarch, got {:?}", execute.effect); + }; + assert_eq!( + player_filter_controller_ref(target), + Some(&ControllerRef::Opponent), + "{name}: CR 115.1 opponent restriction must survive, got {target:?}" + ); + } +} + +/// CR 109.5: the UNTARGETED form keeps the printed default. Paired with the two +/// rows above so a fix that binds every subject to a target slot — which would +/// make "you become the monarch" prompt for a target it never had — fails here. +#[test] +fn untargeted_become_monarch_keeps_the_controller_default_cr_109_5() { + use crate::parser::oracle_effect::parse_effect; + + for text in ["become the monarch", "you become the monarch"] { + assert_eq!( + parse_effect(text), + Effect::BecomeMonarch { + target: TargetFilter::Controller + }, + "{text:?} must keep the printed-default subject" + ); + } +} + +/// CR 508.5: the rebind is gated on the trigger clause naming an attacked +/// PLAYER. `Planeswalker` / `Battle` attack scopes name no player antecedent +/// (a battle's anaphor would be its protector, CR 310.8d — a different +/// reference), so a `ScopedPlayer` anchor must survive unchanged there. +/// +/// There are two distinct noun sources, and each is asserted in the shape the +/// PARSER emits: `Player` / `PlayerOrPlaneswalker` carry the named player in +/// `valid_target`, while `Monarch` (CR 725.1) is the noun on its own and comes +/// with `valid_target: None`. +#[test] +fn attack_anaphor_rebind_gate_covers_only_player_yielding_attack_scopes() { + use crate::parser::oracle_trigger::attack_intervening_if_anaphor_is_defending_player; + + let base = |filter: Option| { + let mut def = TriggerDefinition::new(TriggerMode::Attacks); + def.valid_card = Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: None, + properties: vec![], + })); + def.valid_target = Some(TargetFilter::Player); + def.attack_target_filter = filter; + def + }; + + let attack_target_scopes = [ + AttackTargetFilter::Player, + AttackTargetFilter::Planeswalker, + AttackTargetFilter::PlayerOrPlaneswalker, + AttackTargetFilter::Battle, + AttackTargetFilter::Owner, + AttackTargetFilter::OwnerOrPlaneswalker, + AttackTargetFilter::PlayerOrPermanents, + AttackTargetFilter::Monarch, + ]; + for scope in attack_target_scopes { + let expected_rebind = match scope { + AttackTargetFilter::Player + | AttackTargetFilter::PlayerOrPlaneswalker + | AttackTargetFilter::Monarch => true, + AttackTargetFilter::Planeswalker + | AttackTargetFilter::Battle + | AttackTargetFilter::Owner + | AttackTargetFilter::OwnerOrPlaneswalker + | AttackTargetFilter::PlayerOrPermanents => false, + }; + assert_eq!( + attack_intervening_if_anaphor_is_defending_player(&base(Some(scope.clone()))), + expected_rebind, + "{scope:?} rebind classification must be exhaustive" + ); + } + // No attack-target clause at all (Goblin Guide / Ulamog shape). + assert!(!attack_intervening_if_anaphor_is_defending_player(&base( + None + ))); + + // CR 725.1 + CR 508.5: `Monarch` is the one attack scope that names the + // attacked player BY ITSELF, and it is the shape the parser actually emits + // — "attacks the monarch" lowers to `attack_target_filter: Monarch` with + // `valid_target: None` (verified against The Spear of Bashenga's row in + // `data/card-data.json`). Asserting it through `base(..)`, which forces + // `valid_target: Some(Player)`, tested a combination no card produces and + // left the production arm dead. + let mut monarch_production_shape = base(Some(AttackTargetFilter::Monarch)); + monarch_production_shape.valid_target = None; + assert!( + attack_intervening_if_anaphor_is_defending_player(&monarch_production_shape), + "the monarch designation IS the attacked-player noun; requiring \ + `valid_target` makes this arm unreachable: {monarch_production_shape:?}" + ); + // …and it still rebinds in the redundant belt-and-braces shape. + assert!(attack_intervening_if_anaphor_is_defending_player(&base( + Some(AttackTargetFilter::Monarch) + ))); + + // Discrimination guard: `valid_target` is NOT dispensable in general. The + // `Player` / `PlayerOrPlaneswalker` scopes say a player may be attacked, not + // WHICH one, so without the filter the clause is the bare "Whenever ~ + // attacks" shape and supplies no antecedent. + for scope in [ + AttackTargetFilter::Player, + AttackTargetFilter::PlayerOrPlaneswalker, + ] { + let mut no_target = base(Some(scope.clone())); + no_target.valid_target = None; + assert!( + !attack_intervening_if_anaphor_is_defending_player(&no_target), + "{scope:?} without a named attacked player must not rebind" + ); + } + + // CR 508.3d: an OBJECT filter in `valid_source` is an attacking-CREATURE + // noun, not a player noun, and must NOT block the rebind. Discriminates + // against a `valid_source.is_none()` proxy. + let mut object_source = base(Some(AttackTargetFilter::Player)); + object_source.valid_source = Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: None, + properties: vec![], + })); + assert!( + attack_intervening_if_anaphor_is_defending_player(&object_source), + "an object-shaped valid_source must still rebind" + ); + + // "Whenever a PLAYER attacks ..." — the attacking player is also a + // candidate antecedent, so the rebind must not fire (Suppressor Skyguard). + let mut player_source = base(Some(AttackTargetFilter::Player)); + player_source.valid_source = Some(TargetFilter::Player); + assert!(!attack_intervening_if_anaphor_is_defending_player( + &player_source + )); + + // CR 603.2: a non-attack mode reaches the same anaphor with a different + // antecedent (Ghirapur Orrery). + let mut phase_mode = base(Some(AttackTargetFilter::Player)); + phase_mode.mode = TriggerMode::Phase; + assert!(!attack_intervening_if_anaphor_is_defending_player( + &phase_mode + )); +} + +/// CR 603.2: sibling trigger shapes that reach the same "that player" anaphor +/// must be byte-identical after the rebind. Each assertion is paired with a +/// positive reach-guard that the condition is `Some(..)`, so a parse regression +/// cannot make the negative pass vacuously. +#[test] +fn attack_anaphor_rebind_leaves_sibling_trigger_shapes_untouched() { + use crate::parser::oracle_trigger::attack_intervening_if_anaphor_is_defending_player; + + // "Whenever a PLAYER attacks you" — player-scope `valid_source`, so the + // attacking player is also a candidate antecedent. + let skyguard = parse( + "Flying\nWhenever a player attacks you, if that player has another opponent who isn't being attacked, prevent all combat damage that would be dealt to you this combat.", + "Suppressor Skyguard", + &[Keyword::Flying], + &["Creature"], + &["Bird", "Soldier"], + ); + let skyguard_trigger = skyguard + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .expect("Suppressor Skyguard must keep its attack trigger"); + // Reach-guard: the intervening-if is present, so the negative below is not + // vacuous. + assert!( + skyguard_trigger.condition.is_some(), + "Suppressor Skyguard must keep its intervening-if" + ); + assert!( + !attack_intervening_if_anaphor_is_defending_player(skyguard_trigger), + "an attacking-PLAYER clause supplies its own antecedent; the rebind must \ + not fire: {skyguard_trigger:?}" + ); + + // `Phase` mode — same anaphor, different antecedent. + let orrery = parse( + "Each player may play an additional land on each of their turns.\nAt the beginning of each player's upkeep, if that player has no cards in hand, that player draws three cards.", + "Ghirapur Orrery", + &[], + &["Artifact"], + &[], + ); + let orrery_trigger = orrery + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Phase) + .expect("Ghirapur Orrery must keep its upkeep trigger"); + assert!( + orrery_trigger.condition.is_some(), + "Ghirapur Orrery must keep its intervening-if" + ); + assert!( + !attack_intervening_if_anaphor_is_defending_player(orrery_trigger), + "a non-attack mode must not reach the attack anaphor rebind: {orrery_trigger:?}" + ); + + // Aerial Surveyor: `Attacks` mode whose defending-player reference is a + // `ControllerRef` inside a `TargetFilter`, not a `PlayerScope`. It has no + // `valid_target`, so the gate excludes it and its condition is untouched. + let surveyor = parse( + "Flying\nWhenever this Vehicle attacks, if defending player controls more lands than you, search your library for a basic Plains card, put it onto the battlefield tapped, then shuffle.\nCrew 2", + "Aerial Surveyor", + &[Keyword::Flying], + &["Artifact"], + &["Vehicle"], + ); + let surveyor_trigger = surveyor + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .expect("Aerial Surveyor must keep its attack trigger"); + assert!( + surveyor_trigger.condition.is_some(), + "Aerial Surveyor must keep its intervening-if" + ); + assert!( + !attack_intervening_if_anaphor_is_defending_player(surveyor_trigger), + "Aerial Surveyor names no attacked player and must not be rebound: \ + {surveyor_trigger:?}" + ); +} + fn assert_controlled_creature_greatest_power_ability_gate(condition: &AbilityCondition) { let AbilityCondition::QuantityCheck { lhs: QuantityExpr::Ref { diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index 5ff669c1d1..c88a82befe 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -1935,6 +1935,23 @@ pub(crate) fn lower_trigger_ir(ir: &TriggerIr) -> TriggerDefinition { } } + // CR 508.5 + CR 603.2 + CR 603.4: resolve the intervening-if's "that player" + // anaphor against the trigger clause's own player nouns. On + // "Whenever a creature attacks one of your opponents, if that player ...", + // the antecedent is the ATTACKED player, but `parse_inner_condition` cannot + // see the trigger clause and emits the generic `ScopedPlayer` anchor — which + // `targeting::extract_player_from_event` resolves to the ATTACKING player + // for an `AttackersDeclared` event. Placed here, after the intervening-if + // has been ANDed onto `def.condition` above and after the event clause has + // produced `mode`/`valid_source`/`valid_target`/`attack_target_filter`, + // because the condition parser can see none of them. Direct sibling of the + // spell-cast anaphor remap directly above. + if attack_intervening_if_anaphor_is_defending_player(&def) { + if let Some(cond) = def.condition.as_mut() { + rebind_attack_anaphor_to_defending_player(cond); + } + } + // CR 121.1 + CR 603.4: "draw cards equal to the difference" inside a // trigger body (Kozilek the Great Distortion, Damia Sage of Stone, Krang // Master Mind, The Ten Rings, Doctor Octopus). The "if you have fewer than @@ -4425,6 +4442,152 @@ fn remap_self_cast_scope_in_quantity(expr: &mut QuantityExpr) { } } +/// CR 508.5 + CR 603.2 + CR 603.4: True when a bare "that player"/"that +/// opponent" anaphor inside this attack trigger's intervening-if has the +/// ATTACKED player as its only antecedent. +/// +/// Three conjuncts, each with its own reason: +/// - `mode == Attacks` — only attack declarations produce an attacked-player +/// noun. Ghirapur Orrery's `Phase` mode reaches the same anaphor with a +/// different antecedent and must not be touched. +/// - `valid_source` is absent OR is not a player-scope filter — the clause names +/// no ATTACKING-PLAYER noun. Same predicate the runtime matcher gates on in +/// `trigger_matchers::matching_attack_events`. "Whenever a CREATURE attacks +/// one of your opponents" qualifies; "Whenever a PLAYER attacks you" +/// (Suppressor Skyguard) does not, because there the attacking player is also +/// a candidate antecedent. An OBJECT filter in `valid_source` is an +/// attacking-creature noun, not a player noun, and must NOT block the rebind. +/// - the clause names an ATTACKED-PLAYER noun — see +/// [`attack_clause_names_attacked_player`], which owns that question. +/// +/// When the attacked-player noun is the controller ("attacks you"), the attacked +/// player IS the controller, so `DefendingPlayer` and the controller name the +/// same player and the rebind cannot diverge. +// pub(crate) so the `AttackTargetFilter` variant matrix in `oracle_tests.rs` +// can drive the production gate directly rather than a hand-built proxy. +pub(crate) fn attack_intervening_if_anaphor_is_defending_player(def: &TriggerDefinition) -> bool { + def.mode == TriggerMode::Attacks + && def + .valid_source + .as_ref() + .is_none_or(|filter| !filter.is_player_scope()) + && attack_clause_names_attacked_player(def) +} + +/// CR 508.5 + CR 725.1: does this attack clause name a unique ATTACKED PLAYER — +/// the antecedent a bare "that player" anaphor needs? +/// +/// Two distinct noun sources, which is why this is a `match` on +/// `attack_target_filter` rather than one flat `valid_target.is_some()` conjunct: +/// +/// - `Monarch` is SELF-SUFFICIENT. "attacks the monarch" names the attacked +/// player by DESIGNATION (CR 725.1), and the parser emits it with +/// `valid_target: None` — the designation is the whole noun, so there is no +/// residual player filter to record (The Spear of Bashenga). Requiring +/// `valid_target` here made this arm dead on the production path: a real +/// "Whenever a creature attacks the monarch, if that player …" card failed the +/// gate and kept the `ScopedPlayer` anchor, which +/// `targeting::extract_player_from_event` resolves to the ATTACKING player for +/// `AttackersDeclared`. `Monarch` is a Player-type attack per +/// `trigger_matchers::attack_target_type_matches`, with an added CR 725.1 +/// identity constraint, so the defending player is still unique. +/// - `Player` / `PlayerOrPlaneswalker` are attack SCOPES, not nouns: they say a +/// player may be attacked, not WHICH one. The named player lives in +/// `valid_target` ("attacks you", "attacks one of your opponents"), so without +/// it the clause is the bare "Whenever ~ attacks" shape (Goblin Guide) and +/// supplies no antecedent. +/// +/// `Planeswalker` and `Battle` name no player noun at all — the correct anaphor +/// for a battle would be "its protector" (CR 310.8d), a different reference. +/// `Owner`, `OwnerOrPlaneswalker` and `PlayerOrPermanents` are attack-RESTRICTION +/// scopes with no arm in `attack_target_type_matches`, so a trigger carrying one +/// never fires at all. Exhaustive — a future attack scope must decide here. +fn attack_clause_names_attacked_player(def: &TriggerDefinition) -> bool { + match def.attack_target_filter.as_ref() { + Some(AttackTargetFilter::Monarch) => true, + Some(AttackTargetFilter::Player | AttackTargetFilter::PlayerOrPlaneswalker) => { + def.valid_target.is_some() + } + Some( + AttackTargetFilter::Planeswalker + | AttackTargetFilter::Battle + | AttackTargetFilter::Owner + | AttackTargetFilter::OwnerOrPlaneswalker + | AttackTargetFilter::PlayerOrPermanents, + ) + | None => false, + } +} + +/// CR 508.5 + CR 603.2: Rebind [`PlayerScope::ScopedPlayer`] to +/// [`PlayerScope::DefendingPlayer`] throughout an attack trigger's +/// intervening-if. +/// +/// `parse_inner_condition` cannot see the owning trigger, so the generic anaphor +/// lands as `ScopedPlayer`, which resolves through +/// `targeting::extract_player_from_event` — for `GameEvent::AttackersDeclared` +/// that is the ATTACKING player, not the attacked one. Only here, where the +/// trigger clause's player nouns are known, can the antecedent be resolved. +/// Direct sibling of `remap_self_cast_scope_to_triggering_spell` above, which +/// does the same job for the spell-cast "it"/"this spell" anaphor. +/// +/// Generic over the condition tree, not specific to any predicate: it repairs +/// the antecedent for `IsMonarch { player }` and for every `QuantityRef` +/// carrying a player axis (hand size, life total, cards drawn, …). +/// `ControllerRef::ScopedPlayer` inside a nested `TargetFilter` is deliberately +/// NOT rewritten — no "that player controls …" grammar currently produces one, +/// and rewriting controller axes inside arbitrary filters would over-reach. +fn rebind_attack_anaphor_to_defending_player(cond: &mut TriggerCondition) { + match cond { + TriggerCondition::IsMonarch { + player: player @ PlayerScope::ScopedPlayer, + } => *player = PlayerScope::DefendingPlayer, + TriggerCondition::QuantityComparison { lhs, rhs, .. } => { + rebind_attack_anaphor_in_quantity(lhs); + rebind_attack_anaphor_in_quantity(rhs); + } + TriggerCondition::And { conditions } | TriggerCondition::Or { conditions } => { + conditions + .iter_mut() + .for_each(rebind_attack_anaphor_to_defending_player); + } + TriggerCondition::Not { condition } => rebind_attack_anaphor_to_defending_player(condition), + // All other variants are leaves that carry no `PlayerScope`, which + // `TriggerCondition::designation_player_anchor` enforces exhaustively + // for the designation family — nothing to rebind. + _ => {} + } +} + +/// Recursive `QuantityExpr` companion of +/// [`rebind_attack_anaphor_to_defending_player`]. Exhaustive over `QuantityExpr` +/// — no wildcard — so a new arithmetic wrapper is a compile error rather than a +/// silently skipped nested [`PlayerScope`]. The leaf question is delegated to +/// the equally exhaustive [`QuantityRef::player_scope_mut`]. +fn rebind_attack_anaphor_in_quantity(expr: &mut QuantityExpr) { + match expr { + QuantityExpr::Ref { qty } => { + if let Some(player @ PlayerScope::ScopedPlayer) = qty.player_scope_mut() { + *player = PlayerScope::DefendingPlayer; + } + } + QuantityExpr::Fixed { .. } => {} + QuantityExpr::DivideRounded { inner, .. } + | QuantityExpr::Offset { inner, .. } + | QuantityExpr::ClampMin { inner, .. } + | QuantityExpr::Multiply { inner, .. } => rebind_attack_anaphor_in_quantity(inner), + QuantityExpr::UpTo { max } => rebind_attack_anaphor_in_quantity(max), + QuantityExpr::Power { exponent, .. } => rebind_attack_anaphor_in_quantity(exponent), + QuantityExpr::Difference { left, right } => { + rebind_attack_anaphor_in_quantity(left); + rebind_attack_anaphor_in_quantity(right); + } + QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => { + exprs.iter_mut().for_each(rebind_attack_anaphor_in_quantity); + } + } +} + // pub(crate) so the runtime gate tests in game/triggers.rs can drive the // production bridge directly (discriminating against this function rather than a // hand-built filter). Sole non-test caller remains parse_trigger_line below. @@ -4599,9 +4762,14 @@ pub(crate) fn static_condition_to_trigger_condition( StaticCondition::SourceIsTapped => Some(TriggerCondition::Not { condition: Box::new(TriggerCondition::SourceIsTapped), }), - // CR 725.1: "if you're not the monarch" / "if an opponent is the monarch". - StaticCondition::IsMonarch => Some(TriggerCondition::Not { - condition: Box::new(TriggerCondition::IsMonarch), + // CR 725.1 + CR 109.5: "if you're not the monarch" / "if an opponent + // is the monarch". The subject scope must survive the bridge — + // collapsing it to `Controller` here would silently rebind + // "that player is the monarch" to the ability's controller. + StaticCondition::IsMonarch { player } => Some(TriggerCondition::Not { + condition: Box::new(TriggerCondition::IsMonarch { + player: player.clone(), + }), }), // CR 725.1: "if there is a monarch" (negated no-monarch check). StaticCondition::NoMonarch => Some(TriggerCondition::Not { @@ -4662,8 +4830,11 @@ pub(crate) fn static_condition_to_trigger_condition( }) } - // CR 725.1: Monarch status bridges directly. - StaticCondition::IsMonarch => Some(TriggerCondition::IsMonarch), + // CR 725.1 + CR 109.5: Monarch status bridges directly, carrying its + // subject scope. Never collapse `player` to `Controller` here. + StaticCondition::IsMonarch { player } => Some(TriggerCondition::IsMonarch { + player: player.clone(), + }), // CR 726.3: Initiative status bridges directly. StaticCondition::IsInitiative => Some(TriggerCondition::IsInitiative), // CR 725.1: "there is no monarch" bridges directly. diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 86b9390f83..9997dc64b3 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -21073,8 +21073,80 @@ fn parse_hixus_keeps_entered_this_turn_intervening_if() { #[test] fn bridge_monarch() { assert_eq!( - static_condition_to_trigger_condition(&StaticCondition::IsMonarch), - Some(TriggerCondition::IsMonarch), + static_condition_to_trigger_condition(&StaticCondition::IsMonarch { + player: PlayerScope::Controller + }), + Some(TriggerCondition::IsMonarch { + player: PlayerScope::Controller + }), + ); +} + +/// CR 725.1 + CR 109.5: the subject scope must SURVIVE the static→trigger +/// bridge. Dropping it here is the silent-degradation failure mode — the +/// condition would keep parsing but rebind to the ability's controller. +/// +/// Revert-failing against an `IsMonarch { .. } => IsMonarch { Controller }` arm. +#[test] +fn bridge_monarch_carries_a_non_controller_subject_scope() { + assert_eq!( + static_condition_to_trigger_condition(&StaticCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + }), + Some(TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + }), + ); + assert_eq!( + static_condition_to_trigger_condition(&StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + }), + Some(TriggerCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + }), + ); + // The negated bridge arm ("if you're not the monarch") must carry it too. + assert_eq!( + static_condition_to_trigger_condition(&StaticCondition::Not { + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + }), + }), + Some(TriggerCondition::Not { + condition: Box::new(TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + }), + }), + ); +} + +/// CR 725.1: `AbilityCondition` has no player axis, so a scoped monarch gate +/// must FAIL CLOSED rather than lower to the controller-scoped variant. +/// +/// Revert-failing against a `{ .. }`-collapsing arm, which would return +/// `Some(IsMonarch)` for the scoped form and silently rebind it. +#[test] +fn ability_condition_lowering_refuses_a_scoped_monarch_gate() { + use crate::parser::oracle_effect::conditions::static_condition_to_ability_condition; + use crate::parser::oracle_ir::context::ParseContext; + + assert_eq!( + static_condition_to_ability_condition( + &StaticCondition::IsMonarch { + player: PlayerScope::Controller + }, + &mut ParseContext::default() + ), + Some(AbilityCondition::IsMonarch), + ); + assert_eq!( + static_condition_to_ability_condition( + &StaticCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + }, + &mut ParseContext::default() + ), + None, ); } @@ -21083,7 +21155,9 @@ fn bridge_opponent_is_monarch_intervening_if() { let sc = StaticCondition::And { conditions: vec![ StaticCondition::Not { - condition: Box::new(StaticCondition::IsMonarch), + condition: Box::new(StaticCondition::IsMonarch { + player: PlayerScope::Controller, + }), }, StaticCondition::Not { condition: Box::new(StaticCondition::NoMonarch), @@ -21095,7 +21169,9 @@ fn bridge_opponent_is_monarch_intervening_if() { Some(TriggerCondition::And { conditions: vec![ TriggerCondition::Not { - condition: Box::new(TriggerCondition::IsMonarch), + condition: Box::new(TriggerCondition::IsMonarch { + player: PlayerScope::Controller + }), }, TriggerCondition::Not { condition: Box::new(TriggerCondition::NoMonarch), @@ -21121,7 +21197,7 @@ fn queen_marchesa_upkeep_attaches_opponent_monarch_intervening_if() { conditions[0], TriggerCondition::Not { condition: ref inner, - } if matches!(inner.as_ref(), TriggerCondition::IsMonarch) + } if matches!(inner.as_ref(), TriggerCondition::IsMonarch { player: PlayerScope::Controller }) ) && matches!( conditions[1], diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 48e3d75c76..8f7b84c215 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -5873,6 +5873,45 @@ pub enum PlayerScope { SpecificPlayer { id: PlayerId }, } +/// CR 109.5: SINGLE serde default for every [`PlayerScope`] subject axis — a +/// clause with no printed subject means "you", the ability's controller. Keeps +/// pre-field rows (`{"type":"IsMonarch"}`, `GrantNextSpellAbility` without +/// `player`) deserializing unchanged. +/// +/// A named function rather than `#[serde(default)]` because [`PlayerScope`] +/// deliberately has no `Default` impl: most of its variants are only meaningful +/// relative to a context, so a blanket default would be wrong everywhere else. +fn player_scope_controller() -> PlayerScope { + PlayerScope::Controller +} + +/// Skip-serialization predicate paired with [`player_scope_controller`]: omit +/// `player` from JSON when it is the printed default, so existing card-data +/// rows stay byte-identical. +fn is_player_scope_controller(player: &PlayerScope) -> bool { + matches!(player, PlayerScope::Controller) +} + +impl PlayerScope { + /// CR 611.2a + CR 514.2: [`PlayerScope::AnyTurn`] and + /// [`PlayerScope::SpecificPlayer`] exist ONLY to key a `Duration`'s expiry. + /// They are never produced by the parser and carry no value / quantity / + /// player-selection reading, which is why + /// `quantity::resolve_single_player_scope` marks them `unreachable!()`. + /// + /// Any resolver reachable from DESERIALIZED data must reject them here + /// rather than reaching that `unreachable!()`: `IsMonarch { player }` is + /// serde-constructible from `card-data.json` and from mtgish input, so a + /// malformed or hand-authored row must fail closed, not panic the engine + /// inside a trigger-condition check. + pub(crate) fn duration_timing_only(&self) -> bool { + matches!( + self, + PlayerScope::AnyTurn | PlayerScope::SpecificPlayer { .. } + ) + } +} + /// Scope selector for object-axis quantities (Round Π-5). Picks WHICH object /// to read from when a `QuantityRef` (and future per-object conditions) is /// per-object. Mirrors `PlayerScope` for the player axis. @@ -7163,6 +7202,113 @@ pub enum QuantityRef { VoteCount { choice_index: u32 }, } +impl QuantityRef { + /// CR 109.4: mutable access to this reference's single player-relativity + /// axis, when it has one. + /// + /// The mutable sibling of the read-only per-axis classifiers + /// (`ability_scan::scan_player_scope`, `ability_rw::rw_player_scope`), and + /// exhaustive for the same reason `ability_scan::scan_quantity_ref` is: a + /// future reference that carries a [`PlayerScope`] must be a COMPILE ERROR + /// here rather than silently escape an anaphor rebind. Object-axis + /// references and player-axis references that resolve through an + /// `AggregateFunction` population rather than a single scope return `None`. + pub(crate) fn player_scope_mut(&mut self) -> Option<&mut PlayerScope> { + match self { + QuantityRef::HandSize { player } + | QuantityRef::LifeTotal { player } + | QuantityRef::GraveyardSize { player } + | QuantityRef::LifeLostThisTurn { player } + | QuantityRef::PartySize { player } + | QuantityRef::Speed { player } + | QuantityRef::SacrificedThisTurn { player, .. } + | QuantityRef::LifeGainedThisTurn { player } + | QuantityRef::CardsDrawnThisTurn { player } + | QuantityRef::BattlefieldEntriesThisTurn { player, .. } + | QuantityRef::LandsPlayedThisTurn { player, .. } + | QuantityRef::PlayerChosenNumber { player } + | QuantityRef::LoyaltyAbilitiesActivatedThisTurn { player } + | QuantityRef::CardsDiscardedThisTurn { player } + | QuantityRef::TokensCreatedThisTurn { player, .. } + | QuantityRef::PlayerActionsThisTurn { player, .. } => Some(player), + QuantityRef::LifeAboveStarting + | QuantityRef::StartingLifeTotal + | QuantityRef::TriggeringDiscoverValue + | QuantityRef::TriggeringScryLookCount + | QuantityRef::TriggeringScryBottomCount + | QuantityRef::ObjectCount { .. } + | QuantityRef::ObjectCountDistinct { .. } + | QuantityRef::ObjectCountBySharedQuality { .. } + | QuantityRef::PlayerCount { .. } + | QuantityRef::CountersOn { .. } + | QuantityRef::CountersOnObjects { .. } + | QuantityRef::PlayerCounter { .. } + | QuantityRef::TargetControllerCounter { .. } + | QuantityRef::Variable { .. } + | QuantityRef::Power { .. } + | QuantityRef::Intensity { .. } + | QuantityRef::Toughness { .. } + | QuantityRef::ObjectManaValue { .. } + | QuantityRef::TargetObjectManaValue { .. } + | QuantityRef::ObjectColorCount { .. } + | QuantityRef::ObjectNameWordCount { .. } + | QuantityRef::ObjectTypelineComponentCount { .. } + | QuantityRef::ManaSymbolsInManaCost { .. } + | QuantityRef::SelfManaValue + | QuantityRef::Aggregate { .. } + | QuantityRef::ControlledByEachPlayer { .. } + | QuantityRef::TargetZoneCardCount { .. } + | QuantityRef::Devotion { .. } + | QuantityRef::DistinctCardTypes { .. } + | QuantityRef::DistinctSubtypes { .. } + | QuantityRef::CardsExiledBySource + | QuantityRef::ExiledCardPower { .. } + | QuantityRef::ZoneCardCount { .. } + | QuantityRef::BasicLandTypeCount { .. } + | QuantityRef::TrackedSetSize + | QuantityRef::FilteredTrackedSetSize { .. } + | QuantityRef::TrackedSetAggregate { .. } + | QuantityRef::ExiledFromHandThisResolution + | QuantityRef::PreviousEffectAmount { .. } + | QuantityRef::UnspentMana { .. } + | QuantityRef::EventContextAmount + | QuantityRef::EventContextPlayerCount { .. } + | QuantityRef::AttachmentsOnLeavingObject { .. } + | QuantityRef::EventContextSourceCostX + | QuantityRef::EventContextSourceModesChosen + | QuantityRef::SpellsCastThisTurn { .. } + | QuantityRef::SpellsCastBeforeTriggeringSpell { .. } + | QuantityRef::EnteredThisTurn { .. } + | QuantityRef::CrimesCommittedThisTurn + | QuantityRef::BendTypesThisTurn + | QuantityRef::TurnsTaken + | QuantityRef::ZoneChangeCountThisTurn { .. } + | QuantityRef::ZoneChangeAggregateThisTurn { .. } + | QuantityRef::DamageDealtThisTurn { .. } + | QuantityRef::ChosenNumber + | QuantityRef::AttackedThisTurn { .. } + | QuantityRef::DescendedThisTurn + | QuantityRef::SpellsCastLastTurn + | QuantityRef::SpellsCastThisGame { .. } + | QuantityRef::CounterAddedThisTurn { .. } + | QuantityRef::DungeonsCompleted + | QuantityRef::CostXPaid + | QuantityRef::KickerCount + | QuantityRef::AdditionalCostPaymentCount + | QuantityRef::AdditionalCostPaymentCountFor { .. } + | QuantityRef::ConvokedCreatureCount + | QuantityRef::TimesCostPaidThisResolution + | QuantityRef::ManaSpentToCast { .. } + | QuantityRef::ColorsInCommandersColorIdentity + | QuantityRef::CommanderCastFromCommandZoneCount + | QuantityRef::CommanderManaValue { .. } + | QuantityRef::DistinctColorsAmong { .. } + | QuantityRef::DistinctCounterKindsAmong { .. } + | QuantityRef::VoteCount { .. } => None, + } + } +} + /// CR 107.1a: Rounding direction for fractional Oracle-text expressions. /// Every "half X" phrase in Oracle text specifies whether to round up or /// down; this enum records that choice verbatim so resolution is deterministic. @@ -8385,8 +8531,38 @@ pub enum StaticCondition { /// Once a creature is blocked, it remains blocked for the rest of combat even /// if all its blockers leave — mirrors `AttackerInfo.blocked` (sticky flag). SourceIsBlocked, - /// CR 725.1: True when the controller is the monarch. - IsMonarch, + /// CR 725.1: monarch IDENTITY — true when `player` currently holds the + /// monarch designation (CR 725.3: exactly one player at a time; CR 725.4 + /// governs reassignment). Distinct from [`NoMonarch`](Self::NoMonarch), + /// true only when the designation is VACANT, and from `Not(IsMonarch)`, + /// which is also true when vacant. + /// + /// CR 725.5: on the STATIC side, a continuous effect whose result depends + /// on who is currently the monarch does nothing while there is no monarch, + /// and begins to apply once a player becomes one. The + /// `layers::evaluate_condition_with_context` arm and its entry gate + /// implement exactly that. + /// + /// `player` is the CR 109.5 subject axis, parameterized within CR 725 + /// rather than proliferated into `ThatPlayerIsMonarch` siblings: + /// - [`PlayerScope::Controller`] ← "you're the monarch" (printed default) + /// - [`PlayerScope::ScopedPlayer`] ← "that player is the monarch" (an + /// anaphor to the player named by the triggering event) + /// - [`PlayerScope::DefendingPlayer`] ← the same anaphor once an attack + /// trigger's clause has bound it (CR 508.5; see + /// `parser::oracle_trigger::rebind_attack_anaphor_to_defending_player`) + /// + /// A scope the evaluator cannot resolve makes the condition UNANSWERABLE, + /// not false — see the entry-boundary gates in `game::triggers` and + /// `game::layers`, which reject the whole condition so `Not` cannot invert + /// a missing anchor into a firing trigger or an applied restriction. + IsMonarch { + #[serde( + default = "player_scope_controller", + skip_serializing_if = "is_player_scope_controller" + )] + player: PlayerScope, + }, /// CR 726.3: True when the controller has the initiative. IsInitiative, /// CR 725.1: True when no player holds the monarch designation. Distinct @@ -8652,6 +8828,86 @@ pub enum StaticCondition { None, } +impl StaticCondition { + /// CR 109.4 + CR 725.5: the player whose DESIGNATION this leaf tests, when + /// the leaf is a designation predicate at all. + /// + /// Exhaustive by design — there is deliberately no wildcard arm. This is + /// the guard that makes the static-side polarity boundary gate in + /// `game::layers` total: adding a future designation leaf that carries a + /// [`PlayerScope`] (e.g. an `IsInitiative { player }`) is a COMPILE ERROR + /// here, not a latent fail-open under [`StaticCondition::Not`]. + /// + /// Boolean combinators return `None`; the gate recurses them itself. + /// `QuantityComparison` returns `None` BY DEFINITION — it tests a quantity, + /// not a designation. + pub(crate) fn designation_player_anchor(&self) -> Option<&PlayerScope> { + match self { + StaticCondition::IsMonarch { player } => Some(player), + StaticCondition::DevotionGE { .. } + | StaticCondition::IsPresent { .. } + | StaticCondition::ChosenColorIs { .. } + | StaticCondition::ChosenLabelIs { .. } + | StaticCondition::QuantityComparison { .. } + | StaticCondition::HasMaxSpeed + | StaticCondition::SpeedGE { .. } + | StaticCondition::And { .. } + | StaticCondition::Or { .. } + | StaticCondition::Not { .. } + | StaticCondition::DayNightIs { .. } + | StaticCondition::HasCounters { .. } + | StaticCondition::CastVariantPaid { .. } + | StaticCondition::RecipientHasCounters { .. } + | StaticCondition::ClassLevelGE { .. } + | StaticCondition::DefendingPlayerControls { .. } + | StaticCondition::SourceAttackingAlone + | StaticCondition::SourceIsAttacking + | StaticCondition::SourceIsBlocking + | StaticCondition::SourceIsBlocked + | StaticCondition::IsInitiative + | StaticCondition::NoMonarch + | StaticCondition::HasCityBlessing + | StaticCondition::HasEnduringStory + | StaticCondition::CompletedADungeon + | StaticCondition::WasStartingPlayer { .. } + | StaticCondition::SpellCastWithVariantThisTurn { .. } + | StaticCondition::AnyPlayerAttackedYouLastTurn + | StaticCondition::OpponentPoisonAtLeast { .. } + | StaticCondition::UnlessPay { .. } + | StaticCondition::Unrecognized { .. } + | StaticCondition::DuringYourTurn + | StaticCondition::DuringOpponentsTurn + | StaticCondition::SharesColorWithMostCommonColorAmongPermanents + | StaticCondition::SourceEnteredThisTurn + | StaticCondition::SourceHasDealtDamage + | StaticCondition::WasCast { .. } + | StaticCondition::IsRingBearer + | StaticCondition::RingLevelAtLeast { .. } + | StaticCondition::ControlsCommander { .. } + | StaticCondition::SourceIsTapped + | StaticCondition::IsTapped { .. } + | StaticCondition::SourceIsFaceUp + | StaticCondition::SourceIsSaddled + | StaticCondition::SourceControllerEquals { .. } + | StaticCondition::SourceIsEquipped + | StaticCondition::SourceIsEnchanted + | StaticCondition::SourceIsMonstrous + | StaticCondition::SourceIsHarnessed + | StaticCondition::SourceAttachedToCreature + | StaticCondition::SourceMatchesFilter { .. } + | StaticCondition::TopOfLibraryMatches { .. } + | StaticCondition::RecipientMatchesFilter { .. } + | StaticCondition::RecipientAttackingOwnerTarget { .. } + | StaticCondition::SourceIsPaired + | StaticCondition::SourceInZone { .. } + | StaticCondition::EnchantedIsFaceDown + | StaticCondition::AdditionalCostPaid + | StaticCondition::CastingAsVariant { .. } + | StaticCondition::None => None, + } + } +} + // --------------------------------------------------------------------------- // ParsedCondition — typed restriction conditions parsed at build time // --------------------------------------------------------------------------- @@ -12042,8 +12298,37 @@ pub enum Effect { /// CR 701.56a: Time travel — for each permanent you control with a time counter /// and each suspended card you own, you may add or remove a time counter. TimeTravel, - /// CR 725.1: Become the monarch. Sets GameState::monarch to the controller. - BecomeMonarch, + /// CR 725.1 + CR 725.3: Grant the monarch designation to `player`. Exactly + /// one player is the monarch at a time, so resolving this always MOVES the + /// designation rather than adding one. + /// + /// `target` is the CR 109.5 subject axis, parameterized within CR 725 rather + /// than proliferated into a `TargetBecomesMonarch` sibling. It is a + /// [`TargetFilter`] — NOT a [`PlayerScope`] — because the printed grammar is + /// "**target** opponent becomes the monarch", exactly like every sibling + /// "target player does X" effect ([`Effect::Draw`], [`Effect::Mill`], + /// [`Effect::GainLife`], …). Only a `TargetFilter` reaches + /// [`Effect::target_filter`], and only that makes `collect_target_slots` + /// declare a CR 115.1 target slot whose legality is the printed restriction + /// (opponents only). A `PlayerScope::Target` would name the slot without + /// ever creating one, so it would resolve to nobody: + /// - [`TargetFilter::Controller`] ← "you become the monarch" (printed + /// default, and the only shape the pre-axis unit variant could express). + /// A context ref, so it surfaces no target slot. + /// - a player filter ← "target opponent / target player becomes the monarch" + /// (M'Baku, Jabari Chieftain; Garland, Royal Kidnapper; Jared Carthalion, + /// True Heir; Éomer, King of Rohan; Denethor, Stone Seer) + /// + /// Serde-defaulted to `Controller` and skipped when it holds that value, so + /// every pre-existing `{"type":"BecomeMonarch"}` row in `card-data.json` + /// keeps deserializing AND re-serializing byte-identically. + BecomeMonarch { + #[serde( + default = "default_target_filter_controller", + skip_serializing_if = "is_target_filter_controller" + )] + target: TargetFilter, + }, /// CR 101.3 + CR 608.2: An instruction with no game action — "there's no /// effect." Used as the resolved outcome for a choice that has no printed /// clause, e.g. the losing/unlisted option of a single-conditional @@ -13315,7 +13600,7 @@ pub enum Effect { /// `Controller` = "the next spell you cast"; `Target` = "the next /// spell they cast / that player casts" (the player this ability /// targets, e.g. the mana recipient on Bigger on the Inside). - #[serde(default = "default_player_scope_controller")] + #[serde(default = "player_scope_controller")] player: PlayerScope, #[serde(default, skip_serializing_if = "Option::is_none")] spell_filter: Option, @@ -14858,13 +15143,6 @@ fn default_duration_until_end_of_turn() -> Duration { Duration::UntilEndOfTurn } -/// CR 109.5: backward-compatible serde default for `Effect::GrantNextSpellAbility`'s -/// `player` field — pre-field data and "the next spell YOU cast" grants resolve to -/// the controller. -fn default_player_scope_controller() -> PlayerScope { - PlayerScope::Controller -} - fn default_comparator_ge() -> Comparator { Comparator::GE } @@ -15462,6 +15740,31 @@ pub enum VoteVisibility { } impl TargetFilter { + /// CR 508.3d + CR 508.5a: True when this filter denotes a PLAYER population + /// rather than an object population — the distinction + /// `trigger_matchers::matching_attack_events` uses to decide whether an + /// `Attacks` trigger's `valid_source` names the ATTACKING PLAYER ("Whenever + /// a player attacks you") or an attacking OBJECT (`valid_source_matches`). + /// + /// Single authority: the parser's intervening-if anaphor gate + /// (`oracle_trigger::attack_intervening_if_anaphor_is_defending_player`) + /// asks the same question and MUST ask it with this method, not with a + /// `valid_source.is_none()` proxy — that proxy silently excludes every + /// attack trigger whose attacker filter is an OBJECT filter in + /// `valid_source`, leaving the wrong `ScopedPlayer` anchor in place with no + /// compile error to catch it. + pub fn is_player_scope(&self) -> bool { + match self { + TargetFilter::Player | TargetFilter::Controller | TargetFilter::AllPlayers => true, + TargetFilter::Typed(TypedFilter { + type_filters, + controller: Some(_), + properties, + }) => type_filters.is_empty() && properties.is_empty(), + _ => false, + } + } + /// Clone this filter with any `Typed` property equal to `prop` removed. /// Used to strip a per-item discriminator leg (e.g. `IsChosenCreatureType`) /// so the residual base filter can be enumerated across candidate values. @@ -16086,6 +16389,11 @@ impl Effect { match self { // --- Effects with a `target: TargetFilter` field --- Effect::DealDamage { target, .. } + // CR 115.1 + CR 725.1: "target opponent becomes the monarch". The + // printed default (`Controller`, "you become the monarch") is a + // context ref, so `extract_target_filter_from_effect`'s final + // `is_context_ref` guard still surfaces no slot for it. + | Effect::BecomeMonarch { target } | Effect::Draw { target, .. } | Effect::Scry { target, .. } | Effect::Surveil { target, .. } @@ -16441,7 +16749,6 @@ impl Effect { | Effect::Explore | Effect::Investigate | Effect::Tribute { .. } - | Effect::BecomeMonarch | Effect::NoOp | Effect::Proliferate | Effect::Populate @@ -17178,7 +17485,7 @@ impl Effect { | Effect::Attach { .. } | Effect::BecomeBlocked { .. } | Effect::BecomeCopy { .. } - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::BecomePrepared { .. } | Effect::BecomeSaddled { .. } | Effect::BecomeUnprepared { .. } @@ -17800,7 +18107,7 @@ impl Effect { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } @@ -18050,7 +18357,7 @@ impl Effect { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } @@ -18313,7 +18620,7 @@ impl Effect { | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } @@ -18529,7 +18836,7 @@ pub fn effect_variant_name(effect: &Effect) -> &str { Effect::Investigate => "Investigate", Effect::Tribute { .. } => "Tribute", Effect::TimeTravel => "TimeTravel", - Effect::BecomeMonarch => "BecomeMonarch", + Effect::BecomeMonarch { .. } => "BecomeMonarch", Effect::NoOp => "NoOp", Effect::Proliferate => "Proliferate", Effect::ProliferateTarget { .. } => "ProliferateTarget", @@ -19040,7 +19347,7 @@ impl From<&Effect> for EffectKind { Effect::Investigate => EffectKind::Investigate, Effect::Tribute { .. } => EffectKind::Tribute, Effect::TimeTravel => EffectKind::TimeTravel, - Effect::BecomeMonarch => EffectKind::BecomeMonarch, + Effect::BecomeMonarch { .. } => EffectKind::BecomeMonarch, Effect::NoOp => EffectKind::NoOp, Effect::Proliferate => EffectKind::Proliferate, Effect::ProliferateTarget { .. } => EffectKind::ProliferateTarget, @@ -21888,8 +22195,27 @@ pub enum TriggerCondition { /// CR 702.178a: The trigger functions only while its controller has max speed. HasMaxSpeed, - /// CR 725.1: "if you're the monarch" is true when the controller is the monarch. - IsMonarch, + /// CR 725.1 + CR 603.4: monarch IDENTITY as an intervening-if — true when + /// `player` currently holds the monarch designation. Checked at fire time + /// and again as the ability resolves (CR 603.4). + /// + /// `player` is the CR 109.5 subject axis; see + /// [`StaticCondition::IsMonarch`] for the full axis rationale. "if you're + /// the monarch" is [`PlayerScope::Controller`]; "if that player is the + /// monarch" on an attack trigger is [`PlayerScope::DefendingPlayer`] + /// (CR 508.5 — the player the triggering creature is attacking). + /// + /// An unresolvable scope makes the condition UNANSWERABLE, not false: + /// `triggers::check_trigger_condition_with_source` rejects the whole + /// condition at its entry boundary so `Not` cannot invert a missing anchor + /// into a firing trigger. + IsMonarch { + #[serde( + default = "player_scope_controller", + skip_serializing_if = "is_player_scope_controller" + )] + player: PlayerScope, + }, /// CR 726.3: "if you have the initiative" is true when the controller has /// the initiative designation. IsInitiative, @@ -22136,6 +22462,104 @@ pub enum TriggerCondition { Not { condition: Box }, } +impl TriggerCondition { + /// CR 109.4 + CR 603.4: the player whose DESIGNATION this leaf tests, when + /// the leaf is a designation predicate at all. + /// + /// Exhaustive by design — there is deliberately no wildcard arm. This is + /// the guard that makes the polarity boundary gate in `game::triggers` + /// total: adding a future designation leaf that carries a [`PlayerScope`] + /// is a COMPILE ERROR here, not a latent fail-open under + /// [`TriggerCondition::Not`]. + /// + /// Boolean combinators return `None`; the gate recurses them itself. + /// `QuantityComparison` returns `None` BY DEFINITION — it tests a quantity, + /// not a designation. The pre-existing fail-open for unresolvable + /// [`PlayerScope`]s inside a `QuantityExpr` (an unresolved scope yields 0, + /// and `0 > 0` is false, which inverts under `Not`) is orthogonal, affects + /// every existing [`PlayerScope::DefendingPlayer`] card, and is deliberately + /// out of scope here. + pub(crate) fn designation_player_anchor(&self) -> Option<&PlayerScope> { + match self { + TriggerCondition::IsMonarch { player } => Some(player), + TriggerCondition::GainedLife { .. } + | TriggerCondition::LostLife + | TriggerCondition::Descended + | TriggerCondition::ControlsType { .. } + | TriggerCondition::NoSpellsCastLastTurn + | TriggerCondition::TwoOrMoreSpellsCastLastTurn + | TriggerCondition::DuringPlayersTurn { .. } + | TriggerCondition::SourceEnteredThisTurn + | TriggerCondition::SourceAttackedThisCombat + | TriggerCondition::EchoDue + | TriggerCondition::MinCoAttackers { .. } + | TriggerCondition::SolveConditionMet + | TriggerCondition::ClassLevelGE { .. } + | TriggerCondition::SourceIsHarnessed + | TriggerCondition::AttractionVisitRoll { .. } + | TriggerCondition::WasCast { .. } + | TriggerCondition::WasPlayed + | TriggerCondition::AdditionalCostPaid { .. } + | TriggerCondition::SourceIsAttacking + | TriggerCondition::CastVariantPaid { .. } + | TriggerCondition::CastVariantPaidPersistent { .. } + | TriggerCondition::ActivatedAbilityIsNonMana + | TriggerCondition::DealtDamageBySourceThisTurn + | TriggerCondition::DealtDamageThisTurnBySource { .. } + | TriggerCondition::FirstTimeObjectTappedThisTurn + | TriggerCondition::FirstTimeObjectCountersAddedThisTurn + | TriggerCondition::WasType { .. } + | TriggerCondition::LifeTotalGE { .. } + | TriggerCondition::ControlCount { .. } + | TriggerCondition::ControlsNone { .. } + | TriggerCondition::AttackedThisTurn + | TriggerCondition::FirstCombatPhaseOfTurn + | TriggerCondition::CastSpellThisTurn { .. } + | TriggerCondition::QuantityComparison { .. } + | TriggerCondition::HasMaxSpeed + | TriggerCondition::IsInitiative + | TriggerCondition::NoMonarch + | TriggerCondition::WasStartingPlayer { .. } + | TriggerCondition::SpellCastWithVariantThisTurn { .. } + | TriggerCondition::HasCityBlessing + | TriggerCondition::HasEnduringStory + | TriggerCondition::CompletedDungeon { .. } + | TriggerCondition::SourceIsTapped + | TriggerCondition::SourceIsTransformed + | TriggerCondition::SourceIsFaceUp + | TriggerCondition::SourceIsFaceDown + | TriggerCondition::SourceInZone { .. } + | TriggerCondition::CounterAddedThisTurn + | TriggerCondition::LostLifeLastTurn + | TriggerCondition::DefendingPlayerControlsNone { .. } + | TriggerCondition::TributeNotPaid + | TriggerCondition::CastDuringPhase { .. } + | TriggerCondition::CastTimingPermission { .. } + | TriggerCondition::ManaColorSpent { .. } + | TriggerCondition::ManaSpentCondition { .. } + | TriggerCondition::HadCounters { .. } + | TriggerCondition::ControlsCommander { .. } + | TriggerCondition::IsRenowned { .. } + | TriggerCondition::HasCounters { .. } + | TriggerCondition::ZoneChangeObjectMatchesFilter { .. } + | TriggerCondition::ZoneChangeObjectIsTapped + | TriggerCondition::SourceMatchesFilter { .. } + | TriggerCondition::EventDamageSourceMatchesFilter { .. } + | TriggerCondition::EventObjectMatchesFilter { .. } + | TriggerCondition::DamagedPlayerIsEventSourceOwner + | TriggerCondition::ChosenLabelIs { .. } + | TriggerCondition::AttackersDeclaredCount { .. } + | TriggerCondition::ExceptFirstDrawInDrawStep + | TriggerCondition::PlacedByAbilitySource + | TriggerCondition::TriggeringSpellTargetsFilter { .. } + | TriggerCondition::TriggeringSpellMatchesFilter { .. } + | TriggerCondition::And { .. } + | TriggerCondition::Or { .. } + | TriggerCondition::Not { .. } => None, + } + } +} + /// Condition that gates whether a replacement effect applies. /// Checked when determining if the replacement is a candidate for an event. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -30797,3 +31221,190 @@ mod player_target_slot_tests { } } } + +#[cfg(test)] +mod monarch_subject_axis_tests { + use super::*; + + /// CR 725.1 + CR 109.5: every pre-existing `{"type":"IsMonarch"}` row in + /// `card-data.json` and in the committed integration-card fixture must keep + /// deserializing to the controller subject, and must re-serialize + /// byte-identically. + /// + /// Revert-failing twice: without `#[serde(default = ...)]` the deserialize + /// errors outright; without `skip_serializing_if` the re-serialize emits a + /// `player` key and every existing monarch row churns. + #[test] + fn is_monarch_round_trips_without_a_player_key_for_the_controller_subject() { + let trigger: TriggerCondition = serde_json::from_str(r#"{"type":"IsMonarch"}"#).unwrap(); + assert_eq!( + trigger, + TriggerCondition::IsMonarch { + player: PlayerScope::Controller + } + ); + assert_eq!( + serde_json::to_string(&trigger).unwrap(), + r#"{"type":"IsMonarch"}"# + ); + + let stat: StaticCondition = serde_json::from_str(r#"{"type":"IsMonarch"}"#).unwrap(); + assert_eq!( + stat, + StaticCondition::IsMonarch { + player: PlayerScope::Controller + } + ); + assert_eq!( + serde_json::to_string(&stat).unwrap(), + r#"{"type":"IsMonarch"}"# + ); + } + + /// CR 725.1 + CR 109.5: the EFFECT-side subject axis has the same serde + /// contract as the predicate-side one above. Every shipping + /// `{"type":"BecomeMonarch"}` row in `card-data.json` (~40 "you become the + /// monarch" cards) must keep deserializing to the controller subject and + /// re-serialize byte-identically, while a real target filter survives. + /// + /// Revert-failing twice: without `#[serde(default = ...)]` the deserialize + /// errors outright; without `skip_serializing_if` the re-serialize emits a + /// `target` key and every existing monarch row churns. + #[test] + fn become_monarch_round_trips_without_a_target_key_for_the_controller_subject() { + let default_row: Effect = serde_json::from_str(r#"{"type":"BecomeMonarch"}"#).unwrap(); + assert_eq!( + default_row, + Effect::BecomeMonarch { + target: TargetFilter::Controller + } + ); + assert_eq!( + serde_json::to_string(&default_row).unwrap(), + r#"{"type":"BecomeMonarch"}"# + ); + + // CR 115.1: "target opponent becomes the monarch" — the opponent + // restriction must survive export, or the re-imported row would offer + // the controller as a legal target. + let targeted = Effect::BecomeMonarch { + target: TargetFilter::Typed(TypedFilter { + type_filters: vec![], + controller: Some(ControllerRef::Opponent), + properties: vec![], + }), + }; + let json = serde_json::to_string(&targeted).unwrap(); + assert!( + json.contains("\"target\""), + "a non-default subject must be serialized: {json}" + ); + assert_eq!(serde_json::from_str::(&json).unwrap(), targeted); + } + + /// A non-default subject must survive the round trip, or the card-data + /// export would silently rebind M'Baku's anaphor to the controller. + #[test] + fn is_monarch_serializes_a_non_default_subject() { + let trigger = TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer, + }; + let json = serde_json::to_string(&trigger).unwrap(); + assert_eq!( + json, + r#"{"type":"IsMonarch","player":{"type":"DefendingPlayer"}}"# + ); + assert_eq!( + serde_json::from_str::(&json).unwrap(), + trigger + ); + } + + /// CR 611.2a + CR 514.2: a duration-timing-only scope is serde-reachable + /// from a malformed or hand-authored row. It must be REJECTED by + /// `duration_timing_only` before `resolve_single_player_scope`'s + /// `unreachable!()` can panic the engine inside a trigger check. + #[test] + fn duration_timing_only_scopes_are_flagged_for_fail_closed_rejection() { + let malformed: TriggerCondition = + serde_json::from_str(r#"{"type":"IsMonarch","player":{"type":"AnyTurn"}}"#).unwrap(); + let TriggerCondition::IsMonarch { player } = &malformed else { + panic!("expected IsMonarch, got {malformed:?}"); + }; + assert!(player.duration_timing_only()); + + assert!(PlayerScope::SpecificPlayer { id: PlayerId(3) }.duration_timing_only()); + // Everything the parser can actually emit must NOT be rejected. + for ok in [ + PlayerScope::Controller, + PlayerScope::ScopedPlayer, + PlayerScope::DefendingPlayer, + ] { + assert!(!ok.duration_timing_only(), "{ok:?} must resolve normally"); + } + } + + /// The polarity boundary gates are only sound because these accessors are + /// exhaustive. Pin the two answers they must give. + #[test] + fn designation_player_anchor_reports_the_monarch_subject_and_nothing_else() { + assert_eq!( + TriggerCondition::IsMonarch { + player: PlayerScope::DefendingPlayer + } + .designation_player_anchor(), + Some(&PlayerScope::DefendingPlayer) + ); + assert_eq!( + StaticCondition::IsMonarch { + player: PlayerScope::ScopedPlayer + } + .designation_player_anchor(), + Some(&PlayerScope::ScopedPlayer) + ); + // CR 725.1: vacancy is a different predicate and carries no subject. + assert_eq!( + TriggerCondition::NoMonarch.designation_player_anchor(), + None + ); + assert_eq!(StaticCondition::NoMonarch.designation_player_anchor(), None); + // A quantity tests a quantity, not a designation — by definition. + assert_eq!( + TriggerCondition::QuantityComparison { + lhs: QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::DefendingPlayer + } + }, + comparator: Comparator::GT, + rhs: QuantityExpr::Fixed { value: 0 }, + } + .designation_player_anchor(), + None + ); + } + + /// CR 109.4: the mutable player-axis accessor must reach every reference + /// that carries one, and must leave object-axis references alone. + #[test] + fn quantity_ref_player_scope_mut_reaches_the_player_axis() { + let mut life = QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }; + *life.player_scope_mut().unwrap() = PlayerScope::DefendingPlayer; + assert_eq!( + life, + QuantityRef::LifeTotal { + player: PlayerScope::DefendingPlayer + } + ); + + let mut hand = QuantityRef::HandSize { + player: PlayerScope::ScopedPlayer, + }; + assert!(hand.player_scope_mut().is_some()); + + let mut object_axis = QuantityRef::SelfManaValue; + assert!(object_axis.player_scope_mut().is_none()); + } +} diff --git a/crates/engine/src/types/ability_visit.rs b/crates/engine/src/types/ability_visit.rs index d19f4aaec9..de271698d2 100644 --- a/crates/engine/src/types/ability_visit.rs +++ b/crates/engine/src/types/ability_visit.rs @@ -824,7 +824,7 @@ where | Effect::Investigate | Effect::Tribute { .. } | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::ProliferateTarget { .. } diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index aa8d567f5a..81f82af496 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -835,6 +835,7 @@ mod master_of_ceremonies; mod mauhur_swarming_of_moria; mod maze_of_ith_untap_bidirectional_prevent; mod mazemind_tome_existential_counter_state_trigger; +mod mbaku_attacked_monarch_intervening_if; mod mechtitan_core_return_exiled; mod memory_jar_delayed_end_step; mod memory_plunder_free_cast_2884; diff --git a/crates/engine/tests/integration/master_of_ceremonies.rs b/crates/engine/tests/integration/master_of_ceremonies.rs index 73428b5eda..f4199f5c08 100644 --- a/crates/engine/tests/integration/master_of_ceremonies.rs +++ b/crates/engine/tests/integration/master_of_ceremonies.rs @@ -132,7 +132,11 @@ fn make_master_of_ceremonies_vote(controller: PlayerId, source_id: ObjectId) -> build_resolved_from_def(&vote_def, source_id, controller) } -fn make_threshold_vote(controller: PlayerId, source_id: ObjectId) -> ResolvedAbility { +fn make_threshold_vote( + controller: PlayerId, + source_id: ObjectId, + tie_breaker: u8, +) -> ResolvedAbility { let vote_def = AbilityDefinition::new( AbilityKind::Spell, Effect::Vote { @@ -141,13 +145,15 @@ fn make_threshold_vote(controller: PlayerId, source_id: ObjectId) -> ResolvedAbi Box::new(AbilityDefinition::new(AbilityKind::Spell, Effect::NoOp)), Box::new(AbilityDefinition::new( AbilityKind::Spell, - Effect::BecomeMonarch, + Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, )), ], starting_with: ControllerRef::You, voter_scope: VoterScope::AllPlayers, tally_mode: VoteTally::TopVotes { - tie: TieResolution::Breaker(0), + tie: TieResolution::Breaker(tie_breaker), }, subject: VoteSubject::Named, visibility: VoteVisibility::Open, @@ -209,7 +215,7 @@ fn moc_per_choice_bodies_parse_into_distributed_chain() { fn threshold_vote_tie_breaker_survives_choose_option_path() { let mut state = GameState::new_two_player(77); let controller = state.players[0].id; - let ability = make_threshold_vote(controller, ObjectId(9100)); + let ability = make_threshold_vote(controller, ObjectId(9100), 0); let mut events = Vec::new(); resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); @@ -247,6 +253,35 @@ fn threshold_vote_tie_breaker_survives_choose_option_path() { ); } +/// CR 701.38a + CR 725.1: the same production vote continuation executes the +/// selected `BecomeMonarch { Controller }` payload when the guilty choice wins. +#[test] +fn threshold_vote_guilty_winner_makes_the_ability_controller_monarch() { + let mut state = GameState::new_two_player(78); + let controller = state.players[0].id; + let ability = make_threshold_vote(controller, ObjectId(9101), 1); + let mut events = Vec::new(); + + resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); + + for choice in ["innocent", "guilty"] { + let voter = match &state.waiting_for { + WaitingFor::VoteChoice { player, .. } => *player, + other => panic!("expected VoteChoice, got {other:?}"), + }; + apply( + &mut state, + voter, + GameAction::ChooseOption { + choice: choice.to_string(), + }, + ) + .expect("ChooseOption must resolve"); + } + + assert_eq!(state.monarch, Some(controller)); +} + /// CR 800.4g: In a 2-player game, the controller does NOT vote. The /// opponent is the only voter; the `WaitingFor::VoteChoice` lands on them. #[test] diff --git a/crates/engine/tests/integration/mbaku_attacked_monarch_intervening_if.rs b/crates/engine/tests/integration/mbaku_attacked_monarch_intervening_if.rs new file mode 100644 index 0000000000..617709b65c --- /dev/null +++ b/crates/engine/tests/integration/mbaku_attacked_monarch_intervening_if.rs @@ -0,0 +1,417 @@ +//! M'Baku, Jabari Chieftain — both printed abilities. +//! +//! **Ability 1** — "At the beginning of your end step, if there is no monarch, +//! **target opponent** becomes the monarch." The designation must go to the +//! DECLARED TARGET. `Effect::BecomeMonarch` used to be a unit variant whose +//! resolver read `ability.controller`, so this crowned M'Baku's own controller +//! — the one player the clause exists to deny, and the player whose coronation +//! structurally disables ability 2 (an opponent could never become the monarch +//! off this card). +//! +//! **Ability 2** — "Whenever a creature attacks one of your +//! opponents, **if that player is the monarch**, that creature gets +1/+1 and +//! gains trample until end of turn." +//! +//! Before this change the intervening-if was dropped entirely +//! (`condition: null` plus a self-flagged `SwallowedClause/Condition_If`), so +//! the buff applied to every creature attacking any opponent regardless of +//! monarch status. +//! +//! Three independent failure directions are discriminated here: +//! 1. **Parser subject axis missing** — `parse_inner_condition` rejects +//! "that player is the monarch", the condition stays `None`, and the buff +//! applies unconditionally (Test 2 catches this). +//! 2. **Anaphor not rebound** — the condition parses as +//! `IsMonarch { ScopedPlayer }`, which +//! `targeting::extract_player_from_event` resolves to the ATTACKING +//! player for an `AttackersDeclared` event, so nothing is ever buffed +//! (Test 1's positive assertion catches this). +//! 3. **CR 508.5 anchor precedence wrong** — the source's own combat latch +//! answers instead of the triggering attacker's defender, so the buff +//! tracks whoever M'Baku is attacking rather than whoever the buffed +//! creature is attacking (Test 1's two-attacker split and Test 4 catch +//! this). +//! +//! CR references: +//! - CR 508.5 / CR 508.5a: an ability referring to both an attacking creature +//! and a defending player means the player THAT creature is attacking, and +//! in multiplayer that player is determined individually per attacker. +//! - CR 310.8d: a battle's protector, and a planeswalker's controller, are the +//! defending player for an attack against them (Test 5's planeswalker). +//! - CR 603.2 / CR 603.2c: the trigger fires once per matching attacker. +//! - CR 603.4: the intervening-if is checked at fire time AND again as the +//! ability resolves (Test 3). +//! - CR 725.1: the monarch is a single-player designation. +//! - CR 702.19a: trample. + +use engine::game::layers::evaluate_layers; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::TargetRef; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::keywords::Keyword; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; + +use super::rules::AttackTarget; + +const P2: PlayerId = PlayerId(2); + +/// Verbatim Scryfall Oracle text — a paraphrase can take a different parser +/// branch and go green while the real card stays broken. +const MBAKU_ORACLE: &str = "At the beginning of your end step, if there is no monarch, target opponent becomes the monarch.\n\ + Whenever a creature attacks one of your opponents, if that player is the monarch, that creature gets +1/+1 and gains trample until end of turn."; + +struct Board { + runner: GameRunner, + mbaku: ObjectId, + bears: ObjectId, +} + +/// Three-player board: P0 controls M'Baku plus a vanilla 2/2. +fn board(monarch: Option) -> Board { + board_at(Phase::PreCombatMain, monarch) +} + +/// [`board`] starting in `phase`. The end-step rows start POST-combat: the +/// scenario driver's phase advance only walks priority windows, so a pre-combat +/// start parks on the `DeclareAttackers` turn-based action instead of reaching +/// the end step. +fn board_at(phase: Phase, monarch: Option) -> Board { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(phase); + + let mbaku = { + let mut builder = scenario.add_creature(P0, "M'Baku, Jabari Chieftain", 4, 3); + builder.from_oracle_text(MBAKU_ORACLE); + builder.id() + }; + let bears = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + for _ in 0..10 { + scenario.add_card_to_library_top(P0, "Plains"); + scenario.add_card_to_library_top(P1, "Plains"); + } + + let mut runner = scenario.build(); + runner.state_mut().monarch = monarch; + evaluate_layers(runner.state_mut()); + + Board { + runner, + mbaku, + bears, + } +} + +fn power_toughness(runner: &GameRunner, id: ObjectId) -> (Option, Option) { + let object = runner.state().objects.get(&id).expect("object must exist"); + (object.power, object.toughness) +} + +fn has_trample(runner: &GameRunner, id: ObjectId) -> bool { + runner + .state() + .objects + .get(&id) + .expect("object must exist") + .keywords + .contains(&Keyword::Trample) +} + +// --------------------------------------------------------------------------- +// Ability 1 — "At the beginning of your end step, if there is no monarch, +// target opponent becomes the monarch." +// --------------------------------------------------------------------------- + +/// Advance to P0's end step and answer the end-step trigger's target prompt +/// with `choice`. Returns the seat the prompt offered, so callers can assert on +/// the legality set as well as the outcome. +fn crown_via_end_step(runner: &mut GameRunner, choice: PlayerId) -> Vec { + runner.advance_to_end_step(); + assert_eq!( + runner.state().phase, + Phase::End, + "reach-guard: the driver must actually reach the end step" + ); + + let WaitingFor::TriggerTargetSelection { + target_slots, + selection, + .. + } = &runner.state().waiting_for + else { + panic!( + "the end-step trigger must prompt for its target; got {:?}\nstack={:?} monarch={:?}", + runner.state().waiting_for, + runner.stack_names(), + runner.state().monarch, + ); + }; + let legal = target_slots[selection.current_slot].legal_targets.to_vec(); + + runner + .act(GameAction::ChooseTarget { + target: Some(TargetRef::Player(choice)), + }) + .expect("choosing the targeted opponent must be legal"); + runner.advance_until_stack_empty(); + legal +} + +/// **The row that proves the fix.** CR 115.1 + CR 725.1: "target opponent +/// becomes the monarch" crowns the DECLARED TARGET. +/// +/// Revert-failing: restore `Effect::BecomeMonarch` as a unit variant whose +/// resolver reads `ability.controller` and `state.monarch` is `Some(P0)` — the +/// first assertion flips. The `assert_ne!` is the same claim stated against the +/// exact shipped bug, so a future refactor that reintroduces a controller +/// fallback (rather than reverting wholesale) also fails here. +#[test] +fn mbaku_end_step_crowns_the_targeted_opponent_not_its_controller_cr_115_1() { + let Board { mut runner, .. } = board_at(Phase::PostCombatMain, None); + + let legal = crown_via_end_step(&mut runner, P2); + + assert_eq!( + runner.state().monarch, + Some(P2), + "the TARGETED opponent must become the monarch" + ); + assert_ne!( + runner.state().monarch, + Some(P0), + "the ability's controller must never be crowned by `target opponent \ + becomes the monarch` — that is the shipped bug this row pins" + ); + + // Discrimination guard: the prompt really offered more than one seat, so + // the assertion above is about the CHOICE, not about a single forced + // answer that any implementation would land on. + assert!( + legal.len() >= 2, + "the target prompt must offer a real choice; got {legal:?}" + ); + // CR 115.1: "target OPPONENT" — the controller is not a legal target. This + // is the assertion that flips if the slot is built from a bare + // `TargetFilter::Player` instead of the parsed opponent-scoped filter. + assert!( + !legal.contains(&TargetRef::Player(P0)), + "CR 115.1: `target opponent` must not offer the controller; got {legal:?}" + ); +} + +/// The same trigger, the OTHER opponent. Two rows with different answers are +/// what prove the resolver reads the target rather than any fixed seat +/// (`PlayerId(1)`, the first opponent, the active player, …). +#[test] +fn mbaku_end_step_crowns_whichever_opponent_was_targeted() { + let Board { mut runner, .. } = board_at(Phase::PostCombatMain, None); + + crown_via_end_step(&mut runner, P1); + + assert_eq!( + runner.state().monarch, + Some(P1), + "targeting P1 must crown P1, not the seat the other row crowned" + ); +} + +/// CR 603.4: the printed intervening-if still gates the trigger — with a +/// monarch already seated, the end-step ability must not fire at all. +/// +/// Reach-guarded: the two rows above prove the same board DOES prompt and crown +/// when the designation is vacant, so this negative cannot pass vacuously. +#[test] +fn mbaku_end_step_does_not_fire_while_a_monarch_exists_cr_603_4() { + let Board { mut runner, .. } = board_at(Phase::PostCombatMain, Some(P1)); + + runner.advance_to_end_step(); + // Reach-guard: without this the negative below is vacuous — the driver can + // park before the end step and no trigger prompt would appear for reasons + // that have nothing to do with the intervening-if. + assert_eq!( + runner.state().phase, + Phase::End, + "reach-guard: the driver must actually reach the end step" + ); + + assert!( + !matches!( + runner.state().waiting_for, + WaitingFor::TriggerTargetSelection { .. } + ), + "`if there is no monarch` must suppress the trigger entirely; got {:?}", + runner.state().waiting_for + ); + runner.advance_until_stack_empty(); + assert_eq!( + runner.state().monarch, + Some(P1), + "the seated monarch must be untouched" + ); +} + +// --------------------------------------------------------------------------- +// Ability 2 — the attack-trigger intervening-if. +// --------------------------------------------------------------------------- + +/// **The row that proves the fix.** CR 508.5 + CR 603.2c: two creatures attack +/// two different opponents in one declaration. Only the one attacking the +/// MONARCH (P2) is buffed. +/// +/// Revert-failing in three independent directions, all distinguishable: +/// - no anaphor rebind → the condition anchors on the ATTACKING player (P0), +/// who is not the monarch, so NEITHER creature is buffed; +/// - no CR 508.5 precedence fix → M'Baku's own latch (P1) answers for both +/// firings, so the BEARS are not buffed; +/// - no parser subject axis → the condition is absent, so M'BAKU is buffed +/// too. +#[test] +fn mbaku_buffs_only_the_creature_attacking_the_monarch() { + let Board { + mut runner, + mbaku, + bears, + } = board(Some(P2)); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (mbaku, AttackTarget::Player(P1)), + (bears, AttackTarget::Player(P2)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, bears), + (Some(3), Some(3)), + "the creature attacking the monarch (P2) must get +1/+1" + ); + assert!( + has_trample(&runner, bears), + "the creature attacking the monarch must gain trample (CR 702.19a)" + ); + + assert_eq!( + power_toughness(&runner, mbaku), + (Some(4), Some(3)), + "M'Baku attacks P1, who is NOT the monarch — no buff" + ); + assert!( + !has_trample(&runner, mbaku), + "M'Baku must not gain trample while attacking a non-monarch" + ); +} + +/// CR 725.1: attacking a non-monarch opponent grants nothing. +/// +/// Reach-guarded: the same declaration against the monarch DOES buff, so a +/// parse failure cannot make this negative pass vacuously. +#[test] +fn mbaku_no_buff_when_attacked_player_is_not_the_monarch() { + // Reach-guard first: P2 is the monarch and Bears attacks P2 → buffed. + let Board { + mut runner, bears, .. + } = board(Some(P2)); + runner.advance_to_combat(); + runner + .declare_attackers(&[(bears, AttackTarget::Player(P2))]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + assert_eq!( + power_toughness(&runner, bears), + (Some(3), Some(3)), + "reach-guard: the trigger DOES fire and buff when the attacked player is the monarch" + ); + + // Now the real negative: P1 is the monarch, but Bears attacks P2. + let Board { + mut runner, bears, .. + } = board(Some(P1)); + runner.advance_to_combat(); + runner + .declare_attackers(&[(bears, AttackTarget::Player(P2))]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, bears), + (Some(2), Some(2)), + "attacking a non-monarch must not buff" + ); + assert!(!has_trample(&runner, bears), "and must not grant trample"); +} + +/// CR 603.4: the intervening-if is checked AGAIN as the ability resolves. If +/// the monarch changes between declaration and resolution, the ability is +/// removed from the stack and does nothing. +/// +/// This is the row that would fail if the condition had been lowered to the +/// declaration-time event qualifier `AttackTargetFilter::Monarch` instead of a +/// real intervening-if. Reach-guarded by Test 1, which proves the same +/// declaration buffs when the monarch is unchanged. +#[test] +fn mbaku_intervening_if_rechecked_at_resolution_cr_603_4() { + let Board { + mut runner, bears, .. + } = board(Some(P2)); + + runner.advance_to_combat(); + runner + .declare_attackers(&[(bears, AttackTarget::Player(P2))]) + .expect("DeclareAttackers should succeed"); + + // The trigger is on the stack; revoke the monarch designation before it + // resolves. + runner.state_mut().monarch = None; + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, bears), + (Some(2), Some(2)), + "CR 603.4: the resolution-time recheck must remove the ability" + ); + assert!(!has_trample(&runner, bears)); +} + +/// CR 603.2: the source is an ordinary subject of its own observer trigger. When +/// M'Baku itself attacks the monarch it IS buffed — proving the per-attacker +/// event binding is not confused by the source being one of the attackers. +#[test] +fn mbaku_buffs_itself_when_it_attacks_the_monarch() { + let Board { + mut runner, + mbaku, + bears, + } = board(Some(P1)); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (mbaku, AttackTarget::Player(P1)), + (bears, AttackTarget::Player(P2)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, mbaku), + (Some(5), Some(4)), + "M'Baku attacks the monarch (P1) and is itself a creature — CR 603.2" + ); + assert!(has_trample(&runner, mbaku)); + + assert_eq!( + power_toughness(&runner, bears), + (Some(2), Some(2)), + "Bears attacks P2, who is not the monarch" + ); + assert!(!has_trample(&runner, bears)); +} diff --git a/crates/engine/tests/integration/rules/battle.rs b/crates/engine/tests/integration/rules/battle.rs index dbbf599964..d37403b4a5 100644 --- a/crates/engine/tests/integration/rules/battle.rs +++ b/crates/engine/tests/integration/rules/battle.rs @@ -3,9 +3,9 @@ //! Covers: //! - Defense-counter ETB (CR 310.4b) //! - Zero-defense SBA (CR 704.5v + CR 310.7) -//! - Protector choice/getter (CR 310.12a + CR 310.9) -//! - Attack target routing — defending player = protector (CR 508.5 + CR 310.9d) -//! - Protector cannot attack own battle (CR 310.9b) +//! - Protector choice/getter (CR 310.11a + CR 310.8a) +//! - Attack target routing — defending player = protector (CR 508.5 + CR 310.8d) +//! - Protector cannot attack own battle (CR 310.8b) #![allow(unused_imports)] use super::*; @@ -63,7 +63,7 @@ fn battle_has_defense_equal_to_counters() { assert_eq!(obj.counters.get(&CounterType::Defense).copied(), Some(4)); } -/// CR 310.12b + CR 712.14a: Accepting a Siege victory cast during trigger +/// CR 310.11b + CR 712.14a: Accepting a Siege victory cast during trigger /// resolution must preserve `cast_transformed`, so the permanent resolves onto /// the battlefield back face up. #[test] @@ -167,14 +167,14 @@ fn zero_defense_battle_goes_to_graveyard_via_sba() { ); } -/// CR 310.9 + CR 310.9a: The `protector()` getter returns the chosen opponent. +/// CR 310.8 + CR 310.8a: The `protector()` getter returns the chosen opponent. #[test] fn protector_getter_returns_chosen_player() { let (runner, battle) = prime_siege(P0, P1, "Protected Siege", 3); assert_eq!(runner.state().objects[&battle].protector(), Some(P1)); } -/// CR 310.9: Non-battle permanents always return None from `protector()`. +/// CR 310.8: Non-battle permanents always return None from `protector()`. #[test] fn non_battle_has_no_protector() { let mut scenario = GameScenario::new(); @@ -184,10 +184,10 @@ fn non_battle_has_no_protector() { assert_eq!(runner.state().objects[&creature].protector(), None); } -/// CR 508.1b + CR 508.5 + CR 310.9d: When a creature attacks a battle, the +/// CR 508.1b + CR 508.5 + CR 310.8d: When a creature attacks a battle, the /// defending player for combat purposes is the battle's protector, not the /// battle's controller. Controller (P0) can attack their own Siege when the -/// protector (P1) is different — CR 310.9b. +/// protector (P1) is different — CR 310.8b. #[test] fn battle_attack_defending_player_is_protector() { let mut scenario = GameScenario::new(); @@ -233,7 +233,7 @@ fn battle_attack_defending_player_is_protector() { } // --------------------------------------------------------------------------- -// CR 310.11 + CR 704.5w + CR 704.5x: SBA protector reassignment. +// CR 310.10 + CR 704.5w + CR 704.5x: SBA protector reassignment. // Multi-candidate (3+ player) branch must pause with // `WaitingFor::BattleProtectorChoice`; singleton (2-player) must auto-apply. // --------------------------------------------------------------------------- @@ -243,7 +243,7 @@ fn battle_attack_defending_player_is_protector() { #[test] fn battle_protector_auto_applies_with_single_candidate_2p() { let (mut runner, battle) = prime_siege(P0, P0, "Self-Protected Siege", 3); - // Baseline: protector == controller (illegal per CR 310.12a). + // Baseline: protector == controller (illegal per CR 310.11a). assert_eq!(runner.state().objects[&battle].protector(), Some(P0)); let mut events = Vec::new(); @@ -261,7 +261,7 @@ fn battle_protector_auto_applies_with_single_candidate_2p() { assert!(runner.state().battlefield.contains(&battle)); } -/// CR 310.11 + CR 704.5w + CR 704.5x: In a 3-player game the controller has two +/// CR 310.10 + CR 704.5w + CR 704.5x: In a 3-player game the controller has two /// legal opponents, so the SBA must pause with `BattleProtectorChoice`. Submitting /// `ChooseBattleProtector` assigns the chosen player via `ChosenAttribute::Player` /// and resumes the game. @@ -311,7 +311,7 @@ fn battle_protector_pauses_for_choice_with_multiple_candidates_3p() { )); } -/// CR 310.11: Submitting a protector that isn't in the candidate list is rejected. +/// CR 310.10: Submitting a protector that isn't in the candidate list is rejected. #[test] fn battle_protector_choice_rejects_invalid_candidate() { const P2: PlayerId = PlayerId(2); @@ -329,7 +329,7 @@ fn battle_protector_choice_rejects_invalid_candidate() { WaitingFor::BattleProtectorChoice { .. } )); - // P0 is the controller — not a legal Siege protector (CR 310.12a). + // P0 is the controller — not a legal Siege protector (CR 310.11a). let err = runner .act(GameAction::ChooseBattleProtector { protector: P0 }) .expect_err("choosing a non-candidate player must be rejected"); @@ -346,12 +346,12 @@ fn battle_protector_choice_rejects_invalid_candidate() { assert_eq!(runner.state().objects[&battle].protector(), Some(P2)); } -/// CR 310.11 / CR 704.5w: When no legal candidate exists, the battle is put +/// CR 310.10 / CR 704.5w: When no legal candidate exists, the battle is put /// into its owner's graveyard. This preserves the existing 0-candidate fallback. #[test] fn battle_with_no_legal_protector_goes_to_graveyard() { // 2-player Siege whose only opponent (P1) has been eliminated — no legal - // protector exists, so CR 310.11 sends the battle to the graveyard. + // protector exists, so CR 310.10 sends the battle to the graveyard. let (mut runner, battle) = prime_siege(P0, P0, "Abandoned Siege", 3); runner.state_mut().eliminated_players.push(P1); @@ -366,7 +366,7 @@ fn battle_with_no_legal_protector_goes_to_graveyard() { )); } -/// R4l — CR 310.12a (*"must choose its protector from among their opponents"*) + +/// R4l — CR 310.11a (*"must choose its protector from among their opponents"*) + /// CR 704.5w (*"no player **in the game** designated as its protector"*): the protector /// pick is a CHOICE (CR 115.10a), so a phased-out seat is not among the choosable /// opponents (the CR 702.26b MIRROR), and a departed one is not either (CR 800.4 + @@ -414,7 +414,7 @@ fn battle_protector_choice_excludes_a_phased_out_opponent_and_still_offers_the_r /// boundary, and at `1` the engine writes the protector ITSELF and publishes nothing: no /// `WaitingFor`, no events. That is invisible to every `candidates` assertion the R4-family /// shape prescribes, so it needs its own arm. The auto-applied seat is not wrong — it is -/// the sole surviving legal opponent, which CR 310.11 + CR 310.12a make the only +/// the sole surviving legal opponent, which CR 310.10 + CR 310.11a make the only /// appropriate player. What this arm guards is the SILENT DISAPPEARANCE of the prompt. /// /// BOTH halves are required: (a) alone would pass on a board where the SBA never ran at @@ -446,13 +446,13 @@ fn battle_protector_narrowing_to_one_auto_applies_silently() { assert_eq!( runner.state().objects[&battle].protector(), Some(PlayerId(3)), - "the auto-applied seat is the ONLY surviving legal opponent (CR 310.12a)" + "the auto-applied seat is the ONLY surviving legal opponent (CR 310.11a)" ); assert!(runner.state().battlefield.contains(&battle)); } /// R4l arm 3 — the `→ 0` crossing: with every opponent phased out there is no appropriate -/// player, and CR 310.11 / CR 704.5w put the battle into its owner's graveyard. +/// player, and CR 310.10 / CR 704.5w put the battle into its owner's graveyard. /// /// Reached by PHASING rather than by elimination on purpose: eliminating every opponent /// also ends the game (`waiting_for = GameOver`), which would confound the assertions with @@ -474,7 +474,7 @@ fn battle_protector_narrowing_to_zero_sends_the_battle_to_the_graveyard() { assert!( !matches!(runner.state().waiting_for, WaitingFor::GameOver { .. }), "the table must still be LIVE — reaching 0 by phasing rather than by elimination \ - is what keeps this arm about CR 310.11 instead of about the game ending" + is what keeps this arm about CR 310.10 instead of about the game ending" ); } @@ -510,7 +510,7 @@ fn phased_protector_board(phase_out: &[PlayerId]) -> (GameRunner, ObjectId) { (runner, battle) } -/// CR 310.11 + CR 704.5w: AI routing — when the 3-player SBA pauses with a +/// CR 310.10 + CR 704.5w: AI routing — when the 3-player SBA pauses with a /// protector choice, `legal_actions` emits one `ChooseBattleProtector` candidate /// per legal opponent, so the AI has a deterministic decision surface. #[test] @@ -543,7 +543,7 @@ fn battle_protector_choice_emits_ai_candidates_per_opponent() { assert_eq!(picks.len(), 2); } -/// CR 310.9b: A battle's protector cannot attack it — the declaration is illegal. +/// CR 310.8b: A battle's protector cannot attack it — the declaration is illegal. #[test] fn battle_protector_cannot_attack_own_battle() { let mut scenario = GameScenario::new(); diff --git a/crates/mtgish-import/src/convert/action.rs b/crates/mtgish-import/src/convert/action.rs index 8b587fddb5..dab9a82a47 100644 --- a/crates/mtgish-import/src/convert/action.rs +++ b/crates/mtgish-import/src/convert/action.rs @@ -3644,10 +3644,12 @@ pub fn convert(a: &Action) -> ConvResult { expiry: None, target: None, }, - // CR 717.1: The monarch designation. The acting player becomes the + // CR 725.1: The monarch designation. The acting player becomes the // monarch (singleton — replaces any existing monarch), opting into // the end-step draw and the take-damage-yields-monarchy interactions. - Action::BecomeTheMonarch => Effect::BecomeMonarch, + Action::BecomeTheMonarch => Effect::BecomeMonarch { + target: TargetFilter::Controller, + }, // CR 100.6 / "Time Travel" planar mechanic: travel to an adjacent // plane / step a time counter. Engine slot is the zero-arg diff --git a/crates/mtgish-import/src/convert/condition.rs b/crates/mtgish-import/src/convert/condition.rs index 226b6bf655..f5159abc67 100644 --- a/crates/mtgish-import/src/convert/condition.rs +++ b/crates/mtgish-import/src/convert/condition.rs @@ -1893,7 +1893,12 @@ pub fn convert_player_predicate_trigger( // Designation/state predicates with direct engine analogs. Players::IsTheMonarch => { require_you_player(player, "Players::IsTheMonarch (trigger)")?; - TriggerCondition::IsMonarch + // CR 109.5: `require_you_player` above has already proven the + // subject is "you", so the controller scope is the exact + // equivalent of the pre-parameterization variant. + TriggerCondition::IsMonarch { + player: PlayerScope::Controller, + } } Players::HasTheCitysBlessing => { require_you_player(player, "Players::HasTheCitysBlessing (trigger)")?; @@ -2409,7 +2414,11 @@ pub fn convert_player_predicate_static( // Direct StaticCondition analogs. Players::IsTheMonarch => { require_you_player(player, "Players::IsTheMonarch (static)")?; - StaticCondition::IsMonarch + // CR 109.5: see the trigger-side sibling — `require_you_player` + // proves the controller scope. + StaticCondition::IsMonarch { + player: PlayerScope::Controller, + } } Players::HasTheCitysBlessing => { require_you_player(player, "Players::HasTheCitysBlessing (static)")?; diff --git a/crates/phase-ai/src/policies/effect_classify.rs b/crates/phase-ai/src/policies/effect_classify.rs index 8099fc9643..6141d304f5 100644 --- a/crates/phase-ai/src/policies/effect_classify.rs +++ b/crates/phase-ai/src/policies/effect_classify.rs @@ -150,7 +150,14 @@ pub(crate) fn effect_polarity(effect: &Effect) -> EffectPolarity { | Effect::SearchLibrary { .. } | Effect::Surveil { .. } | Effect::Connive { .. } - | Effect::BecomeMonarch + // CR 725.1 + CR 725.2: the monarch draws an extra card each turn, so + // crowning YOURSELF is beneficial. Crowning someone else ("target + // opponent becomes the monarch") hands that advantage away and is NOT, + // so every other subject scope falls through to the `Contextual` + // catch-all rather than inheriting this arm. + | Effect::BecomeMonarch { + target: TargetFilter::Controller, + } | Effect::ExtraTurn { .. } => EffectPolarity::Beneficial, // CR 701.26a: tapping a single permanent is harmful (denies its use). // The mass (`All`) scope is left Contextual via the catch-all, matching @@ -355,6 +362,12 @@ pub(crate) fn effect_polarity(effect: &Effect) -> EffectPolarity { | Effect::RememberCard { .. } | Effect::RemoveFromCombat { .. } | Effect::BecomeBlocked { .. } + // CR 725.1: crowning a player OTHER than yourself ("target opponent + // becomes the monarch"). Whether handing out the designation helps you + // is card-specific — Jared Carthalion wants an opponent crowned so it + // can take it back — so it is Contextual, never the `Beneficial` arm + // above, which is scoped to `PlayerScope::Controller`. + | Effect::BecomeMonarch { .. } | Effect::Renown { .. } | Effect::ReturnAsAura { .. } | Effect::Reveal { .. } diff --git a/crates/phase-ai/src/policies/redundancy_avoidance.rs b/crates/phase-ai/src/policies/redundancy_avoidance.rs index adbc9b6492..4836038fda 100644 --- a/crates/phase-ai/src/policies/redundancy_avoidance.rs +++ b/crates/phase-ai/src/policies/redundancy_avoidance.rs @@ -417,7 +417,7 @@ fn redundancy_delta( | Effect::ExploreAll { .. } | Effect::Investigate | Effect::TimeTravel - | Effect::BecomeMonarch + | Effect::BecomeMonarch { .. } | Effect::NoOp | Effect::Proliferate | Effect::EndTheTurn