From 8d5fdc86e27345f098c1fd0520c606f5956ff927 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 12:40:09 -0700 Subject: [PATCH 01/10] feat(phase-ai): add cycling deck-feature axis + CyclingPayoffPolicy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR 702.29a cycling is card-neutral, so the AI's priors undervalue it and CyclingDisciplinePolicy only adds patience (don't cycle a needed land); self_cost_value explicitly defers cycling value. Nothing modelled the upside: with a "whenever you cycle a card" engine (Astral Drift, Drannith Stinger — CR 702.29c/d) on the battlefield, every cycle is a repeatable value trigger, so the AI should cycle eagerly. New CyclingFeature axis (structural, no card names): - source_count: cyclers (Keyword::Cycling / Typecycling, CR 702.29a/e). - payoff_count: permanents with a controller-scoped, non-self TriggerMode::Cycled / CycledOrDiscarded engine trigger. - commitment: geometric mean over (source, payoff) — both mandatory (cyclers with no engine is just smoothing; an engine with no cyclers never fires). payoff_names carries one entry per unique engine face for the policy's battlefield identity lookup. New CyclingPayoffPolicy: on a Cycling activation, if the AI controls a known engine (identity lookup, since GameObject has no triggers field), score a positive per-engine bonus. Composes with CyclingDiscipline's patience so a payoff deck cycles into its engine while a smoothing deck stays patient. cycling_payoff_bonus registered UNTUNED. Tests: 12 feature + 6 policy (incl a registry-routed regression) + full 1551-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/config.rs | 12 + crates/phase-ai/src/features/cycling.rs | 155 +++++++++++ crates/phase-ai/src/features/mod.rs | 5 + crates/phase-ai/src/features/tests/cycling.rs | 174 +++++++++++++ crates/phase-ai/src/features/tests/mod.rs | 1 + .../phase-ai/src/policies/cycling_payoff.rs | 111 ++++++++ crates/phase-ai/src/policies/mod.rs | 1 + crates/phase-ai/src/policies/registry.rs | 4 + .../src/policies/tests/cycling_payoff.rs | 246 ++++++++++++++++++ crates/phase-ai/src/policies/tests/mod.rs | 1 + 10 files changed, 710 insertions(+) create mode 100644 crates/phase-ai/src/features/cycling.rs create mode 100644 crates/phase-ai/src/features/tests/cycling.rs create mode 100644 crates/phase-ai/src/policies/cycling_payoff.rs create mode 100644 crates/phase-ai/src/policies/tests/cycling_payoff.rs diff --git a/crates/phase-ai/src/config.rs b/crates/phase-ai/src/config.rs index c81cbdf28a..ce215f45be 100644 --- a/crates/phase-ai/src/config.rs +++ b/crates/phase-ai/src/config.rs @@ -499,6 +499,10 @@ pub struct PolicyPenalties { /// threshold, turning a non-creature enchantment into a body. #[serde(default = "default_devotion_god_activation")] pub devotion_god_activation: f64, + /// CR 702.29c/d: card-equivalent value of cycling into one active + /// "whenever you cycle" engine (preference band, per engine). + #[serde(default = "default_cycling_payoff_bonus")] + pub cycling_payoff_bonus: f64, } impl Default for PolicyPenalties { @@ -572,6 +576,7 @@ impl Default for PolicyPenalties { graveyard_types_progress: default_graveyard_types_progress(), devotion_pip_progress: default_devotion_pip_progress(), devotion_god_activation: default_devotion_god_activation(), + cycling_payoff_bonus: default_cycling_payoff_bonus(), } } } @@ -666,6 +671,9 @@ fn default_devotion_pip_progress() -> f64 { fn default_devotion_god_activation() -> f64 { 2.5 } +fn default_cycling_payoff_bonus() -> f64 { + 0.6 +} fn default_sacrifice_token_cost() -> f64 { 0.5 } @@ -810,6 +818,10 @@ pub const UNTUNED_POLICY_PENALTY_FIELDS: &[(&str, &str)] = &[ "devotion_god_activation", "CR 700.5 god-threshold-crossing swing weight — awaiting a paired-seed ai-gate calibration.", ), + ( + "cycling_payoff_bonus", + "CR 702.29c/d per-engine cycling payoff weight — awaiting a paired-seed ai-gate calibration.", + ), ( "poison_clock_pressure", "CR 104.3d win-detector weight — a critical-band term whose magnitude is \ diff --git a/crates/phase-ai/src/features/cycling.rs b/crates/phase-ai/src/features/cycling.rs new file mode 100644 index 0000000000..2ae25772c8 --- /dev/null +++ b/crates/phase-ai/src/features/cycling.rs @@ -0,0 +1,155 @@ +//! Cycling feature — structural detection of a "cycling matters" payoff deck. +//! +//! Parser AST verification — VERIFIED against engine source: +//! - `Keyword::Cycling(CyclingCost)` at `crates/engine/src/types/keywords.rs:694` +//! and `Keyword::Typecycling { .. }` at `keywords.rs:966` (CR 702.29a/e) — the +//! cyclable cards (the enablers). +//! - `TriggerMode::Cycled` at `crates/engine/src/types/triggers.rs:397` +//! (CR 702.29c: "when you cycle this card") and `TriggerMode::CycledOrDiscarded` +//! at `triggers.rs:400` (CR 702.29d: "whenever you cycle or discard a card") — +//! the payoffs. +//! - `TriggerDefinition.valid_card` / `.valid_target` (`Option`) at +//! `ability.rs:4522`/`:4539` — used to keep only controller-scoped, non-self +//! ("whenever you cycle A card") engine payoffs. +//! +//! No parser remediation required — every axis is expressible over existing +//! typed AST. +//! +//! ## Why this axis exists +//! +//! Cycling is card-neutral selection, so the AI's generic priors treat it as +//! marginal — and `CyclingDisciplinePolicy` only adds *patience* (it penalises +//! cycling away a needed land), while `self_cost_value` explicitly defers +//! cycling value (`self_cost_cycling_deferred`). Nothing models the *upside*: in +//! a deck with a "whenever you cycle a card" engine (Astral Drift, Drannith +//! Stinger, New Perspectives), every cycle is a repeatable value trigger, and +//! the AI should cycle eagerly. This axis lets a policy see that engine. +//! +//! ## Boundary with `spellslinger_prowess` / `mill` +//! +//! Spellslinger counts *spell-cast* triggers; cycling triggers on the cycling +//! keyword action (CR 702.29c), a disjoint event. A cycling card that is also an +//! instant/sorcery can read on both axes — that overlap is intentional and the +//! axes stay independent. + +use engine::game::DeckEntry; +use engine::types::ability::{TargetFilter, TriggerDefinition}; +use engine::types::card_type::CoreType; +use engine::types::keywords::Keyword; +use engine::types::triggers::TriggerMode; + +use crate::features::commitment; + +/// Commitment at or above which "cycling matters" is a real plan for this deck +/// rather than incidental card smoothing. Gates `CyclingPayoffPolicy::activation`. +pub const CYCLING_PAYOFF_FLOOR: f32 = 0.35; + +/// CR 702.29: per-deck cycling-payoff classification. +/// +/// Populated once per game from `DeckEntry` data. Detection is structural over +/// `CardFace.keywords` and `CardFace.triggers` — never by card name. +#[derive(Debug, Clone, Default)] +pub struct CyclingFeature { + /// Cyclable cards — CR 702.29a Cycling / CR 702.29e Typecycling. The + /// enablers that feed the payoff engine. + pub source_count: u32, + /// Permanents carrying a "whenever you cycle a card" engine trigger + /// (CR 702.29c/d), controller-scoped and not self-referential — the payoffs + /// that make cycling actively good. + pub payoff_count: u32, + /// `0.0..=1.0` — how central cycling-as-a-payoff is to this deck. Consumed by + /// `CyclingPayoffPolicy::activation` as the single scaling knob. + pub commitment: f32, + /// Names of the detected payoff engines. NOT used for classification — that + /// already happened against the AST. Identity lookup only, so the policy can + /// re-find a payoff on the battlefield (`GameObject` carries no `triggers` + /// field). One entry per UNIQUE face, never per playset copy. + pub payoff_names: Vec, +} + +/// Structural detection over each `DeckEntry`'s `CardFace` AST. +pub fn detect(deck: &[DeckEntry]) -> CyclingFeature { + if deck.is_empty() { + return CyclingFeature::default(); + } + + let mut source_count = 0u32; + let mut payoff_count = 0u32; + let mut total_nonland = 0u32; + let mut payoff_names: Vec = Vec::new(); + + for entry in deck { + let face = &entry.card; + if !face.card_type.core_types.contains(&CoreType::Land) { + total_nonland = total_nonland.saturating_add(entry.count); + } + + if is_cycle_source_parts(&face.keywords) { + source_count = source_count.saturating_add(entry.count); + } + if is_cycle_payoff_parts(&face.triggers) { + payoff_count = payoff_count.saturating_add(entry.count); + // Identity list: one push per UNIQUE face, not per copy. + if !payoff_names.contains(&face.name) { + payoff_names.push(face.name.clone()); + } + } + } + + let commitment = compute_commitment(source_count, payoff_count, total_nonland); + + CyclingFeature { + source_count, + payoff_count, + commitment, + payoff_names, + } +} + +/// CR 702.29a/e: the face is cyclable — it carries Cycling or Typecycling. +pub(crate) fn is_cycle_source_parts(keywords: &[Keyword]) -> bool { + keywords + .iter() + .any(|k| matches!(k, Keyword::Cycling(_) | Keyword::Typecycling { .. })) +} + +/// CR 702.29c/d: the face carries a "whenever you cycle a card" engine trigger — +/// a repeatable payoff, not a one-shot self-cycle bonus. +pub(crate) fn is_cycle_payoff_parts<'a>( + triggers: impl IntoIterator, +) -> bool { + triggers.into_iter().any(trigger_is_cycle_payoff) +} + +fn trigger_is_cycle_payoff(t: &TriggerDefinition) -> bool { + // 1. Mode fires on a cycle event (CR 702.29c/d). + if !matches!(t.mode, TriggerMode::Cycled | TriggerMode::CycledOrDiscarded) { + return false; + } + // 2. Caster-scoped only: an opponent-scoped "whenever an opponent cycles" + // punisher is not your payoff. + if !matches!(&t.valid_target, None | Some(TargetFilter::Controller)) { + return false; + } + // 3. Exclude the pure self-cycle bonus ("when you cycle THIS card"): that is + // a cyclable card with upside (already counted as a source), not a + // battlefield engine that rewards cycling other cards. + !matches!(&t.valid_card, Some(TargetFilter::SelfRef)) +} + +/// Calibration: a dedicated cycling-payoff deck (e.g. Pioneer/Historic cycling: +/// ~18 cyclers + ~6 engines like Astral Drift / Drannith Stinger over ~36 +/// nonland) → commitment ≈ 0.85. Anti-calibration: a control deck that plays two +/// cycling lands and no engine → below `CYCLING_PAYOFF_FLOOR`; a deck with an +/// engine but no cyclers, or cyclers but no engine → 0.0. +/// +/// Geometric mean over (source, payoff): BOTH pillars are mandatory. Cyclers +/// with no engine is just card smoothing (`CyclingDisciplinePolicy` governs it); +/// an engine with no cyclers never triggers. +fn compute_commitment(source_count: u32, payoff_count: u32, total_nonland: u32) -> f32 { + // ~18 cyclers per 60 nonland is a fully-committed cycling shell. + let source_density = (commitment::density_per_60(source_count, total_nonland) / 18.0).min(1.0); + // ~6 engine payoffs per 60 nonland is a fully-committed payoff base. + let payoff_density = (commitment::density_per_60(payoff_count, total_nonland) / 6.0).min(1.0); + commitment::geometric_mean(&[source_density, payoff_density]) +} diff --git a/crates/phase-ai/src/features/mod.rs b/crates/phase-ai/src/features/mod.rs index f83ed388a2..6b9b04fe3d 100644 --- a/crates/phase-ai/src/features/mod.rs +++ b/crates/phase-ai/src/features/mod.rs @@ -12,6 +12,7 @@ pub mod artifacts; pub mod blink; pub mod commitment; pub mod control; +pub mod cycling; pub mod devotion; pub mod enchantments; pub mod energy; @@ -36,6 +37,7 @@ pub use aristocrats::AristocratsFeature; pub use artifacts::ArtifactsFeature; pub use blink::BlinkFeature; pub use control::ControlFeature; +pub use cycling::CyclingFeature; pub use devotion::DevotionFeature; pub use enchantments::EnchantmentsFeature; pub use energy::EnergyFeature; @@ -89,6 +91,8 @@ pub struct DeckFeatures { pub poison: PoisonFeature, /// CR 207.2c + CR 205.2a: delirium / descend graveyard type-diversity. pub graveyard_types: GraveyardTypesFeature, + /// CR 702.29: "cycling matters" payoff density (cyclers + on-cycle engines). + pub cycling: CyclingFeature, /// Declaration-derived: the deck's declared bracket tier. Unlike the /// other fields here, this is not structurally detected from card text — /// it is a per-deck declaration set at deck-analysis time from deck @@ -136,6 +140,7 @@ impl DeckFeatures { energy: energy::detect(deck), poison: poison::detect(deck), graveyard_types: graveyard_types::detect(deck), + cycling: cycling::detect(deck), bracket_tier: tier, } } diff --git a/crates/phase-ai/src/features/tests/cycling.rs b/crates/phase-ai/src/features/tests/cycling.rs new file mode 100644 index 0000000000..064e7a2da9 --- /dev/null +++ b/crates/phase-ai/src/features/tests/cycling.rs @@ -0,0 +1,174 @@ +//! Unit tests for `features::cycling` — CR 702.29 "cycling matters" detection. +//! No `#[cfg(test)]` in SOURCE files; tests live here. + +use engine::game::DeckEntry; +use engine::types::ability::{ + AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerDefinition, +}; +use engine::types::card::CardFace; +use engine::types::card_type::{CardType, CoreType}; +use engine::types::keywords::{CyclingCost, Keyword}; +use engine::types::mana::ManaCost; +use engine::types::triggers::TriggerMode; + +use crate::features::cycling::*; + +fn face(name: &str, core: CoreType) -> CardFace { + CardFace { + name: name.to_string(), + card_type: CardType { + supertypes: Vec::new(), + core_types: vec![core], + subtypes: Vec::new(), + }, + ..Default::default() + } +} + +fn entry(card: CardFace, count: u32) -> DeckEntry { + DeckEntry { card, count } +} + +/// A cyclable card (CR 702.29a). +fn cycler(name: &str) -> CardFace { + let mut f = face(name, CoreType::Creature); + f.keywords = vec![Keyword::Cycling(CyclingCost::Mana(ManaCost::generic(2)))]; + f +} + +fn cycled_trigger( + mode: TriggerMode, + valid_card: Option, + valid_target: Option, +) -> TriggerDefinition { + let mut t = TriggerDefinition::new(mode); + if let Some(vc) = valid_card { + t = t.valid_card(vc); + } + if let Some(vt) = valid_target { + t = t.valid_target(vt); + } + t.execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + )) +} + +/// Astral Drift shape: "whenever you cycle or discard a card, ..." — a broad, +/// controller-scoped engine on a permanent. +fn engine(name: &str) -> CardFace { + let mut f = face(name, CoreType::Enchantment); + f.triggers = vec![cycled_trigger(TriggerMode::CycledOrDiscarded, None, None)]; + f +} + +#[test] +fn empty_deck_produces_defaults() { + let f = detect(&[]); + assert_eq!(f.source_count, 0); + assert_eq!(f.payoff_count, 0); + assert_eq!(f.commitment, 0.0); + assert!(f.payoff_names.is_empty()); +} + +#[test] +fn vanilla_deck_not_registered() { + let f = detect(&[entry(face("Bear", CoreType::Creature), 20)]); + assert_eq!(f.source_count, 0); + assert_eq!(f.payoff_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn detects_cycler_source() { + let f = detect(&[entry(cycler("Shefet Monitor"), 4)]); + assert_eq!(f.source_count, 4); +} + +#[test] +fn detects_engine_payoff_and_names_it() { + let f = detect(&[entry(engine("Astral Drift"), 3)]); + assert_eq!(f.payoff_count, 3); + assert_eq!(f.payoff_names, vec!["Astral Drift".to_string()]); +} + +/// A pure "when you cycle THIS card" self-bonus is a cyclable card with upside, +/// not a battlefield engine — it must not count as a payoff. +#[test] +fn self_cycle_bonus_is_not_a_payoff() { + let mut f = cycler("Radiant Smite"); + f.triggers = vec![cycled_trigger( + TriggerMode::Cycled, + Some(TargetFilter::SelfRef), + None, + )]; + let result = detect(&[entry(f, 4)]); + assert_eq!(result.source_count, 4, "still a cycler"); + assert_eq!(result.payoff_count, 0, "self-cycle bonus is not an engine"); +} + +/// An opponent-scoped "whenever an opponent cycles" punisher is not your payoff. +#[test] +fn opponent_scoped_trigger_ignored() { + let mut f = face("Punisher", CoreType::Enchantment); + f.triggers = vec![cycled_trigger( + TriggerMode::CycledOrDiscarded, + None, + Some(TargetFilter::Opponent), + )]; + assert_eq!(detect(&[entry(f, 2)]).payoff_count, 0); +} + +/// Identity list carries one entry per UNIQUE face, never per playset copy. +#[test] +fn payoff_names_dedup_per_face() { + let f = detect(&[entry(engine("Astral Drift"), 4)]); + assert_eq!(f.payoff_names.len(), 1); +} + +/// Calibration: a dedicated cycling shell (cyclers + engines) clears the floor. +#[test] +fn committed_cycling_deck_hits_floor() { + let deck = vec![ + entry(cycler("Cyc A"), 12), + entry(cycler("Cyc B"), 8), + entry(engine("Astral Drift"), 4), + entry(engine("Drannith Stinger"), 3), + entry(face("Plains", CoreType::Land), 24), + ]; + let f = detect(&deck); + assert!( + f.commitment > 0.6, + "committed cycling deck must clear 0.6, got {}", + f.commitment + ); +} + +/// Both pillars are mandatory: cyclers with no engine is just card smoothing. +#[test] +fn cyclers_without_engine_collapse() { + let deck = vec![ + entry(cycler("Cyc"), 20), + entry(face("Plains", CoreType::Land), 24), + ]; + assert_eq!(detect(&deck).commitment, 0.0); +} + +/// An engine with no cyclers never triggers → not a cycling deck. +#[test] +fn engine_without_cyclers_collapses() { + let deck = vec![ + entry(engine("Astral Drift"), 4), + entry(face("Plains", CoreType::Land), 24), + ]; + assert_eq!(detect(&deck).commitment, 0.0); +} + +#[test] +fn commitment_clamps_to_one() { + let deck = vec![entry(cycler("Cyc"), 40), entry(engine("Astral Drift"), 20)]; + assert!(detect(&deck).commitment <= 1.0); +} diff --git a/crates/phase-ai/src/features/tests/mod.rs b/crates/phase-ai/src/features/tests/mod.rs index 03acc6775c..9859e8fc73 100644 --- a/crates/phase-ai/src/features/tests/mod.rs +++ b/crates/phase-ai/src/features/tests/mod.rs @@ -3,6 +3,7 @@ pub mod artifacts; pub mod blink; +pub mod cycling; pub mod devotion; pub mod enchantments; pub mod energy; diff --git a/crates/phase-ai/src/policies/cycling_payoff.rs b/crates/phase-ai/src/policies/cycling_payoff.rs new file mode 100644 index 0000000000..1d3ff7a8ca --- /dev/null +++ b/crates/phase-ai/src/policies/cycling_payoff.rs @@ -0,0 +1,111 @@ +//! `CyclingPayoffPolicy` — makes an on-battlefield "whenever you cycle" engine a +//! reason the AI can see to cycle EAGERLY. +//! +//! ## The gap this closes +//! +//! CR 702.29a: cycling is card-neutral selection, so the generic activated- +//! ability prior undervalues it and [`CyclingDisciplinePolicy`](super::cycling_discipline) +//! only adds *patience* (don't cycle away a needed land); `self_cost_value` +//! explicitly defers cycling value (`self_cost_cycling_deferred`). Neither sees +//! the upside: with an engine like Astral Drift or Drannith Stinger on the +//! battlefield (CR 702.29c/d), every cycle is a repeatable value trigger — exile +//! a creature, ping each opponent, draw. This policy adds that positive signal, +//! which composes with the discipline penalty so a payoff deck cycles into its +//! engine while a smoothing-only deck stays patient. +//! +//! ## Performance +//! +//! `verdict()` runs per candidate per search node. The card-local check — the +//! candidate is a `Cycling`-tagged activation — runs FIRST and rejects every +//! other activation. Only a confirmed cycling activation pays for the +//! battlefield engine scan, and only in a deck whose `activation` floor is +//! already cleared. No affordability sweep, no `find_legal_targets`. + +use engine::types::ability::AbilityTag; +use engine::types::game_state::GameState; +use engine::types::player::PlayerId; + +use crate::features::cycling::CYCLING_PAYOFF_FLOOR; +use crate::features::DeckFeatures; + +use super::context::PolicyContext; +use super::registry::{DecisionKind, PolicyId, PolicyReason, PolicyVerdict, TacticalPolicy}; + +pub struct CyclingPayoffPolicy; + +/// Cap on how many simultaneous engines are rewarded, so a stacked board can't +/// push a single cycle into the critical band. +const MAX_REWARDED_ENGINES: usize = 3; + +impl TacticalPolicy for CyclingPayoffPolicy { + fn id(&self) -> PolicyId { + PolicyId::CyclingPayoff + } + + fn decision_kinds(&self) -> &'static [DecisionKind] { + &[DecisionKind::ActivateAbility] + } + + fn activation( + &self, + features: &DeckFeatures, + _state: &GameState, + _player: PlayerId, + ) -> Option { + if features.cycling.commitment < CYCLING_PAYOFF_FLOOR { + None + } else { + Some(features.cycling.commitment) + } + } + + fn verdict(&self, ctx: &PolicyContext<'_>) -> PolicyVerdict { + // Card-local first: only a Cycling activation is in scope (CR 702.29a). + let Some(ability) = ctx.effective_activated_ability() else { + return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_na")); + }; + if ability.ability_tag != Some(AbilityTag::Cycling) { + return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_na")); + } + + let Some(feature) = ctx + .context + .session + .features + .get(&ctx.ai_player) + .map(|f| &f.cycling) + else { + return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_na")); + }; + if feature.payoff_names.is_empty() { + return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_no_engine")); + } + + // Only now pay for the battlefield scan. Identity lookup (the sanctioned + // exempt pattern): re-find the structurally-classified engines the AI + // controls, since `GameObject` carries no `triggers` field. + let engines = ctx + .state + .battlefield + .iter() + .filter(|id| { + ctx.state.objects.get(id).is_some_and(|obj| { + obj.controller == ctx.ai_player + && feature.payoff_names.iter().any(|name| name == &obj.name) + }) + }) + .count(); + if engines == 0 { + return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_no_engine")); + } + + // Each active engine turns this cycle into a value trigger — roughly a + // card-equivalent apiece, capped so one cycle stays a preference, not a + // game-deciding swing. + let rewarded = engines.min(MAX_REWARDED_ENGINES) as f64; + PolicyVerdict::score( + ctx.config.policy_penalties.cycling_payoff_bonus * rewarded, + PolicyReason::new("cycling_payoff_engine_active").with_fact("engines", engines as i64), + ) + } +} diff --git a/crates/phase-ai/src/policies/mod.rs b/crates/phase-ai/src/policies/mod.rs index 75686c295e..da6a889cfc 100644 --- a/crates/phase-ai/src/policies/mod.rs +++ b/crates/phase-ai/src/policies/mod.rs @@ -16,6 +16,7 @@ mod control_change_awareness; pub(crate) mod copy_value; mod crew_timing; mod cycling_discipline; +mod cycling_payoff; mod devotion; mod downside_awareness; pub(crate) mod effect_classify; diff --git a/crates/phase-ai/src/policies/registry.rs b/crates/phase-ai/src/policies/registry.rs index 164fe97a34..d719a53c0a 100644 --- a/crates/phase-ai/src/policies/registry.rs +++ b/crates/phase-ai/src/policies/registry.rs @@ -144,6 +144,9 @@ pub enum PolicyId { CombatWithdrawal, /// CR 608.2c: "return a land you control" self-bounce target choice. SelfBounceTarget, + /// CR 702.29c/d: reward cycling into an on-battlefield "whenever you cycle" + /// engine. + CyclingPayoff, } /// Coarse routing kind for a candidate decision. Each policy declares which @@ -399,6 +402,7 @@ impl Default for PolicyRegistry { Box::new(PayoffPolicy::new(&BLINK_PAYOFF)), Box::new(LoopShortcutPolicy), Box::new(super::self_bounce_target::SelfBounceTargetPolicy), + Box::new(super::cycling_payoff::CyclingPayoffPolicy), ]; let mut by_kind: HashMap> = HashMap::new(); for (idx, policy) in policies.iter().enumerate() { diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs new file mode 100644 index 0000000000..320ab98210 --- /dev/null +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -0,0 +1,246 @@ +//! Unit tests for `policies::cycling_payoff` — CR 702.29c/d "cycling matters" +//! payoff policy. No `#[cfg(test)]` in SOURCE files; tests live here. +//! +//! Direct-`verdict` tests cover each branch; a registry-routed regression +//! exercises the production seam (registration + `ActivateAbility` routing), +//! following the poison policy's pattern. + +use std::sync::Arc; + +use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; +use engine::game::zones::create_object; +use engine::types::actions::GameAction; +use engine::types::card_type::CoreType; +use engine::types::format::FormatConfig; +use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::keywords::{CyclingCost, Keyword}; +use engine::types::mana::ManaCost; +use engine::types::player::PlayerId; +use engine::types::zones::Zone; + +use crate::config::AiConfig; +use crate::context::AiContext; +use crate::features::cycling::{CyclingFeature, CYCLING_PAYOFF_FLOOR}; +use crate::features::DeckFeatures; +use crate::policies::context::{PolicyContext, SearchDepth}; +use crate::policies::cycling_payoff::*; +use crate::policies::registry::{ + PolicyId, PolicyReason, PolicyRegistry, PolicyVerdict, TacticalPolicy, +}; +use crate::session::AiSession; + +const AI: PlayerId = PlayerId(0); +const ENGINE_NAME: &str = "Astral Drift"; + +fn state() -> GameState { + GameState::new(FormatConfig::standard(), 2, 42) +} + +/// A cyclable card in hand whose synthesized Cycling ability is the activation. +fn cycler(state: &mut GameState) -> ObjectId { + let kw = Keyword::Cycling(CyclingCost::Mana(ManaCost::generic(2))); + let card_id = CardId(state.next_object_id); + let id = create_object(state, card_id, AI, "Cyc".to_string(), Zone::Hand); + let ability = engine::database::synthesis::cycling_ability_for_keyword(&kw) + .expect("cycling keyword must synthesize an activated ability"); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.base_card_types = obj.card_types.clone(); + Arc::make_mut(&mut obj.abilities).push(ability); + id +} + +/// A payoff engine permanent the AI controls, matched by name. +fn engine_on_battlefield(state: &mut GameState) { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + ENGINE_NAME.to_string(), + Zone::Battlefield, + ); + state + .objects + .get_mut(&id) + .unwrap() + .card_types + .core_types + .push(CoreType::Enchantment); +} + +fn session(commitment: f32, payoff_names: Vec) -> AiSession { + let features = DeckFeatures { + cycling: CyclingFeature { + source_count: 18, + payoff_count: 4, + commitment, + payoff_names, + }, + ..Default::default() + }; + let mut session = AiSession::empty(); + session.features.insert(AI, features); + session +} + +fn context(config: &AiConfig, session: AiSession) -> AiContext { + let mut context = AiContext::empty(&config.weights); + context.session = Arc::new(session); + context.player = AI; + context +} + +fn activate(source_id: ObjectId) -> CandidateAction { + CandidateAction { + action: GameAction::ActivateAbility { + source_id, + ability_index: 0, + }, + metadata: ActionMetadata::for_actor(Some(AI), TacticalClass::Ability), + } +} + +fn ctx<'a>( + state: &'a GameState, + candidate: &'a CandidateAction, + decision: &'a AiDecisionContext, + context: &'a AiContext, + config: &'a AiConfig, +) -> PolicyContext<'a> { + PolicyContext { + state, + decision, + candidate, + ai_player: AI, + config, + context, + cast_facts: None, + search_depth: SearchDepth::Root, + } +} + +fn score_of(verdict: PolicyVerdict) -> (f64, PolicyReason) { + match verdict { + PolicyVerdict::Score { delta, reason } => (delta, reason), + PolicyVerdict::Reject { reason } => panic!("unexpected Reject: {reason:?}"), + } +} + +// ─── activation ────────────────────────────────────────────────────────────── + +#[test] +fn activation_opts_out_below_floor() { + let mut features = DeckFeatures::default(); + features.cycling.commitment = CYCLING_PAYOFF_FLOOR - 0.01; + assert!(CyclingPayoffPolicy + .activation(&features, &state(), AI) + .is_none()); +} + +#[test] +fn activation_opts_in_above_floor() { + let mut features = DeckFeatures::default(); + features.cycling.commitment = 0.9; + assert_eq!( + CyclingPayoffPolicy.activation(&features, &state(), AI), + Some(0.9) + ); +} + +// ─── verdict ───────────────────────────────────────────────────────────────── + +#[test] +fn rewards_cycling_with_an_active_engine() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source = cycler(&mut st); + let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!( + delta > 0.0, + "cycling into an engine must be rewarded, got {delta}" + ); +} + +#[test] +fn neutral_without_an_engine_on_board() { + let config = AiConfig::default(); + let mut st = state(); + // Engine known to the deck, but none is on the battlefield. + let source = cycler(&mut st); + let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +#[test] +fn neutral_for_a_non_cycling_action() { + let config = AiConfig::default(); + let st = state(); + let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + // A cast candidate is not an activated ability at all. + let candidate = CandidateAction { + action: GameAction::ActivateAbility { + source_id: ObjectId(999), + ability_index: 0, + }, + metadata: ActionMetadata::for_actor(Some(AI), TacticalClass::Ability), + }; + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_na"); + assert_eq!(delta, 0.0); +} + +// ─── production seam (registry routing) ───────────────────────────────────── + +#[test] +fn registry_registers_the_policy() { + assert!(PolicyRegistry::default().has_policy(PolicyId::CyclingPayoff)); +} + +/// End-to-end: an `ActivateAbility` on a cycler classifies to +/// `DecisionKind::ActivateAbility`, the policy declares that kind and clears its +/// activation floor, and the engine-active reward comes out of the registry. +#[test] +fn registry_routes_cycling_activation_to_the_policy() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source = cycler(&mut st); + let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = PolicyRegistry::default() + .verdicts(&ctx(&st, &candidate, &decision, &context, &config)) + .into_iter() + .find(|(id, _)| *id == PolicyId::CyclingPayoff) + .map(|(_, v)| score_of(v)) + .expect("the cycling activation must reach the policy through the registry"); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0, "routed reward must be positive, got {delta}"); +} diff --git a/crates/phase-ai/src/policies/tests/mod.rs b/crates/phase-ai/src/policies/tests/mod.rs index a0b59fa5f5..f0bde9e0a8 100644 --- a/crates/phase-ai/src/policies/tests/mod.rs +++ b/crates/phase-ai/src/policies/tests/mod.rs @@ -3,6 +3,7 @@ pub mod activation_marker_lint; pub mod artifact_synergy; pub mod blink_payoff; +pub mod cycling_payoff; pub mod devotion; pub mod effect_classify_snapshot; pub mod enchantments_payoff; From 1bd0d37492a1f3449f26041a99ae9710b9d6ced3 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 15:59:38 -0700 Subject: [PATCH 02/10] fix(phase-ai): detect cycling payoffs by live trigger, not permanent name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review (#6683): CyclingPayoffPolicy counted a battlefield engine solely by its name (feature.payoff_names), so it could reward cycling for a permanent that merely shares the engine's name but carries no usable "whenever you cycle" trigger. The policy now re-classifies each permanent the AI controls STRUCTURALLY against its live `trigger_definitions` (the runtime trigger authority, GameObject.trigger_definitions) via the same `is_cycle_payoff_parts` predicate detection uses — a name match is no longer sufficient. The `payoff_names` field is removed from CyclingFeature (its only consumer was the policy's identity lookup). Target legality is deliberately not checked: that would be a per-candidate `find_legal_targets` sweep and would wrongly drop no-target payoffs like Drannith Stinger. Regressions: a name-only permanent with no live trigger scores neutral; a no-target payoff still rewards. Tests: full 1581-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/features/cycling.rs | 17 +--- crates/phase-ai/src/features/tests/cycling.rs | 11 +-- .../phase-ai/src/policies/cycling_payoff.rs | 35 +++---- .../src/policies/tests/cycling_payoff.rs | 95 ++++++++++++++++--- 4 files changed, 100 insertions(+), 58 deletions(-) diff --git a/crates/phase-ai/src/features/cycling.rs b/crates/phase-ai/src/features/cycling.rs index 2ae25772c8..41b2f729fc 100644 --- a/crates/phase-ai/src/features/cycling.rs +++ b/crates/phase-ai/src/features/cycling.rs @@ -60,11 +60,6 @@ pub struct CyclingFeature { /// `0.0..=1.0` — how central cycling-as-a-payoff is to this deck. Consumed by /// `CyclingPayoffPolicy::activation` as the single scaling knob. pub commitment: f32, - /// Names of the detected payoff engines. NOT used for classification — that - /// already happened against the AST. Identity lookup only, so the policy can - /// re-find a payoff on the battlefield (`GameObject` carries no `triggers` - /// field). One entry per UNIQUE face, never per playset copy. - pub payoff_names: Vec, } /// Structural detection over each `DeckEntry`'s `CardFace` AST. @@ -76,7 +71,6 @@ pub fn detect(deck: &[DeckEntry]) -> CyclingFeature { let mut source_count = 0u32; let mut payoff_count = 0u32; let mut total_nonland = 0u32; - let mut payoff_names: Vec = Vec::new(); for entry in deck { let face = &entry.card; @@ -89,10 +83,6 @@ pub fn detect(deck: &[DeckEntry]) -> CyclingFeature { } if is_cycle_payoff_parts(&face.triggers) { payoff_count = payoff_count.saturating_add(entry.count); - // Identity list: one push per UNIQUE face, not per copy. - if !payoff_names.contains(&face.name) { - payoff_names.push(face.name.clone()); - } } } @@ -102,7 +92,6 @@ pub fn detect(deck: &[DeckEntry]) -> CyclingFeature { source_count, payoff_count, commitment, - payoff_names, } } @@ -113,8 +102,10 @@ pub(crate) fn is_cycle_source_parts(keywords: &[Keyword]) -> bool { .any(|k| matches!(k, Keyword::Cycling(_) | Keyword::Typecycling { .. })) } -/// CR 702.29c/d: the face carries a "whenever you cycle a card" engine trigger — -/// a repeatable payoff, not a one-shot self-cycle bonus. +/// CR 702.29c/d: the triggers carry a "whenever you cycle a card" engine — +/// a repeatable payoff, not a one-shot self-cycle bonus. Parts-based so it +/// classifies both a deck-time `CardFace.triggers` slice and a live +/// `GameObject.trigger_definitions` iterator (the runtime trigger authority). pub(crate) fn is_cycle_payoff_parts<'a>( triggers: impl IntoIterator, ) -> bool { diff --git a/crates/phase-ai/src/features/tests/cycling.rs b/crates/phase-ai/src/features/tests/cycling.rs index 064e7a2da9..b07fb98de7 100644 --- a/crates/phase-ai/src/features/tests/cycling.rs +++ b/crates/phase-ai/src/features/tests/cycling.rs @@ -71,7 +71,6 @@ fn empty_deck_produces_defaults() { assert_eq!(f.source_count, 0); assert_eq!(f.payoff_count, 0); assert_eq!(f.commitment, 0.0); - assert!(f.payoff_names.is_empty()); } #[test] @@ -89,10 +88,9 @@ fn detects_cycler_source() { } #[test] -fn detects_engine_payoff_and_names_it() { +fn detects_engine_payoff() { let f = detect(&[entry(engine("Astral Drift"), 3)]); assert_eq!(f.payoff_count, 3); - assert_eq!(f.payoff_names, vec!["Astral Drift".to_string()]); } /// A pure "when you cycle THIS card" self-bonus is a cyclable card with upside, @@ -122,13 +120,6 @@ fn opponent_scoped_trigger_ignored() { assert_eq!(detect(&[entry(f, 2)]).payoff_count, 0); } -/// Identity list carries one entry per UNIQUE face, never per playset copy. -#[test] -fn payoff_names_dedup_per_face() { - let f = detect(&[entry(engine("Astral Drift"), 4)]); - assert_eq!(f.payoff_names.len(), 1); -} - /// Calibration: a dedicated cycling shell (cyclers + engines) clears the floor. #[test] fn committed_cycling_deck_hits_floor() { diff --git a/crates/phase-ai/src/policies/cycling_payoff.rs b/crates/phase-ai/src/policies/cycling_payoff.rs index 1d3ff7a8ca..6f47ecb403 100644 --- a/crates/phase-ai/src/policies/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/cycling_payoff.rs @@ -18,14 +18,17 @@ //! `verdict()` runs per candidate per search node. The card-local check — the //! candidate is a `Cycling`-tagged activation — runs FIRST and rejects every //! other activation. Only a confirmed cycling activation pays for the -//! battlefield engine scan, and only in a deck whose `activation` floor is -//! already cleared. No affordability sweep, no `find_legal_targets`. +//! battlefield engine scan (a structural trigger match over each permanent's +//! live `trigger_definitions`), and only in a deck whose `activation` floor is +//! already cleared. Target legality is deliberately NOT checked — that would +//! mean a per-candidate `find_legal_targets` sweep, and it would wrongly drop +//! no-target payoffs like Drannith Stinger ("deals damage to each opponent"). use engine::types::ability::AbilityTag; use engine::types::game_state::GameState; use engine::types::player::PlayerId; -use crate::features::cycling::CYCLING_PAYOFF_FLOOR; +use crate::features::cycling::{is_cycle_payoff_parts, CYCLING_PAYOFF_FLOOR}; use crate::features::DeckFeatures; use super::context::PolicyContext; @@ -68,22 +71,10 @@ impl TacticalPolicy for CyclingPayoffPolicy { return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_na")); } - let Some(feature) = ctx - .context - .session - .features - .get(&ctx.ai_player) - .map(|f| &f.cycling) - else { - return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_na")); - }; - if feature.payoff_names.is_empty() { - return PolicyVerdict::neutral(PolicyReason::new("cycling_payoff_no_engine")); - } - - // Only now pay for the battlefield scan. Identity lookup (the sanctioned - // exempt pattern): re-find the structurally-classified engines the AI - // controls, since `GameObject` carries no `triggers` field. + // Only now pay for the battlefield scan. Re-classify each permanent the + // AI controls STRUCTURALLY against its live `trigger_definitions` (CR + // 702.29c/d) — a name match is not enough, the object must actually + // carry the "whenever you cycle" trigger to produce value. let engines = ctx .state .battlefield @@ -91,7 +82,11 @@ impl TacticalPolicy for CyclingPayoffPolicy { .filter(|id| { ctx.state.objects.get(id).is_some_and(|obj| { obj.controller == ctx.ai_player - && feature.payoff_names.iter().any(|name| name == &obj.name) + && is_cycle_payoff_parts( + obj.trigger_definitions + .iter_unchecked() + .map(|entry| &entry.definition), + ) }) }) .count(); diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index 320ab98210..d2c489cb07 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -9,6 +9,9 @@ use std::sync::Arc; use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; use engine::game::zones::create_object; +use engine::types::ability::{ + AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerDefinition, +}; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::format::FormatConfig; @@ -17,6 +20,7 @@ use engine::types::identifiers::{CardId, ObjectId}; use engine::types::keywords::{CyclingCost, Keyword}; use engine::types::mana::ManaCost; use engine::types::player::PlayerId; +use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; use crate::config::AiConfig; @@ -51,8 +55,10 @@ fn cycler(state: &mut GameState) -> ObjectId { id } -/// A payoff engine permanent the AI controls, matched by name. -fn engine_on_battlefield(state: &mut GameState) { +/// A permanent the AI controls, named `ENGINE_NAME`, that carries `trigger` +/// live `trigger_definitions` (or none when `trigger` is `None` — the name-only +/// impostor case). +fn permanent_with_trigger(state: &mut GameState, trigger: Option) { let card_id = CardId(state.next_object_id); let id = create_object( state, @@ -61,22 +67,36 @@ fn engine_on_battlefield(state: &mut GameState) { ENGINE_NAME.to_string(), Zone::Battlefield, ); - state - .objects - .get_mut(&id) - .unwrap() - .card_types - .core_types - .push(CoreType::Enchantment); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + if let Some(trigger) = trigger { + obj.trigger_definitions.push(trigger); + } } -fn session(commitment: f32, payoff_names: Vec) -> AiSession { +/// Astral Drift shape: a live "whenever you cycle or discard a card" engine. +fn engine_on_battlefield(state: &mut GameState) { + permanent_with_trigger(state, Some(cycle_trigger(TargetFilter::Any))); +} + +/// A controller-scoped `CycledOrDiscarded` trigger whose effect targets +/// `target` (use `TargetFilter::Controller` for a no-target payoff shape). +fn cycle_trigger(target: TargetFilter) -> TriggerDefinition { + TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target, + }, + )) +} + +fn session(commitment: f32) -> AiSession { let features = DeckFeatures { cycling: CyclingFeature { source_count: 18, payoff_count: 4, commitment, - payoff_names, }, ..Default::default() }; @@ -157,7 +177,7 @@ fn rewards_cycling_with_an_active_engine() { let mut st = state(); engine_on_battlefield(&mut st); let source = cycler(&mut st); - let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let context = context(&config, session(0.9)); let candidate = activate(source); let decision = AiDecisionContext { waiting_for: WaitingFor::Priority { player: AI }, @@ -178,7 +198,7 @@ fn neutral_without_an_engine_on_board() { let mut st = state(); // Engine known to the deck, but none is on the battlefield. let source = cycler(&mut st); - let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let context = context(&config, session(0.9)); let candidate = activate(source); let decision = AiDecisionContext { waiting_for: WaitingFor::Priority { player: AI }, @@ -194,7 +214,7 @@ fn neutral_without_an_engine_on_board() { fn neutral_for_a_non_cycling_action() { let config = AiConfig::default(); let st = state(); - let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let context = context(&config, session(0.9)); // A cast candidate is not an activated ability at all. let candidate = CandidateAction { action: GameAction::ActivateAbility { @@ -213,6 +233,51 @@ fn neutral_for_a_non_cycling_action() { assert_eq!(delta, 0.0); } +/// [MED review] A permanent that merely SHARES the engine's name but carries no +/// live cycle trigger must not be rewarded — detection is structural over +/// `trigger_definitions`, not name-based. +#[test] +fn name_only_impostor_without_a_live_trigger_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, None); // named "Astral Drift", no trigger + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// A no-target payoff (Drannith Stinger shape — its on-cycle effect hits each +/// opponent, choosing nothing) must still be rewarded; the policy checks the +/// live trigger, not target legality. +#[test] +fn no_target_payoff_still_rewards() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(cycle_trigger(TargetFilter::Controller))); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!( + delta > 0.0, + "no-target payoff must still reward, got {delta}" + ); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] @@ -229,7 +294,7 @@ fn registry_routes_cycling_activation_to_the_policy() { let mut st = state(); engine_on_battlefield(&mut st); let source = cycler(&mut st); - let context = context(&config, session(0.9, vec![ENGINE_NAME.to_string()])); + let context = context(&config, session(0.9)); let candidate = activate(source); let decision = AiDecisionContext { waiting_for: WaitingFor::Priority { player: AI }, From f36177e0a92b68d537208e94d2c2f741b87df158 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 16:31:55 -0700 Subject: [PATCH 03/10] test(phase-ai): lock in all-land-deck commitment boundary (no NaN) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged a potential NaN when a non-empty all-land deck reaches compute_commitment with total_nonland == 0 (NaN would bypass the `commitment < FLOOR` activation gate). commitment::density_per_60 already guards total_nonland == 0 → 0.0, and geometric_mean returns 0.0 for any non-positive pillar, so commitment is a clean 0.0. Adds an all-land regression (including a cycling land, so source_count > 0 while total_nonland == 0) asserting not-NaN and == 0.0. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/features/tests/cycling.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/phase-ai/src/features/tests/cycling.rs b/crates/phase-ai/src/features/tests/cycling.rs index b07fb98de7..916ea797da 100644 --- a/crates/phase-ai/src/features/tests/cycling.rs +++ b/crates/phase-ai/src/features/tests/cycling.rs @@ -163,3 +163,20 @@ fn commitment_clamps_to_one() { let deck = vec![entry(cycler("Cyc"), 40), entry(engine("Astral Drift"), 20)]; assert!(detect(&deck).commitment <= 1.0); } + +/// Boundary: a non-empty all-land deck (including cycling lands) has +/// `total_nonland == 0`. `commitment::density_per_60` guards that to `0.0`, so +/// commitment is a clean `0.0` — never `NaN`, which (since `NaN < FLOOR` is +/// false) would otherwise slip past `CyclingPayoffPolicy::activation`. +#[test] +fn all_land_deck_is_zero_not_nan() { + let mut cycling_land = face("Desert of the True", CoreType::Land); + cycling_land.keywords = vec![Keyword::Cycling(CyclingCost::Mana(ManaCost::generic(2)))]; + let deck = vec![ + entry(cycling_land, 20), + entry(face("Plains", CoreType::Land), 20), + ]; + let commitment = detect(&deck).commitment; + assert!(!commitment.is_nan(), "all-land deck must not produce NaN"); + assert_eq!(commitment, 0.0); +} From 9d07a668cea7af7b59e45583ce8e3da36748de8f Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 17:41:30 -0700 Subject: [PATCH 04/10] fix(phase-ai): gate cycling payoff on live per-turn trigger eligibility Round-2 review (#6683): the payoff scan matched an engine only by its structural trigger shape, so a rate-limited engine (Valiant Rescuer's "whenever you cycle or discard a card ... only once each turn") still earned a bonus on the second cycle even though its trigger cannot fire again. The battlefield scan now pairs the structural classifier with live firing eligibility per trigger entry: a OncePerTurn / OncePerGame engine that has already fired (per the engine's authoritative `triggers_fired_this_turn` / `triggers_fired_this_game` ledgers, keyed via `GameObject:: trigger_definition_ref`) no longer counts. Constraints whose eligibility is a value nuance rather than a hard on/off are treated as live. `trigger_is_cycle_payoff` is exposed as `is_cycle_payoff_trigger` so the per-entry check can pair shape with eligibility. Regressions: a once-per-turn engine already fired this turn scores neutral; the same engine unfired still rewards. Tests: 35 cycling tests + clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/features/cycling.rs | 6 +- .../phase-ai/src/policies/cycling_payoff.rs | 35 ++++++++-- .../src/policies/tests/cycling_payoff.rs | 68 ++++++++++++++++++- 3 files changed, 100 insertions(+), 9 deletions(-) diff --git a/crates/phase-ai/src/features/cycling.rs b/crates/phase-ai/src/features/cycling.rs index 41b2f729fc..9d099cbd0b 100644 --- a/crates/phase-ai/src/features/cycling.rs +++ b/crates/phase-ai/src/features/cycling.rs @@ -109,10 +109,12 @@ pub(crate) fn is_cycle_source_parts(keywords: &[Keyword]) -> bool { pub(crate) fn is_cycle_payoff_parts<'a>( triggers: impl IntoIterator, ) -> bool { - triggers.into_iter().any(trigger_is_cycle_payoff) + triggers.into_iter().any(is_cycle_payoff_trigger) } -fn trigger_is_cycle_payoff(t: &TriggerDefinition) -> bool { +/// Single-trigger structural classifier (mode + scope), exposed so the policy +/// can pair it with live per-turn firing eligibility per trigger entry. +pub(crate) fn is_cycle_payoff_trigger(t: &TriggerDefinition) -> bool { // 1. Mode fires on a cycle event (CR 702.29c/d). if !matches!(t.mode, TriggerMode::Cycled | TriggerMode::CycledOrDiscarded) { return false; diff --git a/crates/phase-ai/src/policies/cycling_payoff.rs b/crates/phase-ai/src/policies/cycling_payoff.rs index 6f47ecb403..90f90e9aef 100644 --- a/crates/phase-ai/src/policies/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/cycling_payoff.rs @@ -28,7 +28,10 @@ use engine::types::ability::AbilityTag; use engine::types::game_state::GameState; use engine::types::player::PlayerId; -use crate::features::cycling::{is_cycle_payoff_parts, CYCLING_PAYOFF_FLOOR}; +use engine::game::game_object::GameObject; +use engine::types::ability::{TriggerConstraint, TriggerEntry}; + +use crate::features::cycling::{is_cycle_payoff_trigger, CYCLING_PAYOFF_FLOOR}; use crate::features::DeckFeatures; use super::context::PolicyContext; @@ -82,11 +85,10 @@ impl TacticalPolicy for CyclingPayoffPolicy { .filter(|id| { ctx.state.objects.get(id).is_some_and(|obj| { obj.controller == ctx.ai_player - && is_cycle_payoff_parts( - obj.trigger_definitions - .iter_unchecked() - .map(|entry| &entry.definition), - ) + && obj.trigger_definitions.iter_unchecked().any(|entry| { + is_cycle_payoff_trigger(&entry.definition) + && trigger_still_fireable(ctx.state, obj, entry) + }) }) }) .count(); @@ -104,3 +106,24 @@ impl TacticalPolicy for CyclingPayoffPolicy { ) } } + +/// CR 603.4 + CR 603.2: a rate-limited engine that has already fired this +/// turn/game cannot fire again, so cycling into it earns nothing more. Consults +/// the engine's authoritative fired-trigger ledgers rather than re-deriving +/// eligibility. Constraints this policy does not model (their eligibility is a +/// value nuance, not a hard on/off) are treated as live. +fn trigger_still_fireable( + state: &engine::types::game_state::GameState, + obj: &GameObject, + entry: &TriggerEntry, +) -> bool { + match &entry.definition.constraint { + Some(TriggerConstraint::OncePerTurn) => !state + .triggers_fired_this_turn + .contains(&obj.trigger_definition_ref(entry)), + Some(TriggerConstraint::OncePerGame) => !state + .triggers_fired_this_game + .contains(&obj.trigger_definition_ref(entry)), + _ => true, + } +} diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index d2c489cb07..ce29d19202 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -10,7 +10,8 @@ use std::sync::Arc; use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; use engine::game::zones::create_object; use engine::types::ability::{ - AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerDefinition, + AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerConstraint, + TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -278,6 +279,71 @@ fn no_target_payoff_still_rewards() { ); } +/// A once-per-turn "whenever you cycle" engine (Valiant Rescuer shape). +fn once_per_turn_engine(state: &mut GameState) -> ObjectId { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + ENGINE_NAME.to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.trigger_definitions + .push(cycle_trigger(TargetFilter::Any).constraint(TriggerConstraint::OncePerTurn)); + id +} + +/// [MED review] A once-per-turn engine that has already fired this turn cannot +/// fire again (CR 603.4), so a second cycle earns nothing — the policy consults +/// the fired-trigger ledger, not just the structural trigger shape. +#[test] +fn rate_limited_engine_already_fired_this_turn_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + let engine_id = once_per_turn_engine(&mut st); + // Mark its trigger as already fired this turn via the ledger authority. + let key = { + let obj = st.objects.get(&engine_id).unwrap(); + let entry = obj.trigger_definitions.iter_unchecked().next().unwrap(); + obj.trigger_definition_ref(entry) + }; + st.triggers_fired_this_turn.insert(key); + + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: the same once-per-turn engine that has NOT fired yet still rewards. +#[test] +fn rate_limited_engine_not_yet_fired_rewards() { + let config = AiConfig::default(); + let mut st = state(); + once_per_turn_engine(&mut st); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0, "an unfired once-per-turn engine still rewards"); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From 6952cb6627f288ad7134d45788cd440e90044139 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 18:15:55 -0700 Subject: [PATCH 05/10] fix(phase-ai): complete cycling payoff trigger-eligibility (exhaustive constraints) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parity with the #6688 review: trigger_still_fireable is now exhaustive over TriggerConstraint (no wildcard). OncePerTurn/Game via the fired-trigger ledgers; OnlyDuringYourTurn / OnlyDuringOpponentsTurn / OnlyDuringYourMainPhase via active_player + phase; and the event/count-dependent constraints (MaxTimesPerTurn, NthSpell/NthDraw, OncePerOpponentPerTurn, AtClassLevel, EventSourceControlledBy) conservatively NOT confirmed so the payoff is never over-credited. Regressions: once-per-game engine already fired -> neutral; OnlyDuringYourTurn engine on the opponent's turn -> neutral. cargo test -p phase-ai --lib cycling_payoff — 13 pass; clippy -p phase-ai --lib --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- .../phase-ai/src/policies/cycling_payoff.rs | 37 ++++++++--- .../src/policies/tests/cycling_payoff.rs | 64 +++++++++++++++++++ 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/crates/phase-ai/src/policies/cycling_payoff.rs b/crates/phase-ai/src/policies/cycling_payoff.rs index 90f90e9aef..2d5b5ddd2a 100644 --- a/crates/phase-ai/src/policies/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/cycling_payoff.rs @@ -30,6 +30,7 @@ use engine::types::player::PlayerId; use engine::game::game_object::GameObject; use engine::types::ability::{TriggerConstraint, TriggerEntry}; +use engine::types::phase::Phase; use crate::features::cycling::{is_cycle_payoff_trigger, CYCLING_PAYOFF_FLOOR}; use crate::features::DeckFeatures; @@ -107,23 +108,41 @@ impl TacticalPolicy for CyclingPayoffPolicy { } } -/// CR 603.4 + CR 603.2: a rate-limited engine that has already fired this -/// turn/game cannot fire again, so cycling into it earns nothing more. Consults -/// the engine's authoritative fired-trigger ledgers rather than re-deriving -/// eligibility. Constraints this policy does not model (their eligibility is a -/// value nuance, not a hard on/off) are treated as live. +/// Whether `obj`'s cycling-payoff trigger `entry` could still fire this turn. +/// Exhaustive over `TriggerConstraint` (no wildcard): the once/timing limits are +/// evaluated against authoritative state (fired-trigger ledgers, `active_player`, +/// phase); event/count-dependent constraints the policy can't confirm at decision +/// time are treated as NOT fireable so the payoff is never over-credited. fn trigger_still_fireable( state: &engine::types::game_state::GameState, obj: &GameObject, entry: &TriggerEntry, ) -> bool { - match &entry.definition.constraint { - Some(TriggerConstraint::OncePerTurn) => !state + let Some(constraint) = &entry.definition.constraint else { + return true; // no constraint — always fireable + }; + match constraint { + // CR 603.4 / CR 603.2: already-consumed "once" limits. + TriggerConstraint::OncePerTurn => !state .triggers_fired_this_turn .contains(&obj.trigger_definition_ref(entry)), - Some(TriggerConstraint::OncePerGame) => !state + TriggerConstraint::OncePerGame => !state .triggers_fired_this_game .contains(&obj.trigger_definition_ref(entry)), - _ => true, + // Turn/phase timing — evaluable from turn state alone. + TriggerConstraint::OnlyDuringYourTurn => state.active_player == obj.controller, + TriggerConstraint::OnlyDuringOpponentsTurn => state.active_player != obj.controller, + TriggerConstraint::OnlyDuringYourMainPhase => { + state.active_player == obj.controller + && matches!(state.phase, Phase::PreCombatMain | Phase::PostCombatMain) + } + // Event- or count-dependent: not confirmable at decision time, so never + // over-credit the payoff. + TriggerConstraint::MaxTimesPerTurn { .. } + | TriggerConstraint::NthSpellThisTurn { .. } + | TriggerConstraint::NthDrawThisTurn { .. } + | TriggerConstraint::OncePerOpponentPerTurn + | TriggerConstraint::AtClassLevel { .. } + | TriggerConstraint::EventSourceControlledBy { .. } => false, } } diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index ce29d19202..db0175a0d3 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -344,6 +344,70 @@ fn rate_limited_engine_not_yet_fired_rewards() { assert!(delta > 0.0, "an unfired once-per-turn engine still rewards"); } +/// An engine trigger with `constraint` on the AI's own permanent. +fn engine_with_constraint(state: &mut GameState, constraint: TriggerConstraint) -> ObjectId { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + ENGINE_NAME.to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.trigger_definitions + .push(cycle_trigger(TargetFilter::Any).constraint(constraint)); + id +} + +/// A once-per-game engine already fired this game earns nothing more. +#[test] +fn once_per_game_engine_already_fired_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + let engine_id = engine_with_constraint(&mut st, TriggerConstraint::OncePerGame); + let key = { + let obj = st.objects.get(&engine_id).unwrap(); + let entry = obj.trigger_definitions.iter_unchecked().next().unwrap(); + obj.trigger_definition_ref(entry) + }; + st.triggers_fired_this_game.insert(key); + + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// An `OnlyDuringYourTurn` engine on the opponent's turn cannot fire, so cycling +/// at instant speed during their turn earns nothing. +#[test] +fn only_during_your_turn_engine_is_neutral_off_turn() { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = PlayerId(1); + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringYourTurn); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From e5d0635f7ae361a903bf9efff4a9482186840a0e Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 23:35:26 -0700 Subject: [PATCH 06/10] fix(phase-ai): engine-owned trigger fireability for cycling payoff (round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both [MED] blockers on head 6952cb66 by moving live cycling-payoff eligibility into a single engine authority that covers the COMPLETE trigger condition, rather than a policy-local approximation of selected variants. Engine (single authority): - `triggers::hypothetical_trigger_fireable(state, source, entry)` — the one place a policy asks "is this on-battlefield payoff live?". It reuses the live pipeline's own `check_trigger_constraint_with_ref` (now `event: Option<&_>`; `Some` = real evaluation, `None` = hypothetical, so event-dependent constraints report NOT-satisfied instead of guessing), conservatively rejects an intervening-if `condition` (CR 603.4), and preflights execution target legality (CR 603.3d — a trigger with no legal choice for a required target is removed from the stack rather than producing its effect). - `ability_utils::execute_targets_satisfiable` answers only from CONFIRMED legality. The cheap single-slot check is a guard; every shape it cannot decide falls through to `has_legal_target_assignment_for_ability`, the same full legal-assignment authority the target walk uses. It builds the ability with `build_resolved_from_def` (CR 113.1a) so a sub-ability chain's own target slots are preflighted too, not just the root effect's. Policy (cycling_payoff.rs): - Deleted the policy-local partial `trigger_still_fireable`. The live scan is now `is_cycle_payoff_trigger && hypothetical_trigger_fireable`, so the policy holds no eligibility rules of its own. - This fixes both findings at once: a target-required payoff with no legal target no longer scores, and `MaxTimesPerTurn` is no longer rejected categorically — the engine permits it while below the configured cap. Tests (external, no cfg(test) in source): - Target legality: no-legal-target neutral / legal-target rewarded. - MaxTimesPerTurn: below-cap rewarded / at-cap neutral. - Multi-slot: no-legal-target neutral / legal-targets rewarded, the latter pinning the fixture's two-slot shape so the negative case cannot silently degrade into a single-slot test. - `permanent_with_trigger` now returns its `ObjectId` so the shape assertion addresses the fixture directly, keeping the `no_name_matching` architectural lint clean. CR 603.2-603.4, CR 603.3d, CR 113.1a grep-verified against docs/MagicCompRules.txt. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/ability_utils.rs | 36 +++ crates/engine/src/game/triggers.rs | 63 ++++- .../phase-ai/src/policies/cycling_payoff.rs | 55 +---- .../src/policies/tests/cycling_payoff.rs | 231 +++++++++++++++++- 4 files changed, 328 insertions(+), 57 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 72ac21e955..92174821a6 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1311,6 +1311,42 @@ pub fn simple_legal_target_assignment_exists_for_ability( )) } +/// CR 603.3d: could `execute` — a trigger's ability, resolving from `source` — +/// either need no target at all, or find a legal target right now? A +/// mandatory-target trigger with no legal choice is removed from the stack +/// rather than producing its effect, so a payoff-eligibility preflight must not +/// credit it. +/// +/// Answers only from *confirmed* legality — never from an "unknown" shape. The +/// cheap single-slot check is tried first as a guard; every shape it cannot +/// decide (multi-slot, relative-controller, distribution, `PairWith`, …) falls +/// through to [`has_legal_target_assignment_for_ability`], the same full +/// legal-assignment authority the interactive target walk uses, so a +/// two-mandatory-target trigger with no legal assignment is correctly rejected. +/// A slot-building error leaves legality unproven and is likewise not credited. +pub fn execute_targets_satisfiable( + state: &GameState, + source: &crate::game::game_object::GameObject, + execute: &AbilityDefinition, +) -> bool { + // CR 113.1a: build the ability exactly as the live trigger pipeline does + // (`build_triggered_ability_from_context`), so a sub-ability chain's own + // target slots are preflighted too — not just the root effect's. + let resolved = build_resolved_from_def(execute, source.id, source.controller); + if target_slot_specs(state, &resolved).is_empty() { + return true; // the effect requires no target + } + // Cheap guard: `Some(false)` = a mandatory target with no legal choice; + // `Some(true)` = legal or optional; `None` = a shape this cheap check + // cannot decide, which the full authority below resolves exactly. + if let Some(decided) = simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]) + { + return decided; + } + build_target_slots(state, &resolved) + .is_ok_and(|slots| has_legal_target_assignment_for_ability(state, &resolved, &slots, &[])) +} + /// CR 115.1 + CR 701.9b: Resolve a `Random`-mode ability's target slots by /// uniformly choosing from each slot's legal-target set using the engine's /// seeded RNG (`state.rng`). The game (not the controller) makes the selection; diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index f23b57f5b7..7d696643d2 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -1450,7 +1450,7 @@ fn collect_matching_triggers_inner( definition_ref.as_ref(), Some(&source_context), controller, - event, + Some(event), ) { continue; } @@ -2629,7 +2629,7 @@ fn collect_latched_batched_zone_triggers( Some(&latched.definition_ref), Some(source_context), source_context.lki.controller, - event, + Some(event), ) || !latched .definition @@ -8006,13 +8006,17 @@ fn delayed_zone_change_filter_matches( /// /// `event` is the triggering event — needed by `NthSpellThisTurn` to identify /// the caster and count their per-player spell total (not the global count). +/// `event` is `Some` for a real trigger evaluation and `None` for a hypothetical +/// preflight (e.g. an AI payoff-eligibility query). In the hypothetical case the +/// event-dependent constraints — which can only be judged against a concrete +/// triggering event — conservatively report NOT satisfied rather than guess. fn check_trigger_constraint_with_ref( state: &GameState, trig_def: &TriggerDefinition, definition_ref: Option<&TriggerDefinitionRef>, source_context: Option<&TriggerSourceContext>, controller: PlayerId, - event: &GameEvent, + event: Option<&GameEvent>, ) -> bool { use crate::types::ability::TriggerConstraint; @@ -8038,7 +8042,7 @@ fn check_trigger_constraint_with_ref( // CR 603.2: The trigger event only matches the first life-loss event // during that opponent's own turn. let opponent_id = match event { - GameEvent::LifeChanged { player_id, .. } => *player_id, + Some(GameEvent::LifeChanged { player_id, .. }) => *player_id, _ => return false, }; if opponent_id == controller || state.active_player != opponent_id { @@ -8064,10 +8068,10 @@ fn check_trigger_constraint_with_ref( controller: ctrl_ref, } => { let event_source = match event { - GameEvent::Discarded { + Some(GameEvent::Discarded { source_id: Some(source_id), .. - } => *source_id, + }) => *source_id, _ => return false, }; let Some(event_source_controller) = state @@ -8091,7 +8095,7 @@ fn check_trigger_constraint_with_ref( // When `filter` contains `TypeFilter::Non(Creature)`, use the noncreature counter. TriggerConstraint::NthSpellThisTurn { n, filter } => { let caster = match event { - GameEvent::SpellCast { controller: c, .. } => *c, + Some(GameEvent::SpellCast { controller: c, .. }) => *c, _ => return false, }; let spells = state.spells_cast_this_turn_by_player.get(&caster); @@ -8127,7 +8131,7 @@ fn check_trigger_constraint_with_ref( // rather than the final per-turn count after a multi-card draw batch. TriggerConstraint::NthDrawThisTurn { n } => { let nth_in_turn = match event { - GameEvent::CardDrawn { nth_in_turn, .. } => *nth_in_turn, + Some(GameEvent::CardDrawn { nth_in_turn, .. }) => *nth_in_turn, _ => return false, }; nth_in_turn == *n @@ -8148,6 +8152,47 @@ fn check_trigger_constraint_with_ref( } } +/// CR 603.2-603.4 + CR 603.3d: could `entry`'s trigger on `source` still fire +/// AND resolve to an effect if its triggering event happened right now? The +/// single authority an AI policy uses to ask "is this on-battlefield payoff +/// live?" — reusing the same constraint check the live trigger pipeline runs. +/// +/// Conservative by construction: an intervening-if `condition` (CR 603.4, not +/// evaluated in this preflight) and any event-dependent constraint whose +/// triggering event is unknown at this decision point are treated as NOT +/// established, so a payoff is never credited value it cannot actually produce. +pub fn hypothetical_trigger_fireable( + state: &GameState, + source: &GameObject, + entry: &TriggerEntry, +) -> bool { + let def = &entry.definition; + // CR 603.4 intervening-if: not preflighted here — treat a conditional + // trigger as not-live rather than assume it fires. + if def.condition.is_some() { + return false; + } + let definition_ref = source.trigger_definition_ref(entry); + // CR 603.2-603.4: the trigger's own constraint, in hypothetical (no-event) + // mode — the shared authority the live pipeline also uses. + if !check_trigger_constraint_with_ref( + state, + def, + Some(&definition_ref), + None, + source.controller, + None, + ) { + return false; + } + // CR 603.3d: a mandatory-target execute with no legal target is removed from + // the stack rather than producing its effect. + match def.execute.as_deref() { + Some(execute) => super::ability_utils::execute_targets_satisfiable(state, source, execute), + None => true, + } +} + /// Evaluates the cast-payment facts carried either by the event subject or by /// the exact trigger source. Keeping this value-level avoids a source-id /// fallback that could bind a later incarnation during an intervening-if @@ -9844,7 +9889,7 @@ fn check_trigger_constraint( .map(|source| trigger_source_context_for_latch(state, source)) .as_ref(), controller, - event, + Some(event), ) } diff --git a/crates/phase-ai/src/policies/cycling_payoff.rs b/crates/phase-ai/src/policies/cycling_payoff.rs index 2d5b5ddd2a..82550feeff 100644 --- a/crates/phase-ai/src/policies/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/cycling_payoff.rs @@ -20,17 +20,19 @@ //! other activation. Only a confirmed cycling activation pays for the //! battlefield engine scan (a structural trigger match over each permanent's //! live `trigger_definitions`), and only in a deck whose `activation` floor is -//! already cleared. Target legality is deliberately NOT checked — that would -//! mean a per-candidate `find_legal_targets` sweep, and it would wrongly drop -//! no-target payoffs like Drannith Stinger ("deals damage to each opponent"). +//! already cleared. Each structurally matching engine is then confirmed LIVE by +//! [`engine::game::triggers::hypothetical_trigger_fireable`] — the engine-owned +//! authority for "could this trigger still fire AND resolve to an effect?", +//! reusing the live pipeline's own constraint check plus a CR 603.3d target +//! preflight (CR 603.2-603.4). The policy holds no eligibility rules of its own. +//! A no-target payoff like Drannith Stinger ("deals damage to each opponent") +//! still qualifies: it has no target slot to satisfy. use engine::types::ability::AbilityTag; use engine::types::game_state::GameState; use engine::types::player::PlayerId; -use engine::game::game_object::GameObject; -use engine::types::ability::{TriggerConstraint, TriggerEntry}; -use engine::types::phase::Phase; +use engine::game::triggers::hypothetical_trigger_fireable; use crate::features::cycling::{is_cycle_payoff_trigger, CYCLING_PAYOFF_FLOOR}; use crate::features::DeckFeatures; @@ -88,7 +90,7 @@ impl TacticalPolicy for CyclingPayoffPolicy { obj.controller == ctx.ai_player && obj.trigger_definitions.iter_unchecked().any(|entry| { is_cycle_payoff_trigger(&entry.definition) - && trigger_still_fireable(ctx.state, obj, entry) + && hypothetical_trigger_fireable(ctx.state, obj, entry) }) }) }) @@ -107,42 +109,3 @@ impl TacticalPolicy for CyclingPayoffPolicy { ) } } - -/// Whether `obj`'s cycling-payoff trigger `entry` could still fire this turn. -/// Exhaustive over `TriggerConstraint` (no wildcard): the once/timing limits are -/// evaluated against authoritative state (fired-trigger ledgers, `active_player`, -/// phase); event/count-dependent constraints the policy can't confirm at decision -/// time are treated as NOT fireable so the payoff is never over-credited. -fn trigger_still_fireable( - state: &engine::types::game_state::GameState, - obj: &GameObject, - entry: &TriggerEntry, -) -> bool { - let Some(constraint) = &entry.definition.constraint else { - return true; // no constraint — always fireable - }; - match constraint { - // CR 603.4 / CR 603.2: already-consumed "once" limits. - TriggerConstraint::OncePerTurn => !state - .triggers_fired_this_turn - .contains(&obj.trigger_definition_ref(entry)), - TriggerConstraint::OncePerGame => !state - .triggers_fired_this_game - .contains(&obj.trigger_definition_ref(entry)), - // Turn/phase timing — evaluable from turn state alone. - TriggerConstraint::OnlyDuringYourTurn => state.active_player == obj.controller, - TriggerConstraint::OnlyDuringOpponentsTurn => state.active_player != obj.controller, - TriggerConstraint::OnlyDuringYourMainPhase => { - state.active_player == obj.controller - && matches!(state.phase, Phase::PreCombatMain | Phase::PostCombatMain) - } - // Event- or count-dependent: not confirmable at decision time, so never - // over-credit the payoff. - TriggerConstraint::MaxTimesPerTurn { .. } - | TriggerConstraint::NthSpellThisTurn { .. } - | TriggerConstraint::NthDrawThisTurn { .. } - | TriggerConstraint::OncePerOpponentPerTurn - | TriggerConstraint::AtClassLevel { .. } - | TriggerConstraint::EventSourceControlledBy { .. } => false, - } -} diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index db0175a0d3..89904aecac 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -8,10 +8,11 @@ use std::sync::Arc; use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; +use engine::game::ability_utils::{build_resolved_from_def, build_target_slots}; use engine::game::zones::create_object; use engine::types::ability::{ AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerConstraint, - TriggerDefinition, + TriggerDefinition, TypedFilter, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -59,7 +60,7 @@ fn cycler(state: &mut GameState) -> ObjectId { /// A permanent the AI controls, named `ENGINE_NAME`, that carries `trigger` /// live `trigger_definitions` (or none when `trigger` is `None` — the name-only /// impostor case). -fn permanent_with_trigger(state: &mut GameState, trigger: Option) { +fn permanent_with_trigger(state: &mut GameState, trigger: Option) -> ObjectId { let card_id = CardId(state.next_object_id); let id = create_object( state, @@ -73,6 +74,7 @@ fn permanent_with_trigger(state: &mut GameState, trigger: Option AbilityDefinition { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 2 }, + target: TargetFilter::Typed(TypedFilter::creature()), + damage_source: None, + excess: None, + }, + ) +} + +/// A "whenever you cycle, deal 2 damage to TARGET creature" payoff — value +/// depends on a legal creature target existing (CR 603.3d). Distinct from the +/// no-target `cycle_trigger` (a `Draw` needs no target slot). +fn targeted_cycle_trigger() -> TriggerDefinition { + TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(damage_target_creature()) +} + +/// The same payoff with a sub-ability chain, so the execute carries TWO +/// mandatory creature target slots — the shape the cheap single-slot check +/// cannot decide, which must fall through to the engine's full +/// legal-assignment authority rather than being assumed satisfiable. +fn two_target_cycle_trigger() -> TriggerDefinition { + TriggerDefinition::new(TriggerMode::CycledOrDiscarded) + .execute(damage_target_creature().sub_ability(damage_target_creature())) +} + +/// How many target slots the engine builds for the payoff permanent's trigger +/// execute — used to assert a fixture's target SHAPE, so a slot-count change +/// surfaces as a precondition failure instead of a silently weakened test. +fn engine_execute_target_slot_count(state: &GameState, engine_id: ObjectId) -> usize { + let obj = state + .objects + .get(&engine_id) + .expect("payoff permanent must be on the battlefield"); + let entry = obj + .trigger_definitions + .iter_unchecked() + .next() + .expect("payoff permanent must carry a trigger"); + let execute = entry + .definition + .execute + .as_deref() + .expect("payoff trigger must have an execute"); + let resolved = build_resolved_from_def(execute, obj.id, obj.controller); + build_target_slots(state, &resolved) + .expect("target slots must build") + .len() +} + +/// Puts an opponent creature on the battlefield — a legal "target creature". +fn add_opponent_creature(state: &mut GameState) { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + PlayerId(1), + "Grizzly Bears".to_string(), + Zone::Battlefield, + ); + state + .objects + .get_mut(&id) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); +} + +/// CR 603.3d: a mandatory-target cycling payoff with no legal target on the board +/// cannot resolve to an effect, so the engine's `hypothetical_trigger_fireable` +/// target-legality preflight rejects it and no bonus is awarded. `permanent_with_trigger` +/// builds an Enchantment, so an empty board truly has no creature to target. +#[test] +fn targeted_payoff_with_no_legal_target_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(targeted_cycle_trigger())); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: once a legal creature target exists, the same targeted payoff is live +/// and cycling is rewarded. +#[test] +fn targeted_payoff_with_a_legal_target_rewards() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(targeted_cycle_trigger())); + add_opponent_creature(&mut st); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// CR 603.3d, multi-slot shape: a payoff whose execute needs TWO mandatory +/// creature targets earns nothing on a creatureless board. This is the shape +/// the cheap single-slot check reports as UNDECIDABLE; answering that +/// optimistically — as an `unwrap_or(true)` would — credits a trigger that +/// cannot resolve to an effect. Legality is instead settled by the engine's own +/// target authority, which reports no legal targets for the mandatory slots. +#[test] +fn multi_target_payoff_with_no_legal_target_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(two_target_cycle_trigger())); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control for the pair above: with creatures available the identical two-target +/// payoff is live and rewarded — so the multi-slot path RESOLVES legality rather +/// than rejecting every shape it cannot cheaply decide. Also pins the fixture's +/// target shape, so a slot-count change surfaces here as a precondition failure +/// instead of silently degrading the negative case into a single-slot test. +#[test] +fn multi_target_payoff_with_legal_targets_rewards() { + let config = AiConfig::default(); + let mut st = state(); + let engine_id = permanent_with_trigger(&mut st, Some(two_target_cycle_trigger())); + add_opponent_creature(&mut st); + add_opponent_creature(&mut st); + assert_eq!( + engine_execute_target_slot_count(&st, engine_id), + 2, + "fixture must carry TWO mandatory target slots" + ); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A `MaxTimesPerTurn { max }` engine that has fired fewer than `max` times this +/// turn can still fire, so cycling is rewarded — the shared engine authority reads +/// the live `trigger_fire_counts_this_turn` ledger rather than rejecting the +/// constraint categorically. +#[test] +fn max_times_per_turn_below_cap_rewards() { + let config = AiConfig::default(); + let mut st = state(); + let engine_id = engine_with_constraint(&mut st, TriggerConstraint::MaxTimesPerTurn { max: 2 }); + let key = { + let obj = st.objects.get(&engine_id).unwrap(); + let entry = obj.trigger_definitions.iter_unchecked().next().unwrap(); + obj.trigger_definition_ref(entry) + }; + st.trigger_fire_counts_this_turn.insert(key, 1); // 1 < 2 → can still fire + + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: the same engine that has already fired `max` times this turn cannot +/// fire again, so cycling earns nothing. +#[test] +fn max_times_per_turn_at_cap_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + let engine_id = engine_with_constraint(&mut st, TriggerConstraint::MaxTimesPerTurn { max: 2 }); + let key = { + let obj = st.objects.get(&engine_id).unwrap(); + let entry = obj.trigger_definitions.iter_unchecked().next().unwrap(); + obj.trigger_definition_ref(entry) + }; + st.trigger_fire_counts_this_turn.insert(key, 2); // 2 == max → exhausted + + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From ee0c9c0260a15b5af29325899da65088395f3ac2 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 00:49:00 -0700 Subject: [PATCH 07/10] fix(engine): preserve source-sensitive constraints in hypothetical trigger authority (round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' AtClassLevel blocker. The shared `hypothetical_trigger_fireable` authority passed `source_context: None`, so the `AtClassLevel` constraint arm — which reads the class level exclusively from the source context (CR 716) — rejected every class-level cycling payoff as unavailable, even at the required level. Fix: build and pass `trigger_source_context_for_latch(state, source)` (a current snapshot of the source) into the shared constraint check, so source-sensitive constraints read real source state. Only the triggering EVENT stays withheld (`None`), so genuinely event-dependent constraints remain conservatively not-fireable. New coverage: `at_class_level_engine_at_required_level_rewards` (level 2, needs level 2 → live) and `at_class_level_engine_at_wrong_level_is_neutral` (level 1, needs level 2 → not live). Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/triggers.rs | 9 ++- .../src/policies/tests/cycling_payoff.rs | 66 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 7d696643d2..521b9a7137 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -8174,12 +8174,17 @@ pub fn hypothetical_trigger_fireable( } let definition_ref = source.trigger_definition_ref(entry); // CR 603.2-603.4: the trigger's own constraint, in hypothetical (no-event) - // mode — the shared authority the live pipeline also uses. + // mode — the shared authority the live pipeline also uses. The source + // context is supplied (a current snapshot of `source`) so SOURCE-sensitive + // constraints like `AtClassLevel` (CR 716) read the real class level; only + // the triggering EVENT is withheld (`None`), so event-dependent constraints + // stay conservatively not-fireable. + let source_context = trigger_source_context_for_latch(state, source); if !check_trigger_constraint_with_ref( state, def, Some(&definition_ref), - None, + Some(&source_context), source.controller, None, ) { diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index 89904aecac..71918f4859 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -635,6 +635,72 @@ fn max_times_per_turn_at_cap_is_neutral() { assert_eq!(delta, 0.0); } +/// A Class-enchantment cycling engine at `class_level` whose level-gated +/// "whenever you cycle" payoff fires only while the Class is at `required_level` +/// (CR 716). The shared hypothetical authority reads the level from the source +/// context. +fn class_engine(state: &mut GameState, class_level: u8, required_level: u8) -> ObjectId { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + ENGINE_NAME.to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + obj.class_level = Some(class_level); + obj.trigger_definitions + .push( + cycle_trigger(TargetFilter::Any).constraint(TriggerConstraint::AtClassLevel { + level: required_level, + }), + ); + id +} + +/// CR 716: an `AtClassLevel` payoff at the required level is live — the shared +/// hypothetical authority passes the source context, so the class level is read +/// correctly rather than treated as absent. +#[test] +fn at_class_level_engine_at_required_level_rewards() { + let config = AiConfig::default(); + let mut st = state(); + class_engine(&mut st, 2, 2); // at level 2, needs level 2 + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: the same Class engine at a DIFFERENT level cannot fire its +/// level-gated payoff, so cycling earns nothing. +#[test] +fn at_class_level_engine_at_wrong_level_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + class_engine(&mut st, 1, 2); // at level 1, but the payoff needs level 2 + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From 04f98f5177d8a4b3e43f4505efeacc3763e40d52 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 01:59:02 -0700 Subject: [PATCH 08/10] fix(engine+phase-ai): reject unsupported/no-execute payoffs + honor execute target constraints (round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' executable-support blocker, and mirrors the two shared-authority fixes made on the sibling draw PR (byte-identical engine). 1. Executable-support contract. `hypothetical_trigger_fireable` credited a trigger with `execute: None` (a `TriggerNoExecute` no-op) and any `Effect::Unimplemented` execute, and the deck classifier `is_cycle_payoff_trigger` had the same gap — inflating both the live bonus and deck commitment for a payoff that produces nothing. New shared engine predicate `ability_utils::ability_definition_supported` recursively rejects an `Effect::Unimplemented` at the root or any nested sub-/else-/mode-ability; both the fireability preflight and the deck classifier now consult it. 2. Execute target constraints. `execute_targets_satisfiable` now threads `execute.target_constraints` into both the cheap and full solvers (CR 115.1 / CR 601.2c), so a constrained multi-target execute is judged against the same target space the live trigger receives. Corrected the preflight annotation from CR 113.1a to CR 603.3d. Coverage: no-execute and unsupported-execute engines → `cycling_payoff_no_engine`; constrained two-target engine neutral (same controller) / rewarded (different controllers); deck-feature payoff counting rejects no-execute + unsupported. Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/ability_utils.rs | 50 +++++- crates/engine/src/game/triggers.rs | 14 +- crates/phase-ai/src/features/cycling.rs | 12 +- crates/phase-ai/src/features/tests/cycling.rs | 25 +++ .../src/policies/tests/cycling_payoff.rs | 162 +++++++++++++++++- 5 files changed, 250 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 92174821a6..e917f86684 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1329,22 +1329,58 @@ pub fn execute_targets_satisfiable( source: &crate::game::game_object::GameObject, execute: &AbilityDefinition, ) -> bool { - // CR 113.1a: build the ability exactly as the live trigger pipeline does - // (`build_triggered_ability_from_context`), so a sub-ability chain's own - // target slots are preflighted too — not just the root effect's. + // CR 603.3d: build the ability the same way the live trigger pipeline does + // (`build_resolved_from_def`) so a sub-ability chain's own target slots are + // preflighted too — not just the root effect's. let resolved = build_resolved_from_def(execute, source.id, source.controller); if target_slot_specs(state, &resolved).is_empty() { return true; // the effect requires no target } + // CR 115.1 + CR 601.2c: preflight against the SAME cross-target constraints + // the live trigger carries (`PendingTrigger::target_constraints`), so a + // constrained multi-target execute is not judged against a broader target + // space than it will actually receive. + let constraints = execute.target_constraints.as_slice(); // Cheap guard: `Some(false)` = a mandatory target with no legal choice; // `Some(true)` = legal or optional; `None` = a shape this cheap check - // cannot decide, which the full authority below resolves exactly. - if let Some(decided) = simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]) + // cannot decide (incl. any constrained set), which the full authority + // below resolves exactly. + if let Some(decided) = + simple_legal_target_assignment_exists_for_ability(state, &resolved, constraints) { return decided; } - build_target_slots(state, &resolved) - .is_ok_and(|slots| has_legal_target_assignment_for_ability(state, &resolved, &slots, &[])) + build_target_slots(state, &resolved).is_ok_and(|slots| { + has_legal_target_assignment_for_ability(state, &resolved, &slots, constraints) + }) +} + +/// True when `def`'s entire ability tree is engine-supported — no +/// `Effect::Unimplemented` gap node at the root or in any nested sub-ability, +/// else-branch, or mode. The live trigger builder converts a `None` execute / +/// unsupported effect into an `Effect::Unimplemented` (`TriggerNoExecute`) no-op +/// that produces no payoff, so payoff eligibility (both the live fireability +/// preflight and the deck-feature classifier) must not credit such a trigger. +/// The single shared support authority both consult. +pub fn ability_definition_supported(def: &AbilityDefinition) -> bool { + if matches!(*def.effect, Effect::Unimplemented { .. }) { + return false; + } + if def + .sub_ability + .as_deref() + .is_some_and(|sub| !ability_definition_supported(sub)) + { + return false; + } + if def + .else_ability + .as_deref() + .is_some_and(|els| !ability_definition_supported(els)) + { + return false; + } + def.mode_abilities.iter().all(ability_definition_supported) } /// CR 115.1 + CR 701.9b: Resolve a `Random`-mode ability's target slots by diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 521b9a7137..9738a139b9 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -8190,12 +8190,18 @@ pub fn hypothetical_trigger_fireable( ) { return false; } + // A trigger with no execute — or an unsupported execute — resolves to a + // `TriggerNoExecute` / `Effect::Unimplemented` no-op that produces no payoff, + // so it is not a live payoff (the shared support authority decides this). + let Some(execute) = def.execute.as_deref() else { + return false; + }; + if !super::ability_utils::ability_definition_supported(execute) { + return false; + } // CR 603.3d: a mandatory-target execute with no legal target is removed from // the stack rather than producing its effect. - match def.execute.as_deref() { - Some(execute) => super::ability_utils::execute_targets_satisfiable(state, source, execute), - None => true, - } + super::ability_utils::execute_targets_satisfiable(state, source, execute) } /// Evaluates the cast-payment facts carried either by the event subject or by diff --git a/crates/phase-ai/src/features/cycling.rs b/crates/phase-ai/src/features/cycling.rs index 9d099cbd0b..d384edca73 100644 --- a/crates/phase-ai/src/features/cycling.rs +++ b/crates/phase-ai/src/features/cycling.rs @@ -32,6 +32,7 @@ //! instant/sorcery can read on both axes — that overlap is intentional and the //! axes stay independent. +use engine::game::ability_utils::ability_definition_supported; use engine::game::DeckEntry; use engine::types::ability::{TargetFilter, TriggerDefinition}; use engine::types::card_type::CoreType; @@ -127,7 +128,16 @@ pub(crate) fn is_cycle_payoff_trigger(t: &TriggerDefinition) -> bool { // 3. Exclude the pure self-cycle bonus ("when you cycle THIS card"): that is // a cyclable card with upside (already counted as a source), not a // battlefield engine that rewards cycling other cards. - !matches!(&t.valid_card, Some(TargetFilter::SelfRef)) + if matches!(&t.valid_card, Some(TargetFilter::SelfRef)) { + return false; + } + // 4. The payoff must resolve to a real effect. A missing execute or an + // unsupported one (`TriggerNoExecute` / `Effect::Unimplemented`) produces + // no value, so it is not an engine — the same shared support authority the + // live fireability preflight consults. + t.execute + .as_deref() + .is_some_and(ability_definition_supported) } /// Calibration: a dedicated cycling-payoff deck (e.g. Pioneer/Historic cycling: diff --git a/crates/phase-ai/src/features/tests/cycling.rs b/crates/phase-ai/src/features/tests/cycling.rs index 916ea797da..7ec25f32e3 100644 --- a/crates/phase-ai/src/features/tests/cycling.rs +++ b/crates/phase-ai/src/features/tests/cycling.rs @@ -93,6 +93,31 @@ fn detects_engine_payoff() { assert_eq!(f.payoff_count, 3); } +/// A "whenever you cycle" trigger with NO execute is a `TriggerNoExecute` no-op — +/// no value — so deck detection must not count it as an engine (else commitment +/// is inflated for an unsupported payoff). +#[test] +fn payoff_without_execute_is_not_counted() { + let mut f = face("No-op Engine", CoreType::Enchantment); + f.triggers = vec![TriggerDefinition::new(TriggerMode::CycledOrDiscarded)]; // no execute + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); +} + +/// A "whenever you cycle" trigger whose execute is an unsupported +/// (`Effect::Unimplemented`) gap node likewise produces no value and is not +/// counted as an engine. +#[test] +fn payoff_with_unsupported_execute_is_not_counted() { + let mut f = face("Unsupported Engine", CoreType::Enchantment); + f.triggers = vec![ + TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("cycling_payoff_test_gap", "unsupported payoff"), + )), + ]; + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); +} + /// A pure "when you cycle THIS card" self-bonus is a cyclable card with upside, /// not a battlefield engine — it must not count as a payoff. #[test] diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index 71918f4859..49fe0ec57c 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -17,7 +17,7 @@ use engine::types::ability::{ use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::format::FormatConfig; -use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::game_state::{GameState, TargetSelectionConstraint, WaitingFor}; use engine::types::identifiers::{CardId, ObjectId}; use engine::types::keywords::{CyclingCost, Keyword}; use engine::types::mana::ManaCost; @@ -701,6 +701,166 @@ fn at_class_level_engine_at_wrong_level_is_neutral() { assert_eq!(delta, 0.0); } +// ─── executable-support + constrained-target contract ─────────────────────── + +fn creature_filter() -> TargetFilter { + TargetFilter::Typed( + TypedFilter::default().with_type(engine::types::ability::TypeFilter::Creature), + ) +} + +/// Adds an AI-controlled creature to the battlefield. +fn add_ai_creature(state: &mut GameState) { + let card_id = CardId(state.next_object_id); + let id = create_object(state, card_id, AI, "Bear".to_string(), Zone::Battlefield); + state + .objects + .get_mut(&id) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); +} + +/// A "whenever you cycle, exchange control of two permanents controlled by +/// DIFFERENT players" engine — the execute carries a `DifferentObjectControllers` +/// cross-target constraint (CR 115.1) the preflight must honor. +fn constrained_two_target_engine(state: &mut GameState) -> ObjectId { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + ENGINE_NAME.to_string(), + Zone::Battlefield, + ); + let mut execute = AbilityDefinition::new( + AbilityKind::Spell, + Effect::ExchangeControl { + target_a: creature_filter(), + target_b: creature_filter(), + }, + ); + execute.target_constraints = vec![TargetSelectionConstraint::DifferentObjectControllers]; + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + obj.trigger_definitions + .push(TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(execute)); + id +} + +/// CR 115.1 + CR 603.3d: two permanents controlled by the SAME player cannot +/// satisfy the engine's `DifferentObjectControllers` constraint, so the trigger +/// has no legal assignment and is not a live payoff. +#[test] +fn constrained_two_target_engine_same_controller_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + constrained_two_target_engine(&mut st); + add_ai_creature(&mut st); + add_ai_creature(&mut st); // both mine → different-controllers can't be met + let source = cycler(&mut st); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = score_of(CyclingPayoffPolicy.verdict(&ctx( + &st, + &candidate, + &decision, + &context(&config, session(0.9)), + &config, + ))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: one permanent per player satisfies `DifferentObjectControllers`, so +/// the constrained engine is live and cycling is rewarded. +#[test] +fn constrained_two_target_engine_different_controllers_rewards() { + let config = AiConfig::default(); + let mut st = state(); + constrained_two_target_engine(&mut st); + add_ai_creature(&mut st); + add_opponent_creature(&mut st); // one each → constraint satisfiable + let source = cycler(&mut st); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = score_of(CyclingPayoffPolicy.verdict(&ctx( + &st, + &candidate, + &decision, + &context(&config, session(0.9)), + &config, + ))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A "whenever you cycle" trigger with NO execute resolves to a `TriggerNoExecute` +/// no-op — no payoff — so it is not a live engine. +#[test] +fn no_execute_engine_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger( + &mut st, + Some(TriggerDefinition::new(TriggerMode::CycledOrDiscarded)), + ); + let source = cycler(&mut st); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = score_of(CyclingPayoffPolicy.verdict(&ctx( + &st, + &candidate, + &decision, + &context(&config, session(0.9)), + &config, + ))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// A "whenever you cycle" trigger whose execute is an unsupported +/// (`Effect::Unimplemented`) gap node produces no payoff, so it is not credited. +#[test] +fn unsupported_execute_engine_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger( + &mut st, + Some( + TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("cycling_payoff_test_gap", "unsupported payoff"), + )), + ), + ); + let source = cycler(&mut st); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = score_of(CyclingPayoffPolicy.verdict(&ctx( + &st, + &candidate, + &decision, + &context(&config, session(0.9)), + &config, + ))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From 6e616c79490dc61d1649c437a4ea54afe72a16f4 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 06:26:11 -0700 Subject: [PATCH 09/10] fix(engine+phase-ai): one modal-choice authority for live dispatch and payoff preflight (round 9) CR 603.3c: a required modal trigger whose every mode needs an unavailable target has no legal mode and is removed from the stack. The payoff preflight never saw those modes -- `execute_targets_satisfiable` walked only the execute root, and a modal execute keeps its real effects (and therefore its target slots) in `mode_abilities`. A "choose one -- deal 2 damage to target creature; or ..." cycling payoff therefore scored on an empty board even though the runtime necessarily drops the trigger. Factor the whole mode choice into `ability_utils::resolve_legal_modal_choice`: the dynamic "choose up to X" cap (CR 700.2 + CR 107.3m), the modes unavailable for non-target reasons, the per-mode target-legality filter (CR 115.1), the cross-mode assignment cap, and the CR 603.3c no-legal-mode verdict. `dispatch_pending_trigger_context` now asks that authority -- inside the trigger event window, so every step still sees the triggering event -- and the hypothetical preflight asks the same function. A hypothetical answer can no longer drift from the live one. CR 700.2: `ability_definition_supported` no longer treats a modal ability's `modal_placeholder` root as a parser gap, since its real effects live in the modes -- unless there are no modes for the placeholder to stand in for, in which case the placeholder itself is what would resolve. Tests: live dispatch returns `DroppedNoLegalMode` for an all-target-required modal on an empty board and does not drop it once a legal target exists; `CyclingPayoffPolicy` is neutral / positive across that same pair; a modal with no modes is neutral; the deck classifier counts a modal payoff but not one whose modes are themselves unsupported. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/ability_utils.rs | 80 +++++++++- crates/engine/src/game/triggers.rs | 144 ++++++++++++++---- crates/phase-ai/src/features/tests/cycling.rs | 64 +++++++- .../src/policies/tests/cycling_payoff.rs | 101 +++++++++++- 4 files changed, 354 insertions(+), 35 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index e917f86684..224eab1ed1 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -963,6 +963,61 @@ pub fn modal_choice_with_target_assignment_limit( Some(effective) } +/// CR 603.3c + CR 700.2: the single authority for "can a legal set of modes be +/// chosen for this modal ability, from this source, right now?". +/// +/// Runs the whole choice in the order the rules announce it: the dynamic +/// "choose up to X" cap ([`modal_choice_for_player`], CR 700.2 + CR 107.3m), +/// the modes unavailable for non-target reasons ([`compute_unavailable_modes`], +/// e.g. a NoRepeat constraint already spent), the per-mode target-legality +/// filter ([`filter_modes_by_target_legality`], CR 115.1), and finally the +/// cross-mode assignment cap ([`modal_choice_with_target_assignment_limit`]). +/// +/// Returns the resolved [`ModalChoice`] plus the unavailable-mode indices, or +/// `None` when no legal mode can be chosen — the case in which a triggered +/// ability is removed from the stack instead of resolving (CR 603.3c). Both the +/// live trigger dispatch and the hypothetical payoff preflight +/// ([`execute_targets_satisfiable`]) call it, so an AI eligibility query can +/// never disagree with what the runtime will actually do. +pub fn resolve_legal_modal_choice( + state: &GameState, + source_id: ObjectId, + controller: PlayerId, + modal: &ModalChoice, + mode_abilities: &[AbilityDefinition], +) -> Option<(ModalChoice, Vec)> { + let modal_for_player = modal_choice_for_player( + state, + controller, + source_id, + modal, + &crate::types::ability::SpellContext::default(), + ); + let mut unavailable_modes = compute_unavailable_modes(state, source_id, &modal_for_player); + filter_modes_by_target_legality( + state, + source_id, + controller, + mode_abilities, + &modal_for_player, + &mut unavailable_modes, + ); + let modal_for_player = modal_choice_with_target_assignment_limit( + state, + source_id, + controller, + &modal_for_player, + mode_abilities, + &unavailable_modes, + )?; + // CR 603.3c: an illegal mode can't be chosen; with every mode unavailable + // there is no choice to announce at all. + if unavailable_modes.len() >= modal_for_player.mode_count { + return None; + } + Some((modal_for_player, unavailable_modes)) +} + fn modal_indices_have_legal_target_assignment( state: &GameState, source_id: ObjectId, @@ -1329,6 +1384,24 @@ pub fn execute_targets_satisfiable( source: &crate::game::game_object::GameObject, execute: &AbilityDefinition, ) -> bool { + // CR 603.3c: a MODAL execute carries a placeholder root and keeps its real + // effects — and therefore its target slots — in `mode_abilities`, which the + // root slot walk below does not descend. Ask the same modal-choice authority + // the live trigger dispatch asks: a required "choose one …" whose every mode + // is target-unavailable has no legal choice and is dropped, so it is not a + // live payoff. + if let Some(modal) = &execute.modal { + if !execute.mode_abilities.is_empty() { + return resolve_legal_modal_choice( + state, + source.id, + source.controller, + modal, + &execute.mode_abilities, + ) + .is_some(); + } + } // CR 603.3d: build the ability the same way the live trigger pipeline does // (`build_resolved_from_def`) so a sub-ability chain's own target slots are // preflighted too — not just the root effect's. @@ -1363,7 +1436,12 @@ pub fn execute_targets_satisfiable( /// preflight and the deck-feature classifier) must not credit such a trigger. /// The single shared support authority both consult. pub fn ability_definition_supported(def: &AbilityDefinition) -> bool { - if matches!(*def.effect, Effect::Unimplemented { .. }) { + // CR 700.2: a modal ability carries a placeholder `Effect::Unimplemented` + // (`modal_placeholder`) root — its real effects live in `mode_abilities`, so + // the placeholder is NOT a gap. It is one again when there are no modes to + // stand in for it: then the placeholder itself is what would resolve. + let modal_placeholder = def.modal.is_some() && !def.mode_abilities.is_empty(); + if matches!(*def.effect, Effect::Unimplemented { .. }) && !modal_placeholder { return false; } if def diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 9738a139b9..2e8377a9e6 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -6532,43 +6532,25 @@ fn dispatch_pending_trigger_context( &trigger_events, trigger.subject_match_count, ); - let modal_for_player = super::ability_utils::modal_choice_for_player( - state, - trigger.controller, - trigger.source_id, - modal_ref, - &crate::types::ability::SpellContext::default(), - ); - let mut unavailable_modes = super::ability_utils::compute_unavailable_modes( - state, - trigger.source_id, - &modal_for_player, - ); - super::ability_utils::filter_modes_by_target_legality( + // CR 603.3c: the whole mode choice — dynamic cap, modes unavailable + // for non-target reasons, per-mode target legality, and the + // cross-mode assignment cap — is settled by the shared + // `resolve_legal_modal_choice` authority, run inside the event window + // so every step sees the triggering event. The AI payoff preflight + // asks the same question of the same function, so its hypothetical + // answer cannot drift from this live one. + let legal_choice = super::ability_utils::resolve_legal_modal_choice( state, trigger.source_id, trigger.controller, + modal_ref, &trigger.mode_abilities, - &modal_for_player, - &mut unavailable_modes, ); restore_trigger_event_context(state, context_snapshot); - let Some(modal_for_player) = - super::ability_utils::modal_choice_with_target_assignment_limit( - state, - trigger.source_id, - trigger.controller, - &modal_for_player, - &trigger.mode_abilities, - &unavailable_modes, - ) - else { - return TriggerDispatchDisposition::DroppedNoLegalMode; - }; - if unavailable_modes.len() >= modal_for_player.mode_count { + let Some((modal_for_player, unavailable_modes)) = legal_choice else { // CR 603.3c: No legal mode; drop the trigger entirely. return TriggerDispatchDisposition::DroppedNoLegalMode; - } + }; let mode_abilities = trigger.mode_abilities.clone(); let controller = trigger.controller; let source_id = trigger.source_id; @@ -16540,6 +16522,110 @@ pub mod tests { ); } + /// Builds and dispatches a "choose one — deal 3 to target creature; or deal + /// 3 to target creature" modal trigger from `source`, returning the live + /// dispatch disposition. + fn dispatch_two_mode_creature_target_modal( + state: &mut GameState, + source: ObjectId, + controller: PlayerId, + ) -> TriggerDispatchDisposition { + let mode = || { + AbilityDefinition::new( + AbilityKind::Database, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Typed( + TypedFilter::default().with_type(TypeFilter::Creature), + ), + damage_source: None, + excess: None, + }, + ) + }; + let pending = PendingTrigger { + source_id: source, + controller, + condition: None, + ability: Box::new(ResolvedAbility::new( + Effect::unimplemented("modal_placeholder", "choose one —"), + vec![], + source, + controller, + )), + timestamp: 1, + target_constraints: Vec::new(), + distribute: None, + trigger_event: Some(GameEvent::SpellCast { + controller, + object_id: source, + card_id: CardId(0x98), + }), + modal: Some(ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 2, + ..Default::default() + }), + mode_abilities: vec![mode(), mode()], + description: None, + may_trigger_origin: None, + subject_match_count: None, + die_result: None, + }; + let context = PendingTriggerContext { + pending, + trigger_events: Vec::new(), + dispatch_origin: PendingTriggerDispatchOrigin::Normal, + }; + let mut events_out = Vec::new(); + dispatch_pending_trigger_context(state, context, &mut events_out) + } + + /// CR 603.3c: a required modal trigger whose every mode needs a target and + /// none is available on an empty board is dropped at dispatch + /// (`DroppedNoLegalMode`) — the live contract the AI payoff preflight + /// (`execute_targets_satisfiable`) mirrors. + #[test] + fn modal_trigger_all_target_required_modes_no_targets_is_dropped() { + let mut state = GameState::new_two_player(42); + let controller = PlayerId(0); + let source = create_object( + &mut state, + CardId(0x0603_3C01), + controller, + "Modal Engine".to_string(), + Zone::Battlefield, + ); + let disposition = dispatch_two_mode_creature_target_modal(&mut state, source, controller); + assert!( + matches!(disposition, TriggerDispatchDisposition::DroppedNoLegalMode), + "all-target-required modal with no legal target must drop, got {disposition:?}" + ); + } + + /// Control: with a legal creature target present, at least one mode is + /// choosable, so the same modal trigger is NOT dropped for lack of a legal + /// mode. + #[test] + fn modal_trigger_with_a_legal_target_is_not_dropped() { + let mut state = GameState::new_two_player(42); + let controller = PlayerId(0); + let source = create_object( + &mut state, + CardId(0x0603_3C02), + controller, + "Modal Engine".to_string(), + Zone::Battlefield, + ); + let _creature = make_creature(&mut state, PlayerId(1), "Bear", 2, 2); + let disposition = dispatch_two_mode_creature_target_modal(&mut state, source, controller); + assert!( + !matches!(disposition, TriggerDispatchDisposition::DroppedNoLegalMode), + "a legal target makes a mode choosable — must not drop, got {disposition:?}" + ); + } + #[test] fn keeper_of_the_accord_creature_intervening_if_false_when_tied() { let def = crate::parser::oracle_trigger::parse_trigger_line( diff --git a/crates/phase-ai/src/features/tests/cycling.rs b/crates/phase-ai/src/features/tests/cycling.rs index 7ec25f32e3..a747cc3ed0 100644 --- a/crates/phase-ai/src/features/tests/cycling.rs +++ b/crates/phase-ai/src/features/tests/cycling.rs @@ -3,7 +3,8 @@ use engine::game::DeckEntry; use engine::types::ability::{ - AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerDefinition, + AbilityDefinition, AbilityKind, Effect, ModalChoice, QuantityExpr, TargetFilter, + TriggerDefinition, }; use engine::types::card::CardFace; use engine::types::card_type::{CardType, CoreType}; @@ -48,13 +49,19 @@ fn cycled_trigger( if let Some(vt) = valid_target { t = t.valid_target(vt); } - t.execute(AbilityDefinition::new( + t.execute(draw_a_card()) +} + +/// A supported, no-target payoff effect — the simplest thing a cycle trigger can +/// resolve to. +fn draw_a_card() -> AbilityDefinition { + AbilityDefinition::new( AbilityKind::Spell, Effect::Draw { count: QuantityExpr::Fixed { value: 1 }, target: TargetFilter::Controller, }, - )) + ) } /// Astral Drift shape: "whenever you cycle or discard a card, ..." — a broad, @@ -118,6 +125,57 @@ fn payoff_with_unsupported_execute_is_not_counted() { assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); } +/// A modal cycling payoff ("whenever you cycle a card, choose one — …") carries +/// a placeholder `Effect::Unimplemented` at its execute root by construction +/// (CR 700.2); its real effects live in `mode_abilities`. That placeholder is +/// not a parser gap, so the deck classifier must still count the card as an +/// engine — otherwise the unsupported-execute check would silently erase every +/// modal payoff from the axis. +#[test] +fn modal_payoff_is_counted() { + let mut f = face("Modal Engine", CoreType::Enchantment); + let mut execute = AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("modal_placeholder", "choose one —"), + ); + execute.modal = Some(ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 2, + ..Default::default() + }); + execute.mode_abilities = vec![draw_a_card(), draw_a_card()]; + f.triggers = vec![TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(execute)]; + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 3); +} + +/// Control: the same modal shape whose modes are themselves unsupported produces +/// no value in any branch, so it is not an engine — the placeholder exemption +/// covers the modal root only, never the modes underneath it. +#[test] +fn modal_payoff_with_unsupported_modes_is_not_counted() { + let mut f = face("Modal Gap Engine", CoreType::Enchantment); + let mut execute = AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("modal_placeholder", "choose one —"), + ); + execute.modal = Some(ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 2, + ..Default::default() + }); + let unsupported_mode = || { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("cycling_payoff_test_gap", "unsupported mode"), + ) + }; + execute.mode_abilities = vec![unsupported_mode(), unsupported_mode()]; + f.triggers = vec![TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(execute)]; + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); +} + /// A pure "when you cycle THIS card" self-bonus is a cyclable card with upside, /// not a battlefield engine — it must not count as a payoff. #[test] diff --git a/crates/phase-ai/src/policies/tests/cycling_payoff.rs b/crates/phase-ai/src/policies/tests/cycling_payoff.rs index 49fe0ec57c..9c18626a68 100644 --- a/crates/phase-ai/src/policies/tests/cycling_payoff.rs +++ b/crates/phase-ai/src/policies/tests/cycling_payoff.rs @@ -11,8 +11,8 @@ use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, Tac use engine::game::ability_utils::{build_resolved_from_def, build_target_slots}; use engine::game::zones::create_object; use engine::types::ability::{ - AbilityDefinition, AbilityKind, Effect, QuantityExpr, TargetFilter, TriggerConstraint, - TriggerDefinition, TypedFilter, + AbilityDefinition, AbilityKind, Effect, ModalChoice, QuantityExpr, TargetFilter, + TriggerConstraint, TriggerDefinition, TypedFilter, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -579,6 +579,103 @@ fn multi_target_payoff_with_legal_targets_rewards() { assert!(delta > 0.0); } +/// A required-modal payoff: "whenever you cycle a card, choose one — deal 2 +/// damage to target creature; or deal 2 damage to target creature". A modal +/// execute keeps a placeholder at its root and its real effects — and therefore +/// its target slots — in `mode_abilities`, so the root slot walk sees no targets +/// at all; the modal choice authority is what settles legality. +fn modal_all_target_required_trigger() -> TriggerDefinition { + let mut execute = AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("modal_placeholder", "choose one —"), + ); + execute.modal = Some(ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 2, + ..Default::default() + }); + execute.mode_abilities = vec![damage_target_creature(), damage_target_creature()]; + TriggerDefinition::new(TriggerMode::CycledOrDiscarded).execute(execute) +} + +/// CR 603.3c: a required "choose one" payoff whose every mode needs a creature +/// target earns nothing on a creatureless board — no mode can legally be chosen, +/// so the live trigger is removed from the stack rather than producing a payoff +/// (`DroppedNoLegalMode`, proven against the real dispatch in +/// `triggers::tests::modal_trigger_all_target_required_modes_no_targets_is_dropped`). +/// The preflight asks the same `resolve_legal_modal_choice` authority the live +/// dispatch asks, so the two cannot disagree. +#[test] +fn modal_payoff_with_no_legal_mode_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(modal_all_target_required_trigger())); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control for the pair above: one legal creature target makes a mode choosable, +/// so the identical modal payoff is live and cycling is rewarded. Without this +/// control, rejecting every modal shape outright would also pass the negative +/// case. +#[test] +fn modal_payoff_with_a_legal_mode_rewards() { + let config = AiConfig::default(); + let mut st = state(); + permanent_with_trigger(&mut st, Some(modal_all_target_required_trigger())); + add_opponent_creature(&mut st); + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A modal with no `mode_abilities` has nothing to stand in for its placeholder +/// root, so the placeholder itself is what would resolve — an unsupported no-op +/// that is not an engine, even with a legal target on the board. +#[test] +fn modal_payoff_without_modes_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + let mut trigger = modal_all_target_required_trigger(); + trigger + .execute + .as_deref_mut() + .expect("fixture trigger must carry an execute") + .mode_abilities + .clear(); + permanent_with_trigger(&mut st, Some(trigger)); + add_opponent_creature(&mut st); // a legal target exists — choosable modes do not + let source = cycler(&mut st); + let context = context(&config, session(0.9)); + let candidate = activate(source); + let decision = AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + }; + let (delta, reason) = + score_of(CyclingPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "cycling_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + /// A `MaxTimesPerTurn { max }` engine that has fired fewer than `max` times this /// turn can still fire, so cycling is rewarded — the shared engine authority reads /// the live `trigger_fire_counts_this_turn` ledger rather than rejecting the From 97fc106298a7e7c611e632fa1d72dc220068f59f Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 19:08:14 -0700 Subject: [PATCH 10/10] fix(engine): carry the announced modal choice to its prompt (round 10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 collapsed the mode prompt's six inline steps into one `resolve_legal_modal_choice` call. That deduplicated the code but not the derivation: dispatch asked the authority when it put the ability on the stack, and the prompt asked it again in a later re-entry. Two answers, two moments. CR 603.3c and CR 700.2b both bind mode legality to the earlier moment — "the controller ... chooses the mode(s) as part of putting that ability on the stack. If one of the modes would be illegal (due to an inability to choose legal targets, for example), that mode can't be chosen." The board can move between the two halves, and the target path in the same function already documents that window: "an effect earlier in the SAME simultaneous cascade removed the only legal target". This is not just a duplicated derivation. When a mode's only legal target leaves the battlefield inside that window, the re-derivation finds no legal mode, drops the trigger, and returns `Ok(None)` — the controller is never prompted at all, losing a mode choice CR 700.2b already granted them. The new regression fails with `got None` when the announcement is bypassed, so it is red on revert rather than merely restating the contract. The announcement now travels with the trigger: - `PendingTrigger::announced_modal_choice` carries the `AnnouncedModalChoice` produced at the push site, set only after `resolve_legal_modal_choice` confirms a legal choice exists. - `begin_pending_trigger_target_selection` surfaces that announcement and falls back to the same single authority only for a trigger parked without one — it never re-implements the mode-choice sequence. - `resolve_legal_modal_choice` returns the named `AnnouncedModalChoice` instead of a loose `(ModalChoice, Vec)` pair, so the value that crosses the pause has one type and one producer. The field is boxed: the `types::game_state_size` stack guard fired, and that module's own instruction is to box a large, rarely-populated field rather than widen a ceiling. It embeds a whole `ModalChoice` and is populated only on the modal dispatch path, which is exactly that shape. The remaining 36 construction sites are mechanical `announced_modal_choice: None`. CR 603.3c, CR 603.3d, CR 700.2b verified against docs/MagicCompRules.txt. Verification: cargo fmt; clippy -D warnings across engine + phase-ai + server-core --all-targets --features proptest; engine 21,876 tests green; phase-ai + server-core 1,968 tests green. cargo ai-gate output is byte-identical to the round-9 parent commit run with the same card database (red 50%/13.8, affinity 30%/11.3, enchantress 10%/14.3; same 2 WARNs, same flip counts and p-values), so this change is gate-neutral and those warnings are pre-existing. --- crates/engine/src/game/ability_utils.rs | 34 ++- crates/engine/src/game/archenemy_tests.rs | 1 + crates/engine/src/game/casting.rs | 1 + crates/engine/src/game/cipher.rs | 1 + crates/engine/src/game/effects/mod.rs | 2 + crates/engine/src/game/effects/venture.rs | 1 + crates/engine/src/game/elimination.rs | 1 + crates/engine/src/game/engine.rs | 97 ++++--- .../game/engine_keyword_action_stack_tests.rs | 1 + crates/engine/src/game/engine_tests.rs | 1 + .../src/game/engine_trigger_target_tests.rs | 10 + crates/engine/src/game/planechase.rs | 1 + crates/engine/src/game/stack.rs | 1 + crates/engine/src/game/triggers.rs | 176 +++++++++++- .../game/triggers_dedup_regression_tests.rs | 3 + .../game/triggers_ordering_parity_tests.rs | 2 + .../game/triggers_pr7_order_template_tests.rs | 1 + .../triggers_push_first_contract_tests.rs | 2 + crates/engine/src/types/ability.rs | 23 ++ crates/engine/src/types/game_state.rs | 2 + .../breeches_blastmaker_coin_flip_copy.rs | 1 + .../cr733_resolved_trigger_collection.rs | 1 + .../game_state_boxed_ability_serde.rs | 1 + crates/engine/tests/integration/main.rs | 1 + .../trigger_modal_choice_single_authority.rs | 263 ++++++++++++++++++ .../phase-ai/src/policies/anti_self_harm.rs | 2 + .../src/policies/evasion_removal_priority.rs | 1 + crates/server-core/src/filter.rs | 1 + 28 files changed, 568 insertions(+), 64 deletions(-) create mode 100644 crates/engine/tests/integration/trigger_modal_choice_single_authority.rs diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 224eab1ed1..3f3197961f 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -2,12 +2,12 @@ use crate::types::ability::TapStateChange; use crate::types::ability::{ AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, AdditionalCost, - CardTypeSetSource, CastManaSpentMetric, CombatRelationSubject, ControllerRef, - CounterMoveSelection, DamageSource, Effect, EffectScope, FilterProp, GameRestriction, - ModalChoice, ModalSelectionCondition, ModalSelectionConstraint, MultiTargetSpec, ObjectScope, - PlayerFilter, PlayerScope, QuantityExpr, QuantityRef, ResolvedAbility, RestrictionPlayerScope, - SpellContext, SubAbilityLink, TargetChoiceTiming, TargetFilter, TargetRef, TriggerDefinition, - TypeFilter, TypedFilter, + AnnouncedModalChoice, CardTypeSetSource, CastManaSpentMetric, CombatRelationSubject, + ControllerRef, CounterMoveSelection, DamageSource, Effect, EffectScope, FilterProp, + GameRestriction, ModalChoice, ModalSelectionCondition, ModalSelectionConstraint, + MultiTargetSpec, ObjectScope, PlayerFilter, PlayerScope, QuantityExpr, QuantityRef, + ResolvedAbility, RestrictionPlayerScope, SpellContext, SubAbilityLink, TargetChoiceTiming, + TargetFilter, TargetRef, TriggerDefinition, TypeFilter, TypedFilter, }; #[cfg(test)] use crate::types::counter::CounterType; @@ -973,19 +973,24 @@ pub fn modal_choice_with_target_assignment_limit( /// filter ([`filter_modes_by_target_legality`], CR 115.1), and finally the /// cross-mode assignment cap ([`modal_choice_with_target_assignment_limit`]). /// -/// Returns the resolved [`ModalChoice`] plus the unavailable-mode indices, or +/// Returns the [`AnnouncedModalChoice`] the controller chooses against, or /// `None` when no legal mode can be chosen — the case in which a triggered -/// ability is removed from the stack instead of resolving (CR 603.3c). Both the -/// live trigger dispatch and the hypothetical payoff preflight -/// ([`execute_targets_satisfiable`]) call it, so an AI eligibility query can -/// never disagree with what the runtime will actually do. +/// ability is removed from the stack instead of resolving (CR 603.3c). +/// +/// Every consumer of a triggered modal's legal-mode set goes through here: +/// the live dispatch announcement (`dispatch_pending_trigger_context`), the mode +/// prompt that surfaces that announcement +/// (`begin_pending_trigger_target_selection`), and the hypothetical payoff +/// preflight ([`execute_targets_satisfiable`]). No caller re-derives the choice, +/// so neither the prompt nor an AI eligibility query can disagree with what the +/// runtime announced. pub fn resolve_legal_modal_choice( state: &GameState, source_id: ObjectId, controller: PlayerId, modal: &ModalChoice, mode_abilities: &[AbilityDefinition], -) -> Option<(ModalChoice, Vec)> { +) -> Option { let modal_for_player = modal_choice_for_player( state, controller, @@ -1015,7 +1020,10 @@ pub fn resolve_legal_modal_choice( if unavailable_modes.len() >= modal_for_player.mode_count { return None; } - Some((modal_for_player, unavailable_modes)) + Some(AnnouncedModalChoice { + modal: modal_for_player, + unavailable_modes, + }) } fn modal_indices_have_legal_target_assignment( diff --git a/crates/engine/src/game/archenemy_tests.rs b/crates/engine/src/game/archenemy_tests.rs index 01a5b9eec3..f204967bd3 100644 --- a/crates/engine/src/game/archenemy_tests.rs +++ b/crates/engine/src/game/archenemy_tests.rs @@ -491,6 +491,7 @@ fn deferred_scheme_trigger_blocks_abandon() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; state.deferred_triggers.push(DeferredTrigger { pending, diff --git a/crates/engine/src/game/casting.rs b/crates/engine/src/game/casting.rs index b5155f545b..99a3c63a82 100644 --- a/crates/engine/src/game/casting.rs +++ b/crates/engine/src/game/casting.rs @@ -15210,6 +15210,7 @@ fn apply_mana_spell_grants( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, ); } diff --git a/crates/engine/src/game/cipher.rs b/crates/engine/src/game/cipher.rs index b18344fc66..5adcb4f117 100644 --- a/crates/engine/src/game/cipher.rs +++ b/crates/engine/src/game/cipher.rs @@ -287,6 +287,7 @@ fn recast_trigger( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, trigger_events: vec![event.clone()], dispatch_origin: PendingTriggerDispatchOrigin::Normal, diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index b6fbb82b2f..2ce8106fd4 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -2188,6 +2188,7 @@ fn try_begin_reflexive_target_selection_inner( // into the later fresh-`apply()` target-assign. subject_match_count: freeze_reflexive_event_count(state, controller, source_id), die_result: state.die_result_this_resolution, + announced_modal_choice: None, }; let trigger_events = crate::game::triggers::take_pending_trigger_event_batch(state, &pending); @@ -2280,6 +2281,7 @@ fn try_begin_reflexive_target_selection_inner( // creating ability so the reflexive entry can re-stamp it when it // resolves as its own stack object. die_result: state.die_result_this_resolution, + announced_modal_choice: None, }; let trigger_events = crate::game::triggers::take_pending_trigger_event_batch(state, &pending); let pending_for_state = pending.clone(); diff --git a/crates/engine/src/game/effects/venture.rs b/crates/engine/src/game/effects/venture.rs index fe7f6635ad..70cad0c363 100644 --- a/crates/engine/src/game/effects/venture.rs +++ b/crates/engine/src/game/effects/venture.rs @@ -255,6 +255,7 @@ fn queue_room_trigger( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; // CR 603.2 + CR 309.4c: Dispatch through the standard diff --git a/crates/engine/src/game/elimination.rs b/crates/engine/src/game/elimination.rs index 2989ce034c..20be050198 100644 --- a/crates/engine/src/game/elimination.rs +++ b/crates/engine/src/game/elimination.rs @@ -948,6 +948,7 @@ fn do_eliminate( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, events, ); diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 5cd704f147..f61d0cd5c3 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -30,7 +30,7 @@ use crate::types::zones::Zone; use super::ability_utils::{ begin_target_selection_for_ability, build_target_slots, cap_distribution_target_slots, - compute_unavailable_modes, has_legal_target_assignment_for_ability, modal_choice_for_player, + has_legal_target_assignment_for_ability, }; use super::casting; use super::casting_costs; @@ -7279,6 +7279,7 @@ fn apply_action( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; super::triggers::push_pending_trigger_to_stack(state, trigger, &mut events); @@ -8615,49 +8616,59 @@ pub(super) fn begin_pending_trigger_target_selection( }; let subject_match_count = trigger.subject_match_count; let modal = modal.clone(); - // CR 603.3c + CR 603.3d: a triggered modal's mode choice is announced as - // the ability is put on the stack, by the same process as casting a spell - // (CR 601.2c-d). The triggering event must be live for the ENTIRE choice, - // including the "choose up to X" dynamic cap resolved by - // modal_choice_for_player -- push the event window BEFORE cap resolution, - // not just around target-legality filtering, so event-context quantity - // refs (e.g. EventContextSourceModesChosen, Riku of Many Paths) resolve - // against the triggering spell rather than an unset event. - let context_snapshot = super::triggers::push_trigger_event_context( - state, - trigger_event.as_ref(), - &trigger_events, - subject_match_count, - ); - let modal = modal_choice_for_player( - state, - player, - source_id, - &modal, - &crate::types::ability::SpellContext::default(), - ); - let mut unavailable_modes = compute_unavailable_modes(state, source_id, &modal); - super::ability_utils::filter_modes_by_target_legality( - state, - source_id, - player, - &mode_abilities, - &modal, - &mut unavailable_modes, - ); - super::triggers::restore_trigger_event_context(state, context_snapshot); - let Some(modal) = super::ability_utils::modal_choice_with_target_assignment_limit( - state, - source_id, - player, - &modal, - &mode_abilities, - &unavailable_modes, - ) else { - super::stack::pop_uncommitted_pending_trigger_entry(state); - state.pending_trigger = None; - return Ok(None); + // CR 603.3c + CR 700.2b: this prompt SURFACES the mode choice that was + // announced when the ability was put on the stack — it does not make a + // second one. Both rules bind mode legality to that earlier moment, and + // the game state can move in between (an effect earlier in the same + // simultaneous cascade may remove a mode's only legal target, exactly + // as the target path below documents), so the announcement carried on + // the pending trigger is authoritative over anything re-derived here. + // + // Only a trigger parked without an announcement falls back — and it + // falls back to `resolve_legal_modal_choice`, the same single authority + // that produced the announcement and that the AI payoff preflight + // asks. This step must never re-implement the mode-choice sequence + // (dynamic cap → non-target unavailability → per-mode target legality → + // cross-mode assignment cap → CR 603.3c no-legal-mode verdict): a + // second copy is free to drift from the announcement, and it is the + // prompt that the controller is bound by. + let announced = match trigger.announced_modal_choice.clone() { + Some(announced) => *announced, + None => { + // CR 603.3d: the triggering event must be live for the ENTIRE + // choice, including the "choose up to X" dynamic cap -- push the + // event window BEFORE the authority runs, so event-context + // quantity refs (e.g. EventContextSourceModesChosen, Riku of + // Many Paths) resolve against the triggering spell rather than + // an unset event, exactly as the announcement path does. + let context_snapshot = super::triggers::push_trigger_event_context( + state, + trigger_event.as_ref(), + &trigger_events, + subject_match_count, + ); + let resolved = super::ability_utils::resolve_legal_modal_choice( + state, + source_id, + player, + &modal, + &mode_abilities, + ); + super::triggers::restore_trigger_event_context(state, context_snapshot); + let Some(resolved) = resolved else { + // CR 603.3c: no legal mode can be chosen — the ability is + // removed from the stack rather than resolving. + super::stack::pop_uncommitted_pending_trigger_entry(state); + state.pending_trigger = None; + return Ok(None); + }; + resolved + } }; + let crate::types::ability::AnnouncedModalChoice { + modal, + unavailable_modes, + } = announced; // CR 700.2b (override) + CR 701.9b (analogous): "choose ... at // random" modal triggers (Cult of Skaro) are resolved inline by diff --git a/crates/engine/src/game/engine_keyword_action_stack_tests.rs b/crates/engine/src/game/engine_keyword_action_stack_tests.rs index c01d07b7ce..6dc5439949 100644 --- a/crates/engine/src/game/engine_keyword_action_stack_tests.rs +++ b/crates/engine/src/game/engine_keyword_action_stack_tests.rs @@ -907,6 +907,7 @@ fn issue_3660_finalize_copy_retarget_stashes_offers_on_deferred_pause() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, trigger_events: Vec::new(), dispatch_origin: PendingTriggerDispatchOrigin::Normal, diff --git a/crates/engine/src/game/engine_tests.rs b/crates/engine/src/game/engine_tests.rs index 7502520b5b..f1e00dbc02 100644 --- a/crates/engine/src/game/engine_tests.rs +++ b/crates/engine/src/game/engine_tests.rs @@ -169,6 +169,7 @@ fn pending_trigger_with_no_legal_target_at_choose_time_drops_not_errors() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let entry_id = ObjectId(state.next_object_id); state.next_object_id += 1; diff --git a/crates/engine/src/game/engine_trigger_target_tests.rs b/crates/engine/src/game/engine_trigger_target_tests.rs index efff440280..4120af280e 100644 --- a/crates/engine/src/game/engine_trigger_target_tests.rs +++ b/crates/engine/src/game/engine_trigger_target_tests.rs @@ -106,6 +106,7 @@ fn trigger_target_selection_select_targets_pushes_to_stack() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -218,6 +219,7 @@ fn trigger_target_selection_rejects_illegal_target() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -306,6 +308,7 @@ fn triggered_modal_modes_with_targets_wait_for_target_selection() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -448,6 +451,7 @@ fn setup_vindictive_lich_pending_trigger(state: &mut GameState) { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -590,6 +594,7 @@ fn triggered_modal_modes_without_targets_consume_pending_trigger() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -715,6 +720,7 @@ fn triggered_commander_modal_cap_uses_controller_board_state() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -800,6 +806,7 @@ fn trigger_target_selection_enforces_different_player_constraint() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -949,6 +956,7 @@ fn choose_target_action_advances_trigger_selection_from_engine_state() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -1067,6 +1075,7 @@ fn triggered_modal_modes_reject_unsatisfiable_target_constraints() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let mut setup_events = Vec::new(); @@ -1180,6 +1189,7 @@ fn all_modes_exhausted_clears_pending_trigger() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let pending_for_state = pending.clone(); let stack_before = state.stack.len(); diff --git a/crates/engine/src/game/planechase.rs b/crates/engine/src/game/planechase.rs index efe5841d38..6782d2acca 100644 --- a/crates/engine/src/game/planechase.rs +++ b/crates/engine/src/game/planechase.rs @@ -211,6 +211,7 @@ fn queue_planeswalk_trigger( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; crate::game::triggers::dispatch_synthetic_trigger(state, pending, events); } diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index 4e42efddec..7b4043d695 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -4639,6 +4639,7 @@ mod tests { may_trigger_origin: Some(MayTriggerOrigin::Printed { trigger_index: 0 }), subject_match_count: None, die_result: None, + announced_modal_choice: None, })); state.waiting_for = WaitingFor::Priority { player: PlayerId(0), diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 2e8377a9e6..476e5a14f4 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -3,12 +3,12 @@ use std::collections::HashSet; use crate::database::synthesis::KeywordTriggerInstaller; use crate::types::ability::{ AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, AdditionalCostOrigin, - BounceSelection, CardTypeSetSource, CastManaSpentMetric, ChosenAttribute, CommanderOwnership, - ControllerRef, CopyRetargetPermission, DelayedTriggerCondition, Effect, ModalChoice, - ObjectScope, OriginConstraint, PlayerFilter, PtValue, QuantityExpr, QuantityRef, RenownSubject, - ResolvedAbility, SacrificeCost, TargetFilter, TargetRef, TributeOutcome, TriggerCondition, - TriggerDefinition, TriggerDefinitionOccurrenceRef, TriggerDefinitionRef, TriggerEntry, - TriggerGrantProducerKey, TypeFilter, TypedFilter, + AnnouncedModalChoice, BounceSelection, CardTypeSetSource, CastManaSpentMetric, ChosenAttribute, + CommanderOwnership, ControllerRef, CopyRetargetPermission, DelayedTriggerCondition, Effect, + ModalChoice, ObjectScope, OriginConstraint, PlayerFilter, PtValue, QuantityExpr, QuantityRef, + RenownSubject, ResolvedAbility, SacrificeCost, TargetFilter, TargetRef, TributeOutcome, + TriggerCondition, TriggerDefinition, TriggerDefinitionOccurrenceRef, TriggerDefinitionRef, + TriggerEntry, TriggerGrantProducerKey, TypeFilter, TypedFilter, }; #[cfg(test)] use crate::types::ability::{EffectScope, TapStateChange}; @@ -86,6 +86,32 @@ pub struct PendingTrigger { pub modal: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub mode_abilities: Vec, + /// CR 603.3c + CR 700.2b: The mode choice `dispatch_pending_trigger_context` + /// announced as this ability was put on the stack — set only on the modal + /// dispatch path, and only after `resolve_legal_modal_choice` confirmed a + /// legal choice exists. + /// + /// Both rules fix mode legality *at the moment the ability is put on the + /// stack*: "if one of the modes would be illegal (due to an inability to + /// choose legal targets, for example), that mode can't be chosen". The + /// engine raises the controller's prompt in a later re-entry + /// (`begin_pending_trigger_target_selection`), and the game state can move + /// in between — an effect earlier in the same simultaneous cascade may + /// remove a mode's only legal target, exactly as the target path documents. + /// Re-deriving legality at prompt time would answer that later state, so the + /// announcement travels with the trigger instead. + /// + /// `None` for every non-modal trigger and for any trigger parked by a path + /// that never announced modes; the prompt then asks the same + /// `resolve_legal_modal_choice` authority itself rather than re-implementing + /// the choice. + /// + /// Boxed per the stack budget in [`crate::types::game_state_size`]: it embeds + /// a whole [`ModalChoice`] and is populated only on the modal dispatch path — + /// exactly the "large, rarely-populated field" that module says to box rather + /// than widen a ceiling for. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub announced_modal_choice: Option>, /// Human-readable trigger description from the Oracle text. #[serde(default, skip_serializing_if = "Option::is_none")] pub description: Option, @@ -1715,6 +1741,7 @@ fn collect_matching_triggers_inner( }, subject_match_count, die_result: None, + announced_modal_choice: None, }, trigger_events, batched: trig_def.batched, @@ -2729,6 +2756,7 @@ fn collect_latched_batched_zone_triggers( }), subject_match_count, die_result: None, + announced_modal_choice: None, }, trigger_events, batched: true, @@ -3001,6 +3029,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3048,6 +3077,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3091,6 +3121,7 @@ fn collect_pending_triggers_with_collection( }), subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3136,6 +3167,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3182,6 +3214,7 @@ fn collect_pending_triggers_with_collection( }), subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3267,6 +3300,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3680,6 +3714,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -3742,6 +3777,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } @@ -3802,6 +3838,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } @@ -3923,6 +3960,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } @@ -3994,6 +4032,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } @@ -4066,6 +4105,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4124,6 +4164,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4156,6 +4197,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4191,6 +4233,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4286,6 +4329,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4330,6 +4374,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4375,6 +4420,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); mark_speed_trigger_used(state, trigger_controller); } @@ -4422,6 +4468,7 @@ fn collect_pending_triggers_with_collection( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); } } @@ -4665,6 +4712,7 @@ fn ring_pending_trigger( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } @@ -6547,14 +6595,24 @@ fn dispatch_pending_trigger_context( &trigger.mode_abilities, ); restore_trigger_event_context(state, context_snapshot); - let Some((modal_for_player, unavailable_modes)) = legal_choice else { + let Some(announced) = legal_choice else { // CR 603.3c: No legal mode; drop the trigger entirely. return TriggerDispatchDisposition::DroppedNoLegalMode; }; + let AnnouncedModalChoice { + modal: modal_for_player, + unavailable_modes, + } = announced.clone(); let mode_abilities = trigger.mode_abilities.clone(); let controller = trigger.controller; let source_id = trigger.source_id; - let pending_for_state = trigger.clone(); + // CR 603.3c + CR 700.2b: the announcement is complete HERE — this is + // the moment the ability is put on the stack, and it is the moment + // both rules bind mode legality to. Carry it on the parked trigger so + // the controller's prompt surfaces this answer instead of a fresh + // derivation against whatever state exists when the prompt is raised. + let mut pending_for_state = trigger.clone(); + pending_for_state.announced_modal_choice = Some(Box::new(announced)); let entry_id = push_pending_trigger_to_stack_with_event_batch( state, trigger, @@ -7294,6 +7352,7 @@ pub fn check_state_triggers(state: &mut GameState) { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }); } } @@ -7412,6 +7471,7 @@ fn delayed_trigger_to_context( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } @@ -11329,6 +11389,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); state.waiting_for = WaitingFor::OptionalEffectChoice { player: controller, @@ -15993,6 +16054,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let mut events = Vec::new(); @@ -16072,6 +16134,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let draw_trig = parse_trigger_line(DRAW_ETB, "Curiosity Crafter"); @@ -16095,6 +16158,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; dispatch_collected_triggers( @@ -16572,6 +16636,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let context = PendingTriggerContext { pending, @@ -16626,6 +16691,93 @@ pub mod tests { ); } + /// CR 603.3c + CR 700.2b: mode legality is fixed WHEN THE ABILITY IS PUT ON + /// THE STACK — "if one of the modes would be illegal (due to an inability to + /// choose legal targets, for example), that mode can't be chosen". The + /// controller's prompt is raised in a later re-entry + /// (`begin_pending_trigger_target_selection`), and the board can move in + /// between: the target path in that same function documents the case where + /// "an effect earlier in the SAME simultaneous cascade removed the only legal + /// target". This test makes that window explicit — the sole legal target for + /// both modes leaves the battlefield after the announcement — and pins that + /// the prompt still offers the ANNOUNCED choice. + /// + /// Discriminating, and verified so by experiment: with the carried + /// announcement bypassed, this test fails with `got None` — re-deriving mode + /// legality at prompt time finds no legal target for either mode, so it drops + /// the trigger and never raises a prompt at all. That is the defect, not just + /// a duplicated derivation: the controller loses a mode choice CR 700.2b + /// already granted them. The ability may still be removed afterwards, but by + /// CR 603.3d once a chosen mode has no legal target — the rule that actually + /// governs that removal. + #[test] + fn mode_prompt_surfaces_the_announcement_not_a_later_re_derivation() { + let mut state = GameState::new_two_player(42); + let controller = PlayerId(0); + let source = create_object( + &mut state, + CardId(0x0603_3C03), + controller, + "Modal Engine".to_string(), + Zone::Battlefield, + ); + let creature = make_creature(&mut state, PlayerId(1), "Bear", 2, 2); + + // Announcement: the Bear is a legal target, so both modes are choosable. + let disposition = dispatch_two_mode_creature_target_modal(&mut state, source, controller); + assert!( + matches!(disposition, TriggerDispatchDisposition::Paused), + "a choosable modal trigger must park for its mode prompt, got {disposition:?}" + ); + let announced = state + .pending_trigger + .as_ref() + .and_then(|pending| pending.announced_modal_choice.clone()) + .expect("dispatch must carry the announced mode choice on the parked trigger"); + assert!( + announced.unavailable_modes.is_empty(), + "with the Bear present both modes were announced as choosable, got {:?}", + announced.unavailable_modes + ); + + // The only legal target leaves the battlefield BEFORE the prompt is + // raised — the simultaneous-cascade window the target path documents. + // Uses the real zone primitive: `create_object` registers battlefield + // membership in a separate zone list, so mutating `GameObject::zone` + // alone would leave the creature targetable and make this test vacuous. + let mut zone_events = Vec::new(); + crate::game::zones::move_to_zone(&mut state, creature, Zone::Graveyard, &mut zone_events); + assert!( + !crate::game::targeting::zone_object_ids(&state, Zone::Battlefield).contains(&creature), + "the injected drift must actually remove the Bear from the battlefield, \ + otherwise this test cannot tell a re-derivation from the announcement" + ); + + let waiting = crate::game::engine::begin_pending_trigger_target_selection(&mut state) + .expect("raising the mode prompt must not error"); + match waiting { + Some(crate::types::game_state::WaitingFor::AbilityModeChoice { + modal, + unavailable_modes, + .. + }) => { + assert_eq!( + unavailable_modes, announced.unavailable_modes, + "the prompt must offer the announced unavailable set (CR 700.2b), \ + not one re-derived after the target left" + ); + assert_eq!( + modal.max_choices, announced.modal.max_choices, + "the prompt must offer the announced cap" + ); + } + other => panic!( + "the announced mode choice must still be prompted (CR 603.3c); a \ + re-derivation would have dropped the trigger instead, got {other:?}" + ), + } + } + #[test] fn keeper_of_the_accord_creature_intervening_if_false_when_tied() { let def = crate::parser::oracle_trigger::parse_trigger_line( @@ -26974,6 +27126,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } @@ -27091,6 +27244,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } @@ -27308,6 +27462,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } @@ -29633,6 +29788,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut Vec::new(), ); @@ -29881,6 +30037,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut Vec::new(), ); @@ -29983,6 +30140,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut Vec::new(), ); @@ -30175,6 +30333,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut Vec::new(), ); @@ -30265,6 +30424,7 @@ pub mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut Vec::new(), ); diff --git a/crates/engine/src/game/triggers_dedup_regression_tests.rs b/crates/engine/src/game/triggers_dedup_regression_tests.rs index 8a94a46688..e37e0cf9e3 100644 --- a/crates/engine/src/game/triggers_dedup_regression_tests.rs +++ b/crates/engine/src/game/triggers_dedup_regression_tests.rs @@ -3326,6 +3326,7 @@ fn order_triggers_distinct_event_context_still_prompt() { may_trigger_origin: None, subject_match_count: Some(count), die_result: None, + announced_modal_choice: None, }) }; let ctx_a = make_ctx(ObjectId(1), 1); @@ -3385,6 +3386,7 @@ fn order_triggers_event_context_ability_still_prompts_on_distinct_events() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) }; let ctx_a = make_ctx(ObjectId(1), ObjectId(11)); @@ -3436,6 +3438,7 @@ fn archenemy_hero_team_orders_triggers_from_multiple_heroes_together() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) }; diff --git a/crates/engine/src/game/triggers_ordering_parity_tests.rs b/crates/engine/src/game/triggers_ordering_parity_tests.rs index c0c8b70456..0389a6758c 100644 --- a/crates/engine/src/game/triggers_ordering_parity_tests.rs +++ b/crates/engine/src/game/triggers_ordering_parity_tests.rs @@ -948,6 +948,7 @@ fn ctx( may_trigger_origin: None, subject_match_count: None, die_result, + announced_modal_choice: None, }) } @@ -1410,6 +1411,7 @@ fn ctx_c( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }) } diff --git a/crates/engine/src/game/triggers_pr7_order_template_tests.rs b/crates/engine/src/game/triggers_pr7_order_template_tests.rs index 769c766fb4..013bf19986 100644 --- a/crates/engine/src/game/triggers_pr7_order_template_tests.rs +++ b/crates/engine/src/game/triggers_pr7_order_template_tests.rs @@ -53,6 +53,7 @@ fn mk_ctx( may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, trigger_events: Vec::new(), dispatch_origin: PendingTriggerDispatchOrigin::Normal, diff --git a/crates/engine/src/game/triggers_push_first_contract_tests.rs b/crates/engine/src/game/triggers_push_first_contract_tests.rs index 60f94276d3..65f8d015c0 100644 --- a/crates/engine/src/game/triggers_push_first_contract_tests.rs +++ b/crates/engine/src/game/triggers_push_first_contract_tests.rs @@ -420,6 +420,7 @@ fn push_first_no_legal_modes_modal_trigger_dropped_silently() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let stack_before = state.stack.len(); @@ -530,6 +531,7 @@ fn random_modal_trigger_resolves_without_prompting() { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let stack_before = state.stack.len(); diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 744429705d..9525decf92 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -16857,6 +16857,29 @@ pub struct ModalChoice { pub dynamic_max_choices: Option, } +/// CR 603.3c + CR 700.2b: The mode choice **announced** for a modal ability as it +/// is put on the stack — the effective [`ModalChoice`] (dynamic cap already +/// resolved) paired with the mode indices that cannot be chosen. +/// +/// CR 603.3c and CR 700.2b both bind a modal triggered ability's mode legality to +/// the moment the ability is put on the stack: "if one of the modes would be +/// illegal (due to an inability to choose legal targets, for example), that mode +/// can't be chosen". This engine necessarily splits that one announcement across +/// a pause — the entry is pushed, then the controller is prompted — so the answer +/// is bound into a single value, produced once by +/// `ability_utils::resolve_legal_modal_choice` and carried on +/// `PendingTrigger::announced_modal_choice`, rather than left as a loose +/// `(ModalChoice, Vec)` pair that each half re-derives for itself. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct AnnouncedModalChoice { + /// The effective modal header the controller chooses against. + pub modal: ModalChoice, + /// Mode indices that cannot be chosen — CR 700.2d repeat constraints and + /// CR 115.1 target-legality failures. Ascending and deduplicated. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub unavailable_modes: Vec, +} + /// Selection constraints attached to a modal choice header. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "type")] diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index 5b9e7c2508..326cc93e33 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -22027,6 +22027,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; let json = serde_json::to_string(&trigger).unwrap(); let deserialized: PendingTrigger = serde_json::from_str(&json).unwrap(); @@ -22093,6 +22094,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); let json = serde_json::to_string(&state).unwrap(); diff --git a/crates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rs b/crates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rs index f305cf3e19..444375b546 100644 --- a/crates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rs +++ b/crates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rs @@ -332,6 +332,7 @@ fn setup_breeches_runtime(seed: u64) -> (GameState, ObjectId, ObjectId) { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, &mut events, ); diff --git a/crates/engine/tests/integration/cr733_resolved_trigger_collection.rs b/crates/engine/tests/integration/cr733_resolved_trigger_collection.rs index 5d462b98f3..772db4710b 100644 --- a/crates/engine/tests/integration/cr733_resolved_trigger_collection.rs +++ b/crates/engine/tests/integration/cr733_resolved_trigger_collection.rs @@ -58,6 +58,7 @@ fn pending_context(source_id: ObjectId) -> PendingTriggerContext { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }, trigger_events: Vec::new(), dispatch_origin: PendingTriggerDispatchOrigin::Normal, diff --git a/crates/engine/tests/integration/game_state_boxed_ability_serde.rs b/crates/engine/tests/integration/game_state_boxed_ability_serde.rs index c98849ec9f..b52195682a 100644 --- a/crates/engine/tests/integration/game_state_boxed_ability_serde.rs +++ b/crates/engine/tests/integration/game_state_boxed_ability_serde.rs @@ -120,6 +120,7 @@ fn populated_state() -> GameState { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); // Populated so the `#[serde(skip)]` assertion in // `boxing_introduces_no_wrapper_level_in_the_wire_shape` discriminates. With diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 2ddb2bf4ca..84a98fac34 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -878,6 +878,7 @@ mod total_war_attacking_player_scope; mod tracked_set_anaphor_quantity_binds; mod treasured_find_regression; mod trespassers_curse_enchanted_player_trigger; +mod trigger_modal_choice_single_authority; mod true_conviction_double_keyword_grant; mod turn_based_draw_step_miracle_offer; mod turn_control_priority_softlock; diff --git a/crates/engine/tests/integration/trigger_modal_choice_single_authority.rs b/crates/engine/tests/integration/trigger_modal_choice_single_authority.rs new file mode 100644 index 0000000000..59b38c9c8d --- /dev/null +++ b/crates/engine/tests/integration/trigger_modal_choice_single_authority.rs @@ -0,0 +1,263 @@ +//! CR 603.3c + CR 603.3d: a modal triggered ability's mode choice is announced +//! when the ability is put on the stack. This engine necessarily splits that one +//! announcement across a pause — `dispatch_pending_trigger_context` resolves the +//! legal choice and pushes the entry, then `begin_pending_trigger_target_selection` +//! raises the `AbilityModeChoice` prompt — and the two halves live in different +//! modules. +//! +//! These tests pin the contract that makes that split safe: the prompt the +//! controller actually sees is the answer of the ONE authority, +//! `ability_utils::resolve_legal_modal_choice`, evaluated in the triggering-event +//! window. Neither half may re-implement the mode-choice sequence (dynamic cap → +//! non-target unavailability → per-mode target legality → cross-mode assignment +//! cap → CR 603.3c no-legal-mode verdict), because a second copy is free to drift +//! from the announcement and it is the *prompt* the player is bound by. +//! +//! Both halves of the announcement are covered non-vacuously by real cards driven +//! through the production cast pipeline: +//! * the CAP, via Riku's `EventContextSourceModesChosen` "choose up to X" +//! (`max_choices` is event-context-dependent, so it also proves the prompt is +//! built inside the trigger-event window); +//! * the UNAVAILABLE-MODE SET, via Bumi's Earthbend mode, which requires a +//! target land (CR 115.1) that does not exist on an empty board. +//! +//! Oracle text is the same engine-authoritative text used by +//! `riku_modal_modes_chosen_cap.rs`. + +use engine::game::ability_utils::resolve_legal_modal_choice; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::{AnnouncedModalChoice, TargetRef}; +use engine::types::actions::GameAction; +use engine::types::game_state::{CastPaymentMode, WaitingFor}; +use engine::types::identifiers::ObjectId; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; + +const RIKU_ORACLE: &str = "Whenever you cast a modal spell, choose up to X, where X is the number of times you chose a mode for that spell —\n\u{2022} Exile the top card of your library. Until the end of your next turn, you may play it.\n\u{2022} Put a +1/+1 counter on Riku. It gains trample until end of turn.\n\u{2022} Create a 1/1 blue Bird creature token with flying."; + +const ABRADE_ORACLE: &str = + "Choose one \u{2014}\n\u{2022} Abrade deals 3 damage to target creature.\n\u{2022} Destroy target artifact."; + +const BUMI_ORACLE: &str = "When Bumi enters, choose up to X, where X is the number of Lesson cards in your graveyard \u{2014}\n\u{2022} Put three +1/+1 counters on Bumi.\n\u{2022} Target player scries 3.\n\u{2022} Earthbend 3."; + +/// What the paused `AbilityModeChoice` prompt offers the controller. +struct PromptedChoice { + max_choices: usize, + mode_count: usize, + unavailable_modes: Vec, +} + +/// Ask the single authority the same question the engine asked when it announced +/// the choice, using the paused `pending_trigger` as the input the announcement +/// used: the RAW modal header off the card, that trigger's own source/controller, +/// and its triggering event restored as the live event window (what +/// `push_trigger_event_context` does around both halves). +/// +/// Panics if no modal pending trigger is parked — that would mean the prompt was +/// reached without the in-flight trigger the contract is about. +fn authority_answer(runner: &mut GameRunner) -> AnnouncedModalChoice { + let pending = runner + .state() + .pending_trigger + .as_ref() + .expect("a modal pending trigger must still be parked while its mode prompt is outstanding") + .clone(); + let modal = pending + .modal + .as_ref() + .expect("the parked trigger must carry its raw modal header"); + + // Restore the trigger-event window the announcement ran inside. The engine + // restores it after each half, so the paused snapshot has no live event. + let state = runner.state_mut(); + state.current_trigger_event = pending.trigger_event.clone(); + state.current_trigger_events = pending.trigger_event.iter().cloned().collect(); + state.current_trigger_match_count = pending.subject_match_count; + + let answer = resolve_legal_modal_choice( + runner.state(), + pending.source_id, + pending.controller, + modal, + &pending.mode_abilities, + ) + .expect("the authority must report a legal choice for a trigger that reached its mode prompt"); + + let state = runner.state_mut(); + state.current_trigger_event = None; + state.current_trigger_events = Vec::new(); + state.current_trigger_match_count = None; + answer +} + +/// Drive the pipeline until a triggered `AbilityModeChoice` is outstanding, +/// answering the cast's own mode/target windows on the way, and return what the +/// prompt offers. Panics if the run settles at `Priority` (the trigger never +/// fired) so a vacuous pass is impossible. +fn drive_to_mode_prompt( + runner: &mut GameRunner, + modes: &[usize], + targets: &[ObjectId], +) -> PromptedChoice { + let mut remaining_targets = targets.to_vec(); + for _ in 0..128 { + match runner.state().waiting_for.clone() { + WaitingFor::AbilityModeChoice { + modal, + unavailable_modes, + .. + } => { + return PromptedChoice { + max_choices: modal.max_choices, + mode_count: modal.mode_count, + unavailable_modes, + } + } + WaitingFor::ModeChoice { .. } => { + runner + .act(GameAction::SelectModes { + indices: modes.to_vec(), + }) + .expect("SelectModes must be accepted"); + } + WaitingFor::TargetSelection { .. } => { + let target = remaining_targets.remove(0); + runner + .act(GameAction::ChooseTarget { + target: Some(TargetRef::Object(target)), + }) + .expect("ChooseTarget must be accepted"); + } + WaitingFor::OrderTriggers { .. } => { + engine::game::triggers::drain_order_triggers_with_identity(runner.state_mut()); + } + WaitingFor::Priority { .. } => { + runner + .act(GameAction::PassPriority) + .expect("passing priority must be accepted"); + } + other => panic!( + "unexpected WaitingFor while driving to the mode prompt: {}", + other.variant_name() + ), + } + } + panic!("pipeline did not reach a triggered AbilityModeChoice within the step budget"); +} + +/// CAP HALF — CR 603.3d + CR 700.2b: Riku's triggered modal offers a "choose up +/// to X" cap that reads the triggering spell's chosen-mode count +/// (`EventContextSourceModesChosen`). The paused prompt's `max_choices` must be +/// exactly what `resolve_legal_modal_choice` reports, and must be the live +/// event-context value (1 for a one-mode Abrade) rather than 0 or Riku's own +/// `mode_count` — so this also proves the prompt is built with the +/// triggering-event window pushed, as the announcement was. +#[test] +fn prompt_cap_equals_modal_choice_authority() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_creature_from_oracle(P0, "Riku, of Many Paths", 2, 4, RIKU_ORACLE); + let dummy = scenario.add_creature(P1, "Target Dummy", 3, 3).id(); + let abrade = scenario + .add_spell_to_hand_from_oracle(P0, "Abrade", true, ABRADE_ORACLE) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + + let card_id = runner.state().objects[&abrade].card_id; + runner + .act(GameAction::CastSpell { + object_id: abrade, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("CastSpell must be accepted"); + + let prompt = drive_to_mode_prompt(&mut runner, &[0], &[dummy]); + + // Non-vacuous: the event-context cap really resolved off the cast spell. + assert_eq!( + prompt.max_choices, 1, + "Riku's cap must be the 1 mode chosen for Abrade (CR 700.2d), proving the \ + prompt resolved the dynamic cap inside the trigger-event window" + ); + assert_eq!(prompt.mode_count, 3, "Riku's header must parse three modes"); + + let announced = authority_answer(&mut runner); + assert_eq!( + prompt.max_choices, announced.modal.max_choices, + "the prompt's cap must BE the single authority's answer, not an \ + independently derived one (CR 603.3c)" + ); + assert_eq!( + prompt.unavailable_modes, announced.unavailable_modes, + "the prompt's unavailable-mode set must BE the single authority's answer" + ); +} + +/// UNAVAILABLE HALF — CR 603.3c + CR 115.1: Bumi's ETB modal includes +/// "Earthbend 3", which requires a target land. With no land on the battlefield +/// that mode cannot be chosen, so the announcement marks it unavailable. The +/// paused prompt's unavailable set must be exactly the authority's — and must be +/// non-empty here, so the equality is not satisfied by two empty vectors. +#[test] +fn prompt_unavailable_modes_equal_modal_choice_authority() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + // Two Lessons in the graveyard so the dynamic cap resolves to 2 (< mode_count), + // keeping the cap live rather than saturated at the clamp. + for i in 0..2 { + scenario + .add_spell_to_graveyard(P0, &format!("Lesson {i}"), false) + .with_subtypes(vec!["Lesson"]); + } + let bumi = scenario + .add_creature_to_hand_from_oracle(P0, "Bumi, King of Three Trials", 4, 4, BUMI_ORACLE) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + + // Reach-guard: the Earthbend mode is unavailable because NO land exists. + assert!( + !runner.state().objects.values().any(|obj| obj.zone + == engine::types::zones::Zone::Battlefield + && obj + .card_types + .core_types + .contains(&engine::types::card_type::CoreType::Land)), + "the fixture must start with no land on the battlefield for Earthbend to be \ + target-unavailable" + ); + + let card_id = runner.state().objects[&bumi].card_id; + runner + .act(GameAction::CastSpell { + object_id: bumi, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("casting Bumi must be accepted"); + + let prompt = drive_to_mode_prompt(&mut runner, &[], &[]); + + // Non-vacuous: at least one mode really is unavailable. + assert!( + !prompt.unavailable_modes.is_empty(), + "Earthbend needs a target land (CR 115.1); with no land its mode must be \ + announced unavailable, got {:?}", + prompt.unavailable_modes + ); + + let announced = authority_answer(&mut runner); + assert_eq!( + prompt.unavailable_modes, announced.unavailable_modes, + "the prompt's unavailable-mode set must BE the single authority's answer, not \ + an independently derived one (CR 603.3c)" + ); + assert_eq!( + prompt.max_choices, announced.modal.max_choices, + "the prompt's cap must BE the single authority's answer" + ); +} diff --git a/crates/phase-ai/src/policies/anti_self_harm.rs b/crates/phase-ai/src/policies/anti_self_harm.rs index 15b8cc17e0..7cd862f97f 100644 --- a/crates/phase-ai/src/policies/anti_self_harm.rs +++ b/crates/phase-ai/src/policies/anti_self_harm.rs @@ -3316,6 +3316,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); let config = AiConfig::default(); @@ -3427,6 +3428,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); let config = AiConfig::default(); diff --git a/crates/phase-ai/src/policies/evasion_removal_priority.rs b/crates/phase-ai/src/policies/evasion_removal_priority.rs index d91d3afac3..8d6d822b0a 100644 --- a/crates/phase-ai/src/policies/evasion_removal_priority.rs +++ b/crates/phase-ai/src/policies/evasion_removal_priority.rs @@ -629,6 +629,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, })); let config = AiConfig::default(); let slot = TargetSelectionSlot { diff --git a/crates/server-core/src/filter.rs b/crates/server-core/src/filter.rs index 05aac9ebd0..2df369702c 100644 --- a/crates/server-core/src/filter.rs +++ b/crates/server-core/src/filter.rs @@ -506,6 +506,7 @@ mod tests { may_trigger_origin: None, subject_match_count: None, die_result: None, + announced_modal_choice: None, }; PendingTriggerContext { pending,