From 7952325ce2fce109e97ac967fc4d99bcf3f2b42e Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 17:13:00 -0700 Subject: [PATCH 01/17] feat(phase-ai): add draw-matters deck-feature axis + DrawPayoffPolicy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR 121.1: with a "whenever you draw a card" engine on the battlefield (The Locust God, Psychosis Crawler, Niv-Mizzet), every extra draw is a repeatable value trigger. card_advantage values having cards but not triggering the engine, so the AI won't lean into extra draws when it has a payoff out. New DrawMattersFeature axis (structural, no card names): - source_count: card-draw enablers (Effect::Draw scoped to Controller). - payoff_count: permanents with a controller-scoped, non-self TriggerMode::Drawn engine trigger. - commitment: geometric mean over (source, payoff) — both mandatory (draw with no engine is just card advantage; an engine with no extra draw only fires on the natural draw for turn). New DrawPayoffPolicy: on a CastSpell/ActivateAbility that draws the controller a card (its own CastFacts primary/ETB effects, or the activated ability), if the AI controls a live draw engine (structural match over trigger_definitions), score a positive per-engine bonus. Composes with card_advantage. draw_payoff_bonus registered UNTUNED. Tests: 11 feature + 6 policy (incl a registry-routed regression and a name-only-impostor neutral case) + full 1582-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/draw_matters.rs | 159 ++++++++++ crates/phase-ai/src/features/mod.rs | 5 + .../src/features/tests/draw_matters.rs | 200 ++++++++++++ crates/phase-ai/src/features/tests/mod.rs | 1 + crates/phase-ai/src/policies/draw_payoff.rs | 122 ++++++++ crates/phase-ai/src/policies/mod.rs | 1 + crates/phase-ai/src/policies/registry.rs | 3 + .../src/policies/tests/draw_payoff.rs | 286 ++++++++++++++++++ crates/phase-ai/src/policies/tests/mod.rs | 1 + 10 files changed, 790 insertions(+) create mode 100644 crates/phase-ai/src/features/draw_matters.rs create mode 100644 crates/phase-ai/src/features/tests/draw_matters.rs create mode 100644 crates/phase-ai/src/policies/draw_payoff.rs create mode 100644 crates/phase-ai/src/policies/tests/draw_payoff.rs diff --git a/crates/phase-ai/src/config.rs b/crates/phase-ai/src/config.rs index c81cbdf28a..e391be8182 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 121.1: card-equivalent value of drawing into one active "whenever you + /// draw" engine (preference band, per engine). + #[serde(default = "default_draw_payoff_bonus")] + pub draw_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(), + draw_payoff_bonus: default_draw_payoff_bonus(), } } } @@ -666,6 +671,9 @@ fn default_devotion_pip_progress() -> f64 { fn default_devotion_god_activation() -> f64 { 2.5 } +fn default_draw_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.", ), + ( + "draw_payoff_bonus", + "CR 121.1 per-engine draw-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/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs new file mode 100644 index 0000000000..257d85453a --- /dev/null +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -0,0 +1,159 @@ +//! Draw-matters feature — structural detection of a "whenever you draw" engine +//! deck. +//! +//! Parser AST verification — VERIFIED against engine source: +//! - `Effect::Draw { count, target }` at `crates/engine/src/types/ability.rs:10108` +//! — the card-draw enablers (scoped to `TargetFilter::Controller`, i.e. "you +//! draw"). +//! - `TriggerMode::Drawn` at `crates/engine/src/types/triggers.rs:319` +//! (CR 121.1: a card was drawn) — the payoffs. +//! - `TriggerDefinition.valid_target` (`Option`) at `ability.rs` +//! — used to keep only YOUR-draw engines (`None`/`Controller`), excluding the +//! "whenever an opponent draws" punisher shape. +//! +//! No parser remediation required — every axis is expressible over existing +//! typed AST. +//! +//! ## Why this axis exists +//! +//! A deck built around a "whenever you draw a card" engine — The Locust God +//! (make an Insect), Psychosis Crawler / Niv-Mizzet (ping), Chulane — turns +//! every extra draw into a repeatable value trigger (CR 121.1). `card_advantage` +//! values *having* cards, but nothing values *triggering* the engine, so the AI +//! will not lean into extra draws when it has a payoff on the battlefield. This +//! axis lets a policy see that engine. +//! +//! ## Boundary with `card_advantage` / `spellslinger_prowess` +//! +//! `card_advantage` scores the card itself (~1 card-equivalent per draw); +//! this axis adds the *extra* value a draw carries when it also fires an engine +//! — the same split as `CyclingDisciplinePolicy` (patience) vs a payoff policy. +//! `spellslinger_prowess` counts spell-cast triggers; a draw event (CR 121.1) is +//! a disjoint trigger. A card can read on both axes — the overlap is intentional +//! and the axes stay independent. + +use engine::game::DeckEntry; +use engine::types::ability::{AbilityDefinition, Effect, TargetFilter, TriggerDefinition}; +use engine::types::card_type::CoreType; +use engine::types::triggers::TriggerMode; + +use crate::ability_chain::{collect_scoped_effects, AbilityScope}; +use crate::features::commitment; + +/// Commitment at or above which "drawing matters" is a real plan for this deck +/// rather than incidental card advantage. Gates `DrawPayoffPolicy::activation`. +pub const DRAW_MATTERS_FLOOR: f32 = 0.35; + +/// CR 121.1: per-deck draw-matters classification. +/// +/// Populated once per game from `DeckEntry` data. Detection is structural over +/// `CardFace.abilities` and `CardFace.triggers` — never by card name. +#[derive(Debug, Clone, Default)] +pub struct DrawMattersFeature { + /// Cards that draw you extra cards (an `Effect::Draw` scoped to the + /// controller) — the enablers that feed the payoff engine. + pub source_count: u32, + /// Permanents carrying a "whenever you draw a card" engine trigger + /// (CR 121.1), controller-scoped and not self-referential — the payoffs that + /// make extra draws actively good. + pub payoff_count: u32, + /// `0.0..=1.0` — how central drawing-as-a-payoff is to this deck. Consumed by + /// `DrawPayoffPolicy::activation` as the single scaling knob. + pub commitment: f32, +} + +/// Structural detection over each `DeckEntry`'s `CardFace` AST. +pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { + if deck.is_empty() { + return DrawMattersFeature::default(); + } + + let mut source_count = 0u32; + let mut payoff_count = 0u32; + let mut total_nonland = 0u32; + + 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_draw_source_parts(&face.abilities) { + source_count = source_count.saturating_add(entry.count); + } + if is_draw_payoff_parts(&face.triggers) { + payoff_count = payoff_count.saturating_add(entry.count); + } + } + + let commitment = compute_commitment(source_count, payoff_count, total_nonland); + + DrawMattersFeature { + source_count, + payoff_count, + commitment, + } +} + +/// CR 121.1: the abilities draw YOU one or more cards — a repeatable enabler for +/// the payoff engine. `AbilityScope::Potential` walks modal / else branches so a +/// draw mode of a modal spell still counts. Parts-based so it classifies both a +/// deck-time `CardFace.abilities` slice and the action's runtime effect chain +/// (`CastFacts::primary_effects` / the activated ability). +pub(crate) fn is_draw_source_parts<'a>( + abilities: impl IntoIterator, +) -> bool { + abilities.into_iter().any(|ability| { + collect_scoped_effects(ability, AbilityScope::Potential) + .iter() + .any(|effect| matches!(effect, Effect::Draw { target, .. } if draws_controller(target))) + }) +} + +/// CR 121.1: the triggers carry a "whenever you draw a card" engine — a +/// repeatable payoff. 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_draw_payoff_parts<'a>( + triggers: impl IntoIterator, +) -> bool { + triggers.into_iter().any(trigger_is_draw_payoff) +} + +fn trigger_is_draw_payoff(t: &TriggerDefinition) -> bool { + // 1. Mode fires on a draw event (CR 121.1). + if !matches!(t.mode, TriggerMode::Drawn) { + return false; + } + // 2. Your-draw only: "whenever an opponent draws" is a punisher for a + // different deck, not a reason for YOU to draw more. + if !matches!(&t.valid_target, None | Some(TargetFilter::Controller)) { + return false; + } + // 3. Exclude a self-referential "when this is drawn" trigger — that fires + // from hand on the card itself, not a battlefield engine. + !matches!(&t.valid_card, Some(TargetFilter::SelfRef)) +} + +/// True when the draw effect draws the controller cards (you), not an opponent. +fn draws_controller(target: &TargetFilter) -> bool { + matches!(target, TargetFilter::Controller) +} + +/// Calibration: a dedicated draw engine deck (e.g. Izzet "draw-two": ~20 card- +/// draw sources + ~5 engines like The Locust God / Niv-Mizzet over ~36 nonland) +/// → commitment ≈ 0.85. Anti-calibration: a blue midrange deck that runs card +/// draw but no engine → below `DRAW_MATTERS_FLOOR`; an engine with no extra draw, +/// or draw with no engine → 0.0. +/// +/// Geometric mean over (source, payoff): BOTH pillars are mandatory. Card draw +/// with no engine is just card advantage (`card_advantage` governs it); an engine +/// with no way to draw extra only triggers on the natural draw for turn. +fn compute_commitment(source_count: u32, payoff_count: u32, total_nonland: u32) -> f32 { + // ~20 draw sources per 60 nonland is a fully-committed draw shell (card draw + // is common, so this pillar saturates later than a keyword pillar). + let source_density = (commitment::density_per_60(source_count, total_nonland) / 20.0).min(1.0); + // ~5 engine payoffs per 60 nonland is a fully-committed payoff base. + let payoff_density = (commitment::density_per_60(payoff_count, total_nonland) / 5.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..98dc265fd2 100644 --- a/crates/phase-ai/src/features/mod.rs +++ b/crates/phase-ai/src/features/mod.rs @@ -13,6 +13,7 @@ pub mod blink; pub mod commitment; pub mod control; pub mod devotion; +pub mod draw_matters; pub mod enchantments; pub mod energy; pub mod equipment; @@ -37,6 +38,7 @@ pub use artifacts::ArtifactsFeature; pub use blink::BlinkFeature; pub use control::ControlFeature; pub use devotion::DevotionFeature; +pub use draw_matters::DrawMattersFeature; pub use enchantments::EnchantmentsFeature; pub use energy::EnergyFeature; pub use equipment::EquipmentFeature; @@ -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 121.1: "whenever you draw" payoff density (draw sources + engines). + pub draw_matters: DrawMattersFeature, /// 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), + draw_matters: draw_matters::detect(deck), bracket_tier: tier, } } diff --git a/crates/phase-ai/src/features/tests/draw_matters.rs b/crates/phase-ai/src/features/tests/draw_matters.rs new file mode 100644 index 0000000000..291355c7de --- /dev/null +++ b/crates/phase-ai/src/features/tests/draw_matters.rs @@ -0,0 +1,200 @@ +//! Unit tests for `features::draw_matters` — CR 121.1 "whenever you draw" +//! 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::triggers::TriggerMode; + +use crate::features::draw_matters::*; + +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 card-draw enabler: a spell that draws YOU cards (CR 121.1). +fn draw_source(name: &str) -> CardFace { + let mut f = face(name, CoreType::Sorcery); + f.abilities = vec![AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 2 }, + target: TargetFilter::Controller, + }, + )]; + f +} + +fn drawn_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::DealDamage { + amount: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Opponent, + damage_source: None, + excess: None, + }, + )) +} + +/// The Locust God / Niv-Mizzet shape: a "whenever you draw a card" engine on a +/// permanent, controller-scoped and broad. +fn engine(name: &str) -> CardFace { + let mut f = face(name, CoreType::Creature); + f.triggers = vec![drawn_trigger(TriggerMode::Drawn, 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); +} + +#[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_draw_source() { + let f = detect(&[entry(draw_source("Divination"), 4)]); + assert_eq!(f.source_count, 4); +} + +/// A draw effect that draws an OPPONENT is not an enabler for your engine. +#[test] +fn opponent_draw_effect_is_not_a_source() { + let mut f = face("Opponent Draws", CoreType::Sorcery); + f.abilities = vec![AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Opponent, + }, + )]; + assert_eq!(detect(&[entry(f, 4)]).source_count, 0); +} + +#[test] +fn detects_engine_payoff() { + let f = detect(&[entry(engine("The Locust God"), 3)]); + assert_eq!(f.payoff_count, 3); +} + +/// An opponent-scoped "whenever an opponent draws" punisher is not your payoff. +#[test] +fn opponent_scoped_trigger_ignored() { + let mut f = face("Notion Thief", CoreType::Creature); + f.triggers = vec![drawn_trigger( + TriggerMode::Drawn, + None, + Some(TargetFilter::Opponent), + )]; + assert_eq!(detect(&[entry(f, 2)]).payoff_count, 0); +} + +/// A self-referential "when this card is drawn" trigger fires from hand on the +/// card itself, not a battlefield engine — not a payoff. +#[test] +fn self_ref_drawn_trigger_is_not_a_payoff() { + let mut f = face("Drawn Trigger Card", CoreType::Instant); + f.triggers = vec![drawn_trigger( + TriggerMode::Drawn, + Some(TargetFilter::SelfRef), + None, + )]; + assert_eq!(detect(&[entry(f, 4)]).payoff_count, 0); +} + +/// Calibration: a dedicated draw-engine shell clears the floor. +#[test] +fn committed_draw_deck_hits_floor() { + let deck = vec![ + entry(draw_source("Cantrip A"), 12), + entry(draw_source("Cantrip B"), 8), + entry(engine("The Locust God"), 3), + entry(engine("Niv-Mizzet"), 2), + entry(face("Island", CoreType::Land), 24), + ]; + let f = detect(&deck); + assert!( + f.commitment > 0.6, + "committed draw deck must clear 0.6, got {}", + f.commitment + ); +} + +/// Both pillars are mandatory: card draw with no engine is just card advantage. +#[test] +fn sources_without_engine_collapse() { + let deck = vec![ + entry(draw_source("Cantrip"), 20), + entry(face("Island", CoreType::Land), 24), + ]; + assert_eq!(detect(&deck).commitment, 0.0); +} + +/// An engine with no extra draw only triggers on the natural draw for turn. +#[test] +fn engine_without_sources_collapses() { + let deck = vec![ + entry(engine("The Locust God"), 3), + entry(face("Island", CoreType::Land), 24), + ]; + assert_eq!(detect(&deck).commitment, 0.0); +} + +#[test] +fn commitment_clamps_to_one() { + let deck = vec![ + entry(draw_source("Cantrip"), 40), + entry(engine("The Locust God"), 20), + ]; + assert!(detect(&deck).commitment <= 1.0); +} + +/// Boundary: a non-empty all-land deck has `total_nonland == 0`; +/// `density_per_60` guards that to `0.0`, so commitment is a clean `0.0`, never +/// `NaN` (which would slip past the activation floor). +#[test] +fn all_land_deck_is_zero_not_nan() { + let deck = vec![ + entry(face("Island", CoreType::Land), 20), + entry(face("Mountain", CoreType::Land), 20), + ]; + let commitment = detect(&deck).commitment; + assert!(!commitment.is_nan()); + assert_eq!(commitment, 0.0); +} diff --git a/crates/phase-ai/src/features/tests/mod.rs b/crates/phase-ai/src/features/tests/mod.rs index 03acc6775c..da91977ba0 100644 --- a/crates/phase-ai/src/features/tests/mod.rs +++ b/crates/phase-ai/src/features/tests/mod.rs @@ -4,6 +4,7 @@ pub mod artifacts; pub mod blink; pub mod devotion; +pub mod draw_matters; pub mod enchantments; pub mod energy; pub mod equipment; diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs new file mode 100644 index 0000000000..b1c0677691 --- /dev/null +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -0,0 +1,122 @@ +//! `DrawPayoffPolicy` — makes an on-battlefield "whenever you draw" engine a +//! reason the AI can see to draw EAGERLY. +//! +//! ## The gap this closes +//! +//! CR 121.1: with an engine like The Locust God, Psychosis Crawler, or +//! Niv-Mizzet on the battlefield, every card the AI draws is a repeatable value +//! trigger — an Insect token, a point of damage to each opponent. `card_advantage` +//! values the card itself but not the extra trigger, so the AI will not lean into +//! an extra-draw spell or ability when it has a payoff out. This policy adds that +//! positive signal. +//! +//! ## Performance +//! +//! `verdict()` runs per candidate per search node. The card-local check — does +//! this action actually draw the controller a card (its own `CastFacts` +//! primary/ETB effects, or the activated ability's effects) — runs FIRST and +//! rejects every non-draw action. Only a confirmed draw 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. No affordability sweep, no `find_legal_targets`. + +use engine::types::actions::GameAction; +use engine::types::game_state::GameState; +use engine::types::player::PlayerId; + +use crate::features::draw_matters::{ + is_draw_payoff_parts, is_draw_source_parts, DRAW_MATTERS_FLOOR, +}; +use crate::features::DeckFeatures; + +use super::context::PolicyContext; +use super::registry::{DecisionKind, PolicyId, PolicyReason, PolicyVerdict, TacticalPolicy}; + +pub struct DrawPayoffPolicy; + +/// Cap on how many simultaneous engines are rewarded, so a stacked board can't +/// push a single draw into the critical band. +const MAX_REWARDED_ENGINES: usize = 3; + +impl TacticalPolicy for DrawPayoffPolicy { + fn id(&self) -> PolicyId { + PolicyId::DrawPayoff + } + + fn decision_kinds(&self) -> &'static [DecisionKind] { + &[DecisionKind::CastSpell, DecisionKind::ActivateAbility] + } + + fn activation( + &self, + features: &DeckFeatures, + _state: &GameState, + _player: PlayerId, + ) -> Option { + if features.draw_matters.commitment < DRAW_MATTERS_FLOOR { + None + } else { + Some(features.draw_matters.commitment) + } + } + + fn verdict(&self, ctx: &PolicyContext<'_>) -> PolicyVerdict { + // Card-local first: does this action actually draw the controller a card? + if !candidate_draws_controller(ctx) { + return PolicyVerdict::neutral(PolicyReason::new("draw_payoff_na")); + } + + // Only now pay for the battlefield scan. Re-classify each permanent the + // AI controls STRUCTURALLY against its live `trigger_definitions` (CR + // 121.1) — the object must actually carry a "whenever you draw" trigger + // to produce value. + let engines = ctx + .state + .battlefield + .iter() + .filter(|id| { + ctx.state.objects.get(id).is_some_and(|obj| { + obj.controller == ctx.ai_player + && is_draw_payoff_parts( + obj.trigger_definitions + .iter_unchecked() + .map(|entry| &entry.definition), + ) + }) + }) + .count(); + if engines == 0 { + return PolicyVerdict::neutral(PolicyReason::new("draw_payoff_no_engine")); + } + + // Each active engine turns this draw into a value trigger — roughly a + // card-equivalent apiece, capped so one draw stays a preference. + let rewarded = engines.min(MAX_REWARDED_ENGINES) as f64; + PolicyVerdict::score( + ctx.config.policy_penalties.draw_payoff_bonus * rewarded, + PolicyReason::new("draw_payoff_engine_active").with_fact("engines", engines as i64), + ) + } +} + +/// True when the candidate action draws the controller one or more cards. +/// +/// * `CastSpell` → the spell's own resolution chain (`CastFacts::primary_effects`) +/// plus its immediate ETB triggers — a cast permanent's *activated* draw +/// ability does not fire on cast, so only these two are inspected. +/// * `ActivateAbility` → the ability at the runtime-enumerated index. +fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { + match &ctx.candidate.action { + GameAction::CastSpell { .. } => ctx.cast_facts().is_some_and(|facts| { + let etb_bodies = facts + .immediate_etb_triggers + .iter() + .filter_map(|trigger| trigger.execute.as_deref()); + is_draw_source_parts(facts.primary_effects.iter().copied().chain(etb_bodies)) + }), + GameAction::ActivateAbility { .. } => ctx + .effective_activated_ability() + .is_some_and(|ability| is_draw_source_parts(std::iter::once(&ability))), + _ => false, + } +} diff --git a/crates/phase-ai/src/policies/mod.rs b/crates/phase-ai/src/policies/mod.rs index 75686c295e..0061f7816e 100644 --- a/crates/phase-ai/src/policies/mod.rs +++ b/crates/phase-ai/src/policies/mod.rs @@ -18,6 +18,7 @@ mod crew_timing; mod cycling_discipline; mod devotion; mod downside_awareness; +mod draw_payoff; pub(crate) mod effect_classify; mod effect_timing; mod equipment_priority; diff --git a/crates/phase-ai/src/policies/registry.rs b/crates/phase-ai/src/policies/registry.rs index 164fe97a34..46a33a3cee 100644 --- a/crates/phase-ai/src/policies/registry.rs +++ b/crates/phase-ai/src/policies/registry.rs @@ -144,6 +144,8 @@ pub enum PolicyId { CombatWithdrawal, /// CR 608.2c: "return a land you control" self-bounce target choice. SelfBounceTarget, + /// CR 121.1: reward drawing into an on-battlefield "whenever you draw" engine. + DrawPayoff, } /// Coarse routing kind for a candidate decision. Each policy declares which @@ -399,6 +401,7 @@ impl Default for PolicyRegistry { Box::new(PayoffPolicy::new(&BLINK_PAYOFF)), Box::new(LoopShortcutPolicy), Box::new(super::self_bounce_target::SelfBounceTargetPolicy), + Box::new(super::draw_payoff::DrawPayoffPolicy), ]; let mut by_kind: HashMap> = HashMap::new(); for (idx, policy) in policies.iter().enumerate() { diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs new file mode 100644 index 0000000000..1d79372a3a --- /dev/null +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -0,0 +1,286 @@ +//! Unit tests for `policies::draw_payoff` — CR 121.1 "whenever you draw" 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 + `CastSpell` routing). + +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; +use engine::types::game_state::{CastPaymentMode, GameState, WaitingFor}; +use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::player::PlayerId; +use engine::types::triggers::TriggerMode; +use engine::types::zones::Zone; + +use crate::config::AiConfig; +use crate::context::AiContext; +use crate::features::draw_matters::{DrawMattersFeature, DRAW_MATTERS_FLOOR}; +use crate::features::DeckFeatures; +use crate::policies::context::{PolicyContext, SearchDepth}; +use crate::policies::draw_payoff::*; +use crate::policies::registry::{ + PolicyId, PolicyReason, PolicyRegistry, PolicyVerdict, TacticalPolicy, +}; +use crate::session::AiSession; + +const AI: PlayerId = PlayerId(0); +const ENGINE_NAME: &str = "The Locust God"; + +fn state() -> GameState { + GameState::new(FormatConfig::standard(), 2, 42) +} + +/// A hand spell that draws YOU cards on resolution (an `AbilityKind::Spell` +/// Draw effect), plus its `(object_id, card_id)` for the cast candidate. +fn spell(state: &mut GameState, effect: Effect) -> (ObjectId, CardId) { + let card_id = CardId(state.next_object_id); + let id = create_object(state, card_id, AI, "Spell".to_string(), Zone::Hand); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Sorcery); + Arc::make_mut(&mut obj.abilities).push(AbilityDefinition::new(AbilityKind::Spell, effect)); + (id, card_id) +} + +fn draw_spell(state: &mut GameState) -> (ObjectId, CardId) { + spell( + state, + Effect::Draw { + count: QuantityExpr::Fixed { value: 2 }, + target: TargetFilter::Controller, + }, + ) +} + +/// A permanent the AI controls, named `ENGINE_NAME`, carrying `trigger` live +/// `trigger_definitions` (or 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, + 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); + if let Some(trigger) = trigger { + obj.trigger_definitions.push(trigger); + } +} + +fn drawn_engine_trigger() -> TriggerDefinition { + TriggerDefinition::new(TriggerMode::Drawn).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Opponent, + damage_source: None, + excess: None, + }, + )) +} + +fn engine_on_battlefield(state: &mut GameState) { + permanent_with_trigger(state, Some(drawn_engine_trigger())); +} + +fn session(commitment: f32) -> AiSession { + let features = DeckFeatures { + draw_matters: DrawMattersFeature { + source_count: 20, + payoff_count: 4, + commitment, + }, + ..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 cast(object_id: ObjectId, card_id: CardId) -> CandidateAction { + CandidateAction { + action: GameAction::CastSpell { + object_id, + card_id, + targets: Vec::new(), + payment_mode: CastPaymentMode::default(), + }, + metadata: ActionMetadata::for_actor(Some(AI), TacticalClass::Spell), + } +} + +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 priority_decision(candidate: &CandidateAction) -> AiDecisionContext { + AiDecisionContext { + waiting_for: WaitingFor::Priority { player: AI }, + candidates: vec![candidate.clone()], + } +} + +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.draw_matters.commitment = DRAW_MATTERS_FLOOR - 0.01; + assert!(DrawPayoffPolicy + .activation(&features, &state(), AI) + .is_none()); +} + +#[test] +fn activation_opts_in_above_floor() { + let mut features = DeckFeatures::default(); + features.draw_matters.commitment = 0.9; + assert_eq!( + DrawPayoffPolicy.activation(&features, &state(), AI), + Some(0.9) + ); +} + +// ─── verdict ───────────────────────────────────────────────────────────────── + +#[test] +fn rewards_drawing_with_an_active_engine() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!( + delta > 0.0, + "drawing into an engine must be rewarded, got {delta}" + ); +} + +#[test] +fn neutral_without_an_engine_on_board() { + let config = AiConfig::default(); + let mut st = state(); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +#[test] +fn neutral_for_a_non_draw_spell() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + // A burn spell draws nothing. + let (oid, cid) = spell( + &mut st, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Any, + damage_source: None, + excess: None, + }, + ); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// A permanent that merely shares the engine's name but carries no live draw +/// 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); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +// ─── production seam (registry routing) ───────────────────────────────────── + +#[test] +fn registry_registers_the_policy() { + assert!(PolicyRegistry::default().has_policy(PolicyId::DrawPayoff)); +} + +/// End-to-end: casting a draw spell classifies to `DecisionKind::CastSpell`, the +/// policy declares that kind and clears its activation floor, and the +/// engine-active reward comes out of the registry. +#[test] +fn registry_routes_draw_cast_to_the_policy() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = PolicyRegistry::default() + .verdicts(&ctx(&st, &candidate, &decision, &context, &config)) + .into_iter() + .find(|(id, _)| *id == PolicyId::DrawPayoff) + .map(|(_, v)| score_of(v)) + .expect("the draw cast must reach the policy through the registry"); + assert_eq!(reason.kind, "draw_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..e455010e7a 100644 --- a/crates/phase-ai/src/policies/tests/mod.rs +++ b/crates/phase-ai/src/policies/tests/mod.rs @@ -4,6 +4,7 @@ pub mod activation_marker_lint; pub mod artifact_synergy; pub mod blink_payoff; pub mod devotion; +pub mod draw_payoff; pub mod effect_classify_snapshot; pub mod enchantments_payoff; pub mod energy_payoff; From 09edbb149db53dd3bc63388736dfb515b8e74069 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 17:44:38 -0700 Subject: [PATCH 02/17] fix(phase-ai): gate draw payoff on live per-turn trigger eligibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parity with the #6683 review fix: the draw-payoff scan matched an engine only by its structural trigger shape, so a rate-limited "whenever you draw ... only once each turn" engine would still earn a bonus on a second draw even though its trigger can't 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. trigger_is_draw_payoff is exposed as is_draw_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. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/features/draw_matters.rs | 6 +- crates/phase-ai/src/policies/draw_payoff.rs | 31 ++++++++-- .../src/policies/tests/draw_payoff.rs | 61 ++++++++++++++++++- 3 files changed, 89 insertions(+), 9 deletions(-) diff --git a/crates/phase-ai/src/features/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs index 257d85453a..2ed0d8ceb1 100644 --- a/crates/phase-ai/src/features/draw_matters.rs +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -117,10 +117,12 @@ pub(crate) fn is_draw_source_parts<'a>( pub(crate) fn is_draw_payoff_parts<'a>( triggers: impl IntoIterator, ) -> bool { - triggers.into_iter().any(trigger_is_draw_payoff) + triggers.into_iter().any(is_draw_payoff_trigger) } -fn trigger_is_draw_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_draw_payoff_trigger(t: &TriggerDefinition) -> bool { // 1. Mode fires on a draw event (CR 121.1). if !matches!(t.mode, TriggerMode::Drawn) { return false; diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index b1c0677691..33ef82b314 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -24,8 +24,11 @@ use engine::types::actions::GameAction; use engine::types::game_state::GameState; use engine::types::player::PlayerId; +use engine::game::game_object::GameObject; +use engine::types::ability::{TriggerConstraint, TriggerEntry}; + use crate::features::draw_matters::{ - is_draw_payoff_parts, is_draw_source_parts, DRAW_MATTERS_FLOOR, + is_draw_payoff_trigger, is_draw_source_parts, DRAW_MATTERS_FLOOR, }; use crate::features::DeckFeatures; @@ -77,11 +80,10 @@ impl TacticalPolicy for DrawPayoffPolicy { .filter(|id| { ctx.state.objects.get(id).is_some_and(|obj| { obj.controller == ctx.ai_player - && is_draw_payoff_parts( - obj.trigger_definitions - .iter_unchecked() - .map(|entry| &entry.definition), - ) + && obj.trigger_definitions.iter_unchecked().any(|entry| { + is_draw_payoff_trigger(&entry.definition) + && trigger_still_fireable(ctx.state, obj, entry) + }) }) }) .count(); @@ -120,3 +122,20 @@ fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { _ => false, } } + +/// CR 603.4: a rate-limited engine that has already fired this turn/game cannot +/// fire again, so drawing 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: &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/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index 1d79372a3a..dbab69246a 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,7 +9,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; @@ -256,6 +257,64 @@ fn name_only_impostor_without_a_live_trigger_is_neutral() { assert_eq!(delta, 0.0); } +/// A once-per-turn "whenever you draw" engine (Chulane / 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(drawn_engine_trigger().constraint(TriggerConstraint::OncePerTurn)); + id +} + +/// [MED review parity with #6683] A once-per-turn engine that has already fired +/// this turn cannot fire again (CR 603.4), so drawing again earns nothing — the +/// policy consults the fired-trigger ledger, not just the 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); + 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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0, "an unfired once-per-turn engine still rewards"); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From 8af6c31658cf42b10e06cd03ebefdcd894e1afc4 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 18:12:29 -0700 Subject: [PATCH 03/17] fix(phase-ai): scope live draw detection to unconditional effects + complete trigger eligibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review (#6688): two ways the live draw-payoff score was overstated. [MED] A modal/else draw was credited before its mode was selected. `is_draw_source_parts` scanned with `AbilityScope::Potential`, so a modal "choose one — deal damage; OR draw a card" got a draw payoff even when the non-draw mode would be chosen. The predicate now takes the scope: deck-time detection keeps `Potential` (a modal draw still marks the card), but the live candidate scan uses `Unconditional` (CR 700.2) so only a draw that always happens is credited pre-mode-selection. [MED] `trigger_still_fireable` treated every constraint except OncePerTurn/Game as live, rewarding engines that cannot fire. It 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. Tests: modal-draw-not-credited (+ deck-time still counts it), OncePerGame fired/unfired, OnlyDuringYourTurn off-turn/on-turn; full 1590-test phase-ai lib suite pass; clippy -p phase-ai --lib --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/features/draw_matters.rs | 21 ++- .../src/features/tests/draw_matters.rs | 26 ++++ crates/phase-ai/src/policies/draw_payoff.rs | 66 ++++++--- .../src/policies/tests/draw_payoff.rs | 132 ++++++++++++++++++ 4 files changed, 221 insertions(+), 24 deletions(-) diff --git a/crates/phase-ai/src/features/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs index 2ed0d8ceb1..b91c81c645 100644 --- a/crates/phase-ai/src/features/draw_matters.rs +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -37,7 +37,8 @@ use engine::types::ability::{AbilityDefinition, Effect, TargetFilter, TriggerDef use engine::types::card_type::CoreType; use engine::types::triggers::TriggerMode; -use crate::ability_chain::{collect_scoped_effects, AbilityScope}; +use crate::ability_chain::collect_scoped_effects; +pub(crate) use crate::ability_chain::AbilityScope; use crate::features::commitment; /// Commitment at or above which "drawing matters" is a real plan for this deck @@ -78,7 +79,9 @@ pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { total_nonland = total_nonland.saturating_add(entry.count); } - if is_draw_source_parts(&face.abilities) { + // Deck-time: a modal card whose draw lives in a branch still counts as a + // draw enabler for the archetype, so scan the full potential tree. + if is_draw_source_parts(&face.abilities, AbilityScope::Potential) { source_count = source_count.saturating_add(entry.count); } if is_draw_payoff_parts(&face.triggers) { @@ -96,15 +99,19 @@ pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { } /// CR 121.1: the abilities draw YOU one or more cards — a repeatable enabler for -/// the payoff engine. `AbilityScope::Potential` walks modal / else branches so a -/// draw mode of a modal spell still counts. Parts-based so it classifies both a -/// deck-time `CardFace.abilities` slice and the action's runtime effect chain -/// (`CastFacts::primary_effects` / the activated ability). +/// the payoff engine. Parts-based so it classifies both a deck-time +/// `CardFace.abilities` slice and the action's runtime effect chain +/// (`CastFacts::primary_effects` / the activated ability). The caller chooses the +/// `scope`: `Potential` for deck-time (a modal draw mode still marks the card), +/// `Unconditional` for a live candidate before its mode is selected (CR 700.2 — +/// a modal "choose one — draw / …" must NOT be credited a draw until the draw +/// mode is actually chosen). pub(crate) fn is_draw_source_parts<'a>( abilities: impl IntoIterator, + scope: AbilityScope, ) -> bool { abilities.into_iter().any(|ability| { - collect_scoped_effects(ability, AbilityScope::Potential) + collect_scoped_effects(ability, scope) .iter() .any(|effect| matches!(effect, Effect::Draw { target, .. } if draws_controller(target))) }) diff --git a/crates/phase-ai/src/features/tests/draw_matters.rs b/crates/phase-ai/src/features/tests/draw_matters.rs index 291355c7de..08f6a3c666 100644 --- a/crates/phase-ai/src/features/tests/draw_matters.rs +++ b/crates/phase-ai/src/features/tests/draw_matters.rs @@ -113,6 +113,32 @@ fn detects_engine_payoff() { assert_eq!(f.payoff_count, 3); } +/// Deck-time uses `AbilityScope::Potential`: a modal "choose one — burn / draw" +/// card whose draw lives in the `else` branch still marks the card as a draw +/// enabler for the archetype (the policy is the one that must be stricter live). +#[test] +fn modal_draw_mode_still_counts_as_a_deck_source() { + let mut modal = AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Any, + damage_source: None, + excess: None, + }, + ); + modal.else_ability = Some(Box::new(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ))); + let mut f = face("Modal Burn-or-Draw", CoreType::Instant); + f.abilities = vec![modal]; + assert_eq!(detect(&[entry(f, 4)]).source_count, 4); +} + /// An opponent-scoped "whenever an opponent draws" punisher is not your payoff. #[test] fn opponent_scoped_trigger_ignored() { diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index 33ef82b314..380e372326 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -20,15 +20,15 @@ //! `trigger_definitions`), and only in a deck whose `activation` floor is already //! cleared. No affordability sweep, no `find_legal_targets`. +use engine::game::game_object::GameObject; +use engine::types::ability::{TriggerConstraint, TriggerEntry}; use engine::types::actions::GameAction; use engine::types::game_state::GameState; +use engine::types::phase::Phase; use engine::types::player::PlayerId; -use engine::game::game_object::GameObject; -use engine::types::ability::{TriggerConstraint, TriggerEntry}; - use crate::features::draw_matters::{ - is_draw_payoff_trigger, is_draw_source_parts, DRAW_MATTERS_FLOOR, + is_draw_payoff_trigger, is_draw_source_parts, AbilityScope, DRAW_MATTERS_FLOOR, }; use crate::features::DeckFeatures; @@ -108,34 +108,66 @@ impl TacticalPolicy for DrawPayoffPolicy { /// ability does not fire on cast, so only these two are inspected. /// * `ActivateAbility` → the ability at the runtime-enumerated index. fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { + // CR 700.2: a live candidate is scored before its modes are chosen, so only + // an UNCONDITIONAL draw counts — a modal "choose one — draw / …" must not be + // credited a draw here. match &ctx.candidate.action { GameAction::CastSpell { .. } => ctx.cast_facts().is_some_and(|facts| { let etb_bodies = facts .immediate_etb_triggers .iter() .filter_map(|trigger| trigger.execute.as_deref()); - is_draw_source_parts(facts.primary_effects.iter().copied().chain(etb_bodies)) + is_draw_source_parts( + facts.primary_effects.iter().copied().chain(etb_bodies), + AbilityScope::Unconditional, + ) }), - GameAction::ActivateAbility { .. } => ctx - .effective_activated_ability() - .is_some_and(|ability| is_draw_source_parts(std::iter::once(&ability))), + GameAction::ActivateAbility { .. } => { + ctx.effective_activated_ability().is_some_and(|ability| { + is_draw_source_parts(std::iter::once(&ability), AbilityScope::Unconditional) + }) + } _ => false, } } -/// CR 603.4: a rate-limited engine that has already fired this turn/game cannot -/// fire again, so drawing 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 draw-engine trigger `entry` could still fire this turn — so +/// that drawing into it is actually worth something. Exhaustive over +/// `TriggerConstraint` (no wildcard) so a new constraint forces a decision here. +/// +/// - The once/timing constraints are evaluated against authoritative state (the +/// fired-trigger ledgers, `active_player`, and the phase) rather than +/// re-derived. +/// - Constraints whose fireability depends on the triggering event or a per-turn +/// *count* the policy can't cheaply/correctly evaluate at decision time +/// (`MaxTimesPerTurn`, `NthDrawThisTurn`, …) are treated as NOT confirmed, so +/// the payoff is never OVER-credited (the review concern). These are rare on +/// draw engines; the conservative miss is preferable to a false bonus. fn trigger_still_fireable(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. + TriggerConstraint::MaxTimesPerTurn { .. } + | TriggerConstraint::NthSpellThisTurn { .. } + | TriggerConstraint::NthDrawThisTurn { .. } + | TriggerConstraint::OncePerOpponentPerTurn + | TriggerConstraint::AtClassLevel { .. } + | TriggerConstraint::EventSourceControlledBy { .. } => false, } } diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index dbab69246a..d3ef654cdc 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -315,6 +315,138 @@ fn rate_limited_engine_not_yet_fired_rewards() { assert!(delta > 0.0, "an unfired once-per-turn engine still rewards"); } +/// [MED review] A modal "choose one — deal 3 damage; OR draw a card" spell (the +/// draw lives in the `else` branch) is scored before its mode is chosen, so the +/// runtime scan (Unconditional) must NOT credit it a draw. +fn modal_burn_or_draw_spell(state: &mut GameState) -> (ObjectId, CardId) { + let card_id = CardId(state.next_object_id); + let id = create_object(state, card_id, AI, "Modal".to_string(), Zone::Hand); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Instant); + let mut ability = AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Any, + damage_source: None, + excess: None, + }, + ); + ability.else_ability = Some(Box::new(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ))); + Arc::make_mut(&mut obj.abilities).push(ability); + (id, card_id) +} + +#[test] +fn modal_draw_not_credited_before_mode_selected() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = modal_burn_or_draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// An engine trigger with a per-game 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(drawn_engine_trigger().constraint(constraint)); + id +} + +#[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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +#[test] +fn once_per_game_engine_unfired_rewards() { + let config = AiConfig::default(); + let mut st = state(); + engine_with_constraint(&mut st, TriggerConstraint::OncePerGame); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// [MED review] An `OnlyDuringYourTurn` engine on the opponent's turn cannot +/// fire, so an instant-speed draw 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); // the opponent's turn + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringYourTurn); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: the same `OnlyDuringYourTurn` engine on YOUR turn still rewards. +#[test] +fn only_during_your_turn_engine_rewards_on_your_turn() { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = AI; + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringYourTurn); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From db3fe363d886e033f762dd20494a6125c891c7aa Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 21:36:04 -0700 Subject: [PATCH 04/17] fix(phase-ai): engine-owned trigger fireability for draw-payoff (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' four [MED] blockers on #6688 by moving live "whenever you draw" payoff eligibility into a single engine authority that covers the COMPLETE trigger condition, not just selected constraint 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 `check_trigger_constraint_with_ref` (now `event: Option<&_>`; Some = real eval, None = hypothetical → event-dependent constraints report NOT-satisfied rather than guess), rejects an intervening-if `condition` (CR 603.4, conservative), and preflights execution target legality via `ability_utils::execute_targets_satisfiable` (CR 603.3d — a mandatory-target trigger with no legal target produces no effect). - Deleted the policy-local partial `trigger_still_fireable` reimplementation. Policy (draw_payoff.rs): - Live engine scan now `is_draw_payoff_trigger && hypothetical_trigger_fireable`. - Immediate-ETB draw path skips conditional ETBs (`condition.is_none()`), so a Latchkey Faerie prowl cantrip is not credited a draw until it will fire. Feature (draw_matters.rs): - `detect()` now also counts self-ETB draw cantrips (Elvish Visionary) as deck draw sources via `is_etb_draw_source`, matching what the live policy rewards. Tests (external, no cfg(test) in source): - Wizard-Class zero-target neutral / legal-target reward (target legality). - Latchkey conditional-ETB not credited / unconditional-ETB credited. - ActivateAbility draw reward / non-draw neutral. - OnlyDuringOpponentsTurn positive+negative; OnlyDuringYourMainPhase both main phases positive + off-phase (upkeep) negative; MaxTimesPerTurn below/at cap. - Feature: ETB-cantrip source-count regression + opponent-draw control. Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/ability_utils.rs | 26 ++ crates/engine/src/game/triggers.rs | 63 ++- crates/phase-ai/src/features/draw_matters.rs | 26 +- .../src/features/tests/draw_matters.rs | 41 ++ crates/phase-ai/src/policies/draw_payoff.rs | 61 +-- .../src/policies/tests/draw_payoff.rs | 379 +++++++++++++++++- 6 files changed, 532 insertions(+), 64 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 72ac21e955..f1c7716d74 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1311,6 +1311,32 @@ 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 rather than +/// producing an effect, so a payoff-eligibility preflight must not credit it. +/// Complex/undecidable target shapes return `true` (don't under-credit — the +/// caller is asking "could this possibly produce value"). +pub fn execute_targets_satisfiable( + state: &GameState, + source: &crate::game::game_object::GameObject, + execute: &AbilityDefinition, +) -> bool { + let resolved = ResolvedAbility::new( + execute.effect.as_ref().clone(), + Vec::new(), + source.id, + source.controller, + ); + let specs = target_slot_specs(state, &resolved); + if specs.is_empty() { + return true; // the effect requires no target + } + // `Some(false)` = a mandatory target with no legal choice; `Some(true)` = + // legal or optional; `None` = a shape this cheap check can't decide. + simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]).unwrap_or(true) +} + /// 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/features/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs index b91c81c645..5e0dc0e231 100644 --- a/crates/phase-ai/src/features/draw_matters.rs +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -36,6 +36,7 @@ use engine::game::DeckEntry; use engine::types::ability::{AbilityDefinition, Effect, TargetFilter, TriggerDefinition}; use engine::types::card_type::CoreType; use engine::types::triggers::TriggerMode; +use engine::types::zones::Zone; use crate::ability_chain::collect_scoped_effects; pub(crate) use crate::ability_chain::AbilityScope; @@ -80,8 +81,12 @@ pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { } // Deck-time: a modal card whose draw lives in a branch still counts as a - // draw enabler for the archetype, so scan the full potential tree. - if is_draw_source_parts(&face.abilities, AbilityScope::Potential) { + // draw enabler for the archetype, so scan the full potential tree — plus + // ETB "cantrip" triggers (Elvish Visionary), which the live policy also + // credits via `CastFacts::immediate_etb_triggers`. + if is_draw_source_parts(&face.abilities, AbilityScope::Potential) + || is_etb_draw_source(&face.triggers) + { source_count = source_count.saturating_add(entry.count); } if is_draw_payoff_parts(&face.triggers) { @@ -149,6 +154,23 @@ fn draws_controller(target: &TargetFilter) -> bool { matches!(target, TargetFilter::Controller) } +/// CR 603.6a: the face carries a self-ETB "when this enters, draw a card" +/// trigger (Elvish Visionary) — the live policy credits these via +/// `CastFacts::immediate_etb_triggers`, so deck-time detection must count them +/// as draw sources too, or an ETB-cantrip deck is undercounted. +fn is_etb_draw_source(triggers: &[TriggerDefinition]) -> bool { + triggers.iter().any(|t| { + t.mode == TriggerMode::ChangesZone + && t.destination == Some(Zone::Battlefield) + && matches!(t.valid_card, Some(TargetFilter::SelfRef)) + && t.execute.as_deref().is_some_and(|execute| { + collect_scoped_effects(execute, AbilityScope::Potential) + .iter() + .any(|e| matches!(e, Effect::Draw { target, .. } if draws_controller(target))) + }) + }) +} + /// Calibration: a dedicated draw engine deck (e.g. Izzet "draw-two": ~20 card- /// draw sources + ~5 engines like The Locust God / Niv-Mizzet over ~36 nonland) /// → commitment ≈ 0.85. Anti-calibration: a blue midrange deck that runs card diff --git a/crates/phase-ai/src/features/tests/draw_matters.rs b/crates/phase-ai/src/features/tests/draw_matters.rs index 08f6a3c666..9f2e9ef0a3 100644 --- a/crates/phase-ai/src/features/tests/draw_matters.rs +++ b/crates/phase-ai/src/features/tests/draw_matters.rs @@ -8,6 +8,7 @@ use engine::types::ability::{ use engine::types::card::CardFace; use engine::types::card_type::{CardType, CoreType}; use engine::types::triggers::TriggerMode; +use engine::types::zones::Zone; use crate::features::draw_matters::*; @@ -93,6 +94,46 @@ fn detects_draw_source() { assert_eq!(f.source_count, 4); } +/// An ETB "cantrip" creature (Elvish Visionary) — "when this enters, draw a card" +/// — has no `Effect::Draw` in `abilities`, only a self-ETB trigger. The live +/// policy credits these via `CastFacts::immediate_etb_triggers`, so deck-time +/// detection must count them as draw sources too (CR 603.6a), or an ETB-cantrip +/// shell is undercounted. +fn etb_draw_source(name: &str, drawn: TargetFilter) -> CardFace { + let mut f = face(name, CoreType::Creature); + let mut t = TriggerDefinition::new(TriggerMode::ChangesZone) + .valid_card(TargetFilter::SelfRef) + .execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: drawn, + }, + )); + t.destination = Some(Zone::Battlefield); + f.triggers = vec![t]; + f +} + +#[test] +fn etb_cantrip_counts_as_a_draw_source() { + let f = detect(&[entry( + etb_draw_source("Elvish Visionary", TargetFilter::Controller), + 4, + )]); + assert_eq!(f.source_count, 4); +} + +/// Control: an ETB that draws an OPPONENT a card is not an enabler for your engine. +#[test] +fn etb_opponent_draw_is_not_a_source() { + let f = detect(&[entry( + etb_draw_source("Opponent Cantrip", TargetFilter::Opponent), + 4, + )]); + assert_eq!(f.source_count, 0); +} + /// A draw effect that draws an OPPONENT is not an enabler for your engine. #[test] fn opponent_draw_effect_is_not_a_source() { diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index 380e372326..658bbbf598 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -20,11 +20,9 @@ //! `trigger_definitions`), and only in a deck whose `activation` floor is already //! cleared. No affordability sweep, no `find_legal_targets`. -use engine::game::game_object::GameObject; -use engine::types::ability::{TriggerConstraint, TriggerEntry}; +use engine::game::triggers::hypothetical_trigger_fireable; use engine::types::actions::GameAction; use engine::types::game_state::GameState; -use engine::types::phase::Phase; use engine::types::player::PlayerId; use crate::features::draw_matters::{ @@ -69,10 +67,12 @@ impl TacticalPolicy for DrawPayoffPolicy { return PolicyVerdict::neutral(PolicyReason::new("draw_payoff_na")); } - // Only now pay for the battlefield scan. Re-classify each permanent the - // AI controls STRUCTURALLY against its live `trigger_definitions` (CR - // 121.1) — the object must actually carry a "whenever you draw" trigger - // to produce value. + // Only now pay for the battlefield scan. A permanent counts only when it + // carries a "whenever you draw" trigger (CR 121.1) that is actually LIVE: + // the engine's `hypothetical_trigger_fireable` authority preflights the + // trigger's constraint AND its execution target legality (CR 603.3d), so + // a rate-limited, off-timing, conditional, or no-legal-target engine is + // not credited value it cannot produce. let engines = ctx .state .battlefield @@ -82,7 +82,7 @@ impl TacticalPolicy for DrawPayoffPolicy { obj.controller == ctx.ai_player && obj.trigger_definitions.iter_unchecked().any(|entry| { is_draw_payoff_trigger(&entry.definition) - && trigger_still_fireable(ctx.state, obj, entry) + && hypothetical_trigger_fireable(ctx.state, obj, entry) }) }) }) @@ -116,6 +116,10 @@ fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { let etb_bodies = facts .immediate_etb_triggers .iter() + // CR 603.4: an ETB trigger with an intervening-if condition + // (Latchkey Faerie's prowl clause) is not preflighted here, so + // its draw is not credited until it is known it will fire. + .filter(|trigger| trigger.condition.is_none()) .filter_map(|trigger| trigger.execute.as_deref()); is_draw_source_parts( facts.primary_effects.iter().copied().chain(etb_bodies), @@ -130,44 +134,3 @@ fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { _ => false, } } - -/// Whether `obj`'s draw-engine trigger `entry` could still fire this turn — so -/// that drawing into it is actually worth something. Exhaustive over -/// `TriggerConstraint` (no wildcard) so a new constraint forces a decision here. -/// -/// - The once/timing constraints are evaluated against authoritative state (the -/// fired-trigger ledgers, `active_player`, and the phase) rather than -/// re-derived. -/// - Constraints whose fireability depends on the triggering event or a per-turn -/// *count* the policy can't cheaply/correctly evaluate at decision time -/// (`MaxTimesPerTurn`, `NthDrawThisTurn`, …) are treated as NOT confirmed, so -/// the payoff is never OVER-credited (the review concern). These are rare on -/// draw engines; the conservative miss is preferable to a false bonus. -fn trigger_still_fireable(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. - TriggerConstraint::MaxTimesPerTurn { .. } - | TriggerConstraint::NthSpellThisTurn { .. } - | TriggerConstraint::NthDrawThisTurn { .. } - | TriggerConstraint::OncePerOpponentPerTurn - | TriggerConstraint::AtClassLevel { .. } - | TriggerConstraint::EventSourceControlledBy { .. } => false, - } -} diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index d3ef654cdc..f4db93a74e 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,14 +9,15 @@ 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, TriggerConstraint, - TriggerDefinition, + AbilityDefinition, AbilityKind, CastVariantPaid, Effect, QuantityExpr, TargetFilter, + TriggerCondition, TriggerConstraint, TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::format::FormatConfig; use engine::types::game_state::{CastPaymentMode, GameState, WaitingFor}; use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::phase::Phase; use engine::types::player::PlayerId; use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; @@ -78,12 +79,29 @@ fn permanent_with_trigger(state: &mut GameState, trigger: Option TriggerDefinition { TriggerDefinition::new(TriggerMode::Drawn).execute(AbilityDefinition::new( AbilityKind::Spell, - Effect::DealDamage { + Effect::GainLife { amount: QuantityExpr::Fixed { value: 1 }, - target: TargetFilter::Opponent, + player: TargetFilter::Controller, + }, + )) +} + +/// A Wizard-Class shape: a "whenever you draw, deal 3 damage to TARGET creature" +/// payoff whose value depends on a legal target existing (CR 603.3d). +fn drawn_targeted_engine_trigger() -> TriggerDefinition { + TriggerDefinition::new(TriggerMode::Drawn).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: TargetFilter::Typed( + engine::types::ability::TypedFilter::default() + .with_type(engine::types::ability::TypeFilter::Creature), + ), damage_source: None, excess: None, }, @@ -447,6 +465,359 @@ fn only_during_your_turn_engine_rewards_on_your_turn() { assert!(delta > 0.0); } +/// An enchantment engine whose "whenever you draw" trigger targets a creature — +/// value depends on a legal target existing (CR 603.3d). Deliberately NOT a +/// creature itself, so with an empty board the trigger has no legal target. +fn targeted_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::Enchantment); + obj.trigger_definitions + .push(drawn_targeted_engine_trigger()); + id +} + +/// Puts an opponent creature on the battlefield — a legal target for a +/// "target creature" trigger. +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 "whenever you draw" engine with no legal target +/// on the board cannot resolve to an effect, so it is not a live payoff — the +/// engine's `hypothetical_trigger_fireable` target-legality preflight rejects it. +#[test] +fn targeted_engine_with_no_legal_target_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + targeted_engine(&mut st); // enchantment, empty board → no creature to hit + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: once a legal creature target exists, the same targeted engine is live +/// and the draw is rewarded. +#[test] +fn targeted_engine_with_a_legal_target_rewards() { + let config = AiConfig::default(); + let mut st = state(); + targeted_engine(&mut st); + add_opponent_creature(&mut st); // now the "target creature" trigger can resolve + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A `MaxTimesPerTurn { max }` engine that has fired fewer than `max` times this +/// turn can still fire, so the draw is rewarded — the engine authority reads the +/// live `trigger_fire_counts_this_turn` ledger. +#[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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: the same engine that has already fired `max` times this turn cannot +/// fire again, so the draw 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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// An `OnlyDuringYourMainPhase` engine is live during BOTH main phases — the +/// pre-combat and the post-combat main — so a draw in either is rewarded. +#[test] +fn only_during_your_main_phase_rewards_in_both_main_phases() { + for phase in [Phase::PreCombatMain, Phase::PostCombatMain] { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = AI; + st.phase = phase; + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringYourMainPhase); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!( + reason.kind, "draw_payoff_engine_active", + "main phase {phase:?} should be live" + ); + assert!(delta > 0.0, "main phase {phase:?} should reward"); + } +} + +/// An `OnlyDuringOpponentsTurn` engine (a punish-on-their-draw payoff) is live +/// only while it is NOT your turn — a draw during the opponent's turn is +/// rewarded. +#[test] +fn only_during_opponents_turn_rewards_off_turn() { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = PlayerId(1); // the opponent's turn + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringOpponentsTurn); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: the same `OnlyDuringOpponentsTurn` engine on YOUR turn cannot fire, +/// so a draw earns nothing. +#[test] +fn only_during_opponents_turn_is_neutral_on_your_turn() { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = AI; + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringOpponentsTurn); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Negative for the main-phase timing: an `OnlyDuringYourMainPhase` engine during +/// a non-main phase (here, upkeep) cannot fire (CR 505.1), so an instant-speed +/// draw in that step earns nothing. +#[test] +fn only_during_your_main_phase_off_phase_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + st.active_player = AI; + st.phase = Phase::Upkeep; // your turn, but not a main phase + engine_with_constraint(&mut st, TriggerConstraint::OnlyDuringYourMainPhase); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// A permanent-spell creature whose self-ETB trigger draws you a card +/// (Elvish Visionary / Latchkey Faerie), with an optional intervening-if +/// `condition` — `qualifies_immediate_etb` picks it up as a `CastFacts` +/// immediate ETB. +fn etb_draw_spell( + state: &mut GameState, + condition: Option, +) -> (ObjectId, CardId) { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + "Elvish Visionary".to_string(), + Zone::Hand, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + let mut etb = TriggerDefinition::new(TriggerMode::ChangesZone).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + )); + etb.destination = Some(Zone::Battlefield); + etb.valid_card = Some(TargetFilter::SelfRef); + etb.condition = condition; + obj.trigger_definitions.push(etb); + (id, card_id) +} + +/// CR 603.4: Latchkey Faerie's "if its prowl cost was paid, draw a card" ETB is +/// an intervening-if the AI cannot confirm at decision time, so its draw is NOT +/// credited — the cast is treated as a non-draw and earns nothing even with an +/// engine out. +#[test] +fn conditional_etb_draw_is_not_credited() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); // a live engine is present… + let (oid, cid) = etb_draw_spell( + &mut st, + Some(TriggerCondition::CastVariantPaid { + variant: CastVariantPaid::Prowl, + }), + ); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + // …but the conditional ETB is not a confirmed draw, so no engine reward. + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// Control: Elvish Visionary's unconditional "when this enters, draw a card" ETB +/// IS a confirmed draw, so with an engine out the cast is rewarded. +#[test] +fn unconditional_etb_draw_is_credited() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = etb_draw_spell(&mut st, None); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A battlefield permanent whose activated ability at index 0 runs `effect`, plus +/// its id for an `ActivateAbility` candidate. +fn activated_permanent(state: &mut GameState, effect: Effect) -> ObjectId { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + AI, + "Draw Engine".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Artifact); + Arc::make_mut(&mut obj.abilities).push(AbilityDefinition::new(AbilityKind::Activated, effect)); + id +} + +fn activate(source_id: ObjectId, ability_index: usize) -> CandidateAction { + CandidateAction { + action: GameAction::ActivateAbility { + source_id, + ability_index, + }, + metadata: ActionMetadata::for_actor(Some(AI), TacticalClass::Ability), + } +} + +/// An activated ability that draws you a card ("{T}: Draw a card") is a draw +/// action, so with an engine out it is rewarded — covering the policy's second +/// `DecisionKind::ActivateAbility` seam. +#[test] +fn activated_draw_ability_rewards() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source_id = activated_permanent( + &mut st, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ); + let context = context(&config, session(0.9)); + let candidate = activate(source_id, 0); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: a non-draw activated ability (gain life) is not a draw action, so it +/// earns nothing regardless of the engine. +#[test] +fn activated_non_draw_ability_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source_id = activated_permanent( + &mut st, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 2 }, + player: TargetFilter::Controller, + }, + ); + let context = context(&config, session(0.9)); + let candidate = activate(source_id, 0); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From 36a63732c94f1f10c17aea0e7ccd03c669f46167 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 00:21:21 -0700 Subject: [PATCH 05/17] fix(phase-ai): gate draw-payoff on draw-delivery + full multi-target legality (round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' two round-3 [MED] blockers on #6688. 1. Draw-delivery gate (CR 120.3 / CR 101.2). A structural `Effect::Draw { target: Controller }` produces no `CardDrawn` event — and fires no "whenever you draw" engine — when the controller cannot actually draw: under a `CantDraw` static, or with a `PerTurnDrawLimit` already exhausted. `candidate_draws_controller` now gates on a new engine authority, `effects::draw::can_draw_at_least_one`, which reuses the same `allowed_draw_count` gate the delivery path runs — so the bonus is never added to a no-op draw. New coverage: `CantDraw` → `draw_payoff_na`; exhausted per-turn limit → `draw_payoff_na`; limit with headroom → rewarded. 2. Multi-target legality (CR 603.3d). `execute_targets_satisfiable` previously treated the cheap single-slot check's `None` (multi-target or other complex shape) as satisfiable, crediting a mandatory multi-target engine with no legal target assignment. It now falls through to `build_target_slots` + `has_legal_target_assignment_for_ability` (the same full solver production target selection uses), so a multi-target execute with no legal assignment is correctly reported not-live. New coverage: a two-target "exchange control of two permanents" engine on an empty board → `draw_payoff_no_engine`; with two exchangeable permanents → rewarded. `cargo fmt`, `clippy -p engine`/`-p phase-ai` (lib+tests) clean; full phase-ai lib suite green (1609 passed). Engine changes ride behind existing live targeting/draw tests. Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/ability_utils.rs | 31 ++- crates/engine/src/game/effects/draw.rs | 10 + crates/phase-ai/src/policies/draw_payoff.rs | 8 + .../src/policies/tests/draw_payoff.rs | 190 +++++++++++++++++- 4 files changed, 232 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index f1c7716d74..3fd7e38dcd 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1312,11 +1312,16 @@ 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 +/// either need no target at all, or find a legal target assignment right now? A /// mandatory-target trigger with no legal choice is removed rather than /// producing an effect, so a payoff-eligibility preflight must not credit it. -/// Complex/undecidable target shapes return `true` (don't under-credit — the -/// caller is asking "could this possibly produce value"). +/// +/// The cheap single-slot check (`simple_legal_target_assignment_exists_for_ability`) +/// decides the common case; when it cannot (multi-target or another complex +/// shape → `None`), this consults the full legal-assignment authority +/// (`has_legal_target_assignment_for_ability`, the same solver production target +/// selection uses) rather than optimistically assuming legality — so a mandatory +/// multi-target execute with no legal assignment is correctly reported not-live. pub fn execute_targets_satisfiable( state: &GameState, source: &crate::game::game_object::GameObject, @@ -1333,8 +1338,24 @@ pub fn execute_targets_satisfiable( return true; // the effect requires no target } // `Some(false)` = a mandatory target with no legal choice; `Some(true)` = - // legal or optional; `None` = a shape this cheap check can't decide. - simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]).unwrap_or(true) + // legal or optional; `None` = a shape the cheap check can't decide, so build + // the real target slots and run the full legal-assignment search over them + // (the same slots + constraints production target selection uses). + match simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]) { + Some(decided) => decided, + None => match build_target_slots(state, &resolved) { + Ok(slots) if slots.is_empty() => true, + Ok(slots) => { + let constraints = resolved + .sub_ability + .as_ref() + .map(|sub| &sub.target_constraints) + .unwrap_or(&resolved.target_constraints); + has_legal_target_assignment_for_ability(state, &resolved, &slots, constraints) + } + Err(_) => false, + }, + } } /// CR 115.1 + CR 701.9b: Resolve a `Random`-mode ability's target slots by diff --git a/crates/engine/src/game/effects/draw.rs b/crates/engine/src/game/effects/draw.rs index a79487e203..03a4eb8857 100644 --- a/crates/engine/src/game/effects/draw.rs +++ b/crates/engine/src/game/effects/draw.rs @@ -11,6 +11,16 @@ use crate::types::statics::StaticMode; #[cfg(test)] use crate::types::zones::Zone; +/// CR 120.3 + CR 121.1: would drawing a card actually put a card into +/// `player_id`'s hand right now — i.e. is at least one requested draw allowed? +/// False when a `CantDraw` static applies or a `PerTurnDrawLimit` is already +/// exhausted, in which case a "draw a card" action produces no `CardDrawn` event +/// and fires no "whenever you draw" trigger. The single engine authority an AI +/// draw-payoff preflight consults so it never credits a no-op draw. +pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player::PlayerId) -> bool { + allowed_draw_count(state, player_id, 1) >= 1 +} + pub(crate) fn allowed_draw_count( state: &GameState, player_id: crate::types::player::PlayerId, diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index 658bbbf598..f6501c73dd 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -108,6 +108,14 @@ impl TacticalPolicy for DrawPayoffPolicy { /// ability does not fire on cast, so only these two are inspected. /// * `ActivateAbility` → the ability at the runtime-enumerated index. fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { + // CR 120.3 / CR 121.1: a structural "draw a card" produces no `CardDrawn` + // event — and fires no "whenever you draw" engine — when the controller + // can't draw right now (a `CantDraw` static, or a `PerTurnDrawLimit` already + // exhausted). The engine's `can_draw_at_least_one` authority gates the whole + // classification so the bonus is never added to a no-op draw. + if !engine::game::effects::draw::can_draw_at_least_one(ctx.state, ctx.ai_player) { + return false; + } // CR 700.2: a live candidate is scored before its modes are chosen, so only // an UNCONDITIONAL draw counts — a modal "choose one — draw / …" must not be // credited a draw here. diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index f4db93a74e..422a6e7c36 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,8 +9,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, CastVariantPaid, Effect, QuantityExpr, TargetFilter, - TriggerCondition, TriggerConstraint, TriggerDefinition, + AbilityDefinition, AbilityKind, CastVariantPaid, Effect, QuantityExpr, StaticDefinition, + TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -19,6 +19,7 @@ use engine::types::game_state::{CastPaymentMode, GameState, WaitingFor}; use engine::types::identifiers::{CardId, ObjectId}; use engine::types::phase::Phase; use engine::types::player::PlayerId; +use engine::types::statics::{ProhibitionScope, StaticMode}; use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; @@ -818,6 +819,191 @@ fn activated_non_draw_ability_is_neutral() { assert_eq!(delta, 0.0); } +// ─── draw-delivery gate (CR 120.3 / CR 121.1) ──────────────────────────────── + +/// Puts a permanent carrying a static that restricts drawing (Spirit of the +/// Labyrinth / Narset shape) on the battlefield, scoped to `who`. +fn add_draw_restricting_static(state: &mut GameState, mode: StaticMode) { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + PlayerId(1), + "Draw Hoser".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.static_definitions.push(StaticDefinition::new(mode)); +} + +fn set_cards_drawn_this_turn(state: &mut GameState, player: PlayerId, n: u32) { + state + .players + .iter_mut() + .find(|p| p.id == player) + .unwrap() + .cards_drawn_this_turn = n; +} + +/// CR 120.3: under a `CantDraw` static the draw produces no `CardDrawn` event, so +/// the "whenever you draw" engine never fires — the delivery gate makes it a +/// no-op and the bonus is withheld even with the engine on the battlefield. +#[test] +fn cant_draw_static_makes_the_draw_a_no_op() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_restricting_static( + &mut st, + StaticMode::CantDraw { + who: ProhibitionScope::AllPlayers, + }, + ); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// CR 101.2: with a `PerTurnDrawLimit` already exhausted this turn, the extra +/// draw draws nothing, so no engine fires and the bonus is withheld. +#[test] +fn exhausted_per_turn_draw_limit_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_restricting_static( + &mut st, + StaticMode::PerTurnDrawLimit { + who: ProhibitionScope::AllPlayers, + max: 1, + }, + ); + set_cards_drawn_this_turn(&mut st, AI, 1); // already at the cap + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// Control: the same per-turn limit with headroom left still lets a draw through, +/// so the engine is rewarded. +#[test] +fn per_turn_draw_limit_with_headroom_rewards() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_restricting_static( + &mut st, + StaticMode::PerTurnDrawLimit { + who: ProhibitionScope::AllPlayers, + max: 1, + }, + ); + set_cards_drawn_this_turn(&mut st, AI, 0); // one draw still allowed + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +// ─── multi-target engine legality (CR 603.3d) ──────────────────────────────── + +/// A creature `TargetFilter`. +fn creature_filter() -> TargetFilter { + TargetFilter::Typed( + engine::types::ability::TypedFilter::default() + .with_type(engine::types::ability::TypeFilter::Creature), + ) +} + +/// A two-target payoff: "whenever you draw, exchange control of two target +/// permanents". A multi-target mandatory execute the cheap single-slot check +/// can't decide, so the engine authority must consult the full legal-assignment +/// solver (CR 603.3d). Enchantment engine, so an empty board has nothing to hit. +fn 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 obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + obj.trigger_definitions + .push( + TriggerDefinition::new(TriggerMode::Drawn).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::ExchangeControl { + target_a: creature_filter(), + target_b: creature_filter(), + }, + )), + ); + id +} + +/// CR 603.3d: a mandatory MULTI-target engine with no legal target assignment is +/// removed rather than producing an effect — the preflight's cheap single-slot +/// check returns "undecided" here, so it falls through to the full solver, which +/// finds no assignment and reports the engine not-live. +#[test] +fn multi_target_engine_with_no_legal_assignment_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + two_target_engine(&mut st); // empty board → no two permanents to exchange + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: once two exchangeable permanents (one per player) exist, the full +/// solver finds a legal assignment and the multi-target engine is rewarded. +#[test] +fn multi_target_engine_with_a_legal_assignment_rewards() { + let config = AiConfig::default(); + let mut st = state(); + two_target_engine(&mut st); + add_opponent_creature(&mut st); // opponent permanent + // an AI-controlled creature so the exchange has two sides + let card_id = CardId(st.next_object_id); + let mine = create_object(&mut st, card_id, AI, "Bear".to_string(), Zone::Battlefield); + st.objects + .get_mut(&mine) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From dd82d32ddbc58e506309db4a4455226943e0e316 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 00:37:59 -0700 Subject: [PATCH 06/17] fix(engine): preserve source-sensitive constraints in hypothetical trigger authority (round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' AtClassLevel blocker (raised on the sibling cycling PR, same shared authority). `hypothetical_trigger_fireable` 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 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/draw_payoff.rs | 61 +++++++++++++++++++ 2 files changed, 68 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/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index 422a6e7c36..afb11deb18 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -819,6 +819,67 @@ fn activated_non_draw_ability_is_neutral() { assert_eq!(delta, 0.0); } +// ─── source-sensitive constraint: AtClassLevel (CR 716) ────────────────────── + +/// A Class-enchantment engine at `class_level` whose level-gated +/// "whenever you draw" payoff fires only while the Class is at `required_level` +/// (CR 716). The engine 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( + drawn_engine_trigger().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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Control: the same Class engine at a DIFFERENT level cannot fire its +/// level-gated payoff, so the draw 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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── draw-delivery gate (CR 120.3 / CR 121.1) ──────────────────────────────── /// Puts a permanent carrying a static that restricts drawing (Spirit of the From 329f4fbd72937ef074ea7e80ed02e787e01bb81b Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 00:56:48 -0700 Subject: [PATCH 07/17] refactor(engine): align execute_targets_satisfiable with trigger-pipeline ability build (round 5) Build the preflight ability via `build_resolved_from_def` (the same construction the live trigger pipeline uses) instead of wrapping only the root effect, so a sub-ability chain's own target slots are preflighted too (CR 113.1a). The cheap single-slot guard still decides the common case; unknown shapes fall through to the full `has_legal_target_assignment_for_ability` authority, and a slot-building error leaves legality unproven (not credited). Keeps this shared engine authority byte-identical with the sibling cycling PR. Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/ability_utils.rs | 59 ++++++++++--------------- 1 file changed, 24 insertions(+), 35 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 3fd7e38dcd..92174821a6 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1312,50 +1312,39 @@ 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 assignment right now? A -/// mandatory-target trigger with no legal choice is removed rather than -/// producing an effect, so a payoff-eligibility preflight must not credit it. +/// 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. /// -/// The cheap single-slot check (`simple_legal_target_assignment_exists_for_ability`) -/// decides the common case; when it cannot (multi-target or another complex -/// shape → `None`), this consults the full legal-assignment authority -/// (`has_legal_target_assignment_for_ability`, the same solver production target -/// selection uses) rather than optimistically assuming legality — so a mandatory -/// multi-target execute with no legal assignment is correctly reported not-live. +/// 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 { - let resolved = ResolvedAbility::new( - execute.effect.as_ref().clone(), - Vec::new(), - source.id, - source.controller, - ); - let specs = target_slot_specs(state, &resolved); - if specs.is_empty() { + // 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 } - // `Some(false)` = a mandatory target with no legal choice; `Some(true)` = - // legal or optional; `None` = a shape the cheap check can't decide, so build - // the real target slots and run the full legal-assignment search over them - // (the same slots + constraints production target selection uses). - match simple_legal_target_assignment_exists_for_ability(state, &resolved, &[]) { - Some(decided) => decided, - None => match build_target_slots(state, &resolved) { - Ok(slots) if slots.is_empty() => true, - Ok(slots) => { - let constraints = resolved - .sub_ability - .as_ref() - .map(|sub| &sub.target_constraints) - .unwrap_or(&resolved.target_constraints); - has_legal_target_assignment_for_ability(state, &resolved, &slots, constraints) - } - Err(_) => false, - }, + // 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 From 1a606bf0da0feec113d1c166baa99f45b63c90f0 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 01:13:27 -0700 Subject: [PATCH 08/17] fix(phase-ai): gate draw-payoff on library delivery + correct draw CR annotations (round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' two [MED] blockers on the 07:52 review. 1. Empty-library deliverability (CR 121.1 / CR 704.5b). `can_draw_at_least_one` previously checked only `allowed_draw_count` (draw restrictions), so an empty-library candidate was credited a draw even though it delivers no card and emits no `CardDrawn` event (CR 704.5b: an empty-library draw only records an attempted draw). It now also routes through `select_cards_to_draw` — the same delivery authority the resolver uses — so it is true only when a card would actually reach hand (CR 121.1). New coverage: `empty_library_draw_is_a_no_op` (draw_payoff_na) and `nonempty_library_draw_rewards`; the test `state()` seeds a non-empty library so existing positive cases stay deliverable. 2. CR annotation correction. The draw-delivery code and tests cited `CR 120.3`, which is the damage-results rule. Replaced with the verified draw rules: `CR 121.1` (drawing = top card of library to hand) and `CR 704.5b` (attempted empty-library draw) in draw.rs, draw_payoff.rs, and the tests. `cargo fmt`, `clippy -p engine`/`-p phase-ai` (lib+tests) clean; draw suite green (71 assertions). Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/effects/draw.rs | 18 +++-- crates/phase-ai/src/policies/draw_payoff.rs | 8 ++- .../src/policies/tests/draw_payoff.rs | 66 ++++++++++++++++++- 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/game/effects/draw.rs b/crates/engine/src/game/effects/draw.rs index 03a4eb8857..1d5edbc8ac 100644 --- a/crates/engine/src/game/effects/draw.rs +++ b/crates/engine/src/game/effects/draw.rs @@ -11,14 +11,18 @@ use crate::types::statics::StaticMode; #[cfg(test)] use crate::types::zones::Zone; -/// CR 120.3 + CR 121.1: would drawing a card actually put a card into -/// `player_id`'s hand right now — i.e. is at least one requested draw allowed? -/// False when a `CantDraw` static applies or a `PerTurnDrawLimit` is already -/// exhausted, in which case a "draw a card" action produces no `CardDrawn` event -/// and fires no "whenever you draw" trigger. The single engine authority an AI -/// draw-payoff preflight consults so it never credits a no-op draw. +/// CR 121.1 + CR 704.5b: would drawing a card actually put a card into +/// `player_id`'s hand right now (CR 121.1: a draw moves the top library card to +/// hand)? False when a `CantDraw` static applies or a `PerTurnDrawLimit` is +/// exhausted (no draw permitted), AND false when the library is empty — an +/// empty-library draw only records an attempted draw (CR 704.5b) and delivers no +/// card, so it produces no `CardDrawn` event and fires no "whenever you draw" +/// trigger. Routes through the same `allowed_draw_count` gate and +/// `select_cards_to_draw` delivery authority the resolver uses, so an AI +/// draw-payoff preflight never credits a no-op draw. pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player::PlayerId) -> bool { - allowed_draw_count(state, player_id, 1) >= 1 + let allowed = allowed_draw_count(state, player_id, 1); + !select_cards_to_draw(state, player_id, allowed as usize).is_empty() } pub(crate) fn allowed_draw_count( diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index f6501c73dd..8224215c8a 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -108,11 +108,13 @@ impl TacticalPolicy for DrawPayoffPolicy { /// ability does not fire on cast, so only these two are inspected. /// * `ActivateAbility` → the ability at the runtime-enumerated index. fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { - // CR 120.3 / CR 121.1: a structural "draw a card" produces no `CardDrawn` + // CR 121.1 / CR 704.5b: a structural "draw a card" produces no `CardDrawn` // event — and fires no "whenever you draw" engine — when the controller // can't draw right now (a `CantDraw` static, or a `PerTurnDrawLimit` already - // exhausted). The engine's `can_draw_at_least_one` authority gates the whole - // classification so the bonus is never added to a no-op draw. + // exhausted) OR the library is empty (an empty-library draw only records an + // attempt, CR 704.5b, delivering no card). The engine's `can_draw_at_least_one` + // authority gates the whole classification so the bonus is never added to a + // no-op draw. if !engine::game::effects::draw::can_draw_at_least_one(ctx.state, ctx.ai_player) { return false; } diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index afb11deb18..bba7c2c7fb 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -38,7 +38,26 @@ const AI: PlayerId = PlayerId(0); const ENGINE_NAME: &str = "The Locust God"; fn state() -> GameState { - GameState::new(FormatConfig::standard(), 2, 42) + let mut st = GameState::new(FormatConfig::standard(), 2, 42); + // Deliverable draws by default: seed the AI a non-empty library so a draw + // actually puts a card into hand (CR 121.1). Empty-library behavior is + // exercised explicitly by clearing this in the dedicated test. + seed_library(&mut st, AI, 3); + st +} + +/// Puts `n` cards into `player`'s library so draws are deliverable. +fn seed_library(state: &mut GameState, player: PlayerId, n: usize) { + for _ in 0..n { + let card_id = CardId(state.next_object_id); + create_object( + state, + card_id, + player, + "Library Card".to_string(), + Zone::Library, + ); + } } /// A hand spell that draws YOU cards on resolution (an `AbilityKind::Spell` @@ -880,7 +899,7 @@ fn at_class_level_engine_at_wrong_level_is_neutral() { assert_eq!(delta, 0.0); } -// ─── draw-delivery gate (CR 120.3 / CR 121.1) ──────────────────────────────── +// ─── draw-delivery gate (CR 121.1 / CR 704.5b) ─────────────────────────────── /// Puts a permanent carrying a static that restricts drawing (Spirit of the /// Labyrinth / Narset shape) on the battlefield, scoped to `who`. @@ -907,7 +926,7 @@ fn set_cards_drawn_this_turn(state: &mut GameState, player: PlayerId, n: u32) { .cards_drawn_this_turn = n; } -/// CR 120.3: under a `CantDraw` static the draw produces no `CardDrawn` event, so +/// CR 121.1: under a `CantDraw` static the draw produces no `CardDrawn` event, so /// the "whenever you draw" engine never fires — the delivery gate makes it a /// no-op and the bonus is withheld even with the engine on the battlefield. #[test] @@ -981,6 +1000,47 @@ fn per_turn_draw_limit_with_headroom_rewards() { assert!(delta > 0.0); } +/// CR 704.5b: with an empty library, a "draw a card" only records an attempted +/// draw (a state-based loss) and puts no card into hand — no `CardDrawn` event, +/// so the engine never fires. The delivery preflight withholds the bonus. +#[test] +fn empty_library_draw_is_a_no_op() { + let config = AiConfig::default(); + let mut st = state(); + st.players + .iter_mut() + .find(|p| p.id == AI) + .unwrap() + .library + .clear(); // empty deck + engine_on_battlefield(&mut st); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// Control: with cards left in the library the draw is deliverable (CR 121.1), +/// so the engine is rewarded. (The default `state()` seeds a non-empty library.) +#[test] +fn nonempty_library_draw_rewards() { + let config = AiConfig::default(); + let mut st = state(); // seeded library + engine_on_battlefield(&mut st); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + // ─── multi-target engine legality (CR 603.3d) ──────────────────────────────── /// A creature `TargetFilter`. From a129a921620d117c1c0b12e9a7cbb252b914650a Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 01:31:26 -0700 Subject: [PATCH 09/17] 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 Two shared-authority fixes (also applied identically to the sibling cycling PR). 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_draw_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` preflighted the root effect's per-slot filters but sent no cross-target constraints, so a constrained multi-target execute (e.g. "two permanents controlled by different players") was judged against a broader space than the live trigger receives (`PendingTrigger::target_constraints`). It now threads `execute.target_constraints` into both the cheap and full solvers (CR 115.1 / CR 601.2c). Corrected the preflight annotation from CR 113.1a (defines abilities) to CR 603.3d (removal for no legal choice). Coverage: no-execute and unsupported-execute engines → `draw_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/draw_matters.rs | 12 +- .../src/features/tests/draw_matters.rs | 25 ++++ .../src/policies/tests/draw_payoff.rs | 126 +++++++++++++++++- 5 files changed, 214 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 cf1341a00d..81d1e21ddf 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -8165,12 +8165,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/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs index 5e0dc0e231..21ee586ecf 100644 --- a/crates/phase-ai/src/features/draw_matters.rs +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -32,6 +32,7 @@ //! a disjoint trigger. A card can read on both axes — the overlap is intentional //! and the axes stay independent. +use engine::game::ability_utils::ability_definition_supported; use engine::game::DeckEntry; use engine::types::ability::{AbilityDefinition, Effect, TargetFilter, TriggerDefinition}; use engine::types::card_type::CoreType; @@ -146,7 +147,16 @@ pub(crate) fn is_draw_payoff_trigger(t: &TriggerDefinition) -> bool { } // 3. Exclude a self-referential "when this is drawn" trigger — that fires // from hand on the card itself, not a battlefield engine. - !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) } /// True when the draw effect draws the controller cards (you), not an opponent. diff --git a/crates/phase-ai/src/features/tests/draw_matters.rs b/crates/phase-ai/src/features/tests/draw_matters.rs index 9f2e9ef0a3..937d2111c1 100644 --- a/crates/phase-ai/src/features/tests/draw_matters.rs +++ b/crates/phase-ai/src/features/tests/draw_matters.rs @@ -154,6 +154,31 @@ fn detects_engine_payoff() { assert_eq!(f.payoff_count, 3); } +/// A "whenever you draw" trigger with NO execute is a `TriggerNoExecute` no-op — +/// it produces 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::Creature); + f.triggers = vec![TriggerDefinition::new(TriggerMode::Drawn)]; // no execute + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); +} + +/// A "whenever you draw" 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::Creature); + f.triggers = vec![ + TriggerDefinition::new(TriggerMode::Drawn).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("draw_payoff_test_gap", "unsupported payoff"), + )), + ]; + assert_eq!(detect(&[entry(f, 3)]).payoff_count, 0); +} + /// Deck-time uses `AbilityScope::Potential`: a modal "choose one — burn / draw" /// card whose draw lives in the `else` branch still marks the card as a draw /// enabler for the archetype (the policy is the one that must be stricter live). diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index bba7c2c7fb..249d9ab2d8 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -15,7 +15,9 @@ 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::{CastPaymentMode, GameState, WaitingFor}; +use engine::types::game_state::{ + CastPaymentMode, GameState, TargetSelectionConstraint, WaitingFor, +}; use engine::types::identifiers::{CardId, ObjectId}; use engine::types::phase::Phase; use engine::types::player::PlayerId; @@ -1125,6 +1127,128 @@ fn multi_target_engine_with_a_legal_assignment_rewards() { assert!(delta > 0.0); } +/// 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 two-target "exchange control of two target permanents controlled by +/// DIFFERENT players" engine — the execute carries a +/// `DifferentObjectControllers` cross-target constraint (CR 115.1). The preflight +/// must honor that constraint, not just the per-slot filters. +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::Drawn).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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: one permanent per player satisfies `DifferentObjectControllers`, so +/// the constrained engine is live and the draw 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 (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// A "whenever you draw" 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::Drawn))); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// A "whenever you draw" 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::Drawn).execute(AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("draw_payoff_test_gap", "unsupported payoff"), + )), + ), + ); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + // ─── production seam (registry routing) ───────────────────────────────────── #[test] From a5e2dfeabb666d1b805b087a7c3924b1e0725137 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 02:19:03 -0700 Subject: [PATCH 10/17] fix(phase-ai): replacement-aware draw preflight + registry-routed activated-draw coverage (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses matthewevans' two round-8 [MED] blockers on #6688. 1. Replacement-aware deliverability (CR 614.1 / CR 614.6 / CR 614.11). `can_draw_at_least_one` checked draw restrictions and library delivery but not the replacement pipeline, so a candidate whose individual draw is obligatorily replaced (a mandatory `QuantityModification::Prevent` draw replacement — Living Conundrum's "skip that draw instead") was still credited even though the replaced event never happens and no `CardDrawn` fires. It now also scans `active_replacements` and conservatively blocks delivery when a mandatory prevent-draw replacement (`ReplacementEvent::Draw`/`DrawCards` + `QuantityModification::Prevent`) is active. New case: `mandatory_prevent_draw_replacement_is_a_no_op` → `draw_payoff_na`. 2. Registry-routed activated-draw coverage. The activated-draw assertions called `verdict` directly; only a cast was registry-routed. Added `registry_routes_activated_draw_to_the_policy` (a `GameAction::ActivateAbility` draw routed through `PolicyRegistry::verdicts` → rewarded) and `registry_activated_non_draw_is_not_rewarded` (control), so a regression in the action classifier / decision-kind registration / `by_kind` dispatch can't leave activated draws silently unscored. `cargo fmt`, `clippy -p engine`/`-p phase-ai` (lib+tests) clean; draw suite green (80 assertions). Co-Authored-By: Claude Opus 4.8 --- crates/engine/src/game/effects/draw.rs | 45 ++++++-- .../src/policies/tests/draw_payoff.rs | 100 +++++++++++++++++- 2 files changed, 133 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/game/effects/draw.rs b/crates/engine/src/game/effects/draw.rs index 1d5edbc8ac..f961ce905c 100644 --- a/crates/engine/src/game/effects/draw.rs +++ b/crates/engine/src/game/effects/draw.rs @@ -11,18 +11,43 @@ use crate::types::statics::StaticMode; #[cfg(test)] use crate::types::zones::Zone; -/// CR 121.1 + CR 704.5b: would drawing a card actually put a card into -/// `player_id`'s hand right now (CR 121.1: a draw moves the top library card to -/// hand)? False when a `CantDraw` static applies or a `PerTurnDrawLimit` is -/// exhausted (no draw permitted), AND false when the library is empty — an -/// empty-library draw only records an attempted draw (CR 704.5b) and delivers no -/// card, so it produces no `CardDrawn` event and fires no "whenever you draw" -/// trigger. Routes through the same `allowed_draw_count` gate and -/// `select_cards_to_draw` delivery authority the resolver uses, so an AI -/// draw-payoff preflight never credits a no-op draw. +/// CR 121.1 + CR 704.5b + CR 614.6: would drawing a card actually put a card into +/// `player_id`'s hand right now, emitting a `GameEvent::CardDrawn`? False when: +/// - a `CantDraw` static applies or a `PerTurnDrawLimit` is exhausted (no draw +/// permitted); or +/// - the library is empty — an empty-library draw only records an attempted +/// draw (CR 704.5b) and delivers no card; or +/// - a mandatory `Prevent` draw replacement is active (CR 614.6: "skip that draw +/// instead", Living Conundrum) — the replaced draw event never happens. +/// +/// In each case the draw fires no "whenever you draw" trigger. Routes through the +/// same `allowed_draw_count` gate and `select_cards_to_draw` delivery authority +/// the resolver uses; the replacement leg is conservative — a payoff preflight +/// can't establish the post-replacement draw event without running the +/// state-mutating pipeline, so an active mandatory prevent-draw replacement +/// blocks delivery. The single engine authority an AI draw-payoff preflight +/// consults so it never credits a no-op draw. pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player::PlayerId) -> bool { + use crate::types::ability::QuantityModification; + use crate::types::replacements::ReplacementEvent; let allowed = allowed_draw_count(state, player_id, 1); - !select_cards_to_draw(state, player_id, allowed as usize).is_empty() + if select_cards_to_draw(state, player_id, allowed as usize).is_empty() { + return false; + } + // CR 614.1 / CR 614.6 / CR 614.11: a mandatory draw replacement that prevents + // the draw suppresses the event entirely. Conservatively block delivery when + // one is active rather than assume the draw survives. + let mandatory_prevent = + crate::game::functioning_abilities::active_replacements(state).any(|(_, _, def)| { + matches!( + def.event, + ReplacementEvent::Draw | ReplacementEvent::DrawCards + ) && matches!( + def.quantity_modification, + Some(QuantityModification::Prevent) + ) + }); + !mandatory_prevent } pub(crate) fn allowed_draw_count( diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index 249d9ab2d8..d19fed24b0 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,8 +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, CastVariantPaid, Effect, QuantityExpr, StaticDefinition, - TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, + AbilityDefinition, AbilityKind, CastVariantPaid, Effect, QuantityExpr, QuantityModification, + ReplacementDefinition, StaticDefinition, TargetFilter, TriggerCondition, TriggerConstraint, + TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -21,6 +22,7 @@ use engine::types::game_state::{ use engine::types::identifiers::{CardId, ObjectId}; use engine::types::phase::Phase; use engine::types::player::PlayerId; +use engine::types::replacements::ReplacementEvent; use engine::types::statics::{ProhibitionScope, StaticMode}; use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; @@ -1043,6 +1045,43 @@ fn nonempty_library_draw_rewards() { assert!(delta > 0.0); } +/// Puts a permanent carrying a mandatory `Prevent` draw replacement (Living +/// Conundrum shape) on the battlefield. +fn add_prevent_draw_replacement(state: &mut GameState) { + let card_id = CardId(state.next_object_id); + let id = create_object( + state, + card_id, + PlayerId(1), + "Living Conundrum".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw); + repl.quantity_modification = Some(QuantityModification::Prevent); + obj.replacement_definitions.push(repl); +} + +/// CR 614.6: a mandatory `Prevent` draw replacement suppresses the draw entirely +/// — the replaced event never happens, so no `CardDrawn` fires and the engine +/// never triggers. The delivery preflight withholds the bonus. +#[test] +fn mandatory_prevent_draw_replacement_is_a_no_op() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st); + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + // ─── multi-target engine legality (CR 603.3d) ──────────────────────────────── /// A creature `TargetFilter`. @@ -1277,3 +1316,60 @@ fn registry_routes_draw_cast_to_the_policy() { assert_eq!(reason.kind, "draw_payoff_engine_active"); assert!(delta > 0.0, "routed reward must be positive, got {delta}"); } + +/// End-to-end: an activated DRAW ability routes through `DecisionKind::ActivateAbility` +/// to the policy and is rewarded — covering the second decision kind the policy +/// registers, not just the direct-`verdict` path. +#[test] +fn registry_routes_activated_draw_to_the_policy() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source_id = activated_permanent( + &mut st, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ); + let context = context(&config, session(0.9)); + let candidate = activate(source_id, 0); + let decision = priority_decision(&candidate); + let (delta, reason) = PolicyRegistry::default() + .verdicts(&ctx(&st, &candidate, &decision, &context, &config)) + .into_iter() + .find(|(id, _)| *id == PolicyId::DrawPayoff) + .map(|(_, v)| score_of(v)) + .expect("the activated draw must reach the policy through the registry"); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0, "routed reward must be positive, got {delta}"); +} + +/// Control: an activated NON-draw ability routes to the policy but is not +/// rewarded — guards against the classifier crediting every activation. +#[test] +fn registry_activated_non_draw_is_not_rewarded() { + let config = AiConfig::default(); + let mut st = state(); + engine_on_battlefield(&mut st); + let source_id = activated_permanent( + &mut st, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 2 }, + player: TargetFilter::Controller, + }, + ); + let context = context(&config, session(0.9)); + let candidate = activate(source_id, 0); + let decision = priority_decision(&candidate); + let routed = PolicyRegistry::default() + .verdicts(&ctx(&st, &candidate, &decision, &context, &config)) + .into_iter() + .find(|(id, _)| *id == PolicyId::DrawPayoff) + .map(|(_, v)| score_of(v)); + // Either the policy is absent for this action, or it returns a neutral verdict. + if let Some((delta, reason)) = routed { + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); + } +} From 8b4c6202553e089201fe914d05f5ba1ca9aa2ad7 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Mon, 27 Jul 2026 02:49:04 -0700 Subject: [PATCH 11/17] fix(PR-6688): freeze draw replacement test producer --- crates/phase-ai/src/policies/tests/draw_payoff.rs | 9 +++++---- scripts/draw-replacement-producers.txt | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index d19fed24b0..1f9ee3154a 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,9 +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, CastVariantPaid, Effect, QuantityExpr, QuantityModification, - ReplacementDefinition, StaticDefinition, TargetFilter, TriggerCondition, TriggerConstraint, - TriggerDefinition, + AbilityDefinition, AbilityKind, CastVariantPaid, DrawReplacementScope, Effect, QuantityExpr, + QuantityModification, ReplacementDefinition, StaticDefinition, TargetFilter, TriggerCondition, + TriggerConstraint, TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -1058,7 +1058,8 @@ fn add_prevent_draw_replacement(state: &mut GameState) { ); let obj = state.objects.get_mut(&id).unwrap(); obj.card_types.core_types.push(CoreType::Enchantment); - let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw); + let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw) + .draw_scope(DrawReplacementScope::IndividualDraw); repl.quantity_modification = Some(QuantityModification::Prevent); obj.replacement_definitions.push(repl); } diff --git a/scripts/draw-replacement-producers.txt b/scripts/draw-replacement-producers.txt index 054a98dbf6..2c29de197e 100644 --- a/scripts/draw-replacement-producers.txt +++ b/scripts/draw-replacement-producers.txt @@ -53,3 +53,4 @@ crates/engine/src/parser/oracle_replacement.rs parse_conditional_draw_replacemen crates/engine/src/parser/oracle_replacement.rs parse_replacement_line_inner constructor 1 crates/engine/src/types/replacements.rs from_str event-decode 2 crates/mtgish-import/src/convert/replacement.rs convert_replace_would_draw struct-literal 1 +crates/phase-ai/src/policies/tests/draw_payoff.rs add_prevent_draw_replacement constructor 1 From 3c868634621972bdea98cea56d9ee1bb6daaedee Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 06:23:25 -0700 Subject: [PATCH 12/17] fix(engine+phase-ai): route draw preflight through the live replacement authority + modal-aware execute preflight (round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Draw preflight (review blocker). `can_draw_at_least_one` scanned `active_replacements` by event + `QuantityModification::Prevent` alone, so any functioning prevent-draw definition — opponent-scoped, conditional, optional, or a `DrawCards` stub — made the AI classify a deliverable draw as a no-op and withhold the payoff bonus. It now models the individual draw as the `ProposedEvent::Draw` the live path proposes (CR 121.2) and consults the live applicability authority, so source/player scope, the condition gate, the handler matcher, and optionality are all honored. - `replacement.rs`: parameterize `counter_placement_prevention_applies` into `mandatory_prevention_applies(state, candidates, events)` and expose a pure, read-only preflight `event_is_mandatorily_prevented(state, event)` that routes through `find_applicable_replacements` (CR 614.1 / CR 614.6). Only a MANDATORY Prevent suppresses the event — an optional one is an accept/decline choice. Modal execute preflight. A modal ability's root is an `Effect::Unimplemented` placeholder with its real effects in `mode_abilities`, so round 7's support check reported every modal payoff unsupported, and the target preflight never descended into the modes. - `ability_utils.rs`: `ability_definition_supported` treats an `Unimplemented` root as a gap only on a non-modal ability (CR 700.2); modes are still checked recursively. `execute_targets_satisfiable` mirrors the live trigger dispatch for modal executes by delegating to `filter_modes_by_target_legality` + `modal_choice_with_target_assignment_limit` (CR 603.3c / CR 603.3d). Tests. Five discriminating draw-replacement policy cases, all fail-on-revert against the old scan: the mandatory prevent now scoped to the DRAWING player, plus opponent-scoped / false-conditional / optional / `DrawCards`-stub positive controls. Registered engine dispatch tests assert the live `DroppedNoLegalMode` contract the modal preflight mirrors. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/ability_utils.rs | 37 +++- crates/engine/src/game/effects/draw.rs | 41 ++-- crates/engine/src/game/replacement.rs | 40 +++- crates/engine/src/game/triggers.rs | 107 ++++++++++ .../src/policies/tests/draw_payoff.rs | 186 ++++++++++++++++-- 5 files changed, 367 insertions(+), 44 deletions(-) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index e917f86684..02fc297767 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -1329,6 +1329,37 @@ 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 its targets in + // `mode_abilities` (which the root slot walk does not descend). Mirror the + // live trigger dispatch: filter each mode by its own target legality, then + // require a legal modal choice — a required "choose one/two …" whose modes + // are all target-unavailable is dropped (`DroppedNoLegalMode`), so it is not + // a live payoff. + if let Some(modal) = &execute.modal { + let mut unavailable_modes = Vec::new(); + filter_modes_by_target_legality( + state, + source.id, + source.controller, + &execute.mode_abilities, + modal, + &mut unavailable_modes, + ); + if unavailable_modes.len() >= modal.mode_count { + return false; // CR 603.3c: no legal mode + } + // CR 603.3d: the required choose-count must be satisfiable with legal + // target assignments across the surviving modes. + return modal_choice_with_target_assignment_limit( + state, + source.id, + source.controller, + modal, + &execute.mode_abilities, + &unavailable_modes, + ) + .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 +1394,11 @@ 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. Only an `Unimplemented` root on a + // non-modal ability is a true unsupported node. + if matches!(*def.effect, Effect::Unimplemented { .. }) && def.modal.is_none() { return false; } if def diff --git a/crates/engine/src/game/effects/draw.rs b/crates/engine/src/game/effects/draw.rs index f961ce905c..42d91fd59a 100644 --- a/crates/engine/src/game/effects/draw.rs +++ b/crates/engine/src/game/effects/draw.rs @@ -17,37 +17,32 @@ use crate::types::zones::Zone; /// permitted); or /// - the library is empty — an empty-library draw only records an attempted /// draw (CR 704.5b) and delivers no card; or -/// - a mandatory `Prevent` draw replacement is active (CR 614.6: "skip that draw -/// instead", Living Conundrum) — the replaced draw event never happens. +/// - a mandatory `Prevent` draw replacement applies to *this player's* draw +/// (CR 614.6: "skip that draw instead", Living Conundrum) — the replaced draw +/// event never happens. /// /// In each case the draw fires no "whenever you draw" trigger. Routes through the /// same `allowed_draw_count` gate and `select_cards_to_draw` delivery authority -/// the resolver uses; the replacement leg is conservative — a payoff preflight -/// can't establish the post-replacement draw event without running the -/// state-mutating pipeline, so an active mandatory prevent-draw replacement -/// blocks delivery. The single engine authority an AI draw-payoff preflight -/// consults so it never credits a no-op draw. +/// the resolver uses, and models the individual draw as the `ProposedEvent::Draw` +/// the live path proposes so the replacement leg consults the live applicability +/// authority (`replacement::event_is_mandatorily_prevented`) rather than scanning +/// definitions by event alone. An unrelated or opponent-scoped source, a false +/// conditional, and an optional ("may") replacement therefore all leave the draw +/// deliverable. The single engine authority an AI draw-payoff preflight consults +/// so it never credits a no-op draw. pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player::PlayerId) -> bool { - use crate::types::ability::QuantityModification; - use crate::types::replacements::ReplacementEvent; let allowed = allowed_draw_count(state, player_id, 1); if select_cards_to_draw(state, player_id, allowed as usize).is_empty() { return false; } - // CR 614.1 / CR 614.6 / CR 614.11: a mandatory draw replacement that prevents - // the draw suppresses the event entirely. Conservatively block delivery when - // one is active rather than assume the draw survives. - let mandatory_prevent = - crate::game::functioning_abilities::active_replacements(state).any(|(_, _, def)| { - matches!( - def.event, - ReplacementEvent::Draw | ReplacementEvent::DrawCards - ) && matches!( - def.quantity_modification, - Some(QuantityModification::Prevent) - ) - }); - !mandatory_prevent + // CR 121.2: the individual draw the payoff would ride on — the same event + // shape `draw_through_replacement_with_applied` proposes for one card. + let proposed = ProposedEvent::Draw { + player_id, + count: 1, + applied: HashSet::new(), + }; + !replacement::event_is_mandatorily_prevented(state, &proposed) } pub(crate) fn allowed_draw_count( diff --git a/crates/engine/src/game/replacement.rs b/crates/engine/src/game/replacement.rs index 5e63f39104..997184d37c 100644 --- a/crates/engine/src/game/replacement.rs +++ b/crates/engine/src/game/replacement.rs @@ -8697,16 +8697,52 @@ fn is_counter_placement_event(event: &ProposedEvent) -> bool { ) } -fn counter_placement_prevention_applies(state: &GameState, candidates: &[ReplacementId]) -> bool { +/// CR 614.6: does any already-applicable candidate obligatorily replace the +/// event away? A `QuantityModification::Prevent` definition kills the event only +/// when it is MANDATORY — an optional one is offered to a player as an +/// accept/decline choice (`replacement_mode_is_optional`), so it cannot be +/// assumed to apply. `events` scopes the check to the +/// replacement events that actually govern the proposed event, so a differently +/// evented `Prevent` sibling on the same source can never suppress it. +/// +/// `candidates` must come from the live applicability authority +/// (`find_applicable_replacements`), which has already enforced the handler +/// matcher, source/player scope, condition, and optional-decline gates. Virtual +/// rules-source candidates carry no definition and are never preventive here. +fn mandatory_prevention_applies( + state: &GameState, + candidates: &[ReplacementId], + events: &[ReplacementEvent], +) -> bool { candidates.iter().any(|rid| { replacement_definition_for_id(state, *rid).is_some_and(|def| { - def.event == ReplacementEvent::AddCounter + events.contains(&def.event) && def.quantity_modification == Some(QuantityModification::Prevent) && !replacement_mode_is_optional(&def.mode) }) }) } +fn counter_placement_prevention_applies(state: &GameState, candidates: &[ReplacementId]) -> bool { + mandatory_prevention_applies(state, candidates, &[ReplacementEvent::AddCounter]) +} + +/// CR 614.1 + CR 614.6: pure preflight — would `event` be obligatorily replaced +/// away before it happens, so that it never occurs and emits no `GameEvent`? +/// +/// Routes the proposed event through the same applicability authority the live +/// pipeline consults (`find_applicable_replacements`), so an unrelated or +/// opponent-scoped source, a false conditional, an optional ("may") replacement, +/// and a recognized-but-stub replacement event all correctly leave the event +/// deliverable. Read-only: it consults applicability without running any +/// applier, so it can be called from preflights (AI candidate scoring) that must +/// not mutate state. +pub fn event_is_mandatorily_prevented(state: &GameState, event: &ProposedEvent) -> bool { + let registry = replacement_registry(); + let candidates = find_applicable_replacements(state, event, registry); + mandatory_prevention_applies(state, &candidates, &replacement_event_keys_for_event(event)) +} + fn replacement_definition_for_id( state: &GameState, rid: ReplacementId, diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 81d1e21ddf..6003c99cb5 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -16515,6 +16515,113 @@ 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 { + name: "modal_placeholder".to_string(), + description: None, + }, + 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/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index 1f9ee3154a..a82ba6a6f9 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -9,9 +9,10 @@ use std::sync::Arc; use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; use engine::game::zones::create_object; use engine::types::ability::{ - AbilityDefinition, AbilityKind, CastVariantPaid, DrawReplacementScope, Effect, QuantityExpr, - QuantityModification, ReplacementDefinition, StaticDefinition, TargetFilter, TriggerCondition, - TriggerConstraint, TriggerDefinition, + AbilityDefinition, AbilityKind, CastVariantPaid, DrawReplacementScope, Effect, ModalChoice, + QuantityExpr, QuantityModification, ReplacementCondition, ReplacementDefinition, + ReplacementMode, StaticDefinition, TargetFilter, TriggerCondition, TriggerConstraint, + TriggerDefinition, }; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; @@ -1045,14 +1046,21 @@ fn nonempty_library_draw_rewards() { assert!(delta > 0.0); } -/// Puts a permanent carrying a mandatory `Prevent` draw replacement (Living -/// Conundrum shape) on the battlefield. -fn add_prevent_draw_replacement(state: &mut GameState) { +/// Puts a permanent under `controller` carrying a `Prevent` draw replacement +/// (Living Conundrum shape) on the battlefield, letting `customize` adjust the +/// definition first. `controller` is the replacement's source player: with the +/// default `valid_player` scope (CR 614.1a) the replacement applies only to +/// THAT player's draws, which is what makes source-scope discriminating. +fn add_prevent_draw_replacement( + state: &mut GameState, + controller: PlayerId, + customize: impl FnOnce(&mut ReplacementDefinition), +) { let card_id = CardId(state.next_object_id); let id = create_object( state, card_id, - PlayerId(1), + controller, "Living Conundrum".to_string(), Zone::Battlefield, ); @@ -1061,28 +1069,91 @@ fn add_prevent_draw_replacement(state: &mut GameState) { let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw) .draw_scope(DrawReplacementScope::IndividualDraw); repl.quantity_modification = Some(QuantityModification::Prevent); + customize(&mut repl); obj.replacement_definitions.push(repl); } -/// CR 614.6: a mandatory `Prevent` draw replacement suppresses the draw entirely -/// — the replaced event never happens, so no `CardDrawn` fires and the engine -/// never triggers. The delivery preflight withholds the bonus. -#[test] -fn mandatory_prevent_draw_replacement_is_a_no_op() { +/// Scores a cast-a-draw-spell candidate with the payoff engine already out. +fn draw_spell_verdict(st: &mut GameState) -> (f64, PolicyReason) { let config = AiConfig::default(); - let mut st = state(); - engine_on_battlefield(&mut st); - add_prevent_draw_replacement(&mut st); - let (oid, cid) = draw_spell(&mut st); + let (oid, cid) = draw_spell(st); let context = context(&config, session(0.9)); let candidate = cast(oid, cid); let decision = priority_decision(&candidate); - let (delta, reason) = - score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + score_of(DrawPayoffPolicy.verdict(&ctx(st, &candidate, &decision, &context, &config))) +} + +/// CR 614.6: a mandatory `Prevent` draw replacement whose source scopes it to +/// the drawing player suppresses the draw entirely — the replaced event never +/// happens, so no `CardDrawn` fires and the engine never triggers. The delivery +/// preflight withholds the bonus. +#[test] +fn mandatory_prevent_draw_replacement_is_a_no_op() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st, AI, |_| {}); + let (delta, reason) = draw_spell_verdict(&mut st); assert_eq!(reason.kind, "draw_payoff_na"); assert_eq!(delta, 0.0); } +/// CR 614.1a: the same definition on an OPPONENT's permanent replaces that +/// player's draws, not the AI's. Control for scanning `active_replacements` by +/// event alone — the AI's draw is still deliverable, so the payoff still pays. +#[test] +fn opponent_scoped_prevent_draw_replacement_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st, PlayerId(1), |_| {}); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// CR 614.1d: a conditional replacement whose condition does not hold is not +/// applicable, so it cannot suppress the draw. `UnlessPlayerLifeAtMost { 20 }` +/// is false at starting life totals. +#[test] +fn false_conditional_prevent_draw_replacement_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st, AI, |repl| { + repl.condition = Some(ReplacementCondition::UnlessPlayerLifeAtMost { amount: 20 }); + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// An optional replacement is offered as an accept/decline choice, so it never +/// obligatorily suppresses the draw — the preflight must not assume it applies. +#[test] +fn optional_prevent_draw_replacement_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st, AI, |repl| { + repl.mode = ReplacementMode::Optional { decline: None }; + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// `ReplacementEvent::DrawCards` is a recognized-but-stub registry entry, not a +/// runtime draw handler — it replaces nothing at resolution, so it must not +/// suppress the payoff either. +#[test] +fn draw_cards_stub_prevent_replacement_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_prevent_draw_replacement(&mut st, AI, |repl| { + repl.event = ReplacementEvent::DrawCards; + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + // ─── multi-target engine legality (CR 603.3d) ──────────────────────────────── /// A creature `TargetFilter`. @@ -1093,6 +1164,85 @@ fn creature_filter() -> TargetFilter { ) } +/// A required-modal payoff: "whenever you draw, choose one — deal 3 to target +/// creature; or deal 3 to target creature". The execute is a modal placeholder +/// with all target-required modes (the targets live in `mode_abilities`), so on +/// an empty board every mode is unavailable and the live trigger is dropped +/// (`DroppedNoLegalMode`, CR 603.3c). +fn modal_all_target_required_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 mode = || { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 3 }, + target: creature_filter(), + damage_source: None, + excess: None, + }, + ) + }; + let mut execute = AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("modal_placeholder", ""), + ); + execute.modal = Some(ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 2, + ..Default::default() + }); + execute.mode_abilities = vec![mode(), mode()]; + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Enchantment); + obj.trigger_definitions + .push(TriggerDefinition::new(TriggerMode::Drawn).execute(execute)); + id +} + +/// CR 603.3c: a required "choose one" payoff whose every mode needs a target and +/// none is available on an empty board has no legal mode, so the live trigger is +/// dropped — the modal-aware preflight reports it not-live. +#[test] +fn modal_engine_with_no_legal_mode_is_neutral() { + let config = AiConfig::default(); + let mut st = state(); + modal_all_target_required_engine(&mut st); // empty board → no legal mode + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_no_engine"); + assert_eq!(delta, 0.0); +} + +/// Control: once a legal creature target exists, at least one mode is choosable, +/// so the modal engine is live and the draw is rewarded. +#[test] +fn modal_engine_with_a_legal_mode_rewards() { + let config = AiConfig::default(); + let mut st = state(); + modal_all_target_required_engine(&mut st); + add_opponent_creature(&mut st); // a legal target for a mode + let (oid, cid) = draw_spell(&mut st); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + /// A two-target payoff: "whenever you draw, exchange control of two target /// permanents". A multi-target mandatory execute the cheap single-slot check /// can't decide, so the engine authority must consult the full legal-assignment From 7c2f9fcf49a18526b42f1401ae65d199404be8b7 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 09:16:48 -0700 Subject: [PATCH 13/17] fix(engine+phase-ai): share the draw-substitution classifier with the live pipeline (round 10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The draw-payoff preflight was a partial mirror of the live draw pipeline, and each review round surfaced one more leg the mirror failed to model. Fix the mirror rather than the leg: extract the substitution classification out of `apply_single_replacement` into `draw_is_substituted_away`, and have both the live pipeline and the read-only preflight call it — so the two cannot disagree about whether a draw survives, because there is only one of them. Replace `event_is_mandatorily_prevented` (prevention only, single caller) with `proposed_draw_survives_replacement`, routing every replacement suppression leg through the authority that owns it: - mandatory `QuantityModification::Prevent` (CR 614.6, Living Conundrum), via `mandatory_prevention_applies` - mandatory non-Draw substitute carried in `execute` (Chains of Mephistopheles, Jace Wielder of Mysteries) or `runtime_execute` (Words of Worship/Wilding), CR 614.11, via the shared `draw_is_substituted_away` - mandatory count modification resolving to zero (CR 614.11a), via `draw_replacement_count` — a leg no prior round had modeled Optional-decline and count behavior are preserved: a "you may" replacement is never assumed to apply, and an Alhammarret's Archive-style positive rescale is a surviving draw, not a substitution. Adds `draw_preflight_matches_live_pipeline`: for each shape it asks the preflight for a prediction, then drives the REAL draw through the pipeline and asserts prediction == observed delivery, with expected delivery pinned independently so a regression breaking both sides alike still fails. Covers the restriction, empty-library, prevent, both substitute slots and zero-count legs, plus unreplaced and count-modified surviving controls — preflight/pipeline drift now fails CI in either direction. Six paired `DrawPayoffPolicy` cases cover the same shapes at the policy seam. Consolidates the file's two Draw-definition producers into one parameterized helper and re-freezes the producer census accordingly. --- crates/engine/src/game/effects/draw.rs | 27 +- crates/engine/src/game/replacement.rs | 146 ++++++--- .../draw_preflight_matches_live_pipeline.rs | 282 ++++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + .../src/policies/tests/draw_payoff.rs | 204 ++++++++++++- scripts/draw-replacement-producers.txt | 2 +- 6 files changed, 600 insertions(+), 62 deletions(-) create mode 100644 crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs diff --git a/crates/engine/src/game/effects/draw.rs b/crates/engine/src/game/effects/draw.rs index 42d91fd59a..b88c8d0eae 100644 --- a/crates/engine/src/game/effects/draw.rs +++ b/crates/engine/src/game/effects/draw.rs @@ -17,19 +17,20 @@ use crate::types::zones::Zone; /// permitted); or /// - the library is empty — an empty-library draw only records an attempted /// draw (CR 704.5b) and delivers no card; or -/// - a mandatory `Prevent` draw replacement applies to *this player's* draw -/// (CR 614.6: "skip that draw instead", Living Conundrum) — the replaced draw -/// event never happens. +/// - the replacement pipeline removes the draw before it happens (CR 614.6) — +/// prevented, substituted with a non-Draw chain, or rescaled to zero. /// -/// In each case the draw fires no "whenever you draw" trigger. Routes through the -/// same `allowed_draw_count` gate and `select_cards_to_draw` delivery authority -/// the resolver uses, and models the individual draw as the `ProposedEvent::Draw` -/// the live path proposes so the replacement leg consults the live applicability -/// authority (`replacement::event_is_mandatorily_prevented`) rather than scanning -/// definitions by event alone. An unrelated or opponent-scoped source, a false -/// conditional, and an optional ("may") replacement therefore all leave the draw -/// deliverable. The single engine authority an AI draw-payoff preflight consults -/// so it never credits a no-op draw. +/// In each case the draw fires no "whenever you draw" trigger. Every leg delegates +/// to the authority that owns it rather than re-deriving it: `allowed_draw_count` +/// for draw restrictions, `select_cards_to_draw` for library delivery, and +/// `replacement::proposed_draw_survives_replacement` — which shares its +/// applicability and substitution classifiers with the live pipeline — for the +/// replacement leg. The individual draw is modeled as the same +/// `ProposedEvent::Draw` shape `draw_through_replacement_with_applied` proposes, +/// so the preflight and the resolver ask the identical question. +/// +/// The single engine authority an AI draw-payoff preflight consults so it never +/// credits a no-op draw. pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player::PlayerId) -> bool { let allowed = allowed_draw_count(state, player_id, 1); if select_cards_to_draw(state, player_id, allowed as usize).is_empty() { @@ -42,7 +43,7 @@ pub fn can_draw_at_least_one(state: &GameState, player_id: crate::types::player: count: 1, applied: HashSet::new(), }; - !replacement::event_is_mandatorily_prevented(state, &proposed) + replacement::proposed_draw_survives_replacement(state, &proposed) } pub(crate) fn allowed_draw_count( diff --git a/crates/engine/src/game/replacement.rs b/crates/engine/src/game/replacement.rs index 997184d37c..f7f3f37a53 100644 --- a/crates/engine/src/game/replacement.rs +++ b/crates/engine/src/game/replacement.rs @@ -2840,6 +2840,53 @@ fn draw_replacement_count( } } +/// CR 614.6 + CR 614.11: does the branch being applied substitute the proposed +/// draw with a NON-draw chain, so the original draw never happens and no +/// `GameEvent::CardDrawn` is emitted? +/// +/// `branch_ability` is the AST of the branch the pipeline is applying (`execute` +/// on mandatory/accept, `decline` on decline), so an optional replacement's +/// decline is never classified against the accept-side AST. +/// +/// A one-shot draw replacement (Words of Worship / Wilding) carries its +/// substitute in `runtime_execute` while `execute` is `None`, so `branch_ability` +/// is `None` for those; a non-Draw, non-event-modifier substitute there (GainLife +/// / Token) must still count, or the card would be drawn AND the substitute would +/// run. Damage / Jace's WinTheGame / Abundance shapes carry theirs in `execute`, +/// so `branch_ability` is `Some` and the `runtime_execute` leg never engages. +/// +/// The `draw_replacement_count` guard preserves the count-modifier path +/// (Alhammarret's Archive: count -> 2*count, CR 614.11a) — a rescaled draw is a +/// surviving draw, not a substitution. +/// +/// Single authority: the live pipeline (`apply_single_replacement`) calls this to +/// decide whether to pre-zero the proposed count, and the read-only preflight +/// (`proposed_draw_survives_replacement`) calls it to decide whether a draw can +/// still deliver a card. Neither may re-derive this classification independently +/// — a preflight that mirrors the pipeline instead of sharing it will drift. +fn draw_is_substituted_away( + state: &GameState, + rid: ReplacementId, + repl_def: &ReplacementDefinition, + branch_ability: Option<&AbilityDefinition>, + proposed: &ProposedEvent, +) -> bool { + if !matches!(proposed, ProposedEvent::Draw { .. }) { + return false; + } + match branch_ability { + Some(def) => { + !matches!(*def.effect, Effect::Draw { .. }) + && !EventModifiers::has_only_event_modifier(Some(def)) + && draw_replacement_count(state, rid, proposed).is_none() + } + None => repl_def.runtime_execute.as_deref().is_some_and(|runtime| { + !matches!(runtime.effect, Effect::Draw { .. }) + && !EventModifiers::is_event_modifier_effect(&runtime.effect) + }), + } +} + // --- 4b. Scry --- // CR 614.6: A replacement effect applies only once to a given event. The @@ -7909,34 +7956,14 @@ fn apply_single_replacement( // draw with a non-Draw chain (Jace's WinTheGame, Abundance's // reveal-until), zero the count here so `draw_applier` and // `apply_draw_after_replacement` see a no-op draw — the original draw - // never happens (CR 614.6). Branch-aware via the `ability` binding - // above, so an optional replacement's decline never pre-zeros against - // the accept-side AST. The `draw_replacement_count` guard preserves - // the count-modifier path (Alhammarret's Archive: count -> 2*count). - if matches!(proposed, ProposedEvent::Draw { .. }) { - // CR 614.6 + CR 614.11: A one-shot draw replacement - // (Words of Worship/Wilding) carries its substitute in - // `runtime_execute` (`execute` is `None`), so the `ability` - // binding above is `None`. Inspect that slot too — a non-Draw, - // non-event-modifier substitute (GainLife / Token) must still - // pre-zero the draw, or the card is drawn AND the substitute - // runs (double). Damage/Jace/Abundance use `execute`, so - // `ability` is `Some` and this `runtime` branch never engages. - let is_non_draw_substitute = match ability { - Some(def) => { - !matches!(*def.effect, Effect::Draw { .. }) - && !EventModifiers::has_only_event_modifier(Some(def)) - && draw_replacement_count(state, rid, &proposed).is_none() - } - None => repl_def.runtime_execute.as_deref().is_some_and(|runtime| { - !matches!(runtime.effect, Effect::Draw { .. }) - && !EventModifiers::is_event_modifier_effect(&runtime.effect) - }), - }; - if is_non_draw_substitute { - if let ProposedEvent::Draw { count, .. } = &mut proposed { - *count = 0; - } + // never happens (CR 614.6). The classification itself lives in + // `draw_is_substituted_away`, which is SHARED with the read-only + // preflight `proposed_draw_survives_replacement`: an AI preflight + // therefore cannot disagree with this pipeline about whether a draw + // survives, because both ask the same function. + if draw_is_substituted_away(state, rid, repl_def, ability, &proposed) { + if let ProposedEvent::Draw { count, .. } = &mut proposed { + *count = 0; } } // CR 614.6 + CR 111.1: A CreateToken replacement whose execute is @@ -8727,20 +8754,61 @@ fn counter_placement_prevention_applies(state: &GameState, candidates: &[Replace mandatory_prevention_applies(state, candidates, &[ReplacementEvent::AddCounter]) } -/// CR 614.1 + CR 614.6: pure preflight — would `event` be obligatorily replaced -/// away before it happens, so that it never occurs and emits no `GameEvent`? +/// CR 121.1 + CR 614.6 + CR 614.11: pure preflight — does a proposed draw survive +/// the replacement effects currently applicable to it as a *real* draw, one that +/// puts a card into its player's hand and emits `GameEvent::CardDrawn`? +/// +/// Three legs of the live pipeline remove a proposed draw, and each is answered +/// here by the same authority that owns it in the pipeline, never by a +/// re-derived structural scan: +/// - a mandatory `QuantityModification::Prevent` — `draw_applier` returns +/// `ApplyResult::Prevented`, so the replaced event never happens (CR 614.6, +/// Living Conundrum). Shared via `mandatory_prevention_applies`. +/// - a mandatory non-Draw substitute carried in `execute` or `runtime_execute` — +/// `apply_single_replacement` zeroes the proposed count so the original draw is +/// a no-op and the substitute runs instead (CR 614.11: Words of Worship, +/// Abundance's reveal-until, Jace's WinTheGame). Shared via +/// `draw_is_substituted_away`. +/// - a mandatory count modification that resolves to zero — `draw_applier` +/// returns `Modified` with `count: 0`, and `apply_draw_after_replacement` +/// emits `CardDrawn` only inside its per-delivered-card loop, so a zero-count +/// draw emits none (CR 614.11a). Shared via `draw_replacement_count`. +/// +/// An OPTIONAL replacement (CR 614.6: "you may") is never assumed to apply — the +/// player is offered an accept/decline choice, so the draw is still deliverable +/// and the payoff still stands. A count modification that resolves positive +/// (Alhammarret's Archive: count -> 2*count) is likewise a surviving draw. +/// +/// `find_applicable_replacements` is the live applicability authority, so an +/// unrelated or opponent-scoped source (CR 614.1a), a false conditional +/// (CR 614.1d), and a recognized-but-stub replacement event are already excluded +/// before anything is classified here. /// -/// Routes the proposed event through the same applicability authority the live -/// pipeline consults (`find_applicable_replacements`), so an unrelated or -/// opponent-scoped source, a false conditional, an optional ("may") replacement, -/// and a recognized-but-stub replacement event all correctly leave the event -/// deliverable. Read-only: it consults applicability without running any -/// applier, so it can be called from preflights (AI candidate scoring) that must -/// not mutate state. -pub fn event_is_mandatorily_prevented(state: &GameState, event: &ProposedEvent) -> bool { +/// Read-only: it consults applicability and definition shape without running any +/// applier, so preflights (AI candidate scoring) can call it without mutating +/// state. Non-`Draw` events are outside its remit and always report surviving. +pub fn proposed_draw_survives_replacement(state: &GameState, event: &ProposedEvent) -> bool { + if !matches!(event, ProposedEvent::Draw { .. }) { + return true; + } let registry = replacement_registry(); let candidates = find_applicable_replacements(state, event, registry); - mandatory_prevention_applies(state, &candidates, &replacement_event_keys_for_event(event)) + let events = replacement_event_keys_for_event(event); + if mandatory_prevention_applies(state, &candidates, &events) { + return false; + } + !candidates.iter().any(|rid| { + replacement_definition_for_id(state, *rid).is_some_and(|def| { + // CR 614.6: only a MANDATORY branch is certain to apply, and the live + // pipeline resolves it to `ReplacementBranch::Execute` — so `execute` + // is the branch AST to classify, exactly as `apply_single_replacement` + // binds it. + events.contains(&def.event) + && !replacement_mode_is_optional(&def.mode) + && (draw_is_substituted_away(state, *rid, def, def.execute.as_deref(), event) + || draw_replacement_count(state, *rid, event) == Some(0)) + }) + }) } fn replacement_definition_for_id( diff --git a/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs new file mode 100644 index 0000000000..20c2d29ebf --- /dev/null +++ b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs @@ -0,0 +1,282 @@ +//! CI-enforced equivalence between the read-only draw preflight and the live +//! draw pipeline (CR 121.1 / CR 614.6 / CR 614.11). +//! +//! `game::effects::draw::can_draw_at_least_one` answers "would a draw right now +//! actually put a card into this player's hand, emitting `GameEvent::CardDrawn`?" +//! It exists so an AI payoff policy can decline to reward a draw that will fire +//! no "whenever you draw" trigger. Because it is read-only it cannot run the +//! pipeline — so the standing hazard is that it becomes a PARTIAL MIRROR of the +//! pipeline and silently drifts: each un-modeled suppression leg is a candidate +//! scored as a draw engine that draws nothing. +//! +//! The preflight is built to make drift impossible by construction — its +//! substitution leg calls `draw_is_substituted_away`, the very function +//! `apply_single_replacement` uses to pre-zero the live count, and its +//! applicability comes from `find_applicable_replacements`, the live authority. +//! This test enforces that property from the outside instead of trusting it: +//! every shape below asks the preflight for a prediction, then DRIVES THE REAL +//! DRAW and observes what the pipeline did. A leg the preflight stops modeling +//! shows up here as a prediction/observation mismatch, whichever direction it +//! drifts in. +//! +//! Suppression legs covered — every way `can_draw_at_least_one` can answer "no": +//! 1. a draw restriction — `CantDraw` shown; `PerTurnDrawLimit` exhaustion is +//! the same leg, both resolved by `allowed_draw_count` +//! 2. empty library (CR 704.5b — an attempted draw delivers no card) +//! 3. mandatory `QuantityModification::Prevent` (CR 614.6, Living Conundrum) +//! 4. mandatory non-Draw substitute, in `execute` (Chains of Mephistopheles, +//! Jace Wielder of Mysteries) and in `runtime_execute` (Words of Worship, +//! "{1}: The next time you would draw a card this turn, you gain 5 life +//! instead") — CR 614.11 +//! 5. mandatory count modification resolving to zero (CR 614.11a) +//! +//! Surviving controls: an unreplaced draw, and a count-modifying replacement +//! that rescales rather than removes ("…draw two cards instead" — Alhammarret's +//! Archive, Teferi's Ageless Insight). Without these the equivalence is +//! satisfiable by a preflight that always predicts "no draw". + +use engine::game::effects::draw::can_draw_at_least_one; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::game::zones::create_object; +use engine::types::ability::{ + AbilityDefinition, AbilityKind, DrawReplacementScope, Effect, QuantityExpr, + QuantityModification, ReplacementDefinition, ResolvedAbility, StaticDefinition, TargetFilter, +}; +use engine::types::actions::{DebugAction, GameAction}; +use engine::types::card_type::CoreType; +use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::phase::Phase; +use engine::types::replacements::ReplacementEvent; +use engine::types::statics::{ProhibitionScope, StaticMode}; +use engine::types::zones::Zone; + +/// "…you gain 5 life instead" — a substitute that is not a draw. The classifier +/// keys on "not a `Draw`, not a pure event modifier", so one non-draw effect +/// stands in for the whole class (discard, win-the-game, reveal-until, token). +fn gain_life_substitute() -> AbilityDefinition { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 5 }, + player: TargetFilter::Controller, + }, + ) +} + +/// "…draw two cards instead" — a count modification. Still a draw (CR 614.11a). +fn draw_count_substitute(value: i32) -> AbilityDefinition { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value }, + target: TargetFilter::Controller, + }, + ) +} + +/// Seats P0 with `library` cards, plus a replacement-bearing permanent when +/// `customize` is supplied. P1 always gets a library so no state-based action +/// ends the game mid-test. +fn scenario( + library: usize, + customize: Option<( + &'static str, + Box, + )>, +) -> GameRunner { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + for i in 0..library { + scenario.add_card_to_library_top(P0, &format!("Lib {i}")); + } + for i in 0..5 { + scenario.add_card_to_library_top(P1, &format!("P1 Lib {i}")); + } + // 1/1, not 0/0: the replacement source must survive the state-based-action + // check that runs while the draw resolves, or the pipeline would see a board + // the preflight never predicted against (CR 704.5f). + let source = customize + .as_ref() + .map(|(name, _)| scenario.add_creature(P0, name, 1, 1).id()); + let mut runner = scenario.build(); + runner.state_mut().debug_mode = true; + if let (Some(source), Some((_, shape))) = (source, customize) { + let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw) + .draw_scope(DrawReplacementScope::IndividualDraw); + shape(&mut repl, source); + runner + .state_mut() + .objects + .get_mut(&source) + .expect("replacement source must exist") + .replacement_definitions + .push(repl); + } + runner +} + +/// THE ASSERTION: ask the preflight, then run the real draw and compare. +/// +/// `expected_delivery` pins what the pipeline is supposed to do, so a regression +/// that breaks BOTH sides in the same direction still fails here rather than +/// quietly agreeing at the wrong answer. +fn assert_preflight_matches_pipeline(shape: &str, mut runner: GameRunner, expected_delivery: bool) { + let predicted = can_draw_at_least_one(runner.state(), P0); + let hand_before = runner.state().players[P0.0 as usize].hand.len(); + + runner + .act(GameAction::Debug(DebugAction::DrawCards { + player_id: P0, + count: 1, + })) + .expect("debug draw must be accepted"); + runner.advance_until_stack_empty(); + + let delivered = runner.state().players[P0.0 as usize].hand.len() > hand_before; + + assert_eq!( + delivered, expected_delivery, + "{shape}: the live pipeline delivered={delivered}, but this shape is \ + specified to deliver={expected_delivery} — the test's model of the \ + pipeline is stale, fix that before reading the preflight comparison" + ); + assert_eq!( + predicted, delivered, + "{shape}: can_draw_at_least_one predicted {predicted} but the live draw \ + pipeline delivered {delivered}. The preflight has drifted from the \ + pipeline — an AI draw-payoff bonus is now being awarded to a draw that \ + emits no CardDrawn (or withheld from one that does)." + ); +} + +/// Control: nothing suppresses the draw, so preflight and pipeline both say yes. +/// Without this the equivalence is satisfiable by always predicting "no draw". +#[test] +fn unreplaced_draw_is_predicted_and_delivered() { + assert_preflight_matches_pipeline("unreplaced draw", scenario(3, None), true); +} + +/// CR 704.5b: an empty-library draw records an attempt and delivers no card. +#[test] +fn empty_library_draw_is_predicted_and_not_delivered() { + assert_preflight_matches_pipeline("empty library", scenario(0, None), false); +} + +/// CR 121.1: a `CantDraw` static permits no draw at all, so the draw event never +/// occurs and no card is delivered. The restriction leg — `allowed_draw_count` +/// resolves it, and an exhausted `PerTurnDrawLimit` reaches the same zero the +/// same way. +#[test] +fn cant_draw_static_is_predicted_and_not_delivered() { + let mut runner = scenario(3, None); + let state = runner.state_mut(); + let card_id = CardId(state.next_object_id); + let hoser = create_object( + state, + card_id, + P1, + "Draw Hoser".to_string(), + Zone::Battlefield, + ); + let obj = state + .objects + .get_mut(&hoser) + .expect("the draw-restricting permanent must exist"); + obj.card_types.core_types.push(CoreType::Creature); + obj.static_definitions + .push(StaticDefinition::new(StaticMode::CantDraw { + who: ProhibitionScope::AllPlayers, + })); + assert_preflight_matches_pipeline("CantDraw static", runner, false); +} + +/// CR 614.6: a mandatory `Prevent` replaces the draw away — Living Conundrum's +/// "skip that draw instead". The replaced event never happens. +#[test] +fn mandatory_prevent_is_predicted_and_not_delivered() { + let runner = scenario( + 3, + Some(( + "Living Conundrum", + Box::new(|repl: &mut ReplacementDefinition, _source| { + repl.quantity_modification = Some(QuantityModification::Prevent); + }), + )), + ); + assert_preflight_matches_pipeline("mandatory prevent", runner, false); +} + +/// CR 614.11: a mandatory non-Draw substitute in `execute` — the printed-static +/// half of the class (Chains of Mephistopheles, Jace Wielder of Mysteries). +/// `apply_single_replacement` zeroes the count, so no card is delivered. +#[test] +fn mandatory_execute_substitute_is_predicted_and_not_delivered() { + let runner = scenario( + 3, + Some(( + "Chains of Mephistopheles", + Box::new(|repl: &mut ReplacementDefinition, _source| { + repl.execute = Some(Box::new(gain_life_substitute())); + }), + )), + ); + assert_preflight_matches_pipeline("mandatory execute substitute", runner, false); +} + +/// CR 614.11: the same substitution delivered through `runtime_execute`, the +/// activated-one-shot half of the class (Words of Worship). A preflight that +/// inspects only `execute` misses this leg entirely. +#[test] +fn mandatory_runtime_execute_substitute_is_predicted_and_not_delivered() { + let runner = scenario( + 3, + Some(( + "Words of Worship", + Box::new(|repl: &mut ReplacementDefinition, source: ObjectId| { + repl.runtime_execute = Some(Box::new(ResolvedAbility::new( + gain_life_substitute().effect.as_ref().clone(), + Vec::new(), + source, + P0, + ))); + }), + )), + ); + assert_preflight_matches_pipeline("mandatory runtime_execute substitute", runner, false); +} + +/// CR 614.11a: a count modification RESCALES the draw ("…draw two cards +/// instead") rather than removing it, so a card is still delivered and +/// `CardDrawn` still fires. The discriminating control for the two substitute +/// cases: same mandatory `execute` slot, opposite outcome. +#[test] +fn count_modifying_replacement_is_predicted_and_delivered() { + let runner = scenario( + 3, + Some(( + "Alhammarret's Archive", + Box::new(|repl: &mut ReplacementDefinition, _source| { + repl.execute = Some(Box::new(draw_count_substitute(2))); + }), + )), + ); + assert_preflight_matches_pipeline("count-modifying replacement", runner, true); +} + +/// CR 614.11a: the boundary of that same count surface — a modification +/// resolving to zero leaves no card to draw, so no `CardDrawn` is emitted. The +/// `execute` here IS a draw, so the substitution classifier declines it and only +/// the resolved count discriminates. +#[test] +fn zero_count_replacement_is_predicted_and_not_delivered() { + let runner = scenario( + 3, + Some(( + "Zero-Count Draw Rescaler", + Box::new(|repl: &mut ReplacementDefinition, _source| { + repl.execute = Some(Box::new(draw_count_substitute(0))); + }), + )), + ); + assert_preflight_matches_pipeline("zero-count replacement", runner, false); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 2d91992d5a..c294c2bd0a 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -152,6 +152,7 @@ mod doran_attack_block_pump; mod double_strike_first_strike_trigger_removes_attacker; mod dragonstorm_forecaster_named_or_tutor; mod draw_from_general_post_replacement; +mod draw_preflight_matches_live_pipeline; mod dream_salvage_target_opponent_discards; mod dredgers_insight_mill_from_among; mod druid_of_purification_destroy_chosen_4780; diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index a82ba6a6f9..bd2fa84e08 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -1046,31 +1046,52 @@ fn nonempty_library_draw_rewards() { assert!(delta > 0.0); } -/// Puts a permanent under `controller` carrying a `Prevent` draw replacement -/// (Living Conundrum shape) on the battlefield, letting `customize` adjust the -/// definition first. `controller` is the replacement's source player: with the -/// default `valid_player` scope (CR 614.1a) the replacement applies only to -/// THAT player's draws, which is what makes source-scope discriminating. -fn add_prevent_draw_replacement( +/// Puts a permanent named `name` under `controller` on the battlefield carrying a +/// `ReplacementEvent::Draw` definition that `customize` shapes, and returns it so +/// a `runtime_execute` substitute can bind it as its source. +/// +/// `controller` is the replacement's source player: with the default +/// `valid_player` scope (CR 614.1a) the replacement applies only to THAT player's +/// draws, which is what makes source-scope discriminating. +/// +/// The single Draw-definition producer in this file — every replacement shape +/// below is a `customize` parameterization of it, so +/// `scripts/draw_replacement_census.py` freezes one row rather than one per +/// shape. +fn add_draw_replacement( state: &mut GameState, controller: PlayerId, + name: &str, customize: impl FnOnce(&mut ReplacementDefinition), -) { +) -> ObjectId { let card_id = CardId(state.next_object_id); let id = create_object( state, card_id, controller, - "Living Conundrum".to_string(), + name.to_string(), Zone::Battlefield, ); let obj = state.objects.get_mut(&id).unwrap(); obj.card_types.core_types.push(CoreType::Enchantment); let mut repl = ReplacementDefinition::new(ReplacementEvent::Draw) .draw_scope(DrawReplacementScope::IndividualDraw); - repl.quantity_modification = Some(QuantityModification::Prevent); customize(&mut repl); obj.replacement_definitions.push(repl); + id +} + +/// Living Conundrum shape: "if you would draw a card, skip that draw instead" — +/// a mandatory `Prevent` quantity modification on `controller`'s draws. +fn add_prevent_draw_replacement( + state: &mut GameState, + controller: PlayerId, + customize: impl FnOnce(&mut ReplacementDefinition), +) { + add_draw_replacement(state, controller, "Living Conundrum", |repl| { + repl.quantity_modification = Some(QuantityModification::Prevent); + customize(repl); + }); } /// Scores a cast-a-draw-spell candidate with the payoff engine already out. @@ -1154,6 +1175,171 @@ fn draw_cards_stub_prevent_replacement_still_rewards() { assert!(delta > 0.0); } +// ─── replacement substitution and rescaling (CR 614.11) ────────────────────── +// +// A `Prevent` quantity modification is only ONE of the three ways the pipeline +// removes a draw. It can also be substituted away by a non-Draw chain, or +// rescaled to zero. All three are classified by the shared engine authority +// `replacement::proposed_draw_survives_replacement`, whose substitution leg is +// the very function `apply_single_replacement` uses to pre-zero the live count — +// these cases pin that the preflight and the pipeline stay in agreement. + +/// A non-Draw substitute chain: "instead, you gain 5 life" — the body of Words +/// of Worship, "{1}: The next time you would draw a card this turn, you gain 5 +/// life instead." +/// +/// The classifier keys on "not a `Draw`, not a pure event modifier", so this +/// stands in for the whole substitute class: Chains of Mephistopheles' "that +/// player discards a card instead", Jace, Wielder of Mysteries' "you win the +/// game instead", Abundance's reveal-until. What varies between those cards is +/// which slot carries the substitute and whether it is mandatory — the axes the +/// cases below vary — not the substitute effect itself. +fn gain_life_substitute() -> AbilityDefinition { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 5 }, + player: TargetFilter::Controller, + }, + ) +} + +/// A draw-count substitute: "draw that many cards plus one instead" +/// (Alhammarret's Archive / Teferi's Ageless Insight, CR 614.11a). Still a draw. +fn draw_count_substitute(value: i32) -> AbilityDefinition { + AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value }, + target: TargetFilter::Controller, + }, + ) +} + +/// CR 614.11: a mandatory `execute` substitute that is not a draw replaces the +/// draw event away — `apply_single_replacement` zeroes the proposed count, so no +/// `CardDrawn` is emitted and the "whenever you draw" engine never triggers. The +/// bonus must be withheld even though nothing here is a `Prevent`. +/// +/// The printed-static half of the class: Chains of Mephistopheles ("that player +/// discards a card instead"), Jace, Wielder of Mysteries ("you win the game +/// instead"). Both carry the substitute in `execute`. +#[test] +fn mandatory_execute_substitution_is_a_no_op() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_replacement(&mut st, AI, "Chains of Mephistopheles", |repl| { + repl.execute = Some(Box::new(gain_life_substitute())); + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// CR 614.11: a one-shot draw replacement created by a resolving ability carries +/// its substitute in `runtime_execute` while `execute` stays `None`. That slot +/// substitutes the draw away exactly as `execute` does, so the preflight must +/// inspect it too — the leg a definition-shaped scan of `execute` alone misses. +/// +/// The activated-one-shot half of the class, and an exact fit: Words of Worship +/// is "{1}: The next time you would draw a card this turn, you gain 5 life +/// instead" (Words of Wilding substitutes a 2/2 Bear token the same way). +#[test] +fn mandatory_runtime_execute_substitution_is_a_no_op() { + let mut st = state(); + engine_on_battlefield(&mut st); + let source = add_draw_replacement(&mut st, AI, "Words of Worship", |_| {}); + let runtime = engine::types::ability::ResolvedAbility::new( + gain_life_substitute().effect.as_ref().clone(), + Vec::new(), + source, + AI, + ); + let obj = st.objects.get_mut(&source).unwrap(); + obj.replacement_definitions[0].runtime_execute = Some(Box::new(runtime)); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// CR 614.6: the same substitution offered as "you may" is an accept/decline +/// choice, so it cannot be assumed to apply — the draw is still deliverable and +/// the payoff still pays. Control that the substitution leg gates on mandatory +/// mode rather than on the presence of a non-Draw `execute`. +/// +/// Abundance is the printed case: "If you would draw a card, you MAY instead +/// choose land or nonland and reveal cards from the top of your library…". +#[test] +fn optional_execute_substitution_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_replacement(&mut st, AI, "Abundance", |repl| { + repl.execute = Some(Box::new(gain_life_substitute())); + repl.mode = ReplacementMode::Optional { decline: None }; + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// CR 614.1a: an opponent-sourced mandatory substitution scopes to THAT player's +/// draws, so the AI's draw survives. Control that the substitution leg inherits +/// the live applicability gate rather than scanning definitions by event alone. +#[test] +fn opponent_scoped_execute_substitution_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_replacement(&mut st, PlayerId(1), "Chains of Mephistopheles", |repl| { + repl.execute = Some(Box::new(gain_life_substitute())); + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// CR 614.11a: a count-modifying replacement rescales the draw rather than +/// removing it — Alhammarret's Archive and Teferi's Ageless Insight both read +/// "…draw two cards instead" (each gated on "except the first one you draw in +/// each of your draw steps"; the gate is immaterial here, so the definition is +/// modeled ungated). A rescaled draw still emits `CardDrawn`, so the payoff must +/// be paid. +/// +/// The discriminating positive control for +/// `mandatory_execute_substitution_is_a_no_op`: both carry a mandatory +/// `execute`, and only the non-Draw one suppresses. +#[test] +fn count_modifying_draw_replacement_still_rewards() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_replacement(&mut st, AI, "Alhammarret's Archive", |repl| { + repl.execute = Some(Box::new(draw_count_substitute(2))); + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// CR 614.11a: a mandatory count modification that resolves to ZERO leaves no +/// card to draw — `draw_applier` yields `Modified { count: 0 }` and the delivery +/// loop emits no `CardDrawn`. Third suppression leg, distinct from both `Prevent` +/// and non-Draw substitution: the `execute` here IS a draw, so the substitution +/// classifier correctly declines it and only the resolved count discriminates. +/// +/// A synthetic boundary rather than a printed card — the count-modifier surface +/// accepts any `QuantityExpr`, and zero is the value at which a rescaled draw +/// stops being a draw. Pinned so the leg cannot regress unnoticed. +#[test] +fn zero_count_draw_replacement_is_a_no_op() { + let mut st = state(); + engine_on_battlefield(&mut st); + add_draw_replacement(&mut st, AI, "Zero-Count Draw Rescaler", |repl| { + repl.execute = Some(Box::new(draw_count_substitute(0))); + }); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + // ─── multi-target engine legality (CR 603.3d) ──────────────────────────────── /// A creature `TargetFilter`. diff --git a/scripts/draw-replacement-producers.txt b/scripts/draw-replacement-producers.txt index 2c29de197e..1fa37700ec 100644 --- a/scripts/draw-replacement-producers.txt +++ b/scripts/draw-replacement-producers.txt @@ -53,4 +53,4 @@ crates/engine/src/parser/oracle_replacement.rs parse_conditional_draw_replacemen crates/engine/src/parser/oracle_replacement.rs parse_replacement_line_inner constructor 1 crates/engine/src/types/replacements.rs from_str event-decode 2 crates/mtgish-import/src/convert/replacement.rs convert_replace_would_draw struct-literal 1 -crates/phase-ai/src/policies/tests/draw_payoff.rs add_prevent_draw_replacement constructor 1 +crates/phase-ai/src/policies/tests/draw_payoff.rs add_draw_replacement constructor 1 From fd61fd76aec345295e14fe313216218f065dea71 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 09:51:09 -0700 Subject: [PATCH 14/17] fix(engine): factor the test replacement-shape tuple into a type alias Clippy `type_complexity` on the new equivalence test's `scenario` parameter. The tuple is `(card name, shaper)`; the shaper takes the permanent's ObjectId because a `runtime_execute` substitute binds its own source. Co-Authored-By: Claude Opus 5 (1M context) --- .../draw_preflight_matches_live_pipeline.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs index 20c2d29ebf..4b706784f5 100644 --- a/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs +++ b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs @@ -74,16 +74,18 @@ fn draw_count_substitute(value: i32) -> AbilityDefinition { ) } +/// A replacement-bearing permanent to seat: its card name, and a shaper that +/// fills in the `ReplacementDefinition` given the permanent's `ObjectId` (needed +/// because a `runtime_execute` substitute binds its own source). +type ReplacementShape = ( + &'static str, + Box, +); + /// Seats P0 with `library` cards, plus a replacement-bearing permanent when /// `customize` is supplied. P1 always gets a library so no state-based action /// ends the game mid-test. -fn scenario( - library: usize, - customize: Option<( - &'static str, - Box, - )>, -) -> GameRunner { +fn scenario(library: usize, customize: Option) -> GameRunner { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); for i in 0..library { From 1a411e1c2e73fed826867ed68c7bff0bdc0dfa15 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 10:27:12 -0700 Subject: [PATCH 15/17] fix(phase-ai): card-local gate first + exhaustive action match + bounded-score regression (round 11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three round-11 findings. [MED] hot-path ordering: `candidate_draws_controller` ran the delivery/ replacement preflight BEFORE proving the candidate draws, so every non-draw CastSpell/ActivateAbility candidate paid a battlefield-static scan and `find_applicable_replacements` at every search node — the opposite of the module doc's stated card-local early-out. Split into `candidate_draws_ structurally` (card-local, reads only the candidate AST) and `draw_is_deliverable` (the expensive, candidate-independent engine authority), evaluated in that order. Behavior is unchanged; only non-draw candidates get cheaper, and the module doc is now true. [LOW] exhaustive `GameAction` match: the wildcard `_ => false` let a future action silently bypass the policy. Now enumerates all 127 variants, so a new one fails to compile here and forces an intentional classification. Cast-shaped siblings (madness, miracle, foretell, ninjutsu, free/copy casts) are grouped into their own arm documenting WHY they are neutral — `cast_facts` is populated only for the plain `CastSpell` seam — so that gap is visible rather than silent. [LOW] bounded-score regression: `MAX_REWARDED_ENGINES` had no test. Adds a capped case (MAX+1 engines must not out-score MAX) plus a below-cap scaling control so the pair pins a CAP rather than a constant, and asserts the `engines` fact still reports the true uncapped count. The constant is `pub(crate)` so the assertion tracks it instead of a copied literal. Co-Authored-By: Claude Opus 5 (1M context) --- crates/phase-ai/src/policies/draw_payoff.rs | 186 ++++++++++++++++-- .../src/policies/tests/draw_payoff.rs | 67 +++++++ 2 files changed, 239 insertions(+), 14 deletions(-) diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index 8224215c8a..efd81bbdf6 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -37,7 +37,10 @@ pub struct DrawPayoffPolicy; /// Cap on how many simultaneous engines are rewarded, so a stacked board can't /// push a single draw into the critical band. -const MAX_REWARDED_ENGINES: usize = 3; +/// +/// `pub(crate)` so the bounded-score regression asserts against this constant +/// rather than a copied literal — raising the cap must move the test with it. +pub(crate) const MAX_REWARDED_ENGINES: usize = 3; impl TacticalPolicy for DrawPayoffPolicy { fn id(&self) -> PolicyId { @@ -101,23 +104,42 @@ impl TacticalPolicy for DrawPayoffPolicy { } } -/// True when the candidate action draws the controller one or more cards. +/// True when the candidate action draws the controller one or more cards AND +/// that draw can actually be delivered. +/// +/// Ordered cheapest-discriminator-first, because `verdict` runs for every +/// `CastSpell` and `ActivateAbility` candidate at every search node. The +/// card-local structural test reads only the candidate's own AST and rejects the +/// overwhelming majority of candidates; only a candidate that structurally draws +/// pays for `can_draw_at_least_one`, which scans battlefield statics and consults +/// the replacement applicability authority. Reversing these two costs every +/// non-draw candidate that scan for nothing. +fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { + candidate_draws_structurally(ctx) && draw_is_deliverable(ctx) +} + +/// CR 121.1 / CR 704.5b + CR 614.6: would a draw right now actually put a card +/// into the AI's hand, emitting the `CardDrawn` event a "whenever you draw" +/// engine rides on? False under a `CantDraw` static or an exhausted +/// `PerTurnDrawLimit`, from an empty library, or when the replacement pipeline +/// removes the draw. Delegates wholly to the engine's `can_draw_at_least_one` +/// authority so the bonus is never added to a no-op draw. +/// +/// Deliberately the SECOND gate: it is the expensive one (a battlefield static +/// scan plus replacement applicability), and it is candidate-independent, so it +/// is only worth asking once a candidate is known to draw. +fn draw_is_deliverable(ctx: &PolicyContext<'_>) -> bool { + engine::game::effects::draw::can_draw_at_least_one(ctx.state, ctx.ai_player) +} + +/// Card-local structural test: does this candidate's own AST draw its controller +/// a card? Reads only the candidate, never the board. /// /// * `CastSpell` → the spell's own resolution chain (`CastFacts::primary_effects`) /// plus its immediate ETB triggers — a cast permanent's *activated* draw /// ability does not fire on cast, so only these two are inspected. /// * `ActivateAbility` → the ability at the runtime-enumerated index. -fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { - // CR 121.1 / CR 704.5b: a structural "draw a card" produces no `CardDrawn` - // event — and fires no "whenever you draw" engine — when the controller - // can't draw right now (a `CantDraw` static, or a `PerTurnDrawLimit` already - // exhausted) OR the library is empty (an empty-library draw only records an - // attempt, CR 704.5b, delivering no card). The engine's `can_draw_at_least_one` - // authority gates the whole classification so the bonus is never added to a - // no-op draw. - if !engine::game::effects::draw::can_draw_at_least_one(ctx.state, ctx.ai_player) { - return false; - } +fn candidate_draws_structurally(ctx: &PolicyContext<'_>) -> bool { // CR 700.2: a live candidate is scored before its modes are chosen, so only // an UNCONDITIONAL draw counts — a modal "choose one — draw / …" must not be // credited a draw here. @@ -141,6 +163,142 @@ fn candidate_draws_controller(ctx: &PolicyContext<'_>) -> bool { is_draw_source_parts(std::iter::once(&ability), AbilityScope::Unconditional) }) } - _ => false, + // CR 601.2 + CR 702.34a: cast-shaped siblings of the plain `CastSpell` + // seam (alternative costs, madness, miracle, foretell, ninjutsu, copies). + // `PolicyContext::cast_facts` is populated only for the `CastSpell` + // announcement seam, so this policy has no AST to classify for these and + // must report neutral rather than guess. Listed explicitly, not swept + // into a wildcard: if `cast_facts` later covers one, this arm is where + // the decision to start crediting it gets made. + GameAction::Foretell { .. } + | GameAction::PlayFaceDown { .. } + | GameAction::ActivateNinjutsu { .. } + | GameAction::CastSpellAsSneak { .. } + | GameAction::CastSpellAsWebSlinging { .. } + | GameAction::CastSpellForFree { .. } + | GameAction::CastSpellAsMiracle { .. } + | GameAction::CastSpellAsMadness { .. } + | GameAction::CastPreparedCopy { .. } + | GameAction::CastParadigmCopy { .. } => false, + // Every remaining action: not a spell cast or ability activation, so it + // cannot draw its controller a card as part of the candidate itself. + // Enumerated rather than wildcarded so a newly added `GameAction` fails + // this match at compile time and forces an intentional classification + // instead of silently bypassing the draw payoff (CR 121.1). + GameAction::PassPriority + | GameAction::ChooseMeldPair { .. } + | GameAction::ChooseEntryAttackTarget { .. } + | GameAction::PlayLand { .. } + | GameAction::DeclareAttackers { .. } + | GameAction::DeclareBlockers { .. } + | GameAction::ChooseUntap { .. } + | GameAction::ChooseExert { .. } + | GameAction::ChooseEnlist { .. } + | GameAction::ChooseClashOpponent { .. } + | GameAction::ChooseZoneOpponentChooser { .. } + | GameAction::ChoosePileOpponent { .. } + | GameAction::ChooseAnnouncingOpponent { .. } + | GameAction::ChooseGiftRecipient { .. } + | GameAction::ChooseAssistPlayer { .. } + | GameAction::CommitAssistPayment { .. } + | GameAction::MulliganDecision { .. } + | GameAction::ReorderHand { .. } + | GameAction::TapLandForMana { .. } + | GameAction::UntapLandForMana { .. } + | GameAction::SpendPoolMana { .. } + | GameAction::UnspendPoolMana { .. } + | GameAction::SelectCards { .. } + | GameAction::ChooseRemoveCounterCostDistribution { .. } + | GameAction::SelectCoinFlips { .. } + | GameAction::ChooseOutsideGameCards { .. } + | GameAction::SelectTargets { .. } + | GameAction::ChooseTarget { .. } + | GameAction::ChooseReplacement { .. } + | GameAction::OrderTriggers { .. } + | GameAction::CancelCast + | GameAction::Equip { .. } + | GameAction::CrewVehicle { .. } + | GameAction::ActivateStation { .. } + | GameAction::SaddleMount { .. } + | GameAction::Transform { .. } + | GameAction::TurnFaceUp { .. } + | GameAction::SubmitSideboard { .. } + | GameAction::ChoosePlayDraw { .. } + | GameAction::ChooseOption { .. } + | GameAction::SubmitVoteCandidate { .. } + | GameAction::SubmitSpellbookDraft { .. } + | GameAction::SubmitPilePartition { .. } + | GameAction::ChoosePile { .. } + | GameAction::ChooseBranch { .. } + | GameAction::SubmitLifeRedistribution { .. } + | GameAction::ChooseDamageSource { .. } + | GameAction::SelectModes { .. } + | GameAction::DecideOptionalCost { .. } + | GameAction::ChooseAdventureFace { .. } + | GameAction::ChooseModalFace { .. } + | GameAction::ChooseAlternativeCast { .. } + | GameAction::ChooseCastingVariant { .. } + | GameAction::KeepAllCopyTargets + | GameAction::ChoosePermanentTypeSlot { .. } + | GameAction::DecideOptionalEffect { .. } + | GameAction::RespondToSpliceOffer { .. } + | GameAction::DecideOptionalEffectAndRemember { .. } + | GameAction::PayUnlessCost { .. } + | GameAction::ChooseUnlessCostBranch { .. } + | GameAction::ChooseActivationCostBranch { .. } + | GameAction::PayCombatTax { .. } + | GameAction::ChooseRingBearer { .. } + | GameAction::ChoosePair { .. } + | GameAction::ChooseDungeon { .. } + | GameAction::ChooseDungeonRoom { .. } + | GameAction::UnlockRoomDoor { .. } + | GameAction::RollPlanarDie + | GameAction::ChooseRoomDoor { .. } + | GameAction::TapForConvoke { .. } + | GameAction::HarmonizeTap { .. } + | GameAction::DeclareCompanion { .. } + | GameAction::CompanionToHand + | GameAction::DiscoverChoice { .. } + | GameAction::GraveyardPaidCastChoice { .. } + | GameAction::CascadeChoice { .. } + | GameAction::RippleChoice { .. } + | GameAction::FreeCastWindowChoice { .. } + | GameAction::ChooseTopOrBottom { .. } + | GameAction::ChooseMutateMergeSide { .. } + | GameAction::CipherEncode { .. } + | GameAction::ChooseLegend { .. } + | GameAction::ChooseBattleProtector { .. } + | GameAction::SetAutoPass { .. } + | GameAction::CancelAutoPass + | GameAction::SetPhaseStops { .. } + | GameAction::SetPriorityPassingMode { .. } + | GameAction::SetPriorityYield { .. } + | GameAction::SetMayTriggerAutoChoice { .. } + | GameAction::SetTriggerOrderTemplate { .. } + | GameAction::AssignCombatDamage { .. } + | GameAction::AssignBlockerDamage { .. } + | GameAction::DistributeAmong { .. } + | GameAction::ChooseCounterMoveDistribution { .. } + | GameAction::ChooseCountersToRemove { .. } + | GameAction::SubmitPayAmount { .. } + | GameAction::RetargetSpell { .. } + | GameAction::LearnDecision { .. } + | GameAction::SelectCategoryPermanents { .. } + | GameAction::ChooseKeptCreatures { .. } + | GameAction::ChooseKeptPermanents { .. } + | GameAction::ChooseX { .. } + | GameAction::SubmitPhyrexianChoices { .. } + | GameAction::ChooseManaColor { .. } + | GameAction::PayManaAbilityMana { .. } + | GameAction::ChooseSpecializeColor { .. } + | GameAction::PassParadigmOffer + | GameAction::Debug(..) + | GameAction::GrantDebugPermission { .. } + | GameAction::RevokeDebugPermission { .. } + | GameAction::Concede { .. } + | GameAction::DeclareShortcut { .. } + | GameAction::RespondToShortcut { .. } + | GameAction::DeclineShortcut + | GameAction::PrecastCopyShortcut { .. } => false, } } diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index bd2fa84e08..b7361db367 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -1175,6 +1175,73 @@ fn draw_cards_stub_prevent_replacement_still_rewards() { assert!(delta > 0.0); } +// ─── bounded score (MAX_REWARDED_ENGINES) ──────────────────────────────────── + +/// Reads the `engines` observability fact off a verdict reason. +fn engines_fact(reason: &PolicyReason) -> Option { + reason + .facts + .iter() + .find(|(key, _)| *key == "engines") + .map(|(_, value)| *value) +} + +/// The per-draw reward scales with the number of live engines but is capped at +/// `MAX_REWARDED_ENGINES`, so a stacked board can't push a single draw into the +/// critical band. With one engine PAST the cap the delta must not grow. +/// +/// The `engines` fact deliberately reports the TRUE uncapped count — that is an +/// observability contract (it explains the board to a log reader), distinct from +/// the bounded score. Both halves are asserted so neither can drift. +#[test] +fn reward_is_capped_at_max_rewarded_engines() { + let bonus = AiConfig::default().policy_penalties.draw_payoff_bonus; + + let mut at_cap = state(); + for _ in 0..MAX_REWARDED_ENGINES { + engine_on_battlefield(&mut at_cap); + } + let (delta_at_cap, reason_at_cap) = draw_spell_verdict(&mut at_cap); + + let mut over_cap = state(); + for _ in 0..MAX_REWARDED_ENGINES + 1 { + engine_on_battlefield(&mut over_cap); + } + let (delta_over_cap, reason_over_cap) = draw_spell_verdict(&mut over_cap); + + assert_eq!(reason_at_cap.kind, "draw_payoff_engine_active"); + assert_eq!(reason_over_cap.kind, "draw_payoff_engine_active"); + assert_eq!( + delta_at_cap, + bonus * MAX_REWARDED_ENGINES as f64, + "at the cap the reward is one bonus per live engine" + ); + assert_eq!( + delta_over_cap, delta_at_cap, + "an engine past MAX_REWARDED_ENGINES must not increase the reward — \ + without the cap this would scale without bound" + ); + assert_eq!( + engines_fact(&reason_over_cap), + Some(MAX_REWARDED_ENGINES as i64 + 1), + "the `engines` fact reports the true uncapped count for observability" + ); +} + +/// Below the cap the reward still scales, so the test above is pinning a CAP and +/// not merely a constant score. +#[test] +fn reward_scales_below_the_cap() { + let bonus = AiConfig::default().policy_penalties.draw_payoff_bonus; + let mut st = state(); + engine_on_battlefield(&mut st); + engine_on_battlefield(&mut st); + let (delta, reason) = draw_spell_verdict(&mut st); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert_eq!(delta, bonus * 2.0); + assert_eq!(engines_fact(&reason), Some(2)); +} + // ─── replacement substitution and rescaling (CR 614.11) ────────────────────── // // A `Prevent` quantity modification is only ONE of the three ways the pipeline From 49e22f56c7608505921c9fdcd03319f67e46cb43 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 11:06:22 -0700 Subject: [PATCH 16/17] fix(phase-ai): classify GameAction::EndContinuousEffect in the draw-payoff match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main added a 128th `GameAction` variant while this branch carried a 127-arm exhaustive match, so the merge CI builds were non-exhaustive and every Rust job failed to compile. Classify it neutral: ending a continuous effect draws no card. This is the maintenance cost of an exhaustive action match living inside a single policy — see the PR discussion for centralizing it in the routing authority instead. Co-Authored-By: Claude Opus 5 (1M context) --- crates/phase-ai/src/policies/draw_payoff.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index efd81bbdf6..74906d553e 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -299,6 +299,7 @@ fn candidate_draws_structurally(ctx: &PolicyContext<'_>) -> bool { | GameAction::DeclareShortcut { .. } | GameAction::RespondToShortcut { .. } | GameAction::DeclineShortcut - | GameAction::PrecastCopyShortcut { .. } => false, + | GameAction::PrecastCopyShortcut { .. } + | GameAction::EndContinuousEffect { .. } => false, } } From 7b8bdd3b4b9fe297d1cb16cd4597235d0dd30baa Mon Sep 17 00:00:00 2001 From: minion1227 Date: Mon, 27 Jul 2026 13:12:33 -0700 Subject: [PATCH 17/17] fix(phase-ai): require a positive resolved draw quantity on live candidates (round 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_draw_source_parts` matched every controller-targeted `Effect::Draw` without inspecting its count, while the resolver resolves that quantity (`resolve_quantity_with_targets(..).max(0)`) and emits `CardDrawn` only per delivered card. A candidate whose own instruction draws ZERO therefore received the full draw-payoff bonus despite being unable to fire any engine. The player-level delivery gate added last round does not catch this: it asks whether the PLAYER can draw, not whether this CANDIDATE draws. Parameterizes the one classifier with a typed `DrawQuantity` rather than forking a second copy: `Any` for deck-time classification (a "draw X" card is still an enabler for archetype detection, its count unknowable at deck-build time) and `ResolvesPositive` for a live candidate. The positive check delegates to the engine's `resolve_quantity` authority instead of re-deriving quantity semantics, so it agrees with the resolver by construction. X is genuinely unbound at the candidate seam — `resolve_quantity` supplies no `chosen_x` (only `resolve_quantity_with_targets` does, from a `ResolvedAbility` a spell still being announced does not have), so every `Variable{X}` draw resolves to zero and stays conservatively neutral until known positive. Tests: registry-routed fixed-zero (withheld) and fixed-positive (rewarded) pair; an X case asserting neutrality for both unset and set `cost_x_paid`, documenting that a stale value from an earlier activation must not be mistaken for this candidate's X; and a live-resolver assertion in the equivalence suite that a zero-count draw emits no `CardDrawn`, paired with a positive control. Co-Authored-By: Claude Opus 5 (1M context) --- .../draw_preflight_matches_live_pipeline.rs | 52 +++++++++ crates/phase-ai/src/features/draw_matters.rs | 75 ++++++++++-- crates/phase-ai/src/policies/draw_payoff.rs | 29 ++++- .../src/policies/tests/draw_payoff.rs | 109 +++++++++++++++++- 4 files changed, 250 insertions(+), 15 deletions(-) diff --git a/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs index 4b706784f5..85cefc3e9a 100644 --- a/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs +++ b/crates/engine/tests/integration/draw_preflight_matches_live_pipeline.rs @@ -282,3 +282,55 @@ fn zero_count_replacement_is_predicted_and_not_delivered() { ); assert_preflight_matches_pipeline("zero-count replacement", runner, false); } + +// ─── candidate-instruction quantity (CR 121.1 + CR 107.1b) ─────────────────── +// +// The cases above vary the PLAYER's ability to draw. A draw also fails to fire +// an engine when the instruction's OWN count resolves to zero — a distinct axis, +// gated in `DrawPayoffPolicy` by requiring a positive resolved candidate +// quantity. These pin the live-resolver behavior that gate models: the resolver +// resolves the effect's quantity and emits `CardDrawn` only per delivered card, +// so a zero-count draw emits none even with a healthy library. + +/// Resolves a controller-targeted `Effect::Draw` of `count` on a fresh board and +/// reports whether the live resolver emitted any `CardDrawn` event. +fn live_draw_emits_card_drawn(count: i32) -> bool { + let mut runner = scenario(3, None); + let source = runner.state().players[P0.0 as usize] + .library + .iter() + .next() + .copied() + .expect("seeded library"); + let ability = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Fixed { value: count }, + target: TargetFilter::Controller, + }, + Vec::new(), + source, + P0, + ); + let mut events = Vec::new(); + engine::game::effects::draw::resolve(runner.state_mut(), &ability, &mut events) + .expect("draw resolution must succeed"); + events + .iter() + .any(|e| matches!(e, engine::types::events::GameEvent::CardDrawn { .. })) +} + +/// CR 107.1b: a zero-count draw instruction delivers no card, so the resolver +/// emits no `CardDrawn` and a "whenever you draw" engine never triggers — the +/// live fact behind `DrawPayoffPolicy` requiring a positive candidate quantity. +/// Paired with a positive control so this cannot pass by the resolver breaking. +#[test] +fn zero_count_draw_instruction_emits_no_card_drawn() { + assert!( + !live_draw_emits_card_drawn(0), + "a draw of zero cards must emit no CardDrawn event" + ); + assert!( + live_draw_emits_card_drawn(1), + "control: a draw of one card must emit CardDrawn" + ); +} diff --git a/crates/phase-ai/src/features/draw_matters.rs b/crates/phase-ai/src/features/draw_matters.rs index 21ee586ecf..3bad83f6d6 100644 --- a/crates/phase-ai/src/features/draw_matters.rs +++ b/crates/phase-ai/src/features/draw_matters.rs @@ -33,9 +33,15 @@ //! and the axes stay independent. use engine::game::ability_utils::ability_definition_supported; +use engine::game::quantity::resolve_quantity; use engine::game::DeckEntry; -use engine::types::ability::{AbilityDefinition, Effect, TargetFilter, TriggerDefinition}; +use engine::types::ability::{ + AbilityDefinition, Effect, QuantityExpr, TargetFilter, TriggerDefinition, +}; use engine::types::card_type::CoreType; +use engine::types::game_state::GameState; +use engine::types::identifiers::ObjectId; +use engine::types::player::PlayerId; use engine::types::triggers::TriggerMode; use engine::types::zones::Zone; @@ -85,7 +91,7 @@ pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { // draw enabler for the archetype, so scan the full potential tree — plus // ETB "cantrip" triggers (Elvish Visionary), which the live policy also // credits via `CastFacts::immediate_etb_triggers`. - if is_draw_source_parts(&face.abilities, AbilityScope::Potential) + if is_draw_source_parts(&face.abilities, AbilityScope::Potential, &DrawQuantity::Any) || is_etb_draw_source(&face.triggers) { source_count = source_count.saturating_add(entry.count); @@ -104,22 +110,71 @@ pub fn detect(deck: &[DeckEntry]) -> DrawMattersFeature { } } +/// Whether a draw instruction's COUNT must be established positive. +/// +/// CR 121.1 + CR 107.1b: "draw N cards" resolves its quantity at resolution +/// (`effects::draw::resolve` → `resolve_quantity_with_targets(..).max(0)`), so a +/// count of zero puts no card into hand and emits no `CardDrawn` — it fires no +/// "whenever you draw" engine. Deck classification and live candidate scoring +/// want different answers about that, so the requirement is a parameter of the +/// one classifier rather than a second forked copy of it. +pub(crate) enum DrawQuantity<'a> { + /// Deck-time: any draw instruction marks the card regardless of count. A + /// "draw X" or "draw cards equal to …" card is still a draw enabler for + /// archetype classification — its count is unknowable at deck-build time. + Any, + /// Live candidate: the count must resolve to at least one card *now*. + /// + /// Delegates to the engine's `resolve_quantity` authority rather than + /// re-deriving quantity semantics, so this agrees with the resolver by + /// construction. That also yields the correct conservative behavior for an + /// unbound `X`: `QuantityRef::Variable { "X" }` reads `cost_x_paid` off the + /// source and falls back to 0 when X has not been announced yet, so an + /// unbound dynamic draw stays neutral until it is known positive. + ResolvesPositive { + state: &'a GameState, + controller: PlayerId, + source: ObjectId, + }, +} + +impl DrawQuantity<'_> { + /// CR 121.1: does this draw deliver at least one card under this requirement? + fn is_satisfied_by(&self, count: &QuantityExpr) -> bool { + match self { + DrawQuantity::Any => true, + DrawQuantity::ResolvesPositive { + state, + controller, + source, + } => resolve_quantity(state, count, *controller, *source) >= 1, + } + } +} + /// CR 121.1: the abilities draw YOU one or more cards — a repeatable enabler for /// the payoff engine. Parts-based so it classifies both a deck-time /// `CardFace.abilities` slice and the action's runtime effect chain -/// (`CastFacts::primary_effects` / the activated ability). The caller chooses the -/// `scope`: `Potential` for deck-time (a modal draw mode still marks the card), -/// `Unconditional` for a live candidate before its mode is selected (CR 700.2 — -/// a modal "choose one — draw / …" must NOT be credited a draw until the draw -/// mode is actually chosen). +/// (`CastFacts::primary_effects` / the activated ability). +/// +/// The caller chooses the `scope`: `Potential` for deck-time (a modal draw mode +/// still marks the card), `Unconditional` for a live candidate before its mode is +/// selected (CR 700.2 — a modal "choose one — draw / …" must NOT be credited a +/// draw until the draw mode is actually chosen). +/// +/// The caller also chooses the `quantity` requirement — see [`DrawQuantity`]. A +/// live candidate must pass `ResolvesPositive`, or a "draw zero" instruction is +/// scored as though it fired the engine. pub(crate) fn is_draw_source_parts<'a>( abilities: impl IntoIterator, scope: AbilityScope, + quantity: &DrawQuantity<'_>, ) -> bool { abilities.into_iter().any(|ability| { - collect_scoped_effects(ability, scope) - .iter() - .any(|effect| matches!(effect, Effect::Draw { target, .. } if draws_controller(target))) + collect_scoped_effects(ability, scope).iter().any(|effect| { + matches!(effect, Effect::Draw { target, count } + if draws_controller(target) && quantity.is_satisfied_by(count)) + }) }) } diff --git a/crates/phase-ai/src/policies/draw_payoff.rs b/crates/phase-ai/src/policies/draw_payoff.rs index 74906d553e..1523e59c75 100644 --- a/crates/phase-ai/src/policies/draw_payoff.rs +++ b/crates/phase-ai/src/policies/draw_payoff.rs @@ -26,7 +26,7 @@ use engine::types::game_state::GameState; use engine::types::player::PlayerId; use crate::features::draw_matters::{ - is_draw_payoff_trigger, is_draw_source_parts, AbilityScope, DRAW_MATTERS_FLOOR, + is_draw_payoff_trigger, is_draw_source_parts, AbilityScope, DrawQuantity, DRAW_MATTERS_FLOOR, }; use crate::features::DeckFeatures; @@ -132,8 +132,24 @@ fn draw_is_deliverable(ctx: &PolicyContext<'_>) -> bool { engine::game::effects::draw::can_draw_at_least_one(ctx.state, ctx.ai_player) } +/// CR 121.1 + CR 107.1b: the live-candidate quantity requirement — this draw must +/// resolve to at least one card, or it emits no `CardDrawn` and fires no engine. +/// `source` is the object whose `cost_x_paid` binds an announced `X`, so an +/// un-announced X resolves to zero and the candidate stays neutral. +fn positive_draw_quantity<'a>( + ctx: &PolicyContext<'a>, + source: engine::types::identifiers::ObjectId, +) -> DrawQuantity<'a> { + DrawQuantity::ResolvesPositive { + state: ctx.state, + controller: ctx.ai_player, + source, + } +} + /// Card-local structural test: does this candidate's own AST draw its controller -/// a card? Reads only the candidate, never the board. +/// a card, in a quantity that actually delivers one? Reads the candidate's AST +/// plus the engine's quantity authority; never scans the board. /// /// * `CastSpell` → the spell's own resolution chain (`CastFacts::primary_effects`) /// plus its immediate ETB triggers — a cast permanent's *activated* draw @@ -156,11 +172,16 @@ fn candidate_draws_structurally(ctx: &PolicyContext<'_>) -> bool { is_draw_source_parts( facts.primary_effects.iter().copied().chain(etb_bodies), AbilityScope::Unconditional, + &positive_draw_quantity(ctx, facts.object.id), ) }), - GameAction::ActivateAbility { .. } => { + GameAction::ActivateAbility { source_id, .. } => { ctx.effective_activated_ability().is_some_and(|ability| { - is_draw_source_parts(std::iter::once(&ability), AbilityScope::Unconditional) + is_draw_source_parts( + std::iter::once(&ability), + AbilityScope::Unconditional, + &positive_draw_quantity(ctx, *source_id), + ) }) } // CR 601.2 + CR 702.34a: cast-shaped siblings of the plain `CastSpell` diff --git a/crates/phase-ai/src/policies/tests/draw_payoff.rs b/crates/phase-ai/src/policies/tests/draw_payoff.rs index b7361db367..a9a44f5afb 100644 --- a/crates/phase-ai/src/policies/tests/draw_payoff.rs +++ b/crates/phase-ai/src/policies/tests/draw_payoff.rs @@ -10,7 +10,7 @@ use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, Tac use engine::game::zones::create_object; use engine::types::ability::{ AbilityDefinition, AbilityKind, CastVariantPaid, DrawReplacementScope, Effect, ModalChoice, - QuantityExpr, QuantityModification, ReplacementCondition, ReplacementDefinition, + QuantityExpr, QuantityModification, QuantityRef, ReplacementCondition, ReplacementDefinition, ReplacementMode, StaticDefinition, TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, }; @@ -1175,6 +1175,113 @@ fn draw_cards_stub_prevent_replacement_still_rewards() { assert!(delta > 0.0); } +// ─── candidate draw quantity (CR 121.1 + CR 107.1b) ────────────────────────── +// +// A draw instruction only fires the engine if it actually delivers a card. The +// resolver resolves the effect's own quantity (`resolve_quantity_with_targets(..) +// .max(0)`), so a zero count emits no `CardDrawn` no matter how healthy the +// library is. These pin that the candidate's OWN count is required positive, +// distinct from the player-level "can this player draw at all" delivery gate. + +/// Routes a cast candidate through `PolicyRegistry` and returns its verdict. +fn registry_cast_verdict(st: &GameState, oid: ObjectId, cid: CardId) -> (f64, PolicyReason) { + let config = AiConfig::default(); + let context = context(&config, session(0.9)); + let candidate = cast(oid, cid); + let decision = priority_decision(&candidate); + PolicyRegistry::default() + .verdicts(&ctx(st, &candidate, &decision, &context, &config)) + .into_iter() + .find(|(id, _)| *id == PolicyId::DrawPayoff) + .map(|(_, v)| score_of(v)) + .expect("the cast must reach the policy through the registry") +} + +/// CR 107.1b: a fixed zero-count draw resolves to no cards, so no `CardDrawn` +/// fires and the engine never triggers — the payoff must be withheld even with a +/// live engine and a full library. Registry-routed, so the production seam is +/// what is asserted. +#[test] +fn registry_fixed_zero_count_draw_is_not_rewarded() { + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = spell( + &mut st, + Effect::Draw { + count: QuantityExpr::Fixed { value: 0 }, + target: TargetFilter::Controller, + }, + ); + let (delta, reason) = registry_cast_verdict(&st, oid, cid); + assert_eq!(reason.kind, "draw_payoff_na"); + assert_eq!(delta, 0.0); +} + +/// Discriminating control for the case above: identical shape, positive count. +/// Without this pair the zero-count assertion is satisfiable by a policy that +/// stopped rewarding casts altogether. +#[test] +fn registry_positive_count_draw_is_rewarded() { + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = spell( + &mut st, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ); + let (delta, reason) = registry_cast_verdict(&st, oid, cid); + assert_eq!(reason.kind, "draw_payoff_engine_active"); + assert!(delta > 0.0); +} + +/// Builds a "draw X cards" spell and binds `X` on the source via `cost_x_paid`, +/// the slot `QuantityRef::Variable { "X" }` reads (CR 601.2b). +fn draw_x_spell(state: &mut GameState, x: Option) -> (ObjectId, CardId) { + let (oid, cid) = spell( + state, + Effect::Draw { + count: QuantityExpr::Ref { + qty: QuantityRef::Variable { + name: "X".to_string(), + }, + }, + target: TargetFilter::Controller, + }, + ); + state.objects.get_mut(&oid).unwrap().cost_x_paid = x; + (oid, cid) +} + +/// CR 601.2b: a "draw X cards" candidate is scored BEFORE X is announced, so its +/// count is not knowable at this seam and the policy stays neutral rather than +/// crediting a draw it cannot confirm — the same conservative direction as the +/// trigger-eligibility gate. +/// +/// Asserted for both an unset and a set `cost_x_paid` because the engine's +/// `resolve_quantity` reads X from the RESOLVING ability's `chosen_x`, which only +/// `resolve_quantity_with_targets` supplies from a `ResolvedAbility` — a spell +/// still being announced has none. `cost_x_paid` on the object is therefore not +/// consulted here, and a stale value from an earlier activation must not be +/// mistaken for this candidate's X. Both cases resolve to zero, so both are +/// neutral; this pins that equivalence so a future X-binding change has to come +/// with a deliberate decision about which value the policy trusts. +#[test] +fn registry_x_draw_is_conservatively_neutral_before_announcement() { + for cost_x_paid in [None, Some(2)] { + let mut st = state(); + engine_on_battlefield(&mut st); + let (oid, cid) = draw_x_spell(&mut st, cost_x_paid); + let (delta, reason) = registry_cast_verdict(&st, oid, cid); + assert_eq!( + reason.kind, "draw_payoff_na", + "X is unbound at the candidate seam (cost_x_paid={cost_x_paid:?})" + ); + assert_eq!(delta, 0.0); + } +} + // ─── bounded score (MAX_REWARDED_ENGINES) ──────────────────────────────────── /// Reads the `engines` observability fact off a verdict reason.