From e2d61c20e0cd700da3764f794a448cb47a246c75 Mon Sep 17 00:00:00 2001 From: Jacob Woodson Date: Sun, 16 Aug 2026 14:02:50 -0500 Subject: [PATCH] Fix Namor, Atlantean King --- crates/engine/src/database/synthesis.rs | 19 + crates/engine/src/game/ability_rw.rs | 11 + crates/engine/src/game/ability_scan.rs | 51 ++ crates/engine/src/game/ability_utils.rs | 289 ++++++++ crates/engine/src/game/casting.rs | 1 + crates/engine/src/game/cost_payability.rs | 2 + crates/engine/src/game/coverage.rs | 13 + .../engine/src/game/effects/gain_control.rs | 1 + crates/engine/src/game/effects/token.rs | 1 + crates/engine/src/game/filter.rs | 265 +++++++- crates/engine/src/game/layers.rs | 49 ++ crates/engine/src/game/quantity.rs | 1 + crates/engine/src/game/targeting.rs | 3 + crates/engine/src/game/trigger_matchers.rs | 161 +++++ crates/engine/src/game/zone_pipeline.rs | 1 + .../engine/src/parser/oracle_effect/lower.rs | 1 + crates/engine/src/parser/oracle_effect/mod.rs | 180 +++++ .../src/parser/oracle_nom/primitives.rs | 25 + .../engine/src/parser/oracle_nom/quantity.rs | 2 + .../src/parser/oracle_static/restriction.rs | 1 + crates/engine/src/parser/oracle_target.rs | 83 +++ crates/engine/src/parser/oracle_tests.rs | 634 +++++++++++++++++- crates/engine/src/parser/oracle_trigger.rs | 96 ++- crates/engine/src/types/ability.rs | 117 ++++ crates/engine/src/types/events.rs | 1 + crates/engine/tests/integration/main.rs | 1 + .../namor_attacking_that_player.rs | 574 ++++++++++++++++ crates/mtgish-import/src/convert/condition.rs | 1 + .../phase-ai/src/policies/effect_classify.rs | 12 +- .../phase-ai/src/policies/hand_disruption.rs | 28 +- crates/phase-ai/src/policies/self_cost.rs | 12 +- 31 files changed, 2614 insertions(+), 22 deletions(-) create mode 100644 crates/engine/tests/integration/namor_attacking_that_player.rs diff --git a/crates/engine/src/database/synthesis.rs b/crates/engine/src/database/synthesis.rs index d14792ef3a..e53b8d880b 100644 --- a/crates/engine/src/database/synthesis.rs +++ b/crates/engine/src/database/synthesis.rs @@ -15521,6 +15521,25 @@ mod dethrone_tests { trigger.condition.is_some(), "dethrone trigger must have an intervening-if condition" ); + // TRIPWIRE (CR 702.105a + CR 603.2 / CR 603.4). Dethrone reads "Whenever + // this creature attacks the player with the most life or tied for most + // life" — there is no "if", so by CR 603.4's own parenthetical the + // most-life clause is part of the TRIGGER EVENT (CR 603.2) and must be + // checked once, at declaration. Modelling it as a `TriggerCondition` is + // over-strict: the ability is wrongly removed from the stack if life + // totals change in response. + // + // That defect is KNOWN, DEFERRED, and deliberately NOT changed here; the + // destination is `TargetFilter::PlayerMatching` on `valid_target`, the + // same channel Namor, Atlantean King and Owlbear Cub now use. This + // assertion pins the current shape so the migration must be a deliberate + // edit rather than an accidental drift. + assert_eq!( + trigger.valid_target, None, + "TRIPWIRE: Dethrone still routes its most-life clause through \ + `condition`, not the CR 603.2 `valid_target` channel. Migrating it \ + to `PlayerMatching` must update this assertion on purpose." + ); } #[test] diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index bf91365caa..a563bba22b 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -1358,6 +1358,7 @@ fn scope_of(target: &TargetFilter, chain_root: Option) -> WriteScope | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo @@ -2249,6 +2250,11 @@ fn legacy_controller_ref(x: &ControllerRef) -> bool { /// serde oracle's whole-value walk). `ParentTargetSlot` is deliberately excluded. fn legacy_target_filter(f: &TargetFilter) -> bool { match f { + // CR 102.1: the player-axis crossing. `legacy_player_filter` is the + // authority for whether a player predicate carries a legacy-12 tag + // (`PlayerAttribute`'s quantity payloads can), so delegate rather than + // flattening this to `false`. + TargetFilter::PlayerMatching { player } => legacy_player_filter(player), TargetFilter::TriggeringSpellController | TargetFilter::TriggeringSpellOwner | TargetFilter::TriggeringPlayer @@ -2560,6 +2566,7 @@ fn member_bound_target_filter(f: &TargetFilter) -> bool { | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::DefendingPlayer | TargetFilter::Named { .. } | TargetFilter::Owner @@ -6670,6 +6677,10 @@ fn rw_target_filter(x: &TargetFilter) -> RwProfile { } // CR 607.2d / CR 607.2m (by analogy): durable per-player anchor-label reads. TargetFilter::PlayerWhoChoseLabel { label: _ } => reads_player_of(StateKind::Other), + // CR 102.1: an arbitrary player predicate reads whatever its payload + // reads (life totals, controlled-permanent counts, attack history), so + // delegate to the player-axis profiler instead of flattening it here. + TargetFilter::PlayerMatching { player } => rw_player_filter(player), // CR 608.2h + CR 113.7a: source-controller resolution follows the // source's exact live-or-LKI incarnation. TargetFilter::SourceController => reads_src_of(StateKind::Other), diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 53f5063b8d..4351dbc8c2 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -3112,6 +3112,11 @@ fn scan_target_filter(x: &TargetFilter, ctx: FilterReadContext, mode: ScanMode) }, TargetFilter::SourceChosenPlayer => Axes::NONE, TargetFilter::PlayerWhoChoseLabel { label: _ } => Axes::NONE, + // CR 102.1: the nested player predicate can itself read projected state + // (`ControlsCount` over a whole `TargetFilter`, `PlayerAttribute` over a + // `QuantityExpr`), so RECURSE rather than reporting `Axes::NONE` — + // mirroring the object-axis `FilterProp::ControllerMatches` arm. + TargetFilter::PlayerMatching { player } => scan_player_filter(player, mode), TargetFilter::OriginalController => Axes::NONE, TargetFilter::PostReplacementSourceController => Axes { event: true, @@ -8039,4 +8044,50 @@ mod tests { assert!(!axes.sibling); assert!(!axes.projected); } + + /// V13 — `TargetFilter::PlayerMatching` recursion is CLASSIFIED, not + /// blind-defaulted to `Axes::NONE`. + /// + /// CR 102.1: the nested `PlayerFilter` can read projected per-player state + /// (`PlayerAttribute` over a life total) and can box a whole `TargetFilter` + /// (`ControlsCount`). Reporting `Axes::NONE` for it would let trigger + /// ordering auto-resolve a group whose members really do read + /// order-relevant state. + /// + /// Revert-failing: replace the recursive arm with `Axes::NONE` and the + /// `projected` assertion below flips. + #[test] + fn player_matching_scan_recurses_into_the_nested_player_filter() { + let payload = PlayerFilter::PlayerAttribute { + relation: crate::types::ability::PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }; + let scanned = scan_target_filter( + &TargetFilter::PlayerMatching { + player: Box::new(payload.clone()), + }, + FilterReadContext::SnapshotOrEvent, + ScanMode::Conservative, + ); + let direct = scan_player_filter(&payload, ScanMode::Conservative); + + // The carrier must report exactly what its payload reports, on every axis. + assert_eq!(scanned.event, direct.event); + assert_eq!(scanned.sibling, direct.sibling); + assert_eq!(scanned.projected, direct.projected); + // …and that report must be non-empty: a life-total predicate reads + // projected per-player state, so `Axes::NONE` would be a blind default. + assert!( + scanned.event || scanned.sibling || scanned.projected, + "PlayerMatching over a life-total predicate must not scan as NONE" + ); + } } diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index a4b33bf2e5..4005101047 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -3365,6 +3365,101 @@ fn target_filter_needs_ability_context(filter: &TargetFilter) -> bool { target_filter_contains_chosen_x_ref(filter) || target_filter_contains_amassed_army_ref(filter) || target_filter_contains_scoped_player_ref(filter) + || filter_needs_trigger_source(filter) +} + +/// CR 508.5 + CR 508.5a + CR 603.3d: a filter whose evaluation asks +/// `combat::defending_player_cr508_5` for the attacked-player anaphor needs the +/// resolving ability's `trigger_source`, because that authority's binding rule is +/// `trigger_source.and_then(|_| detection.or(state.current_trigger_event))` — with +/// no `trigger_source` the binding is `DefenderBinding::None`, which skips the +/// attack-entry tiers entirely and leaves only `resolve_defending_player`. That +/// tail resolves a non-attacking source through `extract_source_from_event`, +/// whose `AttackersDeclared` arm is gated on `attacker_ids.len() == 1`, so ANY +/// declaration with two or more attackers yields `None`, +/// `filter::attacking_defender_matches`'s `is_some_and` is false for every +/// candidate, the target slot is empty, and CR 603.3d removes the triggered +/// ability from the stack ("If a choice is required when the triggered ability +/// goes on the stack but no legal choices can be made for it ... the ability is +/// simply removed from the stack"). +/// +/// Routing these filters to `find_legal_targets_for_ability` +/// (`FilterContext::from_ability`) supplies the `trigger_source` that +/// `set_trigger_source_recursive` already put on every instantiated triggered +/// ability, making SLOT-BUILD agree with the CR 608.2b re-validation door +/// (`targeting::validate_targets_for_ability`), which has always used +/// `from_ability`. The disagreement between those two doors is what made this +/// failure silent. +/// +/// SCOPE — exactly the refs that consume `trigger_source`, and no others. +/// `filter::source_controller_ref_player` special-cases three refs: +/// `DefendingPlayer` (reads `trigger_source`), `SourceChosenPlayer` (reads the +/// source), and `EnchantedPlayer` (reads `source.attached_to`); everything else +/// routes to `controller_ref_player`. Only `DefendingPlayer` needs the ability +/// context, so only `DefendingPlayer` is matched here — leaving the existing +/// corpus producers of `Attacking { defender: Some(You | Opponent | +/// SourceChosenPlayer | EnchantedPlayer) }` on their existing door, unchanged. +/// +/// `FilterProp::CombatRelation` is deliberately EXCLUDED: it is evaluated by +/// `filter::matches_combat_relation`, which reads `source.id` and +/// `source.ability` and never calls `source_defending_player`. +/// +/// `TypedFilter { controller: Some(ControllerRef::DefendingPlayer) }` is ALSO +/// deliberately excluded despite having the identical door bug (Greatsword of +/// Tyr class). The deferral is SCOPED AND MEASURED, not open-ended — measured +/// against `data/card-data.json`, the exported engine corpus: +/// +/// | population | cards | +/// |---|---| +/// | reference `ControllerRef::DefendingPlayer` anywhere | 116 | +/// | …of those, inside a TRIGGER's definition chain | 104 | +/// | …of those, inside a trigger's TARGET slot (the door this predicate gates) | 97 | +/// | the `FilterProp::Attacking { defender: DefendingPlayer }` shape fixed here | 3 | +/// +/// So the follow-up's exact enumeration delta is 97 cards (Greatsword of Tyr, +/// Thraximundar, Kogla, Warkite Marauder, …) moving from the bare +/// `find_legal_targets` door to `find_legal_targets_for_ability`. It is a +/// separate change because 97 re-routed target enumerations need their own +/// multi-attacker fixtures and their own blast-radius measurement — not because +/// the size is unknown. The tripwire test +/// `filter_needs_trigger_source_does_not_widen_to_defending_player_controller` +/// keeps the omission a decision rather than an oversight. +/// +/// STRUCTURAL TRAVERSAL IS NOT RE-IMPLEMENTED HERE. The "does this filter +/// mention X anywhere" question has exactly one authority — `filter:: +/// filter_contains` and its `filter_prop_contains` / `player_filter_contains` +/// halves, whose matches are exhaustive (no `_` arm) precisely so a future +/// nesting variant cannot be silently classified as a leaf. A hand-rolled +/// `Typed` / `And` / `Or` / `Not` walk with a `_ => false` tail would miss the +/// prop under `TrackedSetFiltered`, `ChosenDamageSource`, `PlayerMatching { +/// ControlsCount { filter } }`, or any of the six `TargetFilter`-boxing props +/// (`Targets`, `TargetsOnly`, `SharesQuality`, `DistinctFrom`, +/// `DifferentNameFrom`, `CanEnchant`) — each of which would keep the bare +/// `find_legal_targets` door and reproduce the CR 603.3d removal above. +/// +/// What remains local is a pure LEAF-VALUE test ("is this prop the +/// defending-player anaphor?"), including the two prop-level combinators +/// (`AnyOf` / `Not`) that can wrap it. Its `_ => false` is a value verdict on a +/// prop that carries no `defender` axis, not a containment claim about nesting. +fn filter_needs_trigger_source(filter: &TargetFilter) -> bool { + fn prop_needs(prop: &FilterProp) -> bool { + match prop { + FilterProp::Attacking { + defender: Some(ControllerRef::DefendingPlayer), + } + | FilterProp::AttackedThisTurn { + defender: Some(ControllerRef::DefendingPlayer), + } => true, + FilterProp::AnyOf { props } => props.iter().any(prop_needs), + FilterProp::Not { prop } => prop_needs(prop), + _ => false, + } + } + + crate::game::filter::filter_contains( + filter, + &|inner| matches!(inner, TargetFilter::Typed(typed) if typed.properties.iter().any(prop_needs)), + ) } // CR 102.1 + CR 608.2c: "that player controls" filters lowered to @@ -5658,6 +5753,7 @@ fn legal_targets_for_selected_slot( &enchant_filter, *pid, Some(aura_controller), + Some(aura_id), ), }); } @@ -7788,6 +7884,199 @@ fn build_mode_sequences( mod tests { use super::*; use crate::game::zones::create_object; + use crate::types::ability::{CombatRelation, CombatRelationSubject}; + + fn typed_with(props: Vec) -> TargetFilter { + TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + properties: props, + ..Default::default() + }) + } + + fn attacking(defender: ControllerRef) -> FilterProp { + FilterProp::Attacking { + defender: Some(defender), + } + } + + /// V18 — `filter_needs_trigger_source` is PRECISE: it routes the CR 508.5 + /// defending-player anaphor to the context-carrying enumeration door and + /// leaves every other value on the existing bare door. + /// + /// This is the zero-blast-radius proof. `filter::source_controller_ref_player` + /// resolves `Opponent` via `source.controller`, `EnchantedPlayer` via + /// `source.attached_to`, and `SourceChosenPlayer` via the source object — + /// none of them reads `trigger_source` — so leaving the existing corpus + /// producers on the bare door is behaviour-preserving by construction, not + /// by luck. Only `DefendingPlayer` reaches + /// `combat::defending_player_cr508_5`, whose binding rule requires a + /// `trigger_source` to consult the attack entries at all. + #[test] + fn filter_needs_trigger_source_routes_only_the_defending_player_anaphor() { + // Positive: the new value, bare and under every recursive shape. + let bare = typed_with(vec![attacking(ControllerRef::DefendingPlayer)]); + assert!(filter_needs_trigger_source(&bare)); + assert!(filter_needs_trigger_source(&TargetFilter::Or { + filters: vec![typed_with(vec![]), bare.clone()], + })); + assert!(filter_needs_trigger_source(&TargetFilter::And { + filters: vec![typed_with(vec![]), bare.clone()], + })); + assert!(filter_needs_trigger_source(&TargetFilter::Not { + filter: Box::new(bare.clone()), + })); + assert!(filter_needs_trigger_source(&typed_with(vec![ + FilterProp::AnyOf { + props: vec![FilterProp::Token, attacking(ControllerRef::DefendingPlayer)], + } + ]))); + assert!(filter_needs_trigger_source(&typed_with(vec![ + FilterProp::Not { + prop: Box::new(attacking(ControllerRef::DefendingPlayer)), + } + ]))); + // Sibling prop that shares the same `attacking_defender_matches` door. + assert!(filter_needs_trigger_source(&typed_with(vec![ + FilterProp::AttackedThisTurn { + defender: Some(ControllerRef::DefendingPlayer), + } + ]))); + + // Negative: every `Attacking`/`AttackedThisTurn` value that exists in the + // corpus today must stay on the bare door, unchanged. + for prop in [ + FilterProp::Attacking { defender: None }, + attacking(ControllerRef::You), + attacking(ControllerRef::Opponent), + attacking(ControllerRef::SourceChosenPlayer), + attacking(ControllerRef::EnchantedPlayer), + FilterProp::AttackedThisTurn { defender: None }, + FilterProp::AttackedThisTurn { + defender: Some(ControllerRef::You), + }, + FilterProp::CombatRelation { + relation: CombatRelation::BlockingOrBlockedBy, + subject: CombatRelationSubject::Source, + }, + ] { + assert!( + !filter_needs_trigger_source(&typed_with(vec![prop.clone()])), + "{prop:?} does not consume trigger_source and must stay on the bare door" + ); + } + + // And the predicate composes into the existing routing disjunction. + assert!(target_filter_needs_ability_context(&bare)); + } + + /// V18b — the traversal is DELEGATED, so the anaphor is found at every + /// nesting depth `filter::filter_contains` knows about, not only at the top + /// level where the three unlocked cards happen to put it today. + /// + /// Revert-failing: restore the hand-rolled `Typed`/`And`/`Or`/`Not` match + /// with a `_ => false` tail and every row below flips to `false` — each one + /// then keeps the bare `find_legal_targets` door with `trigger_source: + /// None`, which is the empty-slot / CR 603.3d removal this predicate exists + /// to prevent. + #[test] + fn filter_needs_trigger_source_descends_every_nesting_variant() { + use crate::types::ability::PlayerRelation; + + let bare = typed_with(vec![attacking(ControllerRef::DefendingPlayer)]); + let controls_bare = PlayerFilter::ControlsCount { + relation: PlayerRelation::All, + filter: bare.clone(), + comparator: Comparator::GE, + count: Box::new(QuantityExpr::Fixed { value: 1 }), + }; + + // The six `TargetFilter`-boxing props, plus the two player-axis + // crossings. Each is a nesting site `filter_prop_contains` / + // `player_filter_contains` enumerate exhaustively and the hand-rolled + // walk skipped entirely. + for prop in [ + FilterProp::Targets { + filter: Box::new(bare.clone()), + }, + FilterProp::TargetsOnly { + filter: Box::new(bare.clone()), + }, + FilterProp::CanEnchant { + target: Box::new(bare.clone()), + }, + FilterProp::DistinctFrom { + reference: Box::new(bare.clone()), + }, + FilterProp::DifferentNameFrom { + filter: Box::new(bare.clone()), + }, + FilterProp::SharesQuality { + quality: SharedQuality::CreatureType, + reference: Some(Box::new(bare.clone())), + relation: SharedQualityRelation::default(), + }, + FilterProp::ControllerMatches { + player: Box::new(controls_bare.clone()), + }, + ] { + assert!( + filter_needs_trigger_source(&typed_with(vec![prop.clone()])), + "{prop:?} nests a defending-player anaphor and must route to the \ + ability-context door" + ); + } + + // Filter-level nesting variants outside `Typed`/`And`/`Or`/`Not`. + assert!(filter_needs_trigger_source( + &TargetFilter::TrackedSetFiltered { + id: TrackedSetId(1), + filter: Box::new(bare.clone()), + caused_by: None, + } + )); + assert!(filter_needs_trigger_source( + &TargetFilter::ChosenDamageSource { + filter: Some(Box::new(bare.clone())), + } + )); + assert!(filter_needs_trigger_source(&TargetFilter::PlayerMatching { + player: Box::new(controls_bare), + })); + + // Negative control at the same depths: nesting alone does not route. + assert!(!filter_needs_trigger_source( + &TargetFilter::ChosenDamageSource { + filter: Some(Box::new(typed_with(vec![attacking( + ControllerRef::Opponent + )]))), + } + )); + } + + /// V19 — TRIPWIRE. `TypedFilter { controller: Some(DefendingPlayer) }` + /// (Greatsword of Tyr class) has the IDENTICAL slot-build door bug, and is + /// deliberately NOT covered here. The deferral is measured, not open-ended: + /// 97 corpus cards put that shape in a triggered ability's TARGET slot + /// (versus 3 for the shape fixed here) — see the table on + /// `filter_needs_trigger_source`. Widening the predicate re-routes all 97 + /// enumerations at once and needs its own multi-attacker fixtures. + /// + /// A future pass that widens the predicate must delete this assertion on + /// purpose — that is the point. It exists so the omission reads as a + /// decision, not an oversight. + #[test] + fn filter_needs_trigger_source_does_not_widen_to_defending_player_controller() { + let controller_scoped = TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: Some(ControllerRef::DefendingPlayer), + ..Default::default() + }); + assert!( + !filter_needs_trigger_source(&controller_scoped), + "deliberately out of scope; see the doc comment on filter_needs_trigger_source" + ); + } /// CR 700.2a / CR 700.2e: `modal_chooser_candidates` is the one authority /// both spell announcement and trigger construction read. diff --git a/crates/engine/src/game/casting.rs b/crates/engine/src/game/casting.rs index 0e32716b87..ef7e3d61c1 100644 --- a/crates/engine/src/game/casting.rs +++ b/crates/engine/src/game/casting.rs @@ -7971,6 +7971,7 @@ fn target_ref_matches_cost_filter( filter, *player_id, Some(source_controller), + Some(static_source_id), ), } } diff --git a/crates/engine/src/game/cost_payability.rs b/crates/engine/src/game/cost_payability.rs index 63ae33c87c..c784c64804 100644 --- a/crates/engine/src/game/cost_payability.rs +++ b/crates/engine/src/game/cost_payability.rs @@ -70,6 +70,7 @@ pub(crate) fn target_filter_has_x_mana_value_constraint(filter: &TargetFilter) - | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo @@ -153,6 +154,7 @@ pub(crate) fn relax_x_mana_value_constraint(filter: &TargetFilter) -> TargetFilt | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index 92e0f8e531..b778943a83 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -633,6 +633,11 @@ fn fmt_target(filter: &TargetFilter) -> String { TargetFilter::SpecificObject { id } => format!("object #{}", id.0), TargetFilter::SpecificPlayer { id } => format!("player #{}", id.0), TargetFilter::PlayerWhoChoseLabel { label } => format!("player who last chose {label}"), + // CR 102.1: render the nested player predicate through the existing + // PlayerFilter formatter rather than emitting an opaque placeholder. + TargetFilter::PlayerMatching { player } => { + format!("player matching {}", fmt_player_filter(player)) + } TargetFilter::Neighbor { direction } => match direction { SeatDirection::Left => "player to your left".into(), SeatDirection::Right => "player to your right".into(), @@ -682,6 +687,14 @@ fn fmt_typed_filter(tf: &TypedFilter) -> String { None => parts.push("attacking".into()), Some(ControllerRef::You) => parts.push("attacking you".into()), Some(ControllerRef::Opponent) => parts.push("attacking your opponents".into()), + // CR 508.5: the defending-player anaphor ("attacking that + // player"). Rendering it through the `scoped player` catch-all + // below would name a DIFFERENT concept — `ControllerRef:: + // ScopedPlayer` is the resolution-iteration player, not the + // player this creature is attacking. + Some(ControllerRef::DefendingPlayer) => { + parts.push("attacking defending player".into()) + } Some(_) => parts.push("attacking scoped player".into()), }, FilterProp::Blocking => parts.push("blocking".into()), diff --git a/crates/engine/src/game/effects/gain_control.rs b/crates/engine/src/game/effects/gain_control.rs index a06bb59790..75c309b789 100644 --- a/crates/engine/src/game/effects/gain_control.rs +++ b/crates/engine/src/game/effects/gain_control.rs @@ -425,6 +425,7 @@ fn unique_recipient_from_filter( filter, p.id, Some(source_controller), + Some(ability.source_id), ) }) .map(|p| p.id); diff --git a/crates/engine/src/game/effects/token.rs b/crates/engine/src/game/effects/token.rs index 2098c4b082..b3d22cf437 100644 --- a/crates/engine/src/game/effects/token.rs +++ b/crates/engine/src/game/effects/token.rs @@ -3192,6 +3192,7 @@ fn classify_attach_host_authority(filter: &TargetFilter) -> AttachHostAuthority | TargetFilter::ControllerAndControlledPermanents { .. } | TargetFilter::Opponent | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::TriggeringSpellController diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 4885ca4fb3..4c7fa4e3bc 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -172,6 +172,7 @@ pub(crate) fn affected_filter_uses_object_population(filter: &TargetFilter) -> b | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo @@ -379,6 +380,11 @@ pub(crate) fn target_filter_characteristic_reads_at( return CharacteristicKinds::ALL; }; match filter { + // CR 102.1: an arbitrary player predicate can read anything about the + // boards those players control (`ControlsCount` boxes a whole + // `TargetFilter`), so it is undeterminable here — the same verdict the + // object-axis mirror `FilterProp::ControllerMatches` already carries. + TargetFilter::PlayerMatching { .. } => CharacteristicKinds::ALL, TargetFilter::Not { filter: inner } => target_filter_characteristic_reads_at(inner, depth), TargetFilter::Or { filters } | TargetFilter::And { filters } => { filters.iter().fold(CharacteristicKinds::EMPTY, |acc, f| { @@ -819,6 +825,7 @@ pub(crate) fn entered_object_perturbs_affected_filter( | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo @@ -1529,6 +1536,12 @@ pub(crate) fn filter_contains(filter: &TargetFilter, leaf: &dyn Fn(&TargetFilter match filter { TargetFilter::And { filters } | TargetFilter::Or { filters } => filters.iter().any(recurse), TargetFilter::Not { filter } => recurse(filter), + // CR 102.1: the player-axis crossing into `PlayerFilter`, which boxes + // filters of its own (`ControlsCount`, `TrackedSetPossessor`, + // `OpponentDealtDamage`) — the mirror of the + // `FilterProp::ControllerMatches` arm below (which keeps CR 109.4 + // because it really is about an object's controller). NOT a leaf. + TargetFilter::PlayerMatching { player } => player_filter_contains(player, leaf), TargetFilter::TrackedSetFiltered { filter, .. } => recurse(filter), // CR 609.7a: the source a "source of your choice" effect chose. CR 609.7b: // the optional inner filter is the "red source"-style quality the shield @@ -3259,6 +3272,11 @@ fn filter_inner_for_object( // CR 607 (by analogy): PlayerWhoChoseLabel scopes to players, not // objects — no object matches (evaluated on the player axis). TargetFilter::PlayerWhoChoseLabel { .. } => false, + // CR 102.1: PlayerMatching scopes to players, not objects — no object + // matches (it is evaluated on the player axis by + // `trigger_matchers::player_matches_filter` and + // `filter::player_matches_target_filter_in_state`). + TargetFilter::PlayerMatching { .. } => false, // CR 102.1 + CR 103.1: Neighbor scopes to a seating-relative player, // not an object — no object matches. TargetFilter::Neighbor { .. } => false, @@ -3766,6 +3784,9 @@ fn zone_change_filter_inner( // CR 607 (by analogy): PlayerWhoChoseLabel scopes to players, not // objects — a zone-change record is always an object transition. TargetFilter::PlayerWhoChoseLabel { .. } => false, + // CR 102.1: PlayerMatching scopes to players, not objects — a + // zone-change record is always an object transition. + TargetFilter::PlayerMatching { .. } => false, // CR 102.1 + CR 103.1: Neighbor scopes to a seating-relative player, // not an object — a zone-change record is always an object transition. TargetFilter::Neighbor { .. } => false, @@ -4132,6 +4153,7 @@ pub fn spell_record_matches_filter( | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::AttachedTo | TargetFilter::LastCreated @@ -4450,6 +4472,7 @@ fn spell_object_matches_filter_inner( | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::AttachedTo | TargetFilter::LastCreated @@ -5198,6 +5221,7 @@ fn aura_can_enchant_referenced_target( enchant_filter, *player_id, Some(aura.controller), + Some(aura_id), ), } } @@ -6224,9 +6248,13 @@ fn stack_entry_targets_satisfy( }; let check = |t: &TargetRef| match t { TargetRef::Object(id) => matches_target_filter(state, *id, filter, &ctx), - TargetRef::Player(pid) => { - player_matches_target_filter_in_state(state, filter, *pid, ctx.source_controller) - } + TargetRef::Player(pid) => player_matches_target_filter_in_state( + state, + filter, + *pid, + ctx.source_controller, + Some(ctx.source_id), + ), }; if require_all { ability.targets.iter().all(check) @@ -7395,6 +7423,13 @@ pub fn player_matches_target_filter( player_id, source_controller, &|controller, player| controller != player, + // CR 109.5 + CR 608.2c: an arbitrary player predicate is answerable only + // against live game state and a source object (life totals, controlled + // permanents, attack history). This stateless entry point has neither, so + // it fails CLOSED — the same verdict its `TargetPlayer` / `DefendingPlayer` + // / `TriggeringPlayer` siblings already carry, and pinned by + // `player_matching_fails_closed_without_state`. + &|_, _| false, ) } @@ -7402,17 +7437,46 @@ pub fn player_matches_target_filter( /// opponent semantics from the game state. /// CR 102.2 / CR 102.3 / CR 115.9c: Opponent-scoped player targets exclude /// teammates in team multiplayer. +/// +/// `source_id` is the object whose filter this is. It is threaded because +/// `TargetFilter::PlayerMatching`'s payload can be source-relative +/// (`OpponentDealtDamage { source }`, `OwnersOfCardsExiledBySource`, +/// `DefendingPlayer`, `OpponentAttacked`), and it is an `Option` because a few +/// callers legitimately have no source object; those fail CLOSED on the +/// `PlayerMatching` arm rather than answering it against a fabricated id. pub fn player_matches_target_filter_in_state( state: &GameState, filter: &TargetFilter, player_id: PlayerId, source_controller: Option, + source_id: Option, ) -> bool { player_matches_target_filter_with( filter, player_id, source_controller, &|controller, player| crate::game::players::is_opponent(state, controller, player), + // CR 102.1 + CR 109.5 + CR 608.2c: `TargetFilter::PlayerMatching` makes + // every `PlayerFilter` predicate usable anywhere a `TargetFilter` names a + // player, so this door must answer it rather than fall to the wildcard + // tail. This IS the player-target legality door — `targeting:: + // target_ref_matches_resolved_filter`, `casting`'s CR 115.9c "targets + // only" check and `ability_utils`' slot enumeration all arrive here for + // `TargetRef::Player` — so a missing arm enumerates ZERO legal players for + // "target player who has more life than you" and CR 603.3d / CR 601.2c + // silently discards the spell or ability. + // + // Delegates to the single authority `effects::matches_player_scope` + // (which `trigger_matchers::player_matches_filter` also uses) rather than + // re-implementing any predicate here. `source_controller` is CR 109.5 + // "you": with no controller the payload's `relation` axis is unanswerable, + // so fail closed — likewise with no source object. + &|player, candidate| match (source_controller, source_id) { + (Some(controller), Some(source)) => crate::game::effects::matches_player_scope( + state, candidate, player, controller, source, + ), + _ => false, + }, ) } @@ -7421,6 +7485,7 @@ fn player_matches_target_filter_with( player_id: PlayerId, source_controller: Option, is_opponent: &impl Fn(PlayerId, PlayerId) -> bool, + matches_player_scope: &impl Fn(&PlayerFilter, PlayerId) -> bool, ) -> bool { match filter { TargetFilter::Any | TargetFilter::Player => true, @@ -7467,11 +7532,28 @@ fn player_matches_target_filter_with( }, // Typed filters with type_filters don't match players TargetFilter::Typed(_) => false, + // CR 102.1 + CR 109.5: the arbitrary player predicate. Answered by the + // injected scope matcher (live and source-bound in the `_in_state` entry + // point, fail-closed in the stateless one) — see the two call sites above + // for why this is injected rather than resolved inline. + TargetFilter::PlayerMatching { player } => matches_player_scope(player, player_id), TargetFilter::Or { filters } => filters.iter().any(|f| { - player_matches_target_filter_with(f, player_id, source_controller, is_opponent) + player_matches_target_filter_with( + f, + player_id, + source_controller, + is_opponent, + matches_player_scope, + ) }), TargetFilter::And { filters } => filters.iter().all(|f| { - player_matches_target_filter_with(f, player_id, source_controller, is_opponent) + player_matches_target_filter_with( + f, + player_id, + source_controller, + is_opponent, + matches_player_scope, + ) }), // CR 102.1 + CR 103.1: seating-neighbor resolution requires // `state.seat_order`, which is not available in this stateless matcher. @@ -7792,12 +7874,183 @@ mod tests { &state, &opponent_filter, PlayerId(1), - Some(PlayerId(0)) + Some(PlayerId(0)), + None, )); assert!(player_matches_target_filter_in_state( &state, &opponent_filter, PlayerId(2), + Some(PlayerId(0)), + None, + )); + } + + /// CR 102.1 + CR 109.5 + CR 115.9c — `TargetFilter::PlayerMatching` is + /// ANSWERED by the player-target legality door, not silently dropped on its + /// wildcard tail. + /// + /// This is the door `targeting::target_ref_matches_resolved_filter`, + /// `casting`'s CR 115.9c check and `ability_utils`' slot enumeration all use + /// for `TargetRef::Player`, so before the arm existed a "target player who + /// has more life than you" filter enumerated ZERO legal players. + /// + /// Revert-failing: delete the `PlayerMatching` arm from + /// `player_matches_target_filter_with` and the first assertion flips to + /// `false` (the wildcard tail), which is exactly the empty-enumeration bug. + #[test] + fn player_matching_is_answered_by_the_player_target_door() { + use crate::types::ability::{PlayerFilter, PlayerRelation}; + + let mut state = GameState::new(FormatConfig::free_for_all(), 3, 42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Predicate Source".to_string(), + crate::types::zones::Zone::Battlefield, + ); + state.players[0].life = 20; + state.players[1].life = 30; + state.players[2].life = 10; + + // "a player who has more life than you" — the exact shape the Namor + // trigger clause lowers to. + let more_life = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }), + }; + + assert!( + player_matches_target_filter_in_state( + &state, + &more_life, + PlayerId(1), + Some(PlayerId(0)), + Some(source), + ), + "30 > 20 — the predicate must admit this player" + ); + assert!( + !player_matches_target_filter_in_state( + &state, + &more_life, + PlayerId(2), + Some(PlayerId(0)), + Some(source), + ), + "10 is not more than 20 — the predicate must discriminate, not admit all" + ); + assert!( + !player_matches_target_filter_in_state( + &state, + &more_life, + PlayerId(0), + Some(PlayerId(0)), + Some(source), + ), + "20 is not more than 20" + ); + + // Nested under `Or`, proving the recursion carries the injected matcher + // rather than losing it one level down. + let nested = TargetFilter::Or { + filters: vec![TargetFilter::None, more_life.clone()], + }; + assert!(player_matches_target_filter_in_state( + &state, + &nested, + PlayerId(1), + Some(PlayerId(0)), + Some(source), + )); + assert!(!player_matches_target_filter_in_state( + &state, + &nested, + PlayerId(2), + Some(PlayerId(0)), + Some(source), + )); + + // A different payload family through the same single authority, so the + // arm is predicate-generic rather than life-specific. + let opponent_only = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::Opponent), + }; + assert!(player_matches_target_filter_in_state( + &state, + &opponent_only, + PlayerId(1), + Some(PlayerId(0)), + Some(source), + )); + assert!(!player_matches_target_filter_in_state( + &state, + &opponent_only, + PlayerId(0), + Some(PlayerId(0)), + Some(source), + )); + + // DECIDED, not defaulted: with no source object the payload is + // unanswerable, so the arm fails closed. + assert!( + !player_matches_target_filter_in_state( + &state, + &more_life, + PlayerId(1), + Some(PlayerId(0)), + None, + ), + "no source object — fail closed, never fail open" + ); + } + + /// CR 109.5 + CR 608.2c — the STATELESS sibling's fail-closed answer is + /// DECIDED, matching the pins the same matcher already carries for + /// `TargetPlayer` / `DefendingPlayer` / `TriggeringPlayer`. + /// + /// A life total is not readable without `state`, so answering `true` here + /// would be fail-OPEN: `player_matches_target_filter` is the CR 115.9c + /// "targets only" path, where fail-open silently widens a restriction. + #[test] + fn player_matching_fails_closed_without_state() { + use crate::types::ability::{PlayerFilter, PlayerRelation}; + + let filter = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }), + }; + assert!(!player_matches_target_filter( + &filter, + PlayerId(1), + Some(PlayerId(0)) + )); + // Reach guard: the same stateless matcher DOES answer a filter it can + // resolve, so the negative above is not vacuous. + assert!(player_matches_target_filter( + &TargetFilter::Player, + PlayerId(1), Some(PlayerId(0)) )); } diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index de41a8dcf7..edf2a5a97c 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -3459,6 +3459,14 @@ fn target_filter_reads_life_total(filter: &TargetFilter) -> bool { TargetFilter::ChosenDamageSource { filter } => filter .as_deref() .is_some_and(target_filter_reads_life_total), + // CR 102.1 + CR 119 + CR 611.3a: the player-axis crossing. A + // `PlayerAttribute { attr: LifeTotal, .. }` payload reads the life family + // directly (Namor, Atlantean King's "a player who has more life than + // you"), so route it through the same authority the object-axis mirror + // `FilterProp::ControllerMatches` uses. Grouping this with the + // payload-free player references below would under-report the layer + // dependency at a life-change site. + TargetFilter::PlayerMatching { player } => player_filter_reads_life(player), // Payload-free / player-reference / stack-reference / anaphoric filters — // none carry a nested walked payload and none read the life family. // Enumerated explicitly (no wildcard). @@ -21622,6 +21630,47 @@ mod tests { )); } + /// CR 102.1 + CR 119 + CR 611.3a: `TargetFilter::PlayerMatching` — the + /// player-axis mirror of `FilterProp::ControllerMatches` — must ROUTE its + /// payload here, not sit with the payload-free player references. + /// + /// Namor, Atlantean King's "a player who has more life than you" is exactly + /// a `PlayerAttribute { attr: LifeTotal, value: LifeTotal }`, so grouping the + /// variant with the non-reading arms would under-report the layer dependency + /// at a life-change site. Revert-failing: move the arm into that group and + /// the first assertion flips. + #[test] + fn player_matching_routes_its_payloads_life_reads() { + let reads = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }), + }; + assert!(target_filter_reads_life_total(&reads)); + + // Negative sibling: a life-free payload must stay `false`, so the + // assertion above is about the ROUTING and not about the variant + // answering `true` unconditionally. + let life_free = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::ControlsCount { + relation: PlayerRelation::All, + filter: TargetFilter::Typed(TypedFilter::land()), + comparator: Comparator::GE, + count: Box::new(QuantityExpr::Fixed { value: 8 }), + }), + }; + assert!(!target_filter_reads_life_total(&life_free)); + } + /// Filter-routed reads: the classifier descends nested payloads on every /// surface (FilterProp → PlayerFilter, and PlayerCount → PlayerFilter), and /// stays `false` for a life-free filter. diff --git a/crates/engine/src/game/quantity.rs b/crates/engine/src/game/quantity.rs index f4497f2719..c262ecdebd 100644 --- a/crates/engine/src/game/quantity.rs +++ b/crates/engine/src/game/quantity.rs @@ -5424,6 +5424,7 @@ fn damage_record_target_matches( remainder, player_id, filter_ctx.source_controller, + Some(filter_ctx.source_id), ) } } diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index 0fe5a09435..bfefffba7c 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -1042,6 +1042,7 @@ fn target_ref_matches_resolved_filter_with_context( target_filter, *player, ctx.source_controller, + Some(ctx.source_id), ), } } @@ -2210,6 +2211,7 @@ fn stack_spell_entry_matches_filter( constraint, *pid, source_controller_opt, + Some(source_id), ), }) { @@ -2229,6 +2231,7 @@ fn stack_spell_entry_matches_filter( constraint, *pid, source_controller_opt, + Some(source_id), ), }) { diff --git a/crates/engine/src/game/trigger_matchers.rs b/crates/engine/src/game/trigger_matchers.rs index e27534bebb..88d2bd08cd 100644 --- a/crates/engine/src/game/trigger_matchers.rs +++ b/crates/engine/src/game/trigger_matchers.rs @@ -729,6 +729,24 @@ fn player_matches_filter( .map(|obj| obj.controller) == Some(player_id) } + // CR 102.1 + CR 603.2: the candidate player must satisfy an arbitrary + // player predicate. Delegates to the single-authority player-scope + // matcher rather than re-implementing any predicate here. + // + // `trigger_controller` is the TRIGGER SOURCE's controller (bound at the + // top of this function), NOT the attacking player — the `PlayerRelation` + // in the payload is relative to that, per CR 109.5 "you". + // + // This arm is load-bearing: the `_ => true` fallback below is + // fail-OPEN, so omitting it would make every `PlayerMatching` predicate + // match every player with no compile error. + TargetFilter::PlayerMatching { player } => crate::game::effects::matches_player_scope( + state, + player_id, + player, + trigger_controller, + source_event_subject_id(source_context), + ), _ => true, } } @@ -753,6 +771,13 @@ fn is_player_scope_damage_filter(filter: &TargetFilter) -> bool { controller: Some(_), properties, }) => type_filters.is_empty() && properties.is_empty(), + // CR 120.3 + CR 102.2: a damage recipient described by a PLAYER + // predicate ("deals damage to a player who has more life than you") is a + // player recipient, never an object one. Decided, not defaulted: the + // `_ => false` tail below would silently misclassify it as an object + // filter. Unreachable today — no printed card produces this shape — but + // pinned by a unit test so a future flip is deliberate. + TargetFilter::PlayerMatching { .. } => true, _ => false, } } @@ -810,6 +835,9 @@ pub(super) fn target_filter_matches_object( TargetFilter::SpecificPlayer { .. } => false, // CR 607 (by analogy): PlayerWhoChoseLabel scopes to players, not objects. TargetFilter::PlayerWhoChoseLabel { .. } => false, + // CR 102.1: PlayerMatching scopes to players, not objects — it is + // evaluated on the player axis by `player_matches_filter`. + TargetFilter::PlayerMatching { .. } => false, // CR 102.1 + CR 103.1: Neighbor scopes to a seating-relative player, // not an object — never matches an object. TargetFilter::Neighbor { .. } => false, @@ -5089,6 +5117,7 @@ fn stack_entry_targets_only( constraint, *pid, source_controller, + Some(ctx.source_id), ), }) } @@ -5120,6 +5149,7 @@ fn stack_entry_targets_any( constraint, *pid, source_controller, + Some(ctx.source_id), ), }) } @@ -5178,6 +5208,137 @@ mod tests { GameState::new_two_player(42) } + /// CR 102.1 + CR 603.2 + CR 119.1 — the load-bearing `PlayerMatching` arm in + /// `player_matches_filter`. + /// + /// The match ends in `_ => true`, which is FAIL-OPEN: without this arm the + /// predicate would admit every player with no compile error, reproducing + /// exactly the bug this change fixes (Namor firing on every player attack). + /// + /// Revert-failing: delete the arm and the two negative assertions flip. + #[test] + fn player_matching_life_predicate_admits_only_qualifying_players() { + let mut state = GameState::new(crate::types::format::FormatConfig::free_for_all(), 3, 42); + let source_id = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Namor, Atlantean King".to_string(), + Zone::Battlefield, + ); + // Controller P0 at 20; P1 above it, P2 below it, and the boundary case. + state.players[0].life = 20; + state.players[1].life = 30; + state.players[2].life = 5; + + let filter = TargetFilter::PlayerMatching { + player: Box::new(crate::types::ability::PlayerFilter::PlayerAttribute { + relation: crate::types::ability::PlayerRelation::All, + attr: Box::new(crate::types::ability::QuantityRef::LifeTotal { + player: crate::types::ability::PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: crate::types::ability::QuantityRef::LifeTotal { + player: crate::types::ability::PlayerScope::Controller, + }, + }), + }), + }; + let ctx = test_trigger_source_context(&state, source_id); + + assert!( + player_matches_filter(&filter, &state, PlayerId(1), &ctx), + "30 > 20 must match" + ); + assert!( + !player_matches_filter(&filter, &state, PlayerId(2), &ctx), + "5 <= 20 must NOT match — the `_ => true` tail is fail-open, so this \ + is the assertion that catches a missing PlayerMatching arm" + ); + // GT, not GE: the controller's own equal life total does not qualify. + assert!( + !player_matches_filter(&filter, &state, PlayerId(0), &ctx), + "20 is not MORE than 20" + ); + } + + /// CR 109.4 — the `ControlsCount` payload evaluates through the same single + /// authority, so the carrier is genuinely predicate-generic rather than + /// life-specific. + #[test] + fn player_matching_controls_count_predicate_discriminates_players() { + let mut state = GameState::new(crate::types::format::FormatConfig::free_for_all(), 3, 42); + let source_id = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Owlbear Cub".to_string(), + Zone::Battlefield, + ); + // P1 controls two lands; P2 controls none. + for i in 0..2 { + let land = create_object( + &mut state, + CardId(100 + i), + PlayerId(1), + "Forest".to_string(), + Zone::Battlefield, + ); + state + .objects + .get_mut(&land) + .expect("land must exist") + .card_types + .core_types + .push(CoreType::Land); + } + + let filter = TargetFilter::PlayerMatching { + player: Box::new(crate::types::ability::PlayerFilter::ControlsCount { + relation: crate::types::ability::PlayerRelation::All, + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Land], + ..Default::default() + }), + comparator: Comparator::GE, + count: Box::new(QuantityExpr::Fixed { value: 2 }), + }), + }; + let ctx = test_trigger_source_context(&state, source_id); + + assert!( + player_matches_filter(&filter, &state, PlayerId(1), &ctx), + "P1 controls two lands and must match" + ); + assert!( + !player_matches_filter(&filter, &state, PlayerId(2), &ctx), + "P2 controls no lands and must not match" + ); + } + + /// CR 120.3 + CR 102.2 — `is_player_scope_damage_filter` classifies a player + /// predicate as a PLAYER recipient. Decided, not defaulted: the match ends + /// in `_ => false`, so nothing but this pin records the decision. + /// + /// Unreachable today (no printed card produces a `PlayerMatching` damage + /// recipient), which is precisely why it is pinned — a future flip must be + /// deliberate. + #[test] + fn player_matching_is_a_player_scope_damage_recipient() { + let filter = TargetFilter::PlayerMatching { + player: Box::new(crate::types::ability::PlayerFilter::Opponent), + }; + assert!(is_player_scope_damage_filter(&filter)); + // Contrast: a real object filter stays object-scoped. + assert!(!is_player_scope_damage_filter(&TargetFilter::Typed( + TypedFilter { + type_filters: vec![TypeFilter::Creature], + ..Default::default() + } + ))); + } + #[test] fn trigger_matcher_covers_registry_entries() { let registry = build_trigger_registry(); diff --git a/crates/engine/src/game/zone_pipeline.rs b/crates/engine/src/game/zone_pipeline.rs index e5fdc56f36..34122a6aa7 100644 --- a/crates/engine/src/game/zone_pipeline.rs +++ b/crates/engine/src/game/zone_pipeline.rs @@ -1966,6 +1966,7 @@ fn legal_aura_attachment_targets( enchant_filter, player.id, Some(controller), + Some(aura_id), ) { Some(TargetRef::Player(player.id)) } else { diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 39fc9309f5..5a1e4a4d76 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -2777,6 +2777,7 @@ fn ability_reads_last_created(def: &AbilityDefinition) -> bool { | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 6d3e812348..8391851992 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7959,6 +7959,185 @@ pub(crate) fn parse_opponent_most_life_restriction(input: &str) -> OracleResult< )) } +/// CR 119.1 + CR 109.5 + CR 810.9a: "who has more life than you" as a +/// per-candidate player predicate. Consumes its OWN `who ` prefix, matching the +/// convention of [`lower::parse_controls_permanent_object`] so that every arm of +/// [`parse_attacked_player_relative_clause`] starts from the same input +/// position. +/// +/// `attr` is read PER CANDIDATE by `effects::candidate_player_scalar_with_state` +/// (team-aware per CR 810.9a); `value` is the CONTROLLER-relative threshold — +/// CR 109.5 "you" is the trigger source's controller, which is not necessarily +/// the attacking player. Sibling of [`parse_opponent_most_life_restriction`], +/// which parses the superlative form of the same CR 119.1 axis. +pub(crate) fn parse_has_more_life_than_you( + input: &str, + relation: PlayerRelation, +) -> OracleResult<'_, PlayerFilter> { + // Nested prefix dispatch (who -> has -> comparative), not one flat tag over + // the whole sentence. + let (input, _) = preceded( + tag("who "), + preceded(tag("has "), tag("more life than you")), + ) + .parse(input)?; + // CR 608.2c consume-on-success: without a trailing clause boundary the tag + // also matches a PREFIX of a longer clause. Two shapes exist: a conjunct + // ("…more life than you and controls a Forest"), where binding the prefix is + // a strictly WEAKER restriction than the printed text; and the corpus's + // "…more life than you do" phrasing (Keeper of the Flame / Keeper of the + // Light: "Choose target opponent who has more life than you do as you + // activate this ability"), where the tag ends mid-clause. Both decline; an + // under-restricted `valid_target` is the silent-drop bug this change exists + // to eliminate. The shared terminator is the single authority for "the + // clause really ended here". + let (input, ()) = nom_primitives::peek_clause_terminator(input)?; + Ok(( + input, + PlayerFilter::PlayerAttribute { + relation, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }, + )) +} + +/// CR 109.4 + CR 107.1: "who controls N or more " — the numeric-threshold +/// axis of the attacked-player relative clause (Owlbear Cub's "attacks a player +/// who controls eight or more lands"). Composed from the shared number +/// primitive and the shared type-phrase combinator; only the comparator/count +/// pairing is local. +/// +/// # Why this is NOT pushed into `lower::parse_controls_permanent_object` +/// +/// That function is the single authority for the EFFECT-SUBJECT position ("each +/// player who controls …"), and it is deliberately left unchanged. Adding this +/// arm there was measured and rejected: it makes the first sentence of the +/// Natural Balance threshold-land grammar ("Each player who controls 6 or more +/// lands chooses 5 lands they control and sacrifices the rest.") parse its +/// SUBJECT while its predicate still cannot be modelled, so the whole-line +/// recognizer's fail-closed `Effect::Unimplemented` degrades into a bogus +/// `TargetOnly { target: Any }` head with the sacrifice clause dropped — a +/// silent wrong shape replacing an honest gap +/// (`threshold_land_balance_rejects_nonbasic_search_variant` pins it). +/// +/// The trigger-clause position has no such downstream predicate: the clause ends +/// at the comma and the count is the whole meaning. Unifying the two positions +/// requires first teaching the effect-subject predicate path to model +/// "chooses N they control and sacrifices the rest", which is a separate +/// change with its own measurement. +fn parse_controls_count_threshold<'a>( + input: &'a str, + relation: PlayerRelation, + ctx: &mut ParseContext, +) -> OracleResult<'a, PlayerFilter> { + let lower = input.to_lowercase(); + let (count, after_verb) = nom_on_lower(input, &lower, |i| { + preceded( + tag("who "), + preceded( + alt((tag("controls "), tag("control "))), + terminated(nom_primitives::parse_number, tag(" or more ")), + ), + ) + .parse(i) + }) + .ok_or_else(|| oracle_err(input))?; + let count = i32::try_from(count).map_err(|_| oracle_err(input))?; + let (filter, rest) = parse_type_phrase_with_ctx(after_verb, ctx); + // Honest-red guard, mirroring the sibling arms in + // `parse_controls_permanent_object`: an unparsed type phrase must fail the + // clause rather than produce a filter that matches everything. + if matches!(filter, TargetFilter::Any) { + return Err(oracle_err(input)); + } + Ok(( + rest, + PlayerFilter::ControlsCount { + relation, + filter, + comparator: Comparator::GE, + count: Box::new(QuantityExpr::Fixed { value: count }), + }, + )) +} + +/// Adapter: bridge the `Option`-returning `who controls …` core into the nom +/// world, and wrap its tuple into the `PlayerFilter` the caller needs. +/// +/// [`lower::parse_controls_permanent_object`] (a) consumes `who ` ITSELF in +/// every arm, so this adapter must be handed the UNCONSUMED remainder; (b) +/// returns a remainder that is a suffix of `input`, so it composes as a nom +/// remainder without further work; (c) already applies its own +/// `TargetFilter::Any` honest-red guard, so a type phrase that fails to parse +/// arrives here as `None` and becomes a clean parse failure rather than a bogus +/// filter. +/// +/// This mirrors `lower::strip_controls_permanent_clause`, which performs the +/// identical tuple -> `PlayerFilter::ControlsCount` wrap for the effect-SUBJECT +/// path. That function cannot be reused directly because it also requires a +/// non-empty trailing verb phrase — a requirement the trigger-clause position +/// does not have (the clause may end at the comma). +fn controls_clause_player_filter<'a>( + input: &'a str, + relation: PlayerRelation, + ctx: &mut ParseContext, +) -> OracleResult<'a, PlayerFilter> { + let (comparator, count, filter, rest) = + lower::parse_controls_permanent_object(input, ctx).ok_or_else(|| oracle_err(input))?; + Ok(( + rest, + PlayerFilter::ControlsCount { + relation, + filter, + comparator, + count: Box::new(count), + }, + )) +} + +/// CR 508.1b + CR 603.2 + CR 102.1: the `who`-headed relative clause narrowing +/// an ATTACKED player ("attacks a player who has more life than you"). Composed +/// by axis — one arm per predicate family — so a new predicate costs one arm, +/// never a full-sentence `tag`. +/// +/// The `who ` token is consumed by the ARMS, exactly once each, never by this +/// dispatcher: `parse_has_more_life_than_you` opens with `tag("who ")`, and +/// `controls_clause_player_filter`'s delegate opens all of its branches with +/// `tag("who ")`. Stripping `who ` here would break the delegate. +/// +/// `input` MUST be the caller's post-noun slice — the text after the single +/// space that follows the attacked-player noun. Every `parse_attack_target` tag +/// carries a LEADING space, so the raw remainder begins with that space and no +/// arm here would match it. +/// +/// `relation` is supplied by the caller from the base attacked-player noun +/// ("a player" -> All, "one of your opponents" -> Opponent), so the base-scope +/// and predicate axes compose rather than multiply. +pub(crate) fn parse_attacked_player_relative_clause<'a>( + input: &'a str, + relation: PlayerRelation, + ctx: &mut ParseContext, +) -> OracleResult<'a, PlayerFilter> { + if let Ok((rest, filter)) = parse_has_more_life_than_you(input, relation) { + return Ok((rest, filter)); + } + // Threshold form BEFORE the shared core: the core's bare-presence arm would + // otherwise consume "controls " as (GE, 1) and hand "eight or more lands" to + // the type phrase. + if let Ok((rest, filter)) = parse_controls_count_threshold(input, relation, ctx) { + return Ok((rest, filter)); + } + controls_clause_player_filter(input, relation, ctx) +} + fn try_parse_choose_player_to_verb( tp: TextPair<'_>, ctx: &mut ParseContext, @@ -8552,6 +8731,7 @@ fn rebind_controller_scope(filter: &mut TargetFilter, from: ControllerRef, to: C | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo diff --git a/crates/engine/src/parser/oracle_nom/primitives.rs b/crates/engine/src/parser/oracle_nom/primitives.rs index 03640bbfa5..fd81aa611d 100644 --- a/crates/engine/src/parser/oracle_nom/primitives.rs +++ b/crates/engine/src/parser/oracle_nom/primitives.rs @@ -1005,6 +1005,31 @@ pub fn parse_phrase_fragment(input: &str) -> OracleResult<'_, &str> { ))) } +/// CR 608.2c consume-on-success: a subordinate clause has genuinely ENDED here. +/// +/// A relative-clause combinator that succeeds while leaving unconsumed words +/// behind has not modelled the sentence — it has modelled a PREFIX of it, and +/// the caller that discards that remainder silently drops the rest of the +/// restriction. ("attacks a player who has more life than you AND CONTROLS A +/// FOREST" binding only the life half is strictly wrong; "…more life than YOU +/// HAVE" is a different phrasing entirely.) The honest answer for both is to +/// decline the clause, not to bind an under-restricted filter. +/// +/// The clause ends when the remainder is exhausted, or when the next character +/// opens a new clause (`,`) or ends the sentence (`.`). Non-consuming (`peek`), +/// so callers use it purely as a guard. Object-axis sibling: +/// `oracle_target::parse_attacking_status_clause_boundary`, which adds +/// conjunction/conditional lookahead specific to that suffix chain. +pub fn peek_clause_terminator(input: &str) -> OracleResult<'_, ()> { + peek(alt(( + value((), tag(",")), + value((), tag(".")), + // `space0` makes this cover both "" and a trailing-whitespace tail. + value((), (space0, eof)), + ))) + .parse(input) +} + // ── Word-boundary scanning primitives ───────────────────────────────── // // These are the shared building blocks for scanning Oracle text at word diff --git a/crates/engine/src/parser/oracle_nom/quantity.rs b/crates/engine/src/parser/oracle_nom/quantity.rs index c1d6de9e94..77a0ed6423 100644 --- a/crates/engine/src/parser/oracle_nom/quantity.rs +++ b/crates/engine/src/parser/oracle_nom/quantity.rs @@ -2422,6 +2422,7 @@ fn filter_is_population_anchored(filter: &TargetFilter) -> bool { | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo @@ -2555,6 +2556,7 @@ pub(crate) fn objects_filter_zone_is_unambiguous(filter: &TargetFilter) -> bool | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo diff --git a/crates/engine/src/parser/oracle_static/restriction.rs b/crates/engine/src/parser/oracle_static/restriction.rs index c87e4a7e1a..4718069d92 100644 --- a/crates/engine/src/parser/oracle_static/restriction.rs +++ b/crates/engine/src/parser/oracle_static/restriction.rs @@ -2332,6 +2332,7 @@ fn usable_disjunctive_permission_filter(filter: &TargetFilter) -> bool { | TargetFilter::SpecificObject { .. } | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::ScopedPlayer | TargetFilter::AttachedTo diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index a8af054692..780a3e9d53 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -4957,6 +4957,16 @@ fn parse_attacking_defender_suffix(text: &str) -> Option<(FilterProp, usize)> { return Some((prop, text.len() - rest.len())); } + // CR 508.5: the defending-player anaphor is a separate axis from the printed + // defender nouns enumerated in the table below, so it is tried as its own + // composed combinator rather than appended as another literal row. It runs on + // `trimmed`, after the "that's "/"that is "/"that are " relative-clause intro + // has already been stripped, which is what makes Ordruun Mentor's and Echoing + // Assault's "that's attacking that player" work with no extra grammar. + if let Ok((rest, prop)) = parse_attacking_defender_anaphor(trimmed) { + return Some((prop, text.len() - rest.len())); + } + for (pattern, defender) in [ ( "attacking you or a planeswalker you control", @@ -5022,6 +5032,79 @@ fn parse_attacking_alone_suffix_status(input: &str) -> OracleResult<'_, FilterPr Ok((input, FilterProp::AttackingAlone)) } +/// CR 508.5 + CR 608.2c: "attacking that player" — the defending-player ANAPHOR +/// in an object-filter position ("other creatures you control attacking that +/// player", "target creature that's attacking that player"). Inside a trigger +/// body, "that player" is the player the trigger source is attacking (CR 508.5a: +/// one specific defending player in multiplayer), resolved at runtime through +/// `combat::defending_player_cr508_5` via `ControllerRef::DefendingPlayer`. +/// +/// NOT `ControllerRef::TriggeringPlayer`: for `GameEvent::AttackersDeclared`, +/// `targeting::extract_player_from_event` returns the ATTACKING player, which is +/// the opposite referent. +/// +/// A distinct AXIS from the `(pattern, defender)` table in +/// `parse_attacking_defender_suffix` (which enumerates printed defender NOUNS: +/// "you", "your opponents", ...); this arm is the anaphor, so it is a composed +/// combinator rather than another row in that table. +/// +/// # Targeted consumers need `ability_utils::filter_needs_trigger_source` +/// +/// This is the first value of `FilterProp::Attacking { defender }` that resolves +/// through `trigger_source`. Two of the three cards this arm unlocks — Ordruun +/// Mentor and Echoing Assault — place it in a TARGET filter, whose slot-build +/// door (`targeting::find_legal_targets`) builds a context with +/// `trigger_source: None` and would enumerate ZERO legal targets on any +/// multi-attacker declaration (CR 603.3d would then remove the ability from the +/// stack). `ability_utils::filter_needs_trigger_source` routes them to the +/// context-carrying door; do not ship this combinator without it. +/// +/// # Why this cannot steal the token-spec / battlefield-entry corpus +/// +/// Most of the 52 corpus cards containing "attacking that player" sit in a +/// token-spec, battlefield-entry, continuation-sentence, copy-token, or +/// predicative-state-change position, and many of them terminate with a bare "." +/// immediately after the phrase (Ainok Strike Leader, The Vast Scrier, Owlbear +/// Cub), which SATISFIES `parse_attacking_status_clause_boundary` rather than +/// being rejected by it. The boundary guard is therefore NOT what keeps them +/// safe. What keeps them safe is that none of those positions ever routes the +/// phrase through `parse_type_phrase`'s suffix chain, because each consuming +/// path removes or absorbs the clause first: +/// +/// 1. Inline token specs — `oracle_effect::token` scans word boundaries for the +/// `that's|that is|that are` + `tapped and attacking|attacking` clause and +/// TRUNCATES the token body at that byte offset; the trailing "that player" +/// is discarded with the clause. +/// 2. Battlefield-entry tails — `parse_battlefield_entry_qualifiers` matches +/// " tapped and attacking" and its qualifier boundary absorbs the trailing +/// player phrase; only `(enter_tapped, enters_attacking)` flags come back. +/// 3. Continuation sentences ("It/The token enters tapped and attacking that +/// player.") — dispatched at sentence level in `oracle_effect::sequence` into +/// a continuation that patches the PRECEDING effect's flags. No filter built. +/// 4. Copy-token modifiers — `parse_copy_token_entry_modifiers` consumes +/// "tapped and attacking " as a `value(...)` tag before the noun. +/// +/// Predicative state-change sentences (Portal Manipulator "Those creatures are +/// now attacking that player.", Tahngarth "Tahngarth is attacking that player or +/// planeswalker.") are also unreachable: the suffix chain is offered the +/// remainder AFTER a type-phrase noun, and there that remainder begins with a +/// copula ("are now ", "is "), not with `tag("attacking")`. +/// +/// "attacking that opponent" is deliberately EXCLUDED: a corpus scan shows it +/// occurs only in the positions enumerated above (Kaalia of the Vast, Adeline, +/// Mardu Siegebreaker, ...), never as an object-filter suffix, so accepting it +/// here would add zero coverage. +fn parse_attacking_defender_anaphor(input: &str) -> OracleResult<'_, FilterProp> { + let (rest, _) = (tag("attacking"), space1, tag("that player")).parse(input)?; + let (_, _) = parse_attacking_status_clause_boundary(rest)?; + Ok(( + rest, + FilterProp::Attacking { + defender: Some(ControllerRef::DefendingPlayer), + }, + )) +} + fn parse_attacking_status_clause_boundary(input: &str) -> OracleResult<'_, ()> { let trimmed = input.trim_start(); let (_, _) = not(alt(( diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index d6156f6cb5..1348c15568 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -7,7 +7,7 @@ use crate::parser::oracle_ir::doc::{ use crate::parser::oracle_ir::static_ir::StaticIr; use crate::types::ability::{ AdditionalCostOrigin, AdditionalCostPaymentSource, CountScope, CounterAdjustment, DoorLockOp, - SpellStackToGraveyardReplacement, + PlayerRelation, SpellStackToGraveyardReplacement, }; use crate::types::counter::{CounterMatch, CounterType}; use crate::types::triggers::AttackTargetFilter; @@ -25994,3 +25994,635 @@ fn bound_delayed_recalls_are_not_demoted() { ); } } + +// --------------------------------------------------------------------------- +// Namor, Atlantean King — the attacked-player predicate (CR 603.2) and the +// "attacking that player" defending-player anaphor (CR 508.5). +// --------------------------------------------------------------------------- + +/// Namor, Atlantean King — verbatim Scryfall Oracle text (oracle id +/// 171a0a09-4aee-466c-aebd-3b0d4c1f51b4). +const NAMOR_ORACLE: &str = "Flying\nWhenever you cast a noncreature spell, create a 1/1 blue Merfolk creature token.\nWhenever Namor attacks a player who has more life than you, other creatures you control attacking that player get +2/+0 until end of turn."; + +fn parse_namor() -> ParsedAbilities { + parse( + NAMOR_ORACLE, + "Namor, Atlantean King", + &[Keyword::Flying], + &["Creature"], + &["Mutant", "Merfolk", "Noble"], + ) +} + +fn namor_attack_trigger(parsed: &ParsedAbilities) -> &TriggerDefinition { + parsed + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .expect("Namor must have an Attacks trigger") +} + +fn target_filter_has_defending_player_anaphor(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::Typed(typed) => typed.properties.contains(&FilterProp::Attacking { + defender: Some(ControllerRef::DefendingPlayer), + }), + TargetFilter::Not { filter } => target_filter_has_defending_player_anaphor(filter), + TargetFilter::Or { filters } | TargetFilter::And { filters } => filters + .iter() + .any(target_filter_has_defending_player_anaphor), + _ => false, + } +} + +/// V2 — Namor's effect body must scope the pump to the co-attackers on the +/// attacked player, not to `TargetFilter::Any`. +/// +/// Revert-failing: without `parse_attacking_defender_anaphor`, +/// `parse_type_phrase` leaves "attacking that player" unconsumed, +/// `parse_subject_application`'s rest-empty gate fails, and the clause falls +/// through to `parse_numeric_imperative_ast`, which emits the documented +/// `Effect::Pump { target: TargetFilter::Any }` sentinel — a board-wide pump +/// that hits both players' permanents, lands included. +#[test] +fn namor_pump_scopes_to_other_attackers_of_the_defending_player() { + let result = parse_namor(); + + // Positive reach-guard: the card parses with zero Unimplemented, so the + // assertions below cannot pass vacuously via a total parse failure. + assert!( + !parsed_has_unimplemented(&result), + "Namor must parse with zero Unimplemented effects: {result:#?}" + ); + + let attack = namor_attack_trigger(&result); + let execute = attack + .execute + .as_ref() + .expect("attack trigger should have an execute body"); + assert_eq!(execute.duration, Some(Duration::UntilEndOfTurn)); + + let Effect::PumpAll { + power, + toughness, + target, + } = &*execute.effect + else { + panic!( + "expected a class-scoped PumpAll, got {:?} — a `Pump {{ target: Any }}` here means the \ + \"attacking that player\" suffix was dropped and the board-wide sentinel shipped", + execute.effect + ); + }; + assert_eq!(*power, PtValue::Fixed(2)); + assert_eq!(*toughness, PtValue::Fixed(0)); + + let TargetFilter::Typed(typed) = target else { + panic!("expected a Typed pump filter, got {target:?}"); + }; + assert_eq!(typed.controller, Some(ControllerRef::You)); + assert!( + typed.type_filters.contains(&TypeFilter::Creature), + "type_filters={:?}", + typed.type_filters + ); + assert!( + typed.properties.contains(&FilterProp::Attacking { + defender: Some(ControllerRef::DefendingPlayer), + }), + "pump filter must carry the CR 508.5 defending-player anaphor, got {:?}", + typed.properties + ); + assert!( + typed.properties.contains(&FilterProp::Another), + "\"other creatures\" must exclude Namor itself, got {:?}", + typed.properties + ); +} + +/// V10 — the anaphor covers the CLASS, not just Namor: both printed cards that +/// place "attacking that player" in a genuine TARGET filter must now carry the +/// CR 508.5 property. Echoing Assault additionally exercises an EXCLUDED +/// position in its second sentence, so one card proves both halves. +#[test] +fn attacking_that_player_target_filters_carry_the_defending_player_anaphor() { + // Verbatim Oracle text (MTGJSON); a paraphrase can take a different parser + // branch and go green while the real card stays broken. + let ordruun = parse( + "Mentor (Whenever this creature attacks, put a +1/+1 counter on target attacking creature with lesser power.)\nWhenever you attack a player, target creature that's attacking that player gains first strike until end of turn.", + "Ordruun Mentor", + &[Keyword::Mentor], + &["Creature"], + &["Human", "Soldier"], + ); + assert!( + ordruun + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .any(definition_chain_has_defending_player_anaphor), + "Ordruun Mentor's target filter must scope to the attacked player: {ordruun:#?}" + ); + + let echoing = parse( + "Creature tokens you control have menace.\nWhenever you attack a player, choose target nontoken creature that's attacking that player. Create a token that's a copy of that creature, except it's 1/1. The token enters tapped and attacking that player. Sacrifice it at the beginning of the next end step.", + "Echoing Assault", + &[], + &["Enchantment"], + &[], + ); + assert!( + echoing + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .any(definition_chain_has_defending_player_anaphor), + "Echoing Assault's target filter must scope to the attacked player: {echoing:#?}" + ); +} + +fn definition_chain_has_defending_player_anaphor(def: &AbilityDefinition) -> bool { + def.effect + .target_filter() + .is_some_and(target_filter_has_defending_player_anaphor) + || def + .sub_ability + .as_deref() + .is_some_and(definition_chain_has_defending_player_anaphor) + || def + .else_ability + .as_deref() + .is_some_and(definition_chain_has_defending_player_anaphor) +} + +/// V9 — the anaphor must not steal the token-spec / battlefield-entry / +/// continuation / copy-token members of the 52-card "attacking that player" +/// corpus. Many of them terminate with a bare "." right after the phrase, which +/// SATISFIES the clause boundary, so the boundary guard is not what protects +/// them — the excluded positions never route the phrase through +/// `parse_type_phrase`'s suffix chain at all. +#[test] +fn excluded_attacking_that_player_positions_are_not_stolen() { + // Inline token spec, bare "." terminator. Verbatim Oracle text (MTGJSON) — + // the real card wraps the token spec in a "for each opponent" distributive, + // which a paraphrase without it would not exercise. + let ainok = parse( + "Whenever you attack with this creature and/or your commander, for each opponent, create a 1/1 red Goblin creature token that's tapped and attacking that player.\n\ + Sacrifice this creature: Creature tokens you control gain indestructible until end of turn.", + "Ainok Strike Leader", + &[], + &["Creature"], + &["Dog", "Warrior"], + ); + assert!( + !parsed_has_unimplemented(&ainok), + "reach guard: Ainok Strike Leader must parse: {ainok:#?}" + ); + assert!( + ainok + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .any(|d| matches!( + &*d.effect, + Effect::Token { + enters_attacking: true, + tapped: true, + .. + } + )), + "inline token specs must still lower to an attacking token: {ainok:#?}" + ); + assert!( + !ainok + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .any(definition_chain_has_defending_player_anaphor), + "a token spec's entry flag must not become a target-filter suffix: {ainok:#?}" + ); + + // Battlefield-entry tail. Verbatim Oracle text (MTGJSON), including the + // "Soldier, Warrior, or Wizard" restriction and both trailing sentences — + // the paraphrase this row used to carry dropped all three. + let vast_scrier = parse( + "Flying\n\ + Whenever The Vast Scrier attacks a player, you may put a Soldier, Warrior, or Wizard creature card from your hand onto the battlefield tapped and attacking that player. If it has any \"Whenever this creature attacks\" triggers, those trigger. If you don't put a card onto the battlefield this way, scry 2.", + "The Vast Scrier", + &[Keyword::Flying], + &["Creature"], + &["Human", "Cleric"], + ); + // Positive reach-guard: the battlefield-entry tail was consumed as ENTRY + // FLAGS on the `ChangeZone`, which is the whole point of the exclusion. + let entry = vast_scrier + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .find(|d| matches!(&*d.effect, Effect::ChangeZone { .. })) + .unwrap_or_else(|| { + panic!("The Vast Scrier keeps its battlefield-entry effect: {vast_scrier:#?}") + }); + let Effect::ChangeZone { + enter_tapped, + enters_attacking, + target, + .. + } = &*entry.effect + else { + unreachable!("matched above"); + }; + assert!( + matches!(enter_tapped, crate::types::zones::EtbTapState::Tapped) && *enters_attacking, + "\"tapped and attacking that player\" must land as entry flags: {entry:#?}" + ); + assert!( + !target_filter_has_defending_player_anaphor(target), + "battlefield-entry tails must not become filter suffixes: {entry:#?}" + ); + // The two trailing sentences the engine cannot model stay honestly RED + // (`Effect::Unimplemented`), so this row is not asserting coverage the card + // does not have. + assert!( + parsed_has_unimplemented(&vast_scrier), + "the \"those trigger\" rider is unmodelled and must remain honestly red: {vast_scrier:#?}" + ); + + // Predicative state-change sentence — a copula, not a filter suffix. + // Verbatim Oracle text (MTGJSON): the real card is a Creature with an ETB + // trigger, so its content lands in `triggers`, not `abilities`. + let portal = parse( + "Flash\n\ + When this creature enters during the declare attackers step, choose target player and any number of target attacking creatures their opponents control. Those creatures are now attacking that player.", + "Portal Manipulator", + &[Keyword::Flash], + &["Creature"], + &["Human", "Wizard"], + ); + // Positive reach-guard: the sentence really did reach the parser and + // produced a live trigger with a body. Without this the negative below + // passes for the wrong reason on a total parse failure. + let portal_chain: Vec<&AbilityDefinition> = portal + .triggers + .iter() + .filter_map(|t| t.execute.as_deref()) + .collect(); + assert!( + !portal_chain.is_empty(), + "reach guard: Portal Manipulator's ETB trigger must parse to a body: {portal:#?}" + ); + // The negative, over BOTH axes and through the full sub/else chain (the + // shallow `abilities`-only, top-level-effect-only traversal this row used to + // carry could not see a `sub_ability`). + assert!( + !portal_chain + .iter() + .copied() + .any(definition_chain_has_defending_player_anaphor) + && !portal + .abilities + .iter() + .any(definition_chain_has_defending_player_anaphor), + "predicative state-change sentences must not become filter suffixes: {portal:#?}" + ); + // And the sentence the engine cannot model stays honestly RED rather than + // silently degrading into a wrong filter. + assert!( + portal_chain + .iter() + .copied() + .any(def_chain_has_unimplemented), + "the unmodelled copula sentence must remain honestly red: {portal:#?}" + ); +} + +/// V1 — Namor's *event clause* must carry the attacked-player predicate on +/// `valid_target` (CR 603.2, checked once at declaration) and NOT on `condition` +/// (CR 603.4, re-checked at resolution). Namor's text has no "if", so the +/// relative clause is part of the trigger event, not an intervening-if. +/// +/// Revert-failing: before the fix `parse_attack_target`'s remainder was +/// discarded, leaving `valid_target: None` — the trigger fired on every player +/// attack regardless of life totals. +#[test] +fn namor_attack_trigger_binds_the_more_life_predicate_to_the_event_clause() { + let result = parse_namor(); + assert!( + !parsed_has_unimplemented(&result), + "Namor must parse with zero Unimplemented effects: {result:#?}" + ); + + let attack = namor_attack_trigger(&result); + assert_eq!( + attack.attack_target_filter, + Some(AttackTargetFilter::Player), + "base attacked-player scope must survive the relative clause" + ); + assert_eq!(attack.valid_card, Some(TargetFilter::SelfRef)); + + assert_eq!( + attack.valid_target, + Some(TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }), + }), + "\"who has more life than you\" belongs on the CR 603.2 event channel" + ); + + // CR 603.4 does not apply: no "if" immediately follows the trigger event, so + // the predicate must NOT be re-checked at resolution. + assert_eq!( + attack.condition, None, + "a CR 603.2 event clause must not become a CR 603.4 intervening-if" + ); +} + +/// Class coverage for the "who controls N or more " axis (Owlbear Cub). +/// Without the numeric-threshold arm this card would fall to the fail-closed +/// guard and land honestly RED; with it, the predicate is modelled. Verbatim +/// Oracle text (MTGJSON). +#[test] +fn owlbear_cub_attacked_player_land_threshold_predicate_is_bound() { + let parsed = parse( + "Mama's Coming — Whenever this creature attacks a player who controls eight or more lands, look at the top eight cards of your library. You may put a creature card from among them onto the battlefield tapped and attacking that player. Put the rest on the bottom of your library in a random order.", + "Owlbear Cub", + &[], + &["Creature"], + &["Owlbear"], + ); + let attack = parsed + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .expect("Owlbear Cub keeps its attack trigger"); + let Some(TargetFilter::PlayerMatching { player }) = &attack.valid_target else { + panic!( + "Owlbear Cub's land-count predicate must bind to valid_target, got {:?}", + attack.valid_target + ); + }; + let PlayerFilter::ControlsCount { + relation, + filter, + comparator, + count, + } = player.as_ref() + else { + panic!("expected a ControlsCount predicate, got {player:?}"); + }; + assert_eq!(*relation, PlayerRelation::All); + assert_eq!(*comparator, Comparator::GE); + assert_eq!(**count, QuantityExpr::Fixed { value: 8 }); + let TargetFilter::Typed(typed) = filter else { + panic!("expected a typed land filter, got {filter:?}"); + }; + assert!(typed.type_filters.contains(&TypeFilter::Land)); + assert_eq!(attack.condition, None); +} + +/// V11 — consume-on-success: a `who`-headed clause the predicate grammar cannot +/// model must fall to `Effect::Unimplemented`, never silently leave the broad +/// `Player` scope behind (that is precisely the bug being fixed). +/// +/// This is the direct regression test for the guard's SLICE: every +/// `parse_attack_target` tag carries a leading space, so a guard aimed at the +/// raw remainder could never fire. The comma control proves the post-noun +/// fallback binding does not mis-fire when no space follows the noun. +#[test] +fn unmodelled_attacked_player_predicate_fails_closed() { + let declined = parse( + "Whenever ~ attacks a player who has drawn three cards this turn, draw a card.", + "Fail Closed Test", + &[], + &["Creature"], + &[], + ); + // The honest-red marker for an unparsed trigger EVENT is + // `TriggerMode::Unknown` (which `game::coverage` counts as a gap), not an + // `Unimplemented` effect — the body ("draw a card") parses fine; it is the + // event clause that could not be modelled. + assert!( + declined + .triggers + .iter() + .all(|t| matches!(t.mode, TriggerMode::Unknown(_))), + "an unmodelled `who` predicate must fail closed to TriggerMode::Unknown, got {declined:#?}" + ); + assert!( + !declined.triggers.iter().any(|t| t.attack_target_filter + == Some(AttackTargetFilter::Player) + && t.valid_target.is_none()), + "declining must not leave a broad attacked-player scope behind: {:?}", + declined.triggers + ); + + // Paired positive reach-guard: the same sentence shape with a MODELLED + // predicate parses, so the negative above cannot pass vacuously. + let accepted = parse( + "Whenever ~ attacks a player who has more life than you, draw a card.", + "Fail Closed Reach Guard", + &[], + &["Creature"], + &[], + ); + assert!( + !parsed_has_unimplemented(&accepted), + "the modelled predicate must still parse: {accepted:#?}" + ); + assert!( + matches!( + accepted + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .and_then(|t| t.valid_target.as_ref()), + Some(TargetFilter::PlayerMatching { .. }) + ), + "reach guard must bind the predicate: {:?}", + accepted.triggers + ); + + // Comma control: no relative clause at all, so the post-noun fallback + // binding must leave the plain attacked-player scope untouched. + let comma = parse( + "Whenever ~ attacks a player, draw a card.", + "Comma Control", + &[], + &["Creature"], + &[], + ); + let plain = comma + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .expect("plain attack trigger"); + assert_eq!(plain.attack_target_filter, Some(AttackTargetFilter::Player)); + assert_eq!(plain.valid_target, None); + assert!(!parsed_has_unimplemented(&comma)); +} + +/// V8 — sibling grammar is not stolen. A `with `-headed tail modifies the +/// ATTACK, not the attacked player, so it must not become a `PlayerMatching` +/// predicate. The Vast Scrier additionally proves a bare `attacks a player` +/// clause with an "attacking that player" BODY keeps its plain event scope. +#[test] +fn attack_trigger_tails_that_are_not_player_predicates_are_untouched() { + // Real cards carry their VERBATIM MTGJSON Oracle text; the one synthetic row + // is named as such because it probes a grammar fragment (Goad's reminder + // clause) rather than standing in for a printed card. + for (name, text, keywords, subtypes) in [ + ( + "Akiri, Fearless Voyager", + "Whenever you attack a player with one or more equipped creatures, draw a card.\n\ + {W}: You may unattach an Equipment from a creature you control. If you do, tap that creature and it gains indestructible until end of turn.", + &[][..], + &["Kor", "Warrior"][..], + ), + ( + "Goad Reminder (synthetic grammar probe)", + "Whenever ~ attacks a player other than you if able, draw a card.", + &[][..], + &[][..], + ), + ( + "The Vast Scrier", + "Flying\n\ + Whenever The Vast Scrier attacks a player, you may put a Soldier, Warrior, or Wizard creature card from your hand onto the battlefield tapped and attacking that player. If it has any \"Whenever this creature attacks\" triggers, those trigger. If you don't put a card onto the battlefield this way, scry 2.", + &[Keyword::Flying][..], + &["Human", "Cleric"][..], + ), + ] { + let parsed = parse(text, name, keywords, &["Creature"], subtypes); + // Positive reach-guard: the attack sentence reached the attack-trigger + // parser and produced a live trigger with a body. A total parse failure + // (or a `TriggerMode::Unknown` decline) would make the negative below + // pass for the wrong reason. + assert!( + parsed.triggers.iter().any(|t| matches!( + t.mode, + TriggerMode::Attacks | TriggerMode::YouAttack + ) && t.execute.is_some()), + "{name}: reach guard — the attack trigger must parse: {parsed:#?}" + ); + for trigger in &parsed.triggers { + assert!( + !matches!( + trigger.valid_target, + Some(TargetFilter::PlayerMatching { .. }) + ), + "{name}: non-`who` tails are not player predicates, got {:?}", + trigger.valid_target + ); + } + } +} + +/// V11b — CR 608.2c consume-on-success on the REMAINDER. An arm that models +/// only a PREFIX of the relative clause must decline, exactly like an arm that +/// cannot match at all: binding the prefix would leave an UNDER-restricted +/// `valid_target` and silently drop the rest of the printed restriction, which +/// is the same silent-drop failure mode this change was written to eliminate. +/// +/// Revert-failing: delete the `peek_clause_terminator` filter in +/// `oracle_trigger`'s predicate hook (and/or the one inside +/// `parse_has_more_life_than_you`) and both rows below bind +/// `PlayerMatching { PlayerAttribute { .. } }` while the conjunct / trailing +/// verb vanishes. +#[test] +fn attacked_player_predicate_requires_a_clause_boundary() { + for (name, text) in [ + // Conjunct the predicate grammar cannot model: the life half alone is a + // strictly weaker restriction than the printed text. + ( + "Partial Predicate Conjunct", + "Whenever ~ attacks a player who has more life than you and controls a Forest, draw a card.", + ), + // A different phrasing whose PREFIX coincides with the modelled one. + // "…more life than you do" is the corpus's other comparative form + // (Keeper of the Flame / Keeper of the Light), so this is a real + // grammar collision, not an invented one. + ( + "Partial Predicate Trailing Verb", + "Whenever ~ attacks a player who has more life than you do, draw a card.", + ), + ] { + let parsed = parse(text, name, &[], &["Creature"], &[]); + assert!( + parsed + .triggers + .iter() + .all(|t| matches!(t.mode, TriggerMode::Unknown(_))), + "{name}: a partially-modelled `who` clause must fail closed to \ + TriggerMode::Unknown, got {parsed:#?}" + ); + assert!( + !parsed.triggers.iter().any(|t| matches!( + t.valid_target, + Some(TargetFilter::PlayerMatching { .. }) + )), + "{name}: binding the PREFIX is an under-restricted scope, not coverage: {:?}", + parsed.triggers + ); + assert!( + !parsed.triggers.iter().any(|t| t.attack_target_filter + == Some(AttackTargetFilter::Player) + && t.valid_target.is_none()), + "{name}: declining must not leave a broad attacked-player scope behind: {:?}", + parsed.triggers + ); + } + + // Paired positive reach-guard: the clause that DOES end at a boundary still + // binds, so the negatives above cannot pass vacuously. + let accepted = parse( + "Whenever ~ attacks a player who has more life than you, draw a card.", + "Boundary Reach Guard", + &[], + &["Creature"], + &[], + ); + assert!( + matches!( + accepted + .triggers + .iter() + .find(|t| t.mode == TriggerMode::Attacks) + .and_then(|t| t.valid_target.as_ref()), + Some(TargetFilter::PlayerMatching { .. }) + ), + "reach guard must still bind the boundary-terminated predicate: {:?}", + accepted.triggers + ); +} + +/// V17 — Loot Dispute is untouched, not silently absorbed. `parse_attack_target` +/// has no `" the player"` arm, so the definite-article base noun never produces +/// an attacked-player scope and the fail-closed guard's precondition is never +/// met; `PlayerFilter` also has no initiative-designation variant. +/// +/// TRIPWIRE: adding `" the player"` to `parse_attack_target` later must +/// consciously decide whether Loot Dispute flips to honestly RED. +#[test] +fn loot_dispute_initiative_predicate_is_unchanged() { + let parsed = parse( + "Whenever you attack the player who has the initiative, create a Treasure token.", + "Loot Dispute", + &[], + &["Artifact"], + &[], + ); + for trigger in &parsed.triggers { + assert_eq!(trigger.attack_target_filter, None); + assert_eq!(trigger.valid_target, None); + } +} diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index 11ccbafb0f..720fcd56ff 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -10,8 +10,8 @@ use nom::Parser; use super::oracle_effect::conditions::source_saddled_filter; use super::oracle_effect::{ attach_terminal_die_result_branches_before_finalization, condition_text_is_rehomeable, - lower_effect_chain_ir, parse_effect_chain_ir, try_parse_reanimator_aura_etb_effect_ir, - try_parse_reanimator_aura_grant_etb_effect_ir, + lower_effect_chain_ir, parse_attacked_player_relative_clause, parse_effect_chain_ir, + try_parse_reanimator_aura_etb_effect_ir, try_parse_reanimator_aura_grant_etb_effect_ir, }; use super::oracle_ir::ast::parsed_clause; use super::oracle_ir::context::{ParseContext, TriggerConditionScope}; @@ -54,7 +54,7 @@ use crate::types::ability::{ DamageAmountScope, DamageAmountThreshold, DamageChannel, DamageKindFilter, DestinationConstraint, DieResultFilter, Effect, EffectScope, FilterProp, ManaAbilityProducedFilter, ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter, - PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, RenownSubject, + PlayerRelation, PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, RenownSubject, SacrificeAggregateStat, SacrificeCost, SacrificeRequirement, SharedQuality, StaticCondition, SubAbilityLink, TapCreaturesRequirement, TapStateChange, TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, TypeFilter, TypedFilter, UnlessPayModifier, @@ -9529,7 +9529,7 @@ pub(crate) fn parse_trigger_condition( // ctx.diagnostics now contains only pre-existing diagnostics (restored to snapshot) // Parse event verb from the remaining text. - if let Some((mode, mut def)) = try_parse_event(&subject, rest, &lower) { + if let Some((mode, mut def)) = try_parse_event(&subject, rest, &lower, ctx) { // Re-emit subject diagnostics — the trigger parsed but the subject degraded to Any. ctx.diagnostics.extend(subject_diagnostics); if is_batched { @@ -11091,6 +11091,7 @@ fn try_parse_event( subject: &TargetFilter, rest: &str, full_lower: &str, + ctx: &mut ParseContext, ) -> Option<(TriggerMode, TriggerDefinition)> { let rest = rest.trim_start(); @@ -11421,7 +11422,11 @@ fn try_parse_event( )) .parse(input) } - let attack_target_filter = parse_attack_target.parse(after).ok().map(|(_, f)| f); + // Retain the REMAINDER: discarding it here is what silently dropped + // "who has more life than you" (Namor, Atlantean King) and "who controls + // eight or more lands" (Owlbear Cub) from the trigger event clause. + let attack_target_parsed = parse_attack_target.parse(after).ok(); + let attack_target_filter = attack_target_parsed.as_ref().map(|(_, f)| f.clone()); let attacks_one_of_your_opponents = tag::<_, _, OracleError<'_>>(" one of your opponents") .parse(after) .is_ok(); @@ -11458,7 +11463,86 @@ fn try_parse_event( def.valid_card = Some(subject.clone()); } def.attack_target_filter = attack_target_filter; - if attacks_one_of_your_opponents { + // CR 603.2 + CR 508.1b: a `who`-headed relative clause narrows the + // TRIGGER EVENT's defending player ("attacks a player WHO HAS MORE LIFE + // THAN YOU"). It belongs on `valid_target` — read once by + // `trigger_matchers::attack_target_matches` at declaration — and NOT on + // `condition`, which CR 603.4 re-checks at resolution. The clause has no + // "if", so CR 603.4 does not apply to it; putting it on `condition` + // would wrongly remove the ability from the stack if the defender's life + // changed in response. + // + // `AttackTargetFilter::PlayerOrPlaneswalker` is deliberately NOT in this + // gate: it is produced only by the "you or a planeswalker you control" + // tags, after which a `who`-headed clause is grammatically impossible, + // and `attack_target_matches` would resolve such a candidate through + // `defending_player_for_target_or` (a planeswalker's CONTROLLER) — a + // different referent that needs its own fixture, not a quiet inclusion. + let mut declined_unmodelled_predicate = false; + if matches!(def.attack_target_filter, Some(AttackTargetFilter::Player)) { + if let Some((rest_after_noun, _)) = &attack_target_parsed { + // Every `parse_attack_target` arm's tag carries a LEADING space, + // so `rest` begins with the character AFTER the noun: a space + // before a relative clause, or ","/eof otherwise. Bind the + // post-space slice exactly ONCE and use it for BOTH the + // predicate hook and the fail-closed guard, so the two can never + // disagree about where the clause starts. A guard aimed at the + // raw remainder could never fire, because `tag("who ")` cannot + // match a leading " who ". + let after_noun = tag::<_, _, OracleError<'_>>(" ") + .parse(*rest_after_noun) + .map_or(*rest_after_noun, |(after, _)| after); + // CR 102.3: derive the candidate relation from the BASE noun so + // the base-scope and predicate axes compose. "a player" imports + // no opponent relation the printed text does not state — the + // predicate is evaluated against the TRIGGER CONTROLLER, which + // on a player-subject attack trigger need not be the attacker. + let relation = if attacks_one_of_your_opponents { + PlayerRelation::Opponent + } else { + PlayerRelation::All + }; + // CR 603.2 consume-on-success, enforced on the REMAINDER: an arm + // that matches only a PREFIX of the relative clause ("…more life + // than you" inside "…more life than you and controls a Forest") + // must not bind an under-restricted `valid_target` and drop the + // rest. The clause counts as modelled only when what follows it + // is a real clause boundary, checked with the shared + // `peek_clause_terminator` authority; anything else falls into + // the SAME declined branch as a total parse failure. + let modelled = parse_attacked_player_relative_clause(after_noun, relation, ctx) + .ok() + .filter(|(remainder, _)| { + nom_primitives::peek_clause_terminator(remainder).is_ok() + }); + match modelled { + Some((_, player)) => { + def.valid_target = Some(TargetFilter::PlayerMatching { + player: Box::new(player), + }); + } + None => { + // A `who`-headed clause the predicate grammar cannot + // model (or can model only partially) must NOT be + // silently discarded, leaving the broad `Player` scope + // behind — that is exactly the bug this change fixes. + // Detect it with a zero-consumption `peek`, never + // `starts_with`. The trailing space inside the tag IS the + // word boundary, so "whoever"/"whose" cannot match. + declined_unmodelled_predicate = peek(tag::<_, _, OracleError<'_>>("who ")) + .parse(after_noun) + .is_ok(); + } + } + } + } + if declined_unmodelled_predicate { + return None; + } + if def.valid_target.is_some() { + // The predicate branch already bound the attacked-player scope; + // the coarser fallbacks below must not clobber it. + } else if attacks_one_of_your_opponents { def.valid_target = Some(TargetFilter::Typed( TypedFilter::default().controller(ControllerRef::Opponent), )); diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 4847a14e70..7446188e5c 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -5527,6 +5527,48 @@ pub enum TargetFilter { PlayerWhoChoseLabel { label: String, }, + /// CR 102.1 + CR 102.2 / CR 102.3 + CR 109.5: the player(s) satisfying an + /// arbitrary [`PlayerFilter`] predicate. CR 102.1 supplies the population + /// (the people in the game) this selects from, CR 102.2 / CR 102.3 the + /// `relation` axis's opponent semantics, and CR 109.5 the controller-relative + /// "you" every relation is measured against. (Deliberately NOT CR 109.4: + /// that rule governs an OBJECT's controller, which is the object-axis + /// mirror's business, not this variant's.) + /// + /// The PLAYER-axis mirror of [`FilterProp::ControllerMatches`], which is + /// itself documented as the object-axis mirror of `PlayerFilter`; this + /// variant completes the pair. + /// + /// Evaluated by the single-authority `game::effects::matches_player_scope`, + /// so every predicate `PlayerFilter` already expresses — life total + /// (CR 119.1), hand size (CR 402.1), controlled-permanent counts (CR 109.4), + /// counters (CR 122.1), attack history (CR 508.6) — becomes usable anywhere + /// a `TargetFilter` names a player, with no further variants. + /// + /// First consumer: `TriggerDefinition::valid_target` on a CR 508.1a attack + /// trigger whose attacked player carries a relative-clause predicate + /// ("attacks a player who has more life than you"). Per CR 603.2 that clause + /// is part of the TRIGGER EVENT and is checked once at declaration — which + /// is exactly `valid_target`'s contract + /// (`trigger_matchers::attack_target_matches`) and exactly why it is NOT + /// modelled as a `TriggerCondition`, which CR 603.4 re-checks at resolution. + /// + /// The `PlayerFilter`'s `relation` is evaluated against the TRIGGER SOURCE's + /// CONTROLLER, which is not always the attacking player — a player-subject + /// attack trigger routes the attacker into `valid_source` instead. Producers + /// must not assume the two coincide. + /// + /// `Box` breaks the `TargetFilter -> PlayerFilter -> ControlsCount { filter: + /// TargetFilter }` size cycle (same rationale as + /// `FilterProp::ControllerMatches` and `PlayerFilter::AllExcept`). + /// + /// FOLLOW-UP (not this change): [`TargetFilter::PlayerWhoChoseLabel`] is the + /// hard-coded single-predicate sibling this variant supersedes. Retiring it + /// needs a `PlayerFilter::ChoseLabel { label }` backed by the existing single + /// authority `game::players::player_last_chose_label`. + PlayerMatching { + player: Box, + }, /// CR 102.1 + CR 103.1: living player seated immediately to controller's /// left/right; clockwise turn order, right = previous seat; resolved /// against `state.seat_order`. The recipient is computed at the resolver @@ -15863,6 +15905,15 @@ impl TargetFilter { controller: Some(_), properties, }) => type_filters.is_empty() && properties.is_empty(), + // CR 102.1: PlayerMatching denotes a PLAYER population by + // construction — its payload is a `PlayerFilter`, evaluated only on + // the player axis. Decided, not defaulted: this method gates the + // player-subject attack branch in + // `trigger_matchers::matching_attack_events` and the attack anaphor + // rebind gate in `oracle_trigger`, and `false` here would + // misclassify a future "Whenever a player who … attacks" + // `valid_source` as an OBJECT filter, with no compile error. + TargetFilter::PlayerMatching { .. } => true, _ => false, } } @@ -27209,6 +27260,72 @@ mod tests { use crate::types::mana::ZoneSpendPolarity; use crate::types::zones::Zone; + /// CR 102.1 — `TargetFilter::PlayerMatching::is_player_scope()` is a DECIDED + /// arm, not a wildcard default. + /// + /// Adding the variant produced no compile error here (the match ends in + /// `_ => false`), so this pin is the only thing that records the decision. + /// `is_player_scope` gates the player-subject attack branch in + /// `trigger_matchers::matching_attack_events` and the attack-anaphor rebind + /// gate in `oracle_trigger`; classifying a player predicate as an OBJECT + /// filter there would silently mis-route a future "Whenever a player who … + /// attacks" `valid_source`. + #[test] + fn player_matching_is_classified_as_a_player_scope() { + let filter = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::All, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GT, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Controller, + }, + }), + }), + }; + assert!( + filter.is_player_scope(), + "a PlayerFilter payload denotes a PLAYER population by construction" + ); + // Contrast: a genuine object filter must stay object-scoped, so the + // assertion above is about PlayerMatching and not about the method + // answering `true` for everything. + assert!(!TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + ..Default::default() + }) + .is_player_scope()); + } + + /// Serialization pin (no CR governs wire format): the serialized shape is + /// purely ADDITIVE under the existing `#[serde(tag = "type")]`, so no + /// existing `card-data.json` row changes and no migration is needed. + /// Round-trips through the boxed payload. + #[test] + fn player_matching_round_trips_through_serde() { + let filter = TargetFilter::PlayerMatching { + player: Box::new(PlayerFilter::ControlsCount { + relation: PlayerRelation::All, + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Land], + ..Default::default() + }), + comparator: Comparator::GE, + count: Box::new(QuantityExpr::Fixed { value: 8 }), + }), + }; + let json = serde_json::to_string(&filter).expect("serialize PlayerMatching"); + assert!( + json.contains("\"type\":\"PlayerMatching\""), + "additive tagged variant, got {json}" + ); + let back: TargetFilter = serde_json::from_str(&json).expect("deserialize PlayerMatching"); + assert_eq!(back, filter); + } + /// Row 14, degenerate `AnyOf` cases. CR 109.2: a 0- or 1-member union is not /// a union, and an EMPTY one is actively unsound — `characteristic_source_read` /// would fold it to `RwProfile::empty()`, which is FAIL-OPEN for the CR 603.3b diff --git a/crates/engine/src/types/events.rs b/crates/engine/src/types/events.rs index d8d384335f..80643fe704 100644 --- a/crates/engine/src/types/events.rs +++ b/crates/engine/src/types/events.rs @@ -510,6 +510,7 @@ impl EventObjectSnapshot { | TargetFilter::ScopedPlayer | TargetFilter::SpecificPlayer { .. } | TargetFilter::PlayerWhoChoseLabel { .. } + | TargetFilter::PlayerMatching { .. } | TargetFilter::Neighbor { .. } | TargetFilter::DefendingPlayer | TargetFilter::SourceChosenPlayer diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index a83401d7c3..1e2f8aca48 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -870,6 +870,7 @@ mod mycoloth_upkeep_trigger; mod myrkul_crew_phase1_incarnation; mod mystic_forge_regression; mod named_choice_free_entry_contract; +mod namor_attacking_that_player; mod narci_fable_singer_final_chapter_drain; mod narset_jeskai_waymaster_draw_spells_cast; mod natural_balance; diff --git a/crates/engine/tests/integration/namor_attacking_that_player.rs b/crates/engine/tests/integration/namor_attacking_that_player.rs new file mode 100644 index 0000000000..b60e655acc --- /dev/null +++ b/crates/engine/tests/integration/namor_attacking_that_player.rs @@ -0,0 +1,574 @@ +//! Namor, Atlantean King — the attacked-player predicate and the +//! "attacking that player" defending-player anaphor. +//! +//! Verbatim Oracle text (Scryfall oracle id +//! `171a0a09-4aee-466c-aebd-3b0d4c1f51b4`): +//! +//! > Whenever Namor attacks a player who has more life than you, other +//! > creatures you control attacking that player get +2/+0 until end of turn. +//! +//! Two independent defects shipped in the same line, and each has its own +//! discriminating rows here: +//! +//! **Defect A — the event predicate was dropped.** `parse_attack_target` +//! discarded its remainder, so "who has more life than you" vanished +//! (`valid_target: null`) and the trigger fired on EVERY player attack. Rows +//! `fires_when_.._more_life` / `does_not_fire_when_..` / `..equal_life..` +//! discriminate it. +//! +//! **Defect B — the pump was board-wide.** With "attacking that player" +//! unconsumed by `parse_type_phrase`, the clause fell through to the numeric +//! imperative path, which emits the documented +//! `Effect::Pump { target: TargetFilter::Any }` sentinel. `TargetFilter::Any` +//! matches unconditionally, so +2/+0 landed on every permanent on the +//! battlefield — both players' creatures, Namor itself, and lands. Row +//! `pumps_only_co_attackers_of_the_same_defender` discriminates it three ways +//! at once. +//! +//! A third row set covers the SIBLING class this change unlocks: the `YouAttack` +//! cards that put the same anaphor in a TARGET filter (Ordruun Mentor, Echoing +//! Assault). Their slot-build door does not carry `trigger_source`, so before +//! `ability_utils::filter_needs_trigger_source` they enumerated ZERO legal +//! targets on any multi-attacker declaration and CR 603.3d removed the trigger +//! from the stack. Those rows deliberately use a TWO-attacker board, because a +//! single-attacker board passes without the fix. +//! +//! CR references: +//! - CR 508.5 / CR 508.5a: an ability of an attacking creature that refers to +//! a defending player means the player THAT creature is attacking, and in +//! multiplayer that player is determined individually per attacker. +//! - CR 603.2: a trigger event (including a relative clause inside it) is +//! checked once, when the event occurs. +//! - CR 603.3d: a triggered ability with no legal choice for a required +//! target is removed from the stack. +//! - CR 611.2c: a continuous effect from a resolved ability fixes its +//! affected set when it begins. +//! - CR 119.1: life totals. + +use engine::game::combat::AttackerInfo; +use engine::game::layers::evaluate_layers; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::{Effect, TargetFilter, TargetRef}; +use engine::types::card_type::CoreType; +use engine::types::game_state::{StackEntryKind, WaitingFor}; +use engine::types::identifiers::ObjectId; +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 NAMOR_ORACLE: &str = "Flying\n\ + Whenever you cast a noncreature spell, create a 1/1 blue Merfolk creature token.\n\ + Whenever Namor attacks a player who has more life than you, other creatures you control attacking that player get +2/+0 until end of turn."; + +struct Board { + runner: GameRunner, + namor: ObjectId, + /// A co-attacker P0 sends at the SAME defender as Namor. + ally: ObjectId, + /// A co-attacker P0 sends at the OTHER defender, in the same declaration. + other_lane: ObjectId, +} + +/// Three-player board. `p1_life` / `p2_life` set the two potential defenders' +/// life totals; P0 (Namor's controller) is always at 20. +fn board(p1_life: i32, p2_life: i32) -> Board { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(engine::types::phase::Phase::PreCombatMain); + scenario.with_life(P0, 20); + scenario.with_life(P1, p1_life); + scenario.with_life(P2, p2_life); + + let namor = { + let mut builder = scenario.add_creature(P0, "Namor, Atlantean King", 2, 2); + builder.from_oracle_text(NAMOR_ORACLE); + builder.id() + }; + let ally = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + let other_lane = scenario.add_creature(P0, "Runeclaw Bear", 2, 2).id(); + + let mut runner = scenario.build(); + evaluate_layers(runner.state_mut()); + + Board { + runner, + namor, + ally, + other_lane, + } +} + +fn power_toughness(runner: &GameRunner, id: ObjectId) -> (Option, Option) { + let object = runner.state().objects.get(&id).expect("object must exist"); + (object.power, object.toughness) +} + +// --------------------------------------------------------------------------- +// Defect B — the pump's affected set. +// --------------------------------------------------------------------------- + +/// **The row that proves Defect B is fixed.** CR 508.5 + CR 508.5a: "other +/// creatures you control attacking THAT PLAYER" is scoped to the defender NAMOR +/// is attacking, individually determined per attacker. +/// +/// Revert-failing three independent ways: +/// 1. Drop `parse_attacking_defender_anaphor` and the clause lowers to the +/// `Pump { target: TargetFilter::Any }` sentinel — `other_lane` (and +/// Namor, and every other permanent) gets +2/+0. +/// 2. Emit `Attacking { defender: None }` instead of +/// `Some(DefendingPlayer)` and `other_lane` — attacking the OTHER +/// defender in the same declaration — is pumped. +/// 3. Drop `FilterProp::Another` and Namor pumps itself. +/// +/// The two-defender split is the multi-authority fixture: it proves the anaphor +/// binds the SOURCE's defender (CR 508.5a, per-attacker) rather than a batch +/// global, because both lanes live in one `AttackersDeclared` event. +#[test] +fn namor_pumps_only_co_attackers_of_the_same_defender_cr_508_5a() { + // P1 at 30 > P0 at 20, so the trigger's predicate is satisfied for P1. + let Board { + mut runner, + namor, + ally, + other_lane, + } = board(30, 30); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (namor, AttackTarget::Player(P1)), + (ally, AttackTarget::Player(P1)), + (other_lane, AttackTarget::Player(P2)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, ally), + (Some(4), Some(2)), + "the co-attacker on Namor's defender must get +2/+0" + ); + assert_eq!( + power_toughness(&runner, other_lane), + (Some(2), Some(2)), + "a creature attacking the OTHER defender in the same declaration must \ + NOT be pumped — that is the CR 508.5a per-attacker binding, and the \ + assertion that fails if the anaphor degrades to `defender: None`" + ); + assert_eq!( + power_toughness(&runner, namor), + (Some(2), Some(2)), + "\"OTHER creatures you control\" excludes Namor itself" + ); +} + +/// CR 611.2c: the affected set is fixed when the continuous effect begins, so a +/// creature that joins the battlefield after the trigger resolves is not pumped +/// even though it matches the filter's text. +/// +/// The latecomer is built to MATCH the pump filter in full — `CoreType::Creature` +/// for the `type_filters`, controlled by P0, registered as an attacker against +/// the SAME defending player for `Attacking { defender: DefendingPlayer }`, and +/// distinct from Namor for `FilterProp::Another`. That is what makes the row +/// discriminating: a latecomer that failed the filter anyway (no card types, not +/// in `combat.attackers`) would assert the same `(2, 2)` under LIVE +/// re-evaluation, proving nothing about the snapshot. +/// +/// Revert-failing: change `pump::resolve_all` to register one transient +/// continuous effect over the FILTER instead of one `SpecificObject { id }` per +/// matched object, and the latecomer is pumped to 4/2. +/// +/// Reach-guarded twice: the same board provably DOES pump a matching +/// co-attacker, and the latecomer is proved to satisfy the filter live. +#[test] +fn namor_pump_is_a_resolution_snapshot_cr_611_2c() { + let Board { + mut runner, + namor, + ally, + .. + } = board(30, 30); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (namor, AttackTarget::Player(P1)), + (ally, AttackTarget::Player(P1)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + // Positive reach-guard first. + assert_eq!( + power_toughness(&runner, ally), + (Some(4), Some(2)), + "reach-guard: the trigger really did resolve and pump" + ); + + let latecomer = engine::game::zones::create_object( + runner.state_mut(), + engine::types::identifiers::CardId(9_001), + P0, + "Latecomer".to_string(), + engine::types::zones::Zone::Battlefield, + ); + { + let object = runner + .state_mut() + .objects + .get_mut(&latecomer) + .expect("latecomer must exist"); + // `create_object` leaves `card_types` EMPTY, so without this the object + // fails the pump filter's `type_filters: [Creature]` and the row proves + // nothing. + object.card_types.core_types.push(CoreType::Creature); + object.power = Some(2); + object.toughness = Some(2); + object.base_power = Some(2); + object.base_toughness = Some(2); + } + // CR 508.1a: register it as an attacker against the SAME defender Namor is + // attacking, so `FilterProp::Attacking { defender: DefendingPlayer }` is + // satisfied for it too. + runner + .state_mut() + .combat + .as_mut() + .expect("combat is live") + .attackers + .push(AttackerInfo::attacking_player(latecomer, P1)); + evaluate_layers(runner.state_mut()); + + // Reach-guard the negative: the latecomer really does satisfy the pump's + // filter as of NOW. If it did not, the assertion below would hold under live + // re-evaluation too and would not discriminate CR 611.2c at all. + assert!( + engine::game::filter::matches_target_filter( + runner.state(), + latecomer, + &namor_pump_filter(&runner, namor), + &engine::game::filter::FilterContext::from_source(runner.state(), namor), + ), + "reach guard: the latecomer must MATCH the pump filter live — otherwise \ + this row cannot discriminate a snapshot from a live rescan" + ); + + assert_eq!( + power_toughness(&runner, latecomer), + (Some(2), Some(2)), + "CR 611.2c: a creature that entered after resolution is outside the \ + snapshot and must not be pumped" + ); +} + +/// The `PumpAll` filter Namor's attack trigger actually resolved, read off the +/// parsed card rather than re-typed here — a hand-written copy could drift from +/// the parser and silently make the reach-guard above assert the wrong thing. +fn namor_pump_filter(runner: &GameRunner, namor: ObjectId) -> TargetFilter { + let object = runner + .state() + .objects + .get(&namor) + .expect("Namor must be on the battlefield"); + object + .trigger_definitions + .iter_unchecked() + .filter_map(|trigger| trigger.definition().execute.as_deref()) + .find_map(|definition| match &*definition.effect { + Effect::PumpAll { target, .. } => Some(target.clone()), + _ => None, + }) + .expect("Namor's attack trigger lowers to a PumpAll") +} + +// --------------------------------------------------------------------------- +// Defect A — the event predicate. +// --------------------------------------------------------------------------- + +/// CR 603.2 + CR 119.1: the trigger fires when the attacked player has MORE +/// life than the trigger's controller. +#[test] +fn namor_fires_when_the_attacked_player_has_more_life() { + // P1 at 30, P0 at 20. + let Board { + mut runner, + namor, + ally, + .. + } = board(30, 5); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (namor, AttackTarget::Player(P1)), + (ally, AttackTarget::Player(P1)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, ally), + (Some(4), Some(2)), + "30 > 20 satisfies the predicate, so the trigger must fire and pump" + ); +} + +/// **The row that proves Defect A is fixed.** CR 603.2: the trigger must NOT +/// fire when the attacked player does not have more life. +/// +/// Revert-failing: with `valid_target: None` (the shipped bug) the trigger has +/// no defender predicate at all and fires on every player attack, so `ally` +/// would be pumped here. `player_matches_filter`'s `_ => true` tail is +/// fail-OPEN, so this row also catches a `PlayerMatching` arm that was added to +/// the type but never to the matcher. +/// +/// Paired positive reach-guard: `namor_fires_when_the_attacked_player_has_more_life` +/// uses the same board shape and the same attack lane, differing only in the +/// defender's life total — so this negative cannot pass vacuously via a parse +/// failure or an unreachable combat driver. +#[test] +fn namor_does_not_fire_when_the_attacked_player_has_less_life_cr_603_2() { + // P2 at 5 < P0 at 20. + let Board { + mut runner, + namor, + ally, + .. + } = board(30, 5); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (namor, AttackTarget::Player(P2)), + (ally, AttackTarget::Player(P2)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, ally), + (Some(2), Some(2)), + "5 <= 20 fails the predicate, so nothing may be pumped" + ); + assert_eq!( + power_toughness(&runner, namor), + (Some(2), Some(2)), + "and Namor itself is never pumped either" + ); +} + +/// The comparator boundary: "MORE life than you" is strictly greater +/// (`Comparator::GT`), so an attacked player at exactly the controller's life +/// total must not fire the trigger. Discriminates a `GE` mis-lowering, which +/// every other row in this file would pass. +#[test] +fn namor_does_not_fire_on_equal_life_totals_gt_not_ge() { + // P1 at exactly 20 == P0 at 20. + let Board { + mut runner, + namor, + ally, + .. + } = board(20, 5); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (namor, AttackTarget::Player(P1)), + (ally, AttackTarget::Player(P1)), + ]) + .expect("DeclareAttackers should succeed"); + runner.advance_until_stack_empty(); + evaluate_layers(runner.state_mut()); + + assert_eq!( + power_toughness(&runner, ally), + (Some(2), Some(2)), + "equal life is not MORE life — GT, not GE" + ); +} + +// --------------------------------------------------------------------------- +// The sibling TARGETED class — `ability_utils::filter_needs_trigger_source`. +// --------------------------------------------------------------------------- + +/// **The row that proves the enumeration-door fix.** CR 603.3d: before +/// `filter_needs_trigger_source`, the slot-build door +/// (`targeting::find_legal_targets`) built a `FilterContext` with +/// `trigger_source: None`, so `combat::defending_player_cr508_5` fell through to +/// its live-combat tail, whose `AttackersDeclared` arm is gated on +/// `attacker_ids.len() == 1`. On this TWO-attacker board that yields `None`, +/// every candidate fails `attacking_defender_matches`, the slot is EMPTY, and +/// CR 603.3d removes the trigger from the stack — silently. +/// +/// The board is deliberately multi-attacker: a single-attacker board passes +/// without the fix and would be a false green. +/// +/// Revert-failing: remove the `filter_needs_trigger_source` disjunct from +/// `target_filter_needs_ability_context` and the `assert_eq!` below sees an +/// empty offered set (or no prompt at all). +#[test] +fn ordruun_mentor_offers_exactly_the_attackers_of_the_attacked_player_cr_603_3d() { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(engine::types::phase::Phase::PreCombatMain); + + // Verbatim Oracle text (MTGJSON), Mentor reminder included. + let mentor = { + let mut builder = scenario.add_creature(P0, "Ordruun Mentor", 3, 2); + builder.from_oracle_text( + "Mentor (Whenever this creature attacks, put a +1/+1 counter on target attacking creature with lesser power.)\n\ + Whenever you attack a player, target creature that's attacking that player gains first strike until end of turn.", + ); + builder.id() + }; + let attacker_a = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + let attacker_b = scenario.add_creature(P0, "Runeclaw Bear", 2, 2).id(); + // Controlled, but NOT attacking — must never be offered. + let bystander = scenario.add_creature(P0, "Alpha Myr", 2, 1).id(); + + let mut runner = scenario.build(); + evaluate_layers(runner.state_mut()); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (attacker_a, AttackTarget::Player(P1)), + (attacker_b, AttackTarget::Player(P1)), + ]) + .expect("DeclareAttackers should succeed"); + + let offered = first_trigger_target_slot(&runner); + + assert!( + !offered.is_empty(), + "CR 603.3d: the slot must not be empty — an empty set silently removes \ + the trigger from the stack, which is the exact pre-fix failure" + ); + let mut got: Vec = offered + .iter() + .filter_map(|t| match t { + TargetRef::Object(id) => Some(*id), + _ => None, + }) + .collect(); + got.sort(); + let mut want = vec![attacker_a, attacker_b]; + want.sort(); + assert_eq!( + got, want, + "exactly the two creatures attacking P1 may be offered" + ); + assert!( + !got.contains(&bystander), + "a non-attacking creature must never be offered" + ); + assert!( + !got.contains(&mentor), + "Ordruun Mentor is not attacking, so it is not a legal target either" + ); +} + +/// TRIPWIRE — the two-defender measurement the plan refused to guess. +/// +/// With `trigger_source` now bound, `defending_player_cr508_5` finds no attack +/// entry for a NON-attacking Ordruun Mentor and declines its sole-attacker tier +/// on a two-attacker batch, so it falls to the batch-global defender. This row +/// records what the engine actually offers rather than asserting a hoped-for +/// answer — but it may NEVER accept an empty set, because empty means the +/// enumeration-door fix did not take effect. +/// +/// A future change that fires `YouAttack` once per attacked player (carrying a +/// per-firing defender) must update this row deliberately. +#[test] +fn ordruun_mentor_two_defender_board_offers_a_non_empty_measured_set() { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(engine::types::phase::Phase::PreCombatMain); + { + let mut builder = scenario.add_creature(P0, "Ordruun Mentor", 3, 2); + builder.from_oracle_text( + "Mentor (Whenever this creature attacks, put a +1/+1 counter on target attacking creature with lesser power.)\n\ + Whenever you attack a player, target creature that's attacking that player gains first strike until end of turn.", + ); + builder.id() + }; + let lane_p1 = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + let lane_p2 = scenario.add_creature(P0, "Runeclaw Bear", 2, 2).id(); + + let mut runner = scenario.build(); + evaluate_layers(runner.state_mut()); + + runner.advance_to_combat(); + runner + .declare_attackers(&[ + (lane_p1, AttackTarget::Player(P1)), + (lane_p2, AttackTarget::Player(P2)), + ]) + .expect("DeclareAttackers should succeed"); + + let offered = first_trigger_target_slot(&runner); + assert!( + !offered.is_empty(), + "an empty set here means `filter_needs_trigger_source` did not take \ + effect — CR 603.3d would silently remove the trigger. offered={offered:?}" + ); + // TRIPWIRE: coarse batch-global binding on a multi-defender `YouAttack` + // board. Both lanes being offered is the MEASURED behaviour, not an + // endorsement; narrowing it requires per-attacked-player trigger + // cardinality, which changes how many times the ability fires. + let got: Vec = offered + .iter() + .filter_map(|t| match t { + TargetRef::Object(id) => Some(*id), + _ => None, + }) + .collect(); + assert!( + got.contains(&lane_p1) || got.contains(&lane_p2), + "at least one declared attacker must be offered; got {got:?}" + ); +} + +/// The set of objects the trigger's target slot actually admitted. +/// +/// When the slot has two or more legal targets the engine prompts, so the set +/// is read straight off `WaitingFor::TriggerTargetSelection`. When exactly one +/// legal target exists the engine binds it without prompting, so the same +/// information is read off the pinned `ResolvedAbility.targets` of the trigger +/// sitting on the stack. Both branches answer the one question these rows ask — +/// *what did slot-build enumerate?* — and neither may be empty: an empty +/// enumeration is the CR 603.3d removal this change exists to prevent. +fn first_trigger_target_slot(runner: &GameRunner) -> Vec { + if let WaitingFor::TriggerTargetSelection { + target_slots, + selection, + .. + } = &runner.state().waiting_for + { + return target_slots[selection.current_slot].legal_targets.to_vec(); + } + let pinned: Vec = runner + .state() + .stack + .iter() + .filter_map(|entry| match &entry.kind { + StackEntryKind::TriggeredAbility { ability, .. } => Some(ability.targets.clone()), + _ => None, + }) + .flatten() + .collect(); + assert!( + !pinned.is_empty(), + "no target prompt and no pinned trigger target — the slot enumerated \ + nothing and CR 603.3d removed the ability. waiting_for={:?} stack={:?}", + runner.state().waiting_for, + runner.stack_names() + ); + pinned +} diff --git a/crates/mtgish-import/src/convert/condition.rs b/crates/mtgish-import/src/convert/condition.rs index f5159abc67..5a7899b58f 100644 --- a/crates/mtgish-import/src/convert/condition.rs +++ b/crates/mtgish-import/src/convert/condition.rs @@ -1112,6 +1112,7 @@ fn target_filter_variant_name(f: &TargetFilter) -> &'static str { TargetFilter::SourceChosenPlayer => "SourceChosenPlayer", TargetFilter::EventTarget => "EventTarget", TargetFilter::PlayerWhoChoseLabel { .. } => "PlayerWhoChoseLabel", + TargetFilter::PlayerMatching { .. } => "PlayerMatching", } } diff --git a/crates/phase-ai/src/policies/effect_classify.rs b/crates/phase-ai/src/policies/effect_classify.rs index 6141d304f5..e84ed4327b 100644 --- a/crates/phase-ai/src/policies/effect_classify.rs +++ b/crates/phase-ai/src/policies/effect_classify.rs @@ -671,13 +671,20 @@ pub(crate) fn aggregate_player_impact_in(effects: &[&Effect]) -> f64 { } pub(crate) fn targeted_player_impact(ctx: &PolicyContext<'_>, player: PlayerId) -> Option { - let source_controller = ctx.source_object().map(|object| object.controller); - targeted_player_impact_in(ctx.state, source_controller, &ctx.effects(), player) + let source = ctx.source_object(); + targeted_player_impact_in( + ctx.state, + source.map(|object| object.controller), + source.map(|object| object.id), + &ctx.effects(), + player, + ) } pub(crate) fn targeted_player_impact_in( state: &GameState, source_controller: Option, + source_id: Option, effects: &[&Effect], player: PlayerId, ) -> Option { @@ -693,6 +700,7 @@ pub(crate) fn targeted_player_impact_in( filter, player, source_controller, + source_id, ) { found_targeted_effect = true; impact += player_impact(effect); diff --git a/crates/phase-ai/src/policies/hand_disruption.rs b/crates/phase-ai/src/policies/hand_disruption.rs index 6681c0c7a3..c7bf77cbcf 100644 --- a/crates/phase-ai/src/policies/hand_disruption.rs +++ b/crates/phase-ai/src/policies/hand_disruption.rs @@ -6,6 +6,7 @@ use engine::types::ability::{Effect, TargetFilter, TargetRef}; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::game_state::GameState; +use engine::types::identifiers::ObjectId; use engine::types::player::PlayerId; use crate::cast_facts::CastFacts; @@ -89,8 +90,15 @@ impl TacticalPolicy for HandDisruptionPolicy { fn score_reveal_hand_player_target(ctx: &PolicyContext<'_>, target_player: PlayerId) -> f64 { let effects = ctx.effects(); + let source_id = ctx.source_object().map(|object| object.id); if !effects.iter().any(|effect| { - reveal_hand_matches_chosen_player_target(ctx.state, effect, target_player, ctx.ai_player) + reveal_hand_matches_chosen_player_target( + ctx.state, + effect, + target_player, + ctx.ai_player, + source_id, + ) }) { return 0.0; } @@ -123,11 +131,18 @@ fn reveal_hand_matches_chosen_player_target( effect: &Effect, target_player: PlayerId, source_controller: PlayerId, + source_id: Option, ) -> bool { let Effect::RevealHand { target, .. } = effect else { return false; }; - player_matches_target_filter_in_state(state, target, target_player, Some(source_controller)) + player_matches_target_filter_in_state( + state, + target, + target_player, + Some(source_controller), + source_id, + ) } pub(crate) fn disruption_window_score( @@ -702,13 +717,15 @@ mod tests { &state, &opponent_reveal, PlayerId(1), - PlayerId(0) + PlayerId(0), + None )); assert!(!reveal_hand_matches_chosen_player_target( &state, &opponent_reveal, PlayerId(0), - PlayerId(0) + PlayerId(0), + None )); let creature_reveal = Effect::RevealHand { @@ -723,7 +740,8 @@ mod tests { &state, &creature_reveal, PlayerId(1), - PlayerId(0) + PlayerId(0), + None )); } } diff --git a/crates/phase-ai/src/policies/self_cost.rs b/crates/phase-ai/src/policies/self_cost.rs index 0567b104a6..571e73bcb7 100644 --- a/crates/phase-ai/src/policies/self_cost.rs +++ b/crates/phase-ai/src/policies/self_cost.rs @@ -640,8 +640,14 @@ fn predicted_root_player_recipient( let mut opponent_accepted = false; for player in state.players.iter().filter(|player| !player.is_eliminated) { - let impact = targeted_player_impact_in(state, source_controller, effects, player.id) - .unwrap_or(aggregate); + let impact = targeted_player_impact_in( + state, + source_controller, + Some(source_id), + effects, + player.id, + ) + .unwrap_or(aggregate); let prefers_self = if impact > PLAYER_IMPACT_PREFERENCE_BAND { true } else if impact < -PLAYER_IMPACT_PREFERENCE_BAND { @@ -687,6 +693,7 @@ fn recipient_class_for_filter( filter, ai_player, source_controller, + Some(source_id), ); let mut opponent_matches = false; for player in state.players.iter().filter(|player| !player.is_eliminated) { @@ -698,6 +705,7 @@ fn recipient_class_for_filter( filter, player.id, source_controller, + Some(source_id), ) { continue; }