From 31b58c6506917590ae46b6ed753d517f24b1ed9c Mon Sep 17 00:00:00 2001 From: Jacob Woodson Date: Sun, 2 Aug 2026 08:42:31 -0500 Subject: [PATCH 1/3] Partial: Add Static forced-attack requirement parsing ("[creature] attacks [player] each combat if able unless [cond]") mechanic --- crates/engine/src/ai_support/candidates.rs | 2 +- crates/engine/src/database/encore_tests.rs | 11 +- crates/engine/src/game/combat.rs | 66 ++++-- crates/engine/src/game/effects/encore.rs | 7 +- .../engine/src/game/effects/force_attack.rs | 15 +- crates/engine/src/parser/oracle_effect/mod.rs | 2 +- .../src/parser/oracle_static/dispatch.rs | 12 + .../src/parser/oracle_static/evasion.rs | 78 +++++++ crates/engine/src/parser/oracle_static/mod.rs | 4 +- .../engine/src/parser/oracle_static/tests.rs | 106 ++++++++- crates/engine/src/types/game_state.rs | 4 +- crates/engine/src/types/statics.rs | 202 ++++++++++++++++- .../galactus_forced_attack_most_life.rs | 210 ++++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + .../must_attack_player_attribution.rs | 6 +- .../engine/tests/integration/rules/combat.rs | 8 +- crates/phase-ai/tests/scenarios.rs | 2 +- 17 files changed, 691 insertions(+), 45 deletions(-) create mode 100644 crates/engine/tests/integration/galactus_forced_attack_most_life.rs diff --git a/crates/engine/src/ai_support/candidates.rs b/crates/engine/src/ai_support/candidates.rs index 3b7281ae98..7fdcb50a9c 100644 --- a/crates/engine/src/ai_support/candidates.rs +++ b/crates/engine/src/ai_support/candidates.rs @@ -6160,7 +6160,7 @@ mod tests { .static_definitions .push(StaticDefinition::new( crate::types::statics::StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }, )); let goaded = make_creature(&mut state, 2); diff --git a/crates/engine/src/database/encore_tests.rs b/crates/engine/src/database/encore_tests.rs index 250711a935..6bbae9dac5 100644 --- a/crates/engine/src/database/encore_tests.rs +++ b/crates/engine/src/database/encore_tests.rs @@ -25,7 +25,7 @@ use crate::types::keywords::Keyword; use crate::types::mana::{ManaColor, ManaCost, ManaCostShard}; use crate::types::phase::Phase; use crate::types::player::PlayerId; -use crate::types::statics::StaticMode; +use crate::types::statics::{RequiredDefender, StaticMode}; use crate::types::zones::Zone; // --------------------------------------------------------------------------- @@ -207,7 +207,9 @@ fn encore_activation_creates_attacking_haste_copy_per_opponent() { must_attack.modifications.iter().any(|m| matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + mode: StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player }, + }, } if *player == PlayerId(1) )), "token must be required to attack the opponent" @@ -305,7 +307,10 @@ fn encore_three_player_one_token_per_opponent_then_sacrificed_at_end_step() { .find_map(|ce| { ce.modifications.iter().find_map(|m| match m { ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + mode: + StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player }, + }, } => Some(*player), _ => None, }) diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index 3c263b363e..8b38a36e28 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -19,8 +19,8 @@ use crate::types::resolved_commands::{ ResolvedCombatMembershipReplayInvariantError, }; use crate::types::statics::{ - AttackDefenderScope, BlockExceptionKind, CombatAloneAction, CombatAloneRequirement, StaticMode, - StaticModeKind, + AttackDefenderScope, BlockExceptionKind, CombatAloneAction, CombatAloneRequirement, + RequiredDefender, StaticMode, StaticModeKind, }; use crate::types::zones::Zone; @@ -3146,10 +3146,50 @@ pub(crate) fn must_attack_player_directives_for_creature( state: &GameState, obj: &GameObject, ) -> Vec<(PlayerId, Option)> { - super::functioning_abilities::active_static_definitions(state, obj) - .filter_map(|sd| match sd.mode { - StaticMode::MustAttackPlayer { player } => Some((player, sd.source_object)), - _ => None, + // CR 508.1d + CR 611.2 / CR 604.2: MustAttackPlayer directives; the required + // defender may be a resolution-time snapshot (`Fixed`, ForceAttack/Encore) or + // a live static class (`Matching`, Galactus) re-evaluated each + // declare-attackers step. Collect (defender, source_object, source_controller) + // triples first so the `active_static_definitions` borrow is dropped before we + // call `matches_player_scope`, which re-borrows `state.players`. + let directives: Vec<(RequiredDefender, Option, Option)> = + super::functioning_abilities::active_static_definitions(state, obj) + .filter_map(|sd| match &sd.mode { + StaticMode::MustAttackPlayer { player } => { + Some((player.clone(), sd.source_object, sd.source_controller)) + } + _ => None, + }) + .collect(); + directives + .into_iter() + .flat_map(|(defender, src, src_ctrl)| match defender { + // CR 611.2: a snapshotted id — used verbatim. + RequiredDefender::Fixed { player } => vec![(player, src)], + // CR 604.1 / CR 604.2 + CR 102.3: re-evaluate the class each check. + // "you"/"your opponents" resolves to the static's controller (the + // graft-time snapshot, else the carrier's controller). Yields ALL + // players in the class (e.g. every opponent tied for the most life); + // the max-requirement solver (CR 508.1d) then forces attacking one. + RequiredDefender::Matching { filter } => { + let controller = src_ctrl.unwrap_or(obj.controller); + let source_id = src.unwrap_or(obj.id); + // Deliberate O(n^2): `matches_player_scope` re-`find`s the player + // by id (game/effects/mod.rs), so passing each `p.id` re-scans the + // (tiny) player set. Reusing the canonical evaluator is worth the + // redundant lookup at 2-6 players; a batch `players_matching_scope` + // helper is the future extraction if a hot path ever appears. + state + .players + .iter() + .filter(|p| { + crate::game::effects::matches_player_scope( + state, p.id, &filter, controller, source_id, + ) + }) + .map(|p| (p.id, src)) + .collect() + } }) .collect() } @@ -12143,7 +12183,7 @@ mod tests { // so it changes no assertion; it upholds the no-`affected:None` invariant. .push( StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2), + player: PlayerId(2).into(), }) .affected(TargetFilter::SelfRef), ); @@ -12216,7 +12256,7 @@ mod tests { .static_definitions .push( StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(9000)), @@ -12236,7 +12276,7 @@ mod tests { for src in [ObjectId(9001), ObjectId(9002)] { defs.push( StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(src), @@ -12244,7 +12284,7 @@ mod tests { } defs.push( StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2), + player: PlayerId(2).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(9003)), @@ -12677,7 +12717,7 @@ mod tests { .unwrap() .static_definitions .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(2), + player: PlayerId(2).into(), })); // Attacking the wrong player (P1) while P2 is a legal target: illegal. New @@ -12786,7 +12826,7 @@ mod tests { .unwrap() .static_definitions .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), })); // New contract (CR 508.1d): the MustAttackPlayer requirement is scored by the @@ -12806,7 +12846,7 @@ mod tests { .unwrap() .static_definitions .push(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), })); let planeswalker = create_planeswalker(&mut state, PlayerId(1), "Required Player's Walker"); diff --git a/crates/engine/src/game/effects/encore.rs b/crates/engine/src/game/effects/encore.rs index a0f9e59fb1..88910fa0fb 100644 --- a/crates/engine/src/game/effects/encore.rs +++ b/crates/engine/src/game/effects/encore.rs @@ -45,7 +45,7 @@ use crate::types::events::GameEvent; use crate::types::game_state::{DelayedTrigger, GameState}; use crate::types::keywords::Keyword; use crate::types::phase::Phase; -use crate::types::statics::StaticMode; +use crate::types::statics::{RequiredDefender, StaticMode}; /// CR 702.141a: Resolve a card's Encore ability — for each opponent of the /// activating player, create a haste-bearing token copy of the exiled source @@ -94,7 +94,10 @@ pub fn resolve( Duration::UntilEndOfTurn, TargetFilter::SpecificObject { id: token_id }, vec![ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player: opponent }, + // CR 611.2: snapshot the specific opponent at resolution. + mode: StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player: opponent }, + }, }], None, ); diff --git a/crates/engine/src/game/effects/force_attack.rs b/crates/engine/src/game/effects/force_attack.rs index 145d18ede5..3936c6b95a 100644 --- a/crates/engine/src/game/effects/force_attack.rs +++ b/crates/engine/src/game/effects/force_attack.rs @@ -5,7 +5,7 @@ use crate::types::ability::{ }; use crate::types::events::GameEvent; use crate::types::game_state::GameState; -use crate::types::statics::StaticMode; +use crate::types::statics::{RequiredDefender, StaticMode}; /// CR 508.1d: Force attack — the target creature must attack the required player /// this turn/combat if able. @@ -35,7 +35,10 @@ pub fn resolve( duration.clone(), TargetFilter::SpecificObject { id: obj_id }, vec![ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + // CR 611.2: the required defender is snapshotted at resolution. + mode: StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player }, + }, }], None, ); @@ -110,7 +113,9 @@ mod tests { matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + mode: StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player }, + }, } if *player == PlayerId(0) ) })); @@ -161,7 +166,9 @@ mod tests { matches!( m, ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + mode: StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { player }, + }, } if *player == PlayerId(1) ) })); diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 44507b31a3..855d039654 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7320,7 +7320,7 @@ fn parse_for_each_object_copy_parts( /// aggregate: Max }`), composed from existing typed enums rather than a bespoke /// `MostLife` sibling. Consumes the qualifier text; returns the parsed /// restriction so the caller can attach it to `ChoiceType::Opponent`. -fn parse_opponent_most_life_restriction(input: &str) -> OracleResult<'_, PlayerFilter> { +pub(crate) fn parse_opponent_most_life_restriction(input: &str) -> OracleResult<'_, PlayerFilter> { let (input, _) = preceded( tag(" with the most life"), opt(tag(" among your opponents")), diff --git a/crates/engine/src/parser/oracle_static/dispatch.rs b/crates/engine/src/parser/oracle_static/dispatch.rs index 15f79dd190..11b6e37669 100644 --- a/crates/engine/src/parser/oracle_static/dispatch.rs +++ b/crates/engine/src/parser/oracle_static/dispatch.rs @@ -1486,6 +1486,18 @@ pub(crate) fn parse_static_line_inner( return Some(def); } + // CR 508.1d + CR 604.1: " attacks each combat if able + // [unless ...]" — a static attack requirement whose required defender is a + // live-evaluated player class (Galactus: "an opponent with the most life among + // your opponents ... unless you control a creature named ..."). Richer than the + // bare scoped must-attack below: the selector form is disjoint (returns None + // without a defender phrase), and the wrapper's flavor-label strip is a + // retry-on-failure (never fires when the body already parses), so ordering + // before `try_parse_scoped_must_attack_block` is safe. + if let Some(def) = parse_forced_attack_defender_static(&text) { + return Some(def); + } + // CR 508.1d / CR 509.1c: Subject-scoped "attack/block each combat if able" patterns. // These apply MustAttack/MustBlock to a class of creatures (not just self). // Compound forms ("attacks or blocks") produce multiple statics; return the first here. diff --git a/crates/engine/src/parser/oracle_static/evasion.rs b/crates/engine/src/parser/oracle_static/evasion.rs index e17effb34c..2db14196a6 100644 --- a/crates/engine/src/parser/oracle_static/evasion.rs +++ b/crates/engine/src/parser/oracle_static/evasion.rs @@ -2464,6 +2464,84 @@ pub(crate) fn parse_subject_combat_rule_static(text: &str) -> Option OracleResult<'_, PlayerFilter> { + let (input, _) = alt((tag::<_, _, OracleError<'_>>("an "), tag("a "))).parse(input)?; + let (input, _) = tag("opponent").parse(input)?; + // Optional "with the most life [among your opponents]" qualifier; fall back to + // the bare `Opponent` class when the qualifier is absent ("an opponent"). + match super::oracle_effect::parse_opponent_most_life_restriction(input) { + Ok((rest, filter)) => Ok((rest, filter)), + Err(_) => Ok((input, PlayerFilter::Opponent)), + } +} + +/// CR 508.1d: `attacks each combat if able` — the required-attack +/// predicate. Consumes the verb, the defender selector, and the recurring-combat +/// suffix, returning the selected `PlayerFilter` for the required defender. +fn parse_attacks_required_defender_nom(input: &str) -> OracleResult<'_, PlayerFilter> { + let (input, _) = tag::<_, _, OracleError<'_>>("attacks ").parse(input)?; + let (input, filter) = parse_required_defender_selector(input)?; + let (input, _) = alt(( + tag::<_, _, OracleError<'_>>(" each combat if able"), + tag(" each turn if able"), + )) + .parse(input)?; + Ok((input, filter)) +} + +/// CR 508.1d + CR 604.1 / CR 604.2 + CR 102.3: " attacks +/// each combat if able [unless ]" — a static attack requirement whose +/// defending player is a live-evaluated class (Galactus: "an opponent with the +/// most life among your opponents"). Emits +/// `MustAttackPlayer { RequiredDefender::Matching { filter } }`, re-evaluated each +/// declare-attackers step by the combat resolver. +/// +/// The dispatcher receives the self-ref-normalized line WITHOUT the CR 207.2c / +/// CR 207.2d ability-/flavor-word label stripped (Galactus's line arrives as +/// "Insatiable Hunger — ~ attacks …"), so this wrapper tries the line as-is, then +/// strips a leading flavor label via `strip_flavor_word_with_name` and retries +/// ONCE on the body — mirroring the single-hop retry in +/// `parse_static_line_multi_inner`. The strip is class-general (any leading +/// flavor label preceding this static form) and safe: a false-positive strip +/// yields a body that fails the strict subject / "attacks … each combat if able" +/// match and returns `None`. The full Oracle line (label included) is preserved +/// as the definition's description for display / round-trip. +pub(crate) fn parse_forced_attack_defender_static(text: &str) -> Option { + parse_forced_attack_defender_static_body(text).or_else(|| { + let (_label, body) = super::oracle_modal::strip_flavor_word_with_name(text)?; + parse_forced_attack_defender_static_body(&body).map(|def| def.description(text.to_string())) + }) +} + +fn parse_forced_attack_defender_static_body(text: &str) -> Option { + let lower = text.to_lowercase(); + let (subject_lower, filter, rest) = + nom_primitives::scan_preceded(&lower, parse_attacks_required_defender_nom)?; + let subject = text[..subject_lower.len()].trim(); + let affected = parse_rule_static_subject_filter(subject)?; + let mut def = StaticDefinition::new(StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { filter }, + }) + .affected(affected) + .description(text.to_string()); + // Consume an optional trailing period; any remaining tail MUST be a recognized + // `unless` gate (CR 604.1) — otherwise decline so an unrecognized rider cannot + // yield a half-parsed static (coverage stays honest / red). + let (rest, _) = opt(tag::<_, _, OracleError<'_>>(".")).parse(rest).ok()?; + if rest.trim().is_empty() { + return Some(def); + } + let tp = TextPair::new(text, &lower); + def.condition = Some(super::shared::parse_unless_static_condition(&tp)?); + Some(def) +} + /// CR 702.122a / 702.171a / 702.184c: nom parser for the crew/saddle/station /// power-contribution modifier predicate. Composes the named action-list prefix /// (which records the affected keyword actions) with the modifier tail. diff --git a/crates/engine/src/parser/oracle_static/mod.rs b/crates/engine/src/parser/oracle_static/mod.rs index a7572a7e40..93ed59d5ee 100644 --- a/crates/engine/src/parser/oracle_static/mod.rs +++ b/crates/engine/src/parser/oracle_static/mod.rs @@ -66,8 +66,8 @@ mod prelude { CastCostMode, CastExtraCost, CastFreeOrigin, CastFrequency, CastingProhibitionCondition, CombatAloneAction, CombatAloneRequirement, CostModifyMode, CostPaymentProhibition, CrewAction, CrewContributionKind, ExileCardPool, ExileCastCost, ExileCastTiming, - HandSizeModification, ProhibitionScope, StaticMode, SuppressedTriggerEvent, TriggerCause, - ZoneChangeQualifier, + HandSizeModification, ProhibitionScope, RequiredDefender, StaticMode, + SuppressedTriggerEvent, TriggerCause, ZoneChangeQualifier, }; pub(super) use crate::types::zones::Zone; } diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 26e43febd4..338cf2fc92 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -7,7 +7,7 @@ use super::*; use crate::types::ability::{ ActivationRestriction, AggregateFunction, CardTypeSetSource, Comparator, CountScope, DamageKindFilter, Duration, Effect, FilterProp, ObjectProperty, ObjectScope, PlayerFilter, - PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, SharedQuality, + PlayerRelation, PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, SharedQuality, SharedQualityRelation, SubtypeExclusion, TypeFilter, ZoneRef, }; use crate::types::counter::CounterType; @@ -8872,6 +8872,110 @@ fn attacks_each_combat_if_able_unconditional_has_no_condition() { assert!(def.condition.is_none()); } +/// CR 508.1d + CR 604.1 / CR 604.2 + CR 102.3: the "opponent with the most life +/// among your opponents" required-defender class — the live-evaluated filter +/// emitted by [`parse_forced_attack_defender_static`]. Mirrors +/// `parse_opponent_most_life_restriction`'s shape (the reused selector). +fn expected_most_life_defender() -> PlayerFilter { + PlayerFilter::PlayerAttribute { + relation: PlayerRelation::Opponent, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GE, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Opponent { + aggregate: AggregateFunction::Max, + }, + }, + }), + } +} + +#[test] +fn galactus_forced_attack_static_parses_with_flavor_label() { + // CR 508.1d + CR 604.1: Galactus, Devourer of Worlds — "Insatiable Hunger — + // Galactus attacks an opponent with the most life among your opponents each + // combat if able unless you control a creature named Silver Surfer, Galactus's + // Herald." The dispatcher sees the self-ref-normalized line WITH the CR 207.2d + // flavor label still attached, so the parser must strip it before resolving the + // subject. + let def = parse_static_line( + "Insatiable Hunger — ~ attacks an opponent with the most life among your opponents each combat if able unless you control a creature named Silver Surfer, Galactus's Herald.", + ) + .expect("Galactus forced-attack static must parse"); + assert_eq!( + def.mode, + StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { + filter: expected_most_life_defender(), + }, + }, + "must emit a live-evaluated Matching required defender, not a snapshot", + ); + assert_eq!(def.affected, Some(TargetFilter::SelfRef)); + // The `unless you control a creature named …` gate must be a RECOGNIZED + // control-presence condition (CR 604.1), not swallowed as `Unrecognized`. + match &def.condition { + Some(StaticCondition::Not { condition }) => assert!( + matches!(**condition, StaticCondition::IsPresent { .. }), + "expected a recognized named-control gate, got {condition:?}", + ), + other => panic!("expected Not(IsPresent) gate, got {other:?}"), + } +} + +#[test] +fn forced_attack_defender_static_flavor_label_is_optional() { + // Reach-guard for the label strip: the identical line WITHOUT the flavor label + // parses to the same static, proving the strip is a fallback, not a requirement. + let def = parse_static_line( + "~ attacks an opponent with the most life among your opponents each combat if able unless you control a creature named Silver Surfer, Galactus's Herald.", + ) + .expect("unlabeled forced-attack static must parse"); + assert_eq!( + def.mode, + StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { + filter: expected_most_life_defender(), + }, + }, + ); + assert_eq!(def.affected, Some(TargetFilter::SelfRef)); +} + +#[test] +fn forced_attack_defender_static_bare_opponent_selector() { + // Building-block coverage: the bare "an opponent" defender class (no most-life + // qualifier) lowers to `PlayerFilter::Opponent`, and the unconditional form has + // no gate. + let def = parse_static_line("~ attacks an opponent each combat if able.") + .expect("bare-opponent forced-attack static must parse"); + assert_eq!( + def.mode, + StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { + filter: PlayerFilter::Opponent, + }, + }, + ); + assert!(def.condition.is_none()); +} + +#[test] +fn flavor_labeled_non_forced_attack_line_is_not_hijacked() { + // Anti-hijack: the flavor-label strip must not manufacture a forced-attack + // static from an unrelated labeled line. `parse_forced_attack_defender_static` + // declines (no "attacks … each combat if able" predicate), leaving the line to + // the ordinary anthem/PT dispatch. + assert!( + super::evasion::parse_forced_attack_defender_static("Insatiable Hunger — ~ gets +1/+1.") + .is_none(), + "a flavor-labeled non-forced-attack line must not become a MustAttackPlayer static", + ); +} + #[test] fn static_unlicensed_hearse_counts_cards_exiled_with_it() { let def = parse_static_line( diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index c9b89bc4e0..c3d8c59c2a 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -23065,7 +23065,7 @@ mod tests { ); object.static_definitions.push( StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(800)), @@ -23080,7 +23080,7 @@ mod tests { .get_mut(&ObjectId(500)) .unwrap() .static_definitions = vec![StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }) .affected(TargetFilter::SelfRef) .source_object(ObjectId(801))] diff --git a/crates/engine/src/types/statics.rs b/crates/engine/src/types/statics.rs index 3bde7797c7..80445eba82 100644 --- a/crates/engine/src/types/statics.rs +++ b/crates/engine/src/types/statics.rs @@ -806,6 +806,88 @@ pub enum AttackDefenderScope { Controller, } +/// CR 508.1d + CR 611.2 / CR 604.2: how the required defending player of a +/// [`StaticMode::MustAttackPlayer`] requirement is determined. Struct variants +/// (NOT tuple/newtype) so internal `#[serde(tag = "type")]` tagging stays valid +/// — this mirrors [`super::ability::QuantityExpr`] (`Fixed { value }` | +/// `Ref { qty }`), which uses struct variants for the same serde reason: a +/// newtype variant wrapping a `#[serde(transparent)]` scalar (`PlayerId(u8)`) +/// or an already-`#[serde(tag)]` map (`PlayerFilter`) cannot be internally +/// tagged. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(tag = "type")] +pub enum RequiredDefender { + /// CR 611.2: the specific player determined during the resolution of the + /// spell/ability that generates the continuous effect (`Effect::ForceAttack`, + /// Encore) and stored as a literal. Not re-evaluated — contrast the static + /// form below (CR 611.2c: "this works differently than a continuous effect + /// from a static ability"). + Fixed { player: PlayerId }, + /// CR 604.1 / CR 604.2 + CR 508.1d: a player CLASS re-evaluated each + /// declare-attackers step against live game state (Galactus, "an opponent + /// with the most life among your opponents"). A static-ability continuous + /// effect is continuously applied; the requirement is re-checked each + /// declare-attackers step. Resolved via `game::effects::matches_player_scope`. + Matching { filter: PlayerFilter }, +} + +impl From for RequiredDefender { + fn from(player: PlayerId) -> Self { + Self::Fixed { player } + } +} + +/// CR 611.2 / CR 604.2: Back-compatible `Deserialize` for [`RequiredDefender`]. +/// Accepts BOTH the canonical tagged struct form (`{"type":"Fixed","player":N}` +/// / `{"type":"Matching","filter":{…}}`) and the legacy bare `PlayerId` integer +/// (`N`) that pre-`RequiredDefender` `MustAttackPlayer` snapshots stored when the +/// field was a plain `PlayerId`. Mirrors the `QuantityExpr` migration so every +/// serialized static round-trips without regenerating captured data. `Serialize` +/// stays derived (tagged) so new writes are canonical. Struct variants keep this +/// sound: `{"type":"Matching","filter":{"type":"PlayerAttribute",…}}` nests the +/// `PlayerFilter` map under `filter`, so the inner `type` never collides with the +/// outer tag — the exact failure a newtype variant would have. +impl<'de> Deserialize<'de> for RequiredDefender { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let value = serde_json::Value::deserialize(deserializer)?; + match &value { + // Legacy: a bare integer is the old concrete `PlayerId`. + serde_json::Value::Number(n) => { + let raw = n.as_u64().ok_or_else(|| { + serde::de::Error::custom("expected u8 PlayerId for RequiredDefender") + })?; + let id = u8::try_from(raw).map_err(|_| { + serde::de::Error::custom("RequiredDefender PlayerId out of u8 range") + })?; + Ok(RequiredDefender::Fixed { + player: PlayerId(id), + }) + } + // Canonical tagged form — delegate to a derived mirror. + serde_json::Value::Object(_) => { + #[derive(Deserialize)] + #[serde(tag = "type")] + enum Tagged { + Fixed { player: PlayerId }, + Matching { filter: PlayerFilter }, + } + let tagged: Tagged = + serde_json::from_value(value).map_err(serde::de::Error::custom)?; + Ok(match tagged { + Tagged::Fixed { player } => RequiredDefender::Fixed { player }, + Tagged::Matching { filter } => RequiredDefender::Matching { filter }, + }) + } + _ => Err(serde::de::Error::custom( + "expected an integer or a tagged object for RequiredDefender", + )), + } + } +} + /// All static ability modes from Forge's static ability registry. /// Matched case-sensitively against Forge mode strings. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -1145,15 +1227,22 @@ pub enum StaticMode { /// runtime-implemented; other arms are inert. PlayerProtection(super::keywords::ProtectionTarget), MustAttack, - /// CR 508.1d: This creature must attack a *specific* player if able ("target - /// creature attacks you this combat if able"; Alluring Siren, Dulcet Sirens). - /// Unlike the generic - /// [`MustAttack`] (attack any defender), this carries the `PlayerId` that must - /// be attacked. Data-carrying variant — not registry-registered (see - /// `coverage::is_data_carrying_static`); enforced by direct pattern-match in - /// `combat.rs` declare-attackers validation. Mirrors [`MustBlockAttacker`]. + /// CR 508.1d: This creature must attack a *specific* player (or a live player + /// CLASS) if able. The required defender is a [`RequiredDefender`]: + /// - `Fixed { player }` — a resolution-time snapshot id (Alluring Siren, + /// Dulcet Sirens, Encore; grafted via `Effect::ForceAttack`), CR 611.2. + /// - `Matching { filter }` — a printed static's live player class re-evaluated + /// each declare-attackers step (Galactus, "an opponent with the most life + /// among your opponents"), CR 604.1 / CR 604.2. + /// + /// Unlike the generic [`MustAttack`] (attack any defender), this narrows the + /// requirement to specific players. Data-carrying variant — not + /// registry-registered (see `coverage::is_data_carrying_static`); enforced by + /// direct pattern-match in `combat.rs` declare-attackers validation (the + /// resolver at `must_attack_player_directives_for_creature` resolves the + /// `RequiredDefender` to concrete `PlayerId`s). Mirrors [`MustBlockAttacker`]. MustAttackPlayer { - player: PlayerId, + player: RequiredDefender, }, MustBlock, /// CR 702.39a / CR 509.1c: This creature must block a *specific* attacker if @@ -2384,7 +2473,16 @@ impl Hash for StaticMode { } StaticMode::ExtraBlockers { count } => count.hash(state), StaticMode::MustBlockAttacker { attacker } => attacker.hash(state), - StaticMode::MustAttackPlayer { player } => player.hash(state), + // CR 508.1d: `RequiredDefender::Matching` wraps a non-Hash + // `PlayerFilter`; hash the discriminant for both arms and the + // concrete id only for `Fixed` (precedent: the non-Hash TargetFilter + // arms below). Equal values still hash equal. + StaticMode::MustAttackPlayer { player } => { + std::mem::discriminant(player).hash(state); + if let RequiredDefender::Fixed { player: p } = player { + p.hash(state); + } + } StaticMode::MaxAttackersEachCombat { max, defender } => { max.hash(state); defender.hash(state); @@ -4446,6 +4544,92 @@ mod tests { assert_eq!(w2.mode, StaticMode::GrantsExtraVote); } + /// CR 611.2: the grafted `Fixed` required defender serializes to the canonical + /// tagged struct form and round-trips. + #[test] + fn required_defender_fixed_round_trips_tagged() { + let d = RequiredDefender::Fixed { + player: PlayerId(2), + }; + let json = serde_json::to_string(&d).unwrap(); + assert_eq!(json, r#"{"type":"Fixed","player":2}"#); + let back: RequiredDefender = serde_json::from_str(&json).unwrap(); + assert_eq!(back, d); + } + + /// CR 604.1: the `Matching` required defender nests its `#[serde(tag="type")]` + /// `PlayerFilter` under `filter`, so the inner `type` never collides with the + /// outer tag — the exact serde failure a newtype variant would have hit. + #[test] + fn required_defender_matching_round_trips_without_tag_collision() { + let d = RequiredDefender::Matching { + filter: PlayerFilter::Opponent, + }; + let json = serde_json::to_string(&d).unwrap(); + assert_eq!(json, r#"{"type":"Matching","filter":{"type":"Opponent"}}"#); + let back: RequiredDefender = serde_json::from_str(&json).unwrap(); + assert_eq!(back, d); + } + + /// Pre-`RequiredDefender` `MustAttackPlayer` snapshots stored a bare `PlayerId` + /// integer; the custom `Deserialize` must load it as `Fixed { player }`. + #[test] + fn required_defender_deserializes_legacy_bare_player_id() { + let back: RequiredDefender = serde_json::from_str("3").unwrap(); + assert_eq!( + back, + RequiredDefender::Fixed { + player: PlayerId(3) + } + ); + } + + /// A malformed tagged object errors rather than silently defaulting. + #[test] + fn required_defender_rejects_malformed_tag() { + assert!(serde_json::from_str::(r#"{"type":"Bogus"}"#).is_err()); + } + + /// The production serde path: `MustAttackPlayer` carries a `RequiredDefender`, + /// and BOTH forms must round-trip through the derived `StaticMode` + /// (de)serialization (card-data export + game-state snapshots). + #[test] + fn must_attack_player_round_trips_through_static_mode() { + for mode in [ + StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { + player: PlayerId(1), + }, + }, + StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { + filter: PlayerFilter::Opponent, + }, + }, + ] { + let json = serde_json::to_string(&mode).unwrap(); + let back: StaticMode = serde_json::from_str(&json).unwrap(); + assert_eq!(mode, back); + } + } + + /// Legacy `MustAttackPlayer` snapshots serialized the `player` field as a bare + /// `PlayerId` integer; the derived `StaticMode` deser must materialize it as + /// `Fixed { player }` via `RequiredDefender`'s back-compat `Deserialize`. + #[test] + fn must_attack_player_deserializes_legacy_bare_player_field() { + let legacy = r#"{"MustAttackPlayer":{"player":1}}"#; + let mode: StaticMode = serde_json::from_str(legacy).unwrap(); + assert_eq!( + mode, + StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { + player: PlayerId(1) + }, + } + ); + } + /// CR 609.4b: `SpendManaAsAnyColor` widened from a unit variant to a struct /// variant carrying `spell_filter: Option` (Vizier of the /// Menagerie spell-class scoping). This pins three serde behaviors: diff --git a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs new file mode 100644 index 0000000000..60e30fd58d --- /dev/null +++ b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs @@ -0,0 +1,210 @@ +//! CR 508.1d + CR 604.1 / CR 604.2: Galactus, Devourer of Worlds — a printed +//! STATIC forced-attack requirement whose required defender is a LIVE-evaluated +//! player class ("an opponent with the most life among your opponents"), gated by +//! "unless you control a creature named Silver Surfer, Galactus's Herald". +//! +//! These drive the REAL pipeline end-to-end: verbatim Oracle text → +//! `normalize_self_refs_for_static` → parser +//! (`parse_forced_attack_defender_static`) → `MustAttackPlayer { Matching }` → +//! `must_attack_player_directives_for_creature` (the changed runtime seam, +//! re-evaluated each declare-attackers step) → `attacker_constraints_for_active_player` +//! (the DeclareAttackers waiting payload authority) AND the `declare_attackers` +//! legality validator via the `GameAction::DeclareAttackers` route. + +use engine::game::combat::{ + attacker_constraints_for_active_player, get_valid_attacker_ids, AttackTarget, CombatRequirement, +}; +use engine::game::layers::evaluate_layers; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; + +const P2: PlayerId = PlayerId(2); + +/// Galactus, Devourer of Worlds — verbatim Oracle text (Scryfall). The +/// forced-attack line carries the CR 207.2d Universes-Beyond flavor label +/// "Insatiable Hunger — " and the embedded "Galactus's" inside the herald's name. +const GALACTUS_ORACLE: &str = concat!( + "Flying, trample, indestructible\n", + "When Galactus enters, exile target permanent.\n", + "Insatiable Hunger — Galactus attacks an opponent with the most life among ", + "your opponents each combat if able unless you control a creature named ", + "Silver Surfer, Galactus's Herald.", +); + +/// Build a 3-player game with Galactus (parsed from verbatim Oracle text) on P0's +/// battlefield, opponents at the given life totals, optionally with a creature of +/// `herald` name under P0's control, parked at declare-attackers with the +/// materialized statics live. +fn parked_galactus(p1_life: i32, p2_life: i32, herald: Option<&str>) -> (GameRunner, ObjectId) { + let mut scenario = GameScenario::new_n_player(3, 42); + scenario.at_phase(Phase::DeclareAttackers); + let galactus = scenario + .add_creature_from_oracle(P0, "Galactus, Devourer of Worlds", 12, 12, GALACTUS_ORACLE) + .id(); + if let Some(name) = herald { + scenario.add_creature(P0, name, 4, 4); + } + scenario.with_life(P1, p1_life); + scenario.with_life(P2, p2_life); + let mut runner = scenario.build(); + { + let state = runner.state_mut(); + state.active_player = P0; + state.priority_player = P0; + state.phase = Phase::DeclareAttackers; + state.turn_number = 2; + state.layers_dirty.mark_full(); + } + evaluate_layers(runner.state_mut()); + let valid = get_valid_attacker_ids(runner.state()); + runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { + player: P0, + valid_attacker_ids: valid, + valid_attack_targets: vec![AttackTarget::Player(P1), AttackTarget::Player(P2)], + valid_attack_targets_by_attacker: None, + attacker_constraints: Default::default(), + }; + (runner, galactus) +} + +fn declare(runner: &mut GameRunner, galactus: ObjectId, defender: PlayerId) -> Result<(), String> { + runner + .act(GameAction::DeclareAttackers { + attacks: vec![(galactus, AttackTarget::Player(defender))], + bands: vec![], + }) + .map(|_| ()) + .map_err(|e| format!("{e:?}")) +} + +/// The changed seam surfaces through the production requirement authority: the +/// required defender resolves to the most-life opponent (CR 604.1 live class). +/// REVERT-FAIL: if the `RequiredDefender::Matching` arm of +/// `must_attack_player_directives_for_creature` returned nothing, `players` would +/// be empty and this equality fails. +#[test] +fn galactus_requirement_surfaces_most_life_opponent() { + let (runner, galactus) = parked_galactus(30, 20, None); + let valid = get_valid_attacker_ids(runner.state()); + assert!( + valid.contains(&galactus), + "reach-guard: Galactus is a valid attacker (the requirement is non-vacuous)" + ); + let constraints = attacker_constraints_for_active_player(runner.state(), &valid); + let Some(CombatRequirement::MustAttack { players, .. }) = constraints.get(&galactus) else { + panic!( + "expected a MustAttack requirement for Galactus, got {:?}", + constraints.get(&galactus) + ); + }; + assert_eq!( + players, + &vec![P1], + "the live-evaluated required defender is the single most-life opponent" + ); +} + +/// CR 508.1d enforcement: Galactus must attack the most-life opponent (P1@30) — +/// attacking the lower-life opponent (P2@20) is rejected, attacking P1 commits, +/// and declaring no attacker is rejected (it is able to attack). +#[test] +fn galactus_forced_to_attack_most_life_opponent() { + // Wrong opponent (P2, not most life) → rejected. + let (mut wrong, galactus) = parked_galactus(30, 20, None); + assert!( + declare(&mut wrong, galactus, P2).is_err(), + "attacking the lower-life opponent leaves the most-life requirement unmet" + ); + + // Correct opponent (P1, most life) → accepted and committed. + let (mut right, galactus) = parked_galactus(30, 20, None); + declare(&mut right, galactus, P1) + .expect("attacking the most-life opponent satisfies CR 508.1d"); + assert!( + right.state().combat.is_some(), + "the satisfying declaration must commit" + ); + + // Declining entirely → rejected (Galactus is able to attack). + let (mut none, _galactus) = parked_galactus(30, 20, None); + let empty = none.act(GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }); + assert!( + empty.is_err(), + "declaring no attacker leaves the requirement unmet — it still binds" + ); +} + +/// Reach-guard proving the defender is re-evaluated LIVE, not snapshotted: with the +/// life totals swapped (P2@30 now most life), Galactus is forced to attack P2, and +/// attacking P1 (now lower) is rejected. +#[test] +fn galactus_required_defender_reevaluated_live() { + let (mut wrong, galactus) = parked_galactus(20, 30, None); + assert!( + declare(&mut wrong, galactus, P1).is_err(), + "after the life swap P1 is no longer the most-life opponent" + ); + + let (mut right, galactus) = parked_galactus(20, 30, None); + declare(&mut right, galactus, P2).expect("P2 is now the most-life opponent"); + assert!(right.state().combat.is_some()); +} + +/// CR 508.1d tie: when opponents are tied for the most life, attacking EITHER +/// satisfies the requirement (the tied set both surface), but declining does not. +#[test] +fn galactus_tie_allows_either_most_life_opponent() { + let (mut p1, galactus) = parked_galactus(25, 25, None); + declare(&mut p1, galactus, P1).expect("attacking one tied most-life opponent is legal"); + assert!(p1.state().combat.is_some()); + + let (mut p2, galactus) = parked_galactus(25, 25, None); + declare(&mut p2, galactus, P2) + .expect("attacking the other tied most-life opponent is equally legal"); + assert!(p2.state().combat.is_some()); + + // Vacuity guard: the tie must still BIND — declining is rejected. + let (mut none, _galactus) = parked_galactus(25, 25, None); + let empty = none.act(GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }); + assert!( + empty.is_err(), + "a tie expands the required set — it does not drop the requirement" + ); +} + +/// CR 604.1 gate: controlling a creature named exactly "Silver Surfer, Galactus's +/// Herald" suppresses the static (its `condition` is false), so Galactus is free +/// to decline. Paired reach-guard: a creature with a DIFFERENT name does NOT +/// suppress — proving the named-control predicate, not a bare presence check. +#[test] +fn galactus_herald_suppresses_requirement() { + let (mut suppressed, _galactus) = + parked_galactus(30, 20, Some("Silver Surfer, Galactus's Herald")); + suppressed + .act(GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }) + .expect("the herald suppresses the forced-attack requirement — declining is legal"); + + // Wrong name → requirement still binds → declining is rejected. + let (mut still_forced, _galactus) = parked_galactus(30, 20, Some("Silver Surfer")); + let empty = still_forced.act(GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }); + assert!( + empty.is_err(), + "a differently-named creature must NOT suppress the requirement" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 18d62e8b0f..789ad5884a 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -209,6 +209,7 @@ mod fury_sliver_double_strike_grant; mod fuse_runtime; mod gaeas_anthem_team_pump; mod gain_control_multi_target_6205; +mod galactus_forced_attack_most_life; mod game_state_boxed_ability_serde; mod game_state_stack_budget; mod gatta_and_luzzu_regression; diff --git a/crates/engine/tests/integration/must_attack_player_attribution.rs b/crates/engine/tests/integration/must_attack_player_attribution.rs index 9f0988fa24..2273c7648f 100644 --- a/crates/engine/tests/integration/must_attack_player_attribution.rs +++ b/crates/engine/tests/integration/must_attack_player_attribution.rs @@ -47,7 +47,9 @@ fn graft_must_attack_player( Duration::UntilEndOfCombat, TargetFilter::SpecificObject { id: creature }, vec![ContinuousModification::AddStaticMode { - mode: StaticMode::MustAttackPlayer { player }, + mode: StaticMode::MustAttackPlayer { + player: player.into(), + }, }], None, ); @@ -284,7 +286,7 @@ fn static_definition_source_object_serde_default() { // A stamped value round-trips. let stamped = StaticDefinition::new(StaticMode::MustAttackPlayer { - player: PlayerId(1), + player: PlayerId(1).into(), }) .source_object(ObjectId(7)); let round: StaticDefinition = diff --git a/crates/engine/tests/integration/rules/combat.rs b/crates/engine/tests/integration/rules/combat.rs index 86cceb82f6..d349e74cb2 100644 --- a/crates/engine/tests/integration/rules/combat.rs +++ b/crates/engine/tests/integration/rules/combat.rs @@ -1789,10 +1789,10 @@ fn incompatible_must_attack_player_accepts_max_score_declaration() { let attacker = { let mut b = scenario.add_creature(P0, "Doubly Lured Bear", 2, 2); b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1, + player: P1.into(), })); b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P2, + player: P2.into(), })); b.id() }; @@ -2282,7 +2282,7 @@ fn must_attack_whose_only_target_is_taxed_does_not_force_payment() { let attacker = { let mut b = scenario.add_creature(P0, "Lured Bear", 2, 2); b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1, + player: P1.into(), })); b.id() }; @@ -2487,7 +2487,7 @@ fn shared_scaled_tax_taxes_each_attacker_independently() { let lure = |scenario: &mut GameScenario, name: &str| { let mut b = scenario.add_creature(P0, name, 2, 2); b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1, + player: P1.into(), })); b.id() }; diff --git a/crates/phase-ai/tests/scenarios.rs b/crates/phase-ai/tests/scenarios.rs index 849a271852..0b1e934801 100644 --- a/crates/phase-ai/tests/scenarios.rs +++ b/crates/phase-ai/tests/scenarios.rs @@ -1432,7 +1432,7 @@ fn ai_declare_attackers_completion_returns_apply_accepted_legal_action() { let attacker = { let mut b = scenario.add_creature(P0, "Lured Bear", 2, 2); b.with_static_definition(StaticDefinition::new(StaticMode::MustAttackPlayer { - player: P1, + player: P1.into(), })); b.id() }; From bb1dedd7f1afabc95d5e66e2254cf2a7e3dc66df Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:59:58 -0500 Subject: [PATCH 2/3] fix(engine): model Matching forced-attack directives as CR 508.1d alternative-sets Addresses the PR #6885 review (matthewevans + CodeRabbit). 1. CR 508.1d - tied defender grouping (combat.rs). A RequiredDefender::Matching directive that resolves to multiple players (e.g. every opponent tied for the most life) was flattened into per-player requirements and deduped into the shared fixed-player set, losing the alternative-set grouping: a Galactus most-life tie {P1,P2} plus a fixed-P1 requirement could wrongly permit P2. Introduce AttackRequirement::MustAttackAnyOf (obeyed by attacking ANY current member) and ResolvedRequiredDefender so each directive stays one requirement - a Matching directive is a single alternative-set, kept distinct from any coexisting Fixed requirement so the fixed requirement retains its CR 701.15c multiplicity and the max-requirement solver forces the fixed member. 2. Parser rider-swallow (oracle_static/evasion.rs). The forced-attack body parsed the unless clause from the whole line, so an unmodelled rider between the recurring-combat suffix and unless was silently dropped. Require the tail to begin exactly with an unless gate, else decline (honest unsupported result). 3. CR annotations. The static attack-requirement selector cited CR 608.2d (resolution-time choices) for tie resolution; replace with CR 508.1b (active player choosing among tied legal defenders) + CR 508.1d, and cite CR 102.2 (two-player) / CR 102.3 (team multiplayer) for opponent scoping. Tests: solver regression (matching_tie_plus_fixed_forces_the_fixed_defender) plus two brute-force-oracle cases for MustAttackAnyOf; a reach-guarded negative parser test proving an unmodelled rider is not swallowed; and the live re-evaluation integration test now mutates life totals in one fixture and drives reject-then-accept through GameAction::DeclareAttackers. Verified: cargo fmt, parser combinator Gate A, clippy (-D warnings), full phase-engine suite (4374 passed), Galactus integration suite (5 passed). Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/combat.rs | 333 +++++++++++++++--- crates/engine/src/parser/oracle_effect/mod.rs | 10 +- .../src/parser/oracle_static/evasion.rs | 39 +- .../engine/src/parser/oracle_static/tests.rs | 26 ++ .../galactus_forced_attack_most_life.rs | 33 +- 5 files changed, 372 insertions(+), 69 deletions(-) diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index 8b38a36e28..2f74089e7f 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -3121,18 +3121,61 @@ pub(crate) fn must_attack_players_for_creature( ) -> Vec { let mut players: Vec = must_attack_player_directives_for_creature(state, obj) .into_iter() - .map(|(player, _)| player) + .flat_map(|(defender, _)| defender.into_members()) .collect(); // CR 508.1d: players is a SET — a per-player requirement is obeyed by // attacking that player once (CR 508.1d counts requirements), so multiple // directives naming the same player collapse to one entry; otherwise // `score_declaration` would double-count a single requirement and bias // attack selection. Per-directing-source multiplicity lives in `sources`. + // This flat union (Fixed singletons + every `Matching` member) drives the + // "is any required player attackable" gate and the display badge; the CR + // 508.1d SOLVER keeps `Matching` directives as alternative-sets (see the + // requirement builder), which this projection deliberately flattens away. players.sort_unstable_by_key(|p| p.0); players.dedup(); players } +/// CR 508.1d + CR 604.1 / CR 611.2c: one resolved `MustAttackPlayer` directive on +/// a creature — the acceptable defending players of a SINGLE static, kept ungrouped +/// from every other directive. Mirrors [`RequiredDefender`] after live resolution: +/// `Fixed` is a resolution-time snapshot (exactly one player); `Matching` is the +/// live player class (every current member, e.g. all opponents tied for the most +/// life). Preserving the directive boundary is load-bearing for CR 508.1d: a +/// `Matching` directive is ONE alternative-set requirement (attack any member), so +/// flattening its members into a shared deduped player set would merge a tied +/// member with a coexisting `Fixed` requirement and let the max-requirement solver +/// wrongly permit a non-fixed tied member. +pub(crate) enum ResolvedRequiredDefender { + /// CR 611.2: a single snapshotted defending player. + Fixed(PlayerId), + /// CR 604.1 + CR 508.1b/d: the live class members — attacking ANY ONE obeys + /// the single requirement; the active player picks among tied legal defenders + /// (CR 508.1b). + Matching(Vec), +} + +impl ResolvedRequiredDefender { + /// The acceptable defending players (CR 508.1d): a `Fixed` singleton or the + /// live `Matching` class members. Borrows without allocating — both arms are + /// the same `slice::Iter` type. + fn members(&self) -> std::iter::Copied> { + match self { + Self::Fixed(player) => std::slice::from_ref(player).iter().copied(), + Self::Matching(players) => players.as_slice().iter().copied(), + } + } + + /// Consuming form of [`members`](Self::members) for the flat-union projection. + fn into_members(self) -> Vec { + match self { + Self::Fixed(player) => vec![player], + Self::Matching(players) => players, + } + } +} + /// CR 508.1d + CR 611.2c: the (required player, directing carrier) pairs from /// every `MustAttackPlayer` static on `obj`. `source_object` names the object /// that grafted the requirement (ForceAttack / Encore / mass-coerce source); @@ -3145,7 +3188,7 @@ pub(crate) fn must_attack_players_for_creature( pub(crate) fn must_attack_player_directives_for_creature( state: &GameState, obj: &GameObject, -) -> Vec<(PlayerId, Option)> { +) -> Vec<(ResolvedRequiredDefender, Option)> { // CR 508.1d + CR 611.2 / CR 604.2: MustAttackPlayer directives; the required // defender may be a resolution-time snapshot (`Fixed`, ForceAttack/Encore) or // a live static class (`Matching`, Galactus) re-evaluated each @@ -3163,33 +3206,40 @@ pub(crate) fn must_attack_player_directives_for_creature( .collect(); directives .into_iter() - .flat_map(|(defender, src, src_ctrl)| match defender { - // CR 611.2: a snapshotted id — used verbatim. - RequiredDefender::Fixed { player } => vec![(player, src)], - // CR 604.1 / CR 604.2 + CR 102.3: re-evaluate the class each check. - // "you"/"your opponents" resolves to the static's controller (the - // graft-time snapshot, else the carrier's controller). Yields ALL - // players in the class (e.g. every opponent tied for the most life); - // the max-requirement solver (CR 508.1d) then forces attacking one. - RequiredDefender::Matching { filter } => { - let controller = src_ctrl.unwrap_or(obj.controller); - let source_id = src.unwrap_or(obj.id); - // Deliberate O(n^2): `matches_player_scope` re-`find`s the player - // by id (game/effects/mod.rs), so passing each `p.id` re-scans the - // (tiny) player set. Reusing the canonical evaluator is worth the - // redundant lookup at 2-6 players; a batch `players_matching_scope` - // helper is the future extraction if a hot path ever appears. - state - .players - .iter() - .filter(|p| { - crate::game::effects::matches_player_scope( - state, p.id, &filter, controller, source_id, - ) - }) - .map(|p| (p.id, src)) - .collect() - } + .map(|(defender, src, src_ctrl)| { + let resolved = match defender { + // CR 611.2: a snapshotted id — used verbatim. + RequiredDefender::Fixed { player } => ResolvedRequiredDefender::Fixed(player), + // CR 604.1 / CR 604.2 + CR 102.2 / CR 102.3: re-evaluate the class + // each check. "you"/"your opponents" resolves to the static's + // controller (the graft-time snapshot, else the carrier's + // controller). Yields ALL members of the class (e.g. every opponent + // tied for the most life) as ONE alternative-set directive; the + // max-requirement solver (CR 508.1d) then forces attacking one, the + // active player choosing among tied legal defenders (CR 508.1b). + RequiredDefender::Matching { filter } => { + let controller = src_ctrl.unwrap_or(obj.controller); + let source_id = src.unwrap_or(obj.id); + // Deliberate O(n^2): `matches_player_scope` re-`find`s the + // player by id (game/effects/mod.rs), so passing each `p.id` + // re-scans the (tiny) player set. Reusing the canonical + // evaluator is worth the redundant lookup at 2-6 players; a + // batch `players_matching_scope` helper is the future extraction + // if a hot path ever appears. + let members: Vec = state + .players + .iter() + .filter(|p| { + crate::game::effects::matches_player_scope( + state, p.id, &filter, controller, source_id, + ) + }) + .map(|p| p.id) + .collect(); + ResolvedRequiredDefender::Matching(members) + } + }; + (resolved, src) }) .collect() } @@ -3588,18 +3638,38 @@ pub fn propagate_banding_block_state(combat: &mut CombatState) { /// one `MustAttackPlayer` per specific-player static), and CR 701.15c makes each /// distinct goader an additional requirement — hence a flat multiset, not a /// per-creature aggregate. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// +/// Not `Copy`: `MustAttackAnyOf` carries a `Vec` (a live player class +/// can hold more than one member), so the multiset is moved/borrowed, never +/// bit-copied. +#[derive(Debug, Clone, PartialEq, Eq)] enum AttackRequirement { /// CR 508.1d + CR 701.15b (first clause): `creature` attacks this combat if /// able. Obeyed iff `creature` attacks any legal target. MustAttackGeneric { creature: ObjectId }, /// CR 508.1b + CR 508.1d: `creature` must attack `player` directly. Obeyed /// iff `creature` attacks that player (not a planeswalker/battle they control, - /// per CR 508.5). Only emitted when `player` is currently attackable. + /// per CR 508.5). Only emitted when `player` is currently attackable. This is + /// a `RequiredDefender::Fixed` directive (a resolution-time snapshot, e.g. + /// Alluring Siren / a ForceAttack graft). MustAttackPlayer { creature: ObjectId, player: PlayerId, }, + /// CR 508.1b + CR 508.1d + CR 604.1: `creature` must attack ANY ONE of + /// `players` — a single `RequiredDefender::Matching` directive whose live + /// player class currently resolves to these members (e.g. every opponent tied + /// for the most life; CR 508.1b lets the active player pick which tied legal + /// defender to attack). This is ONE requirement (CR 508.1d counts the + /// directive once, NOT once per member), kept distinct from any coexisting + /// `MustAttackPlayer` so a fixed requirement retains its own CR 701.15c + /// multiplicity. `players` is sorted + deduped and holds only currently + /// attackable members; the variant is emitted only when non-empty. Obeyed iff + /// `creature` attacks a player in `players`. + MustAttackAnyOf { + creature: ObjectId, + players: Vec, + }, /// CR 701.15b (second clause) + CR 701.15c: `creature` attacks a player other /// than `avoided` if able. Obeyed iff `creature` attacks a player ≠ /// `avoided`. Two source classes produce it — the goad DESIGNATION @@ -3933,11 +4003,48 @@ impl AttackDeclarationConstraints { if has_generic_must || !avoided.is_empty() { requirements.push(AttackRequirement::MustAttackGeneric { creature: cid }); } - for player in must_attack_players_for_creature(state, obj) { - if attackable_players.contains(&player) { - requirements.push(AttackRequirement::MustAttackPlayer { + // CR 508.1d + CR 604.1: emit ONE requirement per specific-player + // directive, preserving the directive boundary. `Fixed` directives + // collapse by attackable player (multiple sources naming the same + // player are one requirement — CR 508.1d set semantics); each + // `Matching` directive stays a single alternative-set requirement + // (attack ANY current member), never merged into the fixed set, so a + // coexisting fixed requirement keeps its own CR 701.15c multiplicity. + let directives = must_attack_player_directives_for_creature(state, obj); + let mut fixed_players: Vec = directives + .iter() + .filter_map(|(defender, _)| match defender { + ResolvedRequiredDefender::Fixed(player) => Some(*player), + ResolvedRequiredDefender::Matching(_) => None, + }) + .filter(|player| attackable_players.contains(player)) + .collect(); + fixed_players.sort_unstable_by_key(|p| p.0); + fixed_players.dedup(); + for player in fixed_players { + requirements.push(AttackRequirement::MustAttackPlayer { + creature: cid, + player, + }); + } + for (defender, _) in &directives { + let ResolvedRequiredDefender::Matching(members) = defender else { + continue; + }; + // CR 508.1b: only currently-attackable members can satisfy the + // directive; an all-unattackable class contributes no obeyable + // requirement (mirrors the `Fixed` attackable gate above). + let mut players: Vec = members + .iter() + .copied() + .filter(|player| attackable_players.contains(player)) + .collect(); + players.sort_unstable_by_key(|p| p.0); + players.dedup(); + if !players.is_empty() { + requirements.push(AttackRequirement::MustAttackAnyOf { creature: cid, - player, + players, }); } } @@ -4053,6 +4160,12 @@ fn requirement_obeyed(req: &AttackRequirement, attacks: &[(ObjectId, AttackTarge AttackRequirement::MustAttackPlayer { creature, player } => attacks .iter() .any(|(c, t)| c == creature && matches!(t, AttackTarget::Player(p) if p == player)), + // CR 508.1b + CR 508.1d: an alternative-set directive is obeyed by + // attacking ANY current member of its live class (one requirement, any + // member — not one per member). + AttackRequirement::MustAttackAnyOf { creature, players } => attacks.iter().any(|(c, t)| { + c == creature && matches!(t, AttackTarget::Player(p) if players.contains(p)) + }), AttackRequirement::AttackAwayFrom { creature, avoided } => attacks .iter() .any(|(c, t)| c == creature && matches!(t, AttackTarget::Player(p) if p != avoided)), @@ -5107,23 +5220,26 @@ pub fn attacker_constraints_for_active_player( // feeds BOTH the players list (CombatRequirement.players) and the // source collector's carrier list — no second scan, no drift. let directives = must_attack_player_directives_for_creature(state, obj); - // CR 508.1d: players is a SET — dedup so score_declaration counts - // one requirement per player even when two sources force the same - // player. + // Display-only badge (CR 508.1d): the flat union of every + // attackable candidate defender across all directives (`Fixed` + // singletons + every `Matching` member), deduped. The client only + // renders "must attack (one of) these"; the alternative-set + // grouping that legality depends on lives in the solver, not here. let mut players: Vec = directives .iter() - .filter(|(p, _)| attackable.contains(p)) - .map(|(p, _)| *p) + .flat_map(|(defender, _)| defender.members()) + .filter(|p| attackable.contains(p)) .collect(); players.sort_unstable_by_key(|p| p.0); players.dedup(); - // CR 611.2c: resolve each attackable requirement's carrier — the + // CR 611.2c: resolve each attackable directive's carrier — the // directing object (`source_object`), or the creature itself for - // an intrinsic def. Multiplicity retained (multi-source - // attribution); the collector's tail dedups by ObjectId. + // an intrinsic def. One entry per directive with an attackable + // member (multi-source attribution); the collector's tail dedups by + // ObjectId. let attackable_carriers: Vec = directives .iter() - .filter(|(p, _)| attackable.contains(p)) + .filter(|(defender, _)| defender.members().any(|p| attackable.contains(&p))) .map(|(_, src)| src.unwrap_or(obj_id)) .collect(); let sources = @@ -6408,7 +6524,9 @@ mod tests { /// two-player state carries no tax statics, so free_targets == legal_targets. #[test] fn best_free_declaration_matches_brute_force_oracle() { - use AttackRequirement::{AttackAwayFrom, MustAttackGeneric, MustAttackPlayer}; + use AttackRequirement::{ + AttackAwayFrom, MustAttackAnyOf, MustAttackGeneric, MustAttackPlayer, + }; let state = GameState::new_two_player(42); let p = |n: u8| AttackTarget::Player(PlayerId(n)); let pid = PlayerId; @@ -6561,6 +6679,48 @@ mod tests { ), "cap + needs-companion + goad", ), + // CR 508.1d + CR 604.1: a lone tied `Matching` alternative-set directive + // (attack an opponent with the most life; P1/P2 tied) is ONE requirement + // — attacking EITHER member scores 1, not 2. Max 1. + ( + mk_constraints( + vec![(10, vec![p(1), p(2)])], + vec![MustAttackAnyOf { + creature: ObjectId(10), + players: vec![pid(1), pid(2)], + }], + None, + vec![], + vec![], + vec![], + ), + "tied matching alternative-set counts once", + ), + // CR 508.1d regression (the reviewer's case): a tied `Matching` directive + // {P1,P2} PLUS a fixed `MustAttackPlayer` P1. Attacking P1 obeys BOTH (2); + // attacking P2 obeys only the alternative-set (1). Max 2 → the solver must + // force P1. Had the alternative-set been flattened+deduped into the fixed + // player set ({P1,P2}), attacking P2 would tie at 1 and be wrongly legal. + ( + mk_constraints( + vec![(10, vec![p(1), p(2)])], + vec![ + MustAttackAnyOf { + creature: ObjectId(10), + players: vec![pid(1), pid(2)], + }, + MustAttackPlayer { + creature: ObjectId(10), + player: pid(1), + }, + ], + None, + vec![], + vec![], + vec![], + ), + "tied matching + fixed forces the fixed member", + ), ]; for (c, label) in &cases { @@ -12311,6 +12471,89 @@ mod tests { ); } + /// CR 508.1d regression (PR #6885 review): a live `Matching` most-life directive + /// that TIES P1/P2 PLUS a coexisting `Fixed` P1 directive must force P1. The + /// alternative-set is ONE requirement (attack ANY tied member); the fixed + /// directive is a SECOND, independent requirement. Attacking P1 obeys BOTH (2); + /// attacking P2 obeys only the alternative-set (1). Since `max_no_payment` is 2, + /// a P2 declaration (score 1 < 2) is illegal — Galactus is forced onto P1. + /// + /// REVERT-FAIL: had the `Matching` members been flattened + deduped into the + /// shared fixed player set ({P1, P2}, as the pre-fix code did), attacking P1 and + /// P2 would each score 1 and tie, wrongly permitting P2. This exercises the real + /// production seam: `AttackDeclarationConstraints::build` → + /// `must_attack_player_directives_for_creature` → the `MustAttackAnyOf` / + /// `MustAttackPlayer` requirement split → `score_single` / `max_no_payment`. + #[test] + fn matching_tie_plus_fixed_forces_the_fixed_defender() { + // The exact production most-life filter (no hand-built AST): reuse the parser + // helper the forced-attack selector itself calls. + let (_, most_life) = crate::parser::oracle_effect::parse_opponent_most_life_restriction( + " with the most life among your opponents", + ) + .expect("most-life opponent filter must parse"); + + let mut state = GameState::new(FormatConfig::standard(), 3, 42); + state.turn_number = 2; + state.active_player = PlayerId(0); + state.phase = crate::types::phase::Phase::DeclareAttackers; + // Tie the two opponents for the most life so the class holds BOTH. + state.players[1].life = 25; + state.players[2].life = 25; + + let creature = create_creature(&mut state, PlayerId(0), "Galactus-like", 6, 6); + let defs = &mut state.objects.get_mut(&creature).unwrap().static_definitions; + // Live "attacks an opponent with the most life …" — resolves to {P1, P2}. + defs.push( + StaticDefinition::new(StaticMode::MustAttackPlayer { + player: RequiredDefender::Matching { + filter: most_life.clone(), + }, + }) + .affected(TargetFilter::SelfRef), + ); + // A coexisting fixed lure onto P1 (distinct source so it does not collapse + // into the live directive at the def level). + defs.push( + StaticDefinition::new(StaticMode::MustAttackPlayer { + player: RequiredDefender::Fixed { + player: PlayerId(1), + }, + }) + .affected(TargetFilter::SelfRef) + .source_object(ObjectId(9200)), + ); + + // Reach-guard: the live directive is non-vacuous — BOTH tied opponents + // surface in the flat projection. + assert_eq!( + must_attack_players_for_creature(&state, state.objects.get(&creature).unwrap()), + vec![PlayerId(1), PlayerId(2)], + "both tied most-life opponents are candidate defenders" + ); + + let constraints = AttackDeclarationConstraints::build(&state); + let required = max_no_payment(&constraints, &state); + let s_p1 = score_single(&constraints, creature, AttackTarget::Player(PlayerId(1))); + let s_p2 = score_single(&constraints, creature, AttackTarget::Player(PlayerId(2))); + assert_eq!( + s_p1, 2, + "attacking the fixed + tied member obeys BOTH directives" + ); + assert_eq!( + s_p2, 1, + "attacking the other tied member obeys only the alternative-set" + ); + assert_eq!( + required, 2, + "the maximum obeyable requirement count is 2 (attack P1)" + ); + assert!( + s_p2 < required, + "attacking P2 (score 1 < required 2) is illegal under CR 508.1d — P1 is forced" + ); + } + /// CR 508.1c: the real "(from Pacifism)" case the frontend renders — a creature /// restricted by a static on a DISTINCT object surfaces that object as the /// source. REVERT-FAIL: deleting the `check_static_ability_sources` extend in diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 855d039654..53d44b7ce5 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7309,12 +7309,16 @@ fn parse_for_each_object_copy_parts( /// repeat an earlier choice (confirmed by the "Offering" cycle ruling — /// Benevolent/Infernal/Intellectual/Sylvan Offering — issue #6381). Either /// way, the engine index is derived from chain position, not the ordinal. -/// CR 102.3 + CR 608.2d: Recognize the "with the most life [among +/// CR 102.2 / CR 102.3 + CR 608.2d: Recognize the "with the most life [among /// your opponents]" qualifier on a "choose an opponent" instruction and lower it /// to the equivalent `PlayerFilter::PlayerAttribute` restriction: each candidate /// opponent whose life total is `>=` the maximum life total across all opponents. -/// CR 102.3 scopes the candidate set to opponents; CR 608.2d lets the controller -/// pick ONE qualifying opponent (resolving ties) when multiple share the maximum. +/// CR 102.2 (two-player) / CR 102.3 (team multiplayer) scope the candidate set to +/// opponents. For the resolution-time "choose an opponent …" consumers, CR 608.2d +/// lets the controller pick ONE qualifying opponent (resolving ties) when multiple +/// share the maximum. (The static forced-attack consumer reuses this SAME filter +/// but is not a resolution-time choice — there a most-life tie is resolved under +/// CR 508.1b/d at declare-attackers; see `parse_required_defender_selector`.) /// The candidate's life is read PER-CANDIDATE (`PlayerScope::ScopedPlayer`); the /// `value` threshold is the controller-relative max (`PlayerScope::Opponent { /// aggregate: Max }`), composed from existing typed enums rather than a bespoke diff --git a/crates/engine/src/parser/oracle_static/evasion.rs b/crates/engine/src/parser/oracle_static/evasion.rs index 2db14196a6..52059e8d41 100644 --- a/crates/engine/src/parser/oracle_static/evasion.rs +++ b/crates/engine/src/parser/oracle_static/evasion.rs @@ -2464,12 +2464,19 @@ pub(crate) fn parse_subject_combat_rule_static(text: &str) -> Option OracleResult<'_, PlayerFilter> { let (input, _) = alt((tag::<_, _, OracleError<'_>>("an "), tag("a "))).parse(input)?; let (input, _) = tag("opponent").parse(input)?; @@ -2495,10 +2502,12 @@ fn parse_attacks_required_defender_nom(input: &str) -> OracleResult<'_, PlayerFi Ok((input, filter)) } -/// CR 508.1d + CR 604.1 / CR 604.2 + CR 102.3: " attacks -/// each combat if able [unless ]" — a static attack requirement whose -/// defending player is a live-evaluated class (Galactus: "an opponent with the -/// most life among your opponents"). Emits +/// CR 508.1d + CR 508.1b + CR 604.1 / CR 604.2 + CR 102.2 / CR 102.3: " +/// attacks each combat if able [unless ]" — a static +/// attack requirement (CR 508.1d) whose defending player is a live-evaluated class +/// (Galactus: "an opponent with the most life among your opponents"; CR 102.2 / +/// CR 102.3 scope "opponent", CR 508.1b covers the active player's choice among +/// tied legal defenders). Emits /// `MustAttackPlayer { RequiredDefender::Matching { filter } }`, re-evaluated each /// declare-attackers step by the combat resolver. /// @@ -2534,9 +2543,17 @@ fn parse_forced_attack_defender_static_body(text: &str) -> Option>(".")).parse(rest).ok()?; - if rest.trim().is_empty() { + let rest = rest.trim(); + if rest.is_empty() { return Some(def); } + // The ONLY permitted tail is an `unless` clause, and it must begin RIGHT HERE. + // Requiring `rest` to start with `unless ` (rather than letting the whole-text + // `unless` scan below find it anywhere) is what stops an unmodelled rider + // between the recurring-combat suffix and `unless` from being silently + // swallowed — e.g. "... each combat if able unless " must decline, + // not parse as if the rider were absent. + tag::<_, _, OracleError<'_>>("unless ").parse(rest).ok()?; let tp = TextPair::new(text, &lower); def.condition = Some(super::shared::parse_unless_static_condition(&tp)?); Some(def) diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 338cf2fc92..072815ba32 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -8963,6 +8963,32 @@ fn forced_attack_defender_static_bare_opponent_selector() { assert!(def.condition.is_none()); } +#[test] +fn forced_attack_defender_static_rejects_unmodelled_rider() { + // Honesty guard (CR 604.1): the ONLY permitted tail after "… each combat if + // able" is an `unless` clause that begins immediately. An unmodelled rider + // wedged between the recurring-combat suffix and `unless` must make the whole + // line decline — never parse as if the rider were absent (which would silently + // drop text and mark the card falsely supported). + // + // Reach-guard: the SAME line WITHOUT the rider parses (proving the rejection is + // the rider's doing, not an unrelated failure). + let ok = super::evasion::parse_forced_attack_defender_static( + "~ attacks an opponent with the most life among your opponents each combat if able unless you control a creature named Silver Surfer, Galactus's Herald.", + ); + assert!( + ok.is_some(), + "reach-guard: the rider-free line must still parse" + ); + let with_rider = super::evasion::parse_forced_attack_defender_static( + "~ attacks an opponent with the most life among your opponents each combat if able and gains flying unless you control a creature named Silver Surfer, Galactus's Herald.", + ); + assert!( + with_rider.is_none(), + "an unmodelled rider before `unless` must not be swallowed — the line declines", + ); +} + #[test] fn flavor_labeled_non_forced_attack_line_is_not_hijacked() { // Anti-hijack: the flavor-label strip must not manufacture a forced-attack diff --git a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs index 60e30fd58d..b7b2f78661 100644 --- a/crates/engine/tests/integration/galactus_forced_attack_most_life.rs +++ b/crates/engine/tests/integration/galactus_forced_attack_most_life.rs @@ -141,20 +141,33 @@ fn galactus_forced_to_attack_most_life_opponent() { ); } -/// Reach-guard proving the defender is re-evaluated LIVE, not snapshotted: with the -/// life totals swapped (P2@30 now most life), Galactus is forced to attack P2, and -/// attacking P1 (now lower) is rejected. +/// Reach-guard proving the defender is re-evaluated LIVE, not snapshotted at setup: +/// build ONE fixture with P1 as the most-life opponent (30 vs 20), then swap the +/// life totals IN PLACE before declaring. Because the requirement is recomputed +/// from live state at declare-attackers time, attacking P1 (now the lower-life +/// opponent) is rejected and attacking P2 (now the most-life opponent) commits — +/// both through the production `GameAction::DeclareAttackers` path in the SAME game +/// state. A setup-time snapshot of the required defender would still name P1 and +/// wrongly accept the P1 declaration, so this fails on any snapshot regression. #[test] fn galactus_required_defender_reevaluated_live() { - let (mut wrong, galactus) = parked_galactus(20, 30, None); + let (mut runner, galactus) = parked_galactus(30, 20, None); + // Swap the life totals in place: P2 becomes the most-life opponent. (A rejected + // declaration commits nothing — CR 508.1a–e validate before any tap/commit — so + // the reject-then-accept below runs against one continuous game state.) + runner.state_mut().players[P1.0 as usize].life = 20; + runner.state_mut().players[P2.0 as usize].life = 30; + assert!( - declare(&mut wrong, galactus, P1).is_err(), - "after the life swap P1 is no longer the most-life opponent" + declare(&mut runner, galactus, P1).is_err(), + "after the in-place life swap P1 is no longer the most-life opponent" + ); + declare(&mut runner, galactus, P2) + .expect("P2 became the most-life opponent after the live swap — attacking it is legal"); + assert!( + runner.state().combat.is_some(), + "the satisfying declaration commits in the same fixture" ); - - let (mut right, galactus) = parked_galactus(20, 30, None); - declare(&mut right, galactus, P2).expect("P2 is now the most-life opponent"); - assert!(right.state().combat.is_some()); } /// CR 508.1d tie: when opponents are tied for the most life, attacking EITHER From 98415735e9a29528897e1d1fb87757f3d052a5fa Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:55:40 -0500 Subject: [PATCH 3/3] fix(parser): decline forced-attack static when the unless gate is not fully modeled Addresses the PR #6885 re-review blocker (matthewevans). The forced-attack body accepted any tail beginning `unless ` and stored the result of parse_unless_static_condition. For an unrecognized inner condition that helper returns Not(Unrecognized), which (a) the coverage detector (static_has_unimplemented_parts) misses because it only flags a TOP-LEVEL Unrecognized, so the card is falsely reported supported, and (b) evaluates permanently false at runtime (Unrecognized is true; the wrapping Not negates it), silently disabling the whole attack requirement. Decline the parse when the resolved condition contains an Unrecognized clause anywhere in its tree (recursing through Not/And/Or), so a not-fully-modeled `unless` gate leaves the line honestly unsupported (coverage red) instead of shipping a broken, falsely-supported static. Galactus's own gate is fully modeled and still parses. Adds forced_attack_defender_static_rejects_unmodelled_unless_condition: a reach-guarded grammar-class test proving an unknown condition AFTER `unless` declines while a modeled gate still parses (one level deeper than the existing rider test, which guards text BEFORE `unless`). Verified on the current head (7d30f917): cargo fmt, parser combinator Gate A, clippy (-D warnings), full phase-engine suite (18359 lib + 4400 integration tests, 0 failed). Card-data coverage/semantic-audit run in the CI "Card data" job (the card-data/AtomicCards inputs are gitignored and R2-served, absent in a fresh checkout). Co-Authored-By: Claude Opus 4.8 --- .../src/parser/oracle_static/evasion.rs | 36 ++++++++++++++++++- .../engine/src/parser/oracle_static/tests.rs | 28 +++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/parser/oracle_static/evasion.rs b/crates/engine/src/parser/oracle_static/evasion.rs index 52059e8d41..190390abb4 100644 --- a/crates/engine/src/parser/oracle_static/evasion.rs +++ b/crates/engine/src/parser/oracle_static/evasion.rs @@ -2555,10 +2555,44 @@ fn parse_forced_attack_defender_static_body(text: &str) -> Option>("unless ").parse(rest).ok()?; let tp = TextPair::new(text, &lower); - def.condition = Some(super::shared::parse_unless_static_condition(&tp)?); + let condition = super::shared::parse_unless_static_condition(&tp)?; + // Coverage-honesty gate (CR 604.1): only emit the forced-attack static when the + // `unless` gate is a FULLY-MODELED condition. `parse_unless_static_condition` + // wraps an unrecognized inner clause as `Not(Unrecognized)` — which (a) the + // coverage detector's TOP-LEVEL `Unrecognized` check misses, so the card is + // falsely reported supported, and (b) evaluates permanently false at runtime + // (`Unrecognized` is true; the wrapping `Not` negates it), silently disabling + // the whole requirement. Decline instead so the line stays honestly unsupported + // (coverage red) rather than shipping a broken static. + if static_condition_contains_unrecognized(&condition) { + return None; + } + def.condition = Some(condition); Some(def) } +/// True when `condition` contains an `Unrecognized` clause ANYWHERE in its tree +/// (recursing through the `Not` / `And` / `Or` combinators). Used by the +/// forced-attack parser to decline a not-fully-modeled `unless` gate: the +/// coverage detector only flags a TOP-LEVEL `Unrecognized`, so a nested one +/// (`Not(Unrecognized)`, the shape `parse_unless_static_condition` emits for an +/// unknown clause) would otherwise mark the card supported while its requirement +/// is permanently inert at runtime. +fn static_condition_contains_unrecognized(condition: &StaticCondition) -> bool { + match condition { + StaticCondition::Unrecognized { .. } => true, + // The only sub-condition-embedding variants — recurse through them. If a NEW + // combinator variant that nests `StaticCondition` is added, extend this match; + // the leaf wildcard below would otherwise hide an unrecognized clause inside + // it. Every remaining variant is a leaf that cannot contain a nested clause. + StaticCondition::Not { condition } => static_condition_contains_unrecognized(condition), + StaticCondition::And { conditions } | StaticCondition::Or { conditions } => conditions + .iter() + .any(static_condition_contains_unrecognized), + _ => false, + } +} + /// CR 702.122a / 702.171a / 702.184c: nom parser for the crew/saddle/station /// power-contribution modifier predicate. Composes the named action-list prefix /// (which records the affected keyword actions) with the modifier tail. diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 072815ba32..57768a7ba0 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -8989,6 +8989,34 @@ fn forced_attack_defender_static_rejects_unmodelled_rider() { ); } +#[test] +fn forced_attack_defender_static_rejects_unmodelled_unless_condition() { + // Coverage-honesty guard: an `unless` gate whose INNER condition is not modeled + // must make the whole line decline. Otherwise it parses to `Not(Unrecognized)`, + // which the coverage detector's top-level `Unrecognized` check misses (falsely + // "supported") and which evaluates permanently false at runtime — silently + // disabling the forced-attack requirement. This is the grammar-class hole one + // level deeper than the rider test above: the rider sits BEFORE `unless`; here + // the unrecognized clause sits AFTER it. + // + // Reach-guard: the SAME shape with a MODELED `unless` condition still parses, so + // the rejection is the unrecognized condition's doing, not an unrelated failure. + let modeled = super::evasion::parse_forced_attack_defender_static( + "~ attacks an opponent with the most life among your opponents each combat if able unless you control a creature named Silver Surfer, Galactus's Herald.", + ); + assert!( + modeled.is_some(), + "reach-guard: a fully-modeled `unless` gate still parses", + ); + let unmodeled = super::evasion::parse_forced_attack_defender_static( + "~ attacks an opponent each combat if able unless you satisfy an unmodelled condition.", + ); + assert!( + unmodeled.is_none(), + "an unmodeled `unless` condition must decline — never a broken, falsely-supported static", + ); +} + #[test] fn flavor_labeled_non_forced_attack_line_is_not_hijacked() { // Anti-hijack: the flavor-label strip must not manufacture a forced-attack