From 2440b1892fd89cda669c087bec9ff10fffec8d56 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 28 Jul 2026 10:39:37 -0700 Subject: [PATCH 1/6] feat(phase-ai): add cost-reduction deck-feature axis + CostReductionPolicy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `features::mana_ramp` explicitly deferred this shape — "`StaticMode::ModifyCost` is deliberately out of scope — cost reducers are a follow-up feature". This is that follow-up. A Goblin Electromancer / Baral / Foundry Inspector / Medallion effect is acceleration that never taps for mana: every later spell costs less for as long as the permanent survives (CR 601.2f). The engine already applies the discount at cast time, so the AI is never overcharged — but nothing valued *deploying* the reducer. `mana_ramp` only sees effects that add mana, so a deck whose whole acceleration plan is cost reduction read as having no ramp at all. Feature (`features/cost_reduction.rs`), structural over `CardFace` AST: - `reducer_count` — cards carrying a board-wide `ModifyCost { Reduce }` that applies to spells YOU cast. Mirrors the eligibility tests `casting::collect_cost_modifiers` applies at cast time, so deck-time classification and the resolver agree by construction: `Reduce` only (`Raise` is Thalia, `Minimum` is Trinisphere), never a `SelfRef` self-cost reduction (CR 113.6), never opponent-scoped. - `total_discount` — summed generic discount. CR 118.7a: a generic reduction affects only the generic component, so the magnitude is `generic`, not the amount's full mana value. - `discounted_count` / coverage — deck cards a reducer's `spell_filter` actually admits, delegating every type leaf to the engine's CR 205 authority (`matches_type_filter_against_face`, widened to `pub`) rather than re-deriving type semantics. An unverifiable filter reports no coverage, so the axis fails OFF rather than claiming a discount the deck may not get. - `commitment` — geometric mean over (reducer density, coverage): both pillars mandatory. Reducers that discount nothing are blanks; spells with no reducer are just spells. Policy (`policies/cost_reduction.rs`, `CastSpell`): - `cost_reduction_deploy_engine` — credit for deploying a reducer, scaled by capped saved mana across the remaining grip. - `cost_reduction_defer_to_engine` — nudge against casting past an unplayed, cheaper reducer (the `RampTimingPolicy::defer_to_ramp` shape). - The card-local static check runs first, so every non-reducer candidate is rejected after reading one card's AST; only a confirmed reducer pays for the hand walk. No battlefield sweep, no `find_legal_targets`, no affordability query in the search inner loop. Both scoring scalars land in `UNTUNED_POLICY_PENALTY_FIELDS` pending a paired-seed calibration. 33 tests: per-axis detection, both calibration anchors, every exclusion (`Raise`/`Minimum`/`SelfRef`/opponent-scope/zero-generic/unverifiable-filter), format-size neutrality, the bounded-score ceiling, and a registry-routed regression covering registration + `CastSpell` routing. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/filter.rs | 7 +- crates/phase-ai/src/config.rs | 25 + .../phase-ai/src/features/cost_reduction.rs | 285 ++++++++++++ crates/phase-ai/src/features/mana_ramp.rs | 6 +- crates/phase-ai/src/features/mod.rs | 6 + .../src/features/tests/cost_reduction.rs | 366 +++++++++++++++ crates/phase-ai/src/features/tests/mod.rs | 1 + .../phase-ai/src/policies/cost_reduction.rs | 154 ++++++ crates/phase-ai/src/policies/mod.rs | 1 + crates/phase-ai/src/policies/registry.rs | 2 + .../src/policies/tests/cost_reduction.rs | 438 ++++++++++++++++++ crates/phase-ai/src/policies/tests/mod.rs | 1 + 12 files changed, 1289 insertions(+), 3 deletions(-) create mode 100644 crates/phase-ai/src/features/cost_reduction.rs create mode 100644 crates/phase-ai/src/features/tests/cost_reduction.rs create mode 100644 crates/phase-ai/src/policies/cost_reduction.rs create mode 100644 crates/phase-ai/src/policies/tests/cost_reduction.rs diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 840c774939..3244d6a723 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1297,7 +1297,12 @@ pub(crate) fn matches_target_filter_against_face(face: &CardFace, filter: &Targe /// CR 205: Evaluate a single `TypeFilter` against a bare `CardFace`'s printed /// card type line (core types, subtypes, supertypes). Context-free counterpart /// to the object-based type checks in `filter_inner_for_object`. -pub(crate) fn matches_type_filter_against_face(face: &CardFace, filter: &TypeFilter) -> bool { +/// +/// `pub` because deck-time analysis outside this crate (`phase-ai`'s +/// `features::cost_reduction`) classifies bare `CardFace`s against a static's +/// `spell_filter` before any `GameObject` exists, and must use this authority +/// for the type axis rather than re-deriving CR 205 type semantics. +pub fn matches_type_filter_against_face(face: &CardFace, filter: &TypeFilter) -> bool { match filter { TypeFilter::Creature => face.card_type.core_types.contains(&CoreType::Creature), TypeFilter::Land => face.card_type.core_types.contains(&CoreType::Land), diff --git a/crates/phase-ai/src/config.rs b/crates/phase-ai/src/config.rs index e391be8182..922c2d9020 100644 --- a/crates/phase-ai/src/config.rs +++ b/crates/phase-ai/src/config.rs @@ -503,6 +503,14 @@ pub struct PolicyPenalties { /// draw" engine (preference band, per engine). #[serde(default = "default_draw_payoff_bonus")] pub draw_payoff_bonus: f64, + /// CR 601.2f: card-equivalent value of ONE generic mana saved by deploying a + /// cost reducer, multiplied by the capped saved-mana total. + #[serde(default = "default_cost_reduction_deploy_bonus")] + pub cost_reduction_deploy_bonus: f64, + /// CR 601.2f: nudge-band penalty for casting past an unplayed, cheaper cost + /// reducer — the discount should be deployed first. + #[serde(default = "default_cost_reduction_defer_penalty")] + pub cost_reduction_defer_penalty: f64, } impl Default for PolicyPenalties { @@ -577,6 +585,8 @@ impl Default for PolicyPenalties { devotion_pip_progress: default_devotion_pip_progress(), devotion_god_activation: default_devotion_god_activation(), draw_payoff_bonus: default_draw_payoff_bonus(), + cost_reduction_deploy_bonus: default_cost_reduction_deploy_bonus(), + cost_reduction_defer_penalty: default_cost_reduction_defer_penalty(), } } } @@ -674,6 +684,12 @@ fn default_devotion_god_activation() -> f64 { fn default_draw_payoff_bonus() -> f64 { 0.6 } +fn default_cost_reduction_deploy_bonus() -> f64 { + 0.2 +} +fn default_cost_reduction_defer_penalty() -> f64 { + -0.25 +} fn default_sacrifice_token_cost() -> f64 { 0.5 } @@ -822,6 +838,15 @@ pub const UNTUNED_POLICY_PENALTY_FIELDS: &[(&str, &str)] = &[ "draw_payoff_bonus", "CR 121.1 per-engine draw-payoff weight — awaiting a paired-seed ai-gate calibration.", ), + ( + "cost_reduction_deploy_bonus", + "CR 601.2f per-saved-mana deployment weight — awaiting a paired-seed ai-gate calibration.", + ), + ( + "cost_reduction_defer_penalty", + "CR 601.2f sequencing nudge for casting past a cheaper unplayed reducer — \ + 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/cost_reduction.rs b/crates/phase-ai/src/features/cost_reduction.rs new file mode 100644 index 0000000000..9eedaa3719 --- /dev/null +++ b/crates/phase-ai/src/features/cost_reduction.rs @@ -0,0 +1,285 @@ +//! Cost-reduction feature — structural detection of a deck that discounts its +//! own spells (CR 601.2f). +//! +//! Parser AST verification — VERIFIED against engine source: +//! - `StaticMode::ModifyCost { mode, amount, spell_filter, dynamic_count }` at +//! `crates/engine/src/types/statics.rs:1017` — the reducer itself. +//! - `CostModifyMode::{Reduce, Raise, Minimum}` at `statics.rs:698`; only +//! `Reduce` discounts (CR 601.2f), `Raise`/`Minimum` are the Thalia / +//! Trinisphere tax shapes and are NOT this axis. +//! - `StaticDefinition.affected: Option` at +//! `crates/engine/src/types/ability.rs:20740` — carries the caster scope +//! (`TypedFilter.controller`), exactly as `collect_cost_modifiers` reads it. +//! - `CardFace.static_abilities: Vec` at `card.rs:162`; +//! the runtime counterpart is `GameObject.static_definitions`. +//! - `ManaCost::Cost { generic, shards }` at +//! `crates/engine/src/types/mana.rs:1714`. +//! +//! No parser remediation required — every axis is expressible over existing +//! typed AST. `features::mana_ramp` explicitly deferred this shape +//! ("`StaticMode::ModifyCost` is deliberately out of scope — cost reducers are +//! a follow-up feature"); this module is that follow-up. +//! +//! ## Why this axis exists +//! +//! A Goblin Electromancer / Baral / Foundry Inspector / Medallion effect is +//! acceleration that never taps for mana: every subsequent spell costs less for +//! as long as the permanent survives (CR 601.2f). The engine already *applies* +//! the discount when the AI casts, so the AI is never overcharged — but nothing +//! makes it *value deploying the reducer first*. `mana_ramp` only sees effects +//! that add mana (`Effect::Mana`, land-fetch, extra land drops), so a deck whose +//! entire acceleration plan is cost reduction reads as having no ramp at all. +//! This axis lets a policy see that plan. +//! +//! ## Boundary with `mana_ramp` +//! +//! `mana_ramp` measures mana *added* to the pool; this axis measures cost +//! *removed* from spells. The two are disjoint at the AST level (`Effect::Mana` +//! vs `StaticMode::ModifyCost`) and a card is never counted by both. A deck can +//! read high on both — Sol Ring plus Medallions is a real shell — and the axes +//! stay independent. + +use engine::game::filter::matches_type_filter_against_face; +use engine::game::DeckEntry; +use engine::types::ability::{ControllerRef, StaticDefinition, TargetFilter}; +use engine::types::card::CardFace; +use engine::types::card_type::CoreType; +use engine::types::mana::ManaCost; +use engine::types::statics::{CostModifyMode, StaticMode}; + +use crate::features::commitment; + +/// Commitment at or above which discounting your own spells is a real plan for +/// this deck rather than one incidental Medallion. Gates +/// `CostReductionPolicy::activation`. +/// +/// Calibrated so a shell with four two-mana reducers over ~36 nonland cards +/// (commitment ≈ 0.61) activates while a deck running two (≈ 0.43) does not — +/// see [`compute_commitment`]. +pub const COST_REDUCTION_FLOOR: f32 = 0.45; + +/// Reducer density (per 60 nonland) at which the engine pillar saturates. Ten +/// discount permanents per 60 nonland is a fully-committed cost-reduction base. +const REDUCER_SATURATION_PER_60: f32 = 10.0; + +/// CR 601.2f: per-deck cost-reduction classification. +/// +/// Populated once per game from `DeckEntry` data. Detection is structural over +/// `CardFace.static_abilities` — never by card name. +#[derive(Debug, Clone, Default)] +pub struct CostReductionFeature { + /// Cards carrying a board-wide CR 601.2f reducer that applies to spells YOU + /// cast — the engines. Excludes self-cost reductions ("this spell costs {1} + /// less") and opponent-scoped taxes. + pub reducer_count: u32, + /// Summed generic-mana discount those reducers deliver per application. + /// + /// CR 118.7a: a generic cost reduction affects only the generic component of + /// a cost, so the magnitude is the reduction's generic amount — not its full + /// mana value. + pub total_discount: u32, + /// Nonland deck cards that at least one of those reducers actually discounts + /// (its `spell_filter` admits them) — the spells the engines pay off on. + pub discounted_count: u32, + /// `0.0..=1.0` — how central discounting your own spells is to this deck. + /// Consumed by `CostReductionPolicy::activation` as the single scaling knob. + pub commitment: f32, +} + +/// Structural detection over each `DeckEntry`'s `CardFace` AST. +pub fn detect(deck: &[DeckEntry]) -> CostReductionFeature { + if deck.is_empty() { + return CostReductionFeature::default(); + } + + let mut reducer_count = 0u32; + let mut total_discount = 0u32; + let mut total_nonland = 0u32; + // One entry per reducing static, so coverage is measured against every + // discount the deck can put on the battlefield. + let mut spell_filters: Vec> = Vec::new(); + + for entry in deck { + let face = &entry.card; + if !face.card_type.core_types.contains(&CoreType::Land) { + total_nonland = total_nonland.saturating_add(entry.count); + } + + let discount = your_spell_discount_parts(&face.static_abilities); + if discount == 0 { + continue; + } + reducer_count = reducer_count.saturating_add(entry.count); + total_discount = total_discount.saturating_add(discount.saturating_mul(entry.count)); + // Hoisted out of any per-copy loop: the filter set is about WHAT the + // deck discounts, so each unique face contributes its filters once. + for def in &face.static_abilities { + if let Some(filter) = your_spell_discount_filter(def) { + spell_filters.push(filter); + } + } + } + + // Second pass: how much of the deck do those filters actually admit? A + // reducer whose filter matches nothing this deck plays is not an engine. + let mut discounted_count = 0u32; + if !spell_filters.is_empty() { + for entry in deck { + let face = &entry.card; + if face.card_type.core_types.contains(&CoreType::Land) { + continue; + } + if spell_filters + .iter() + .any(|filter| filter_admits_face(filter.as_ref(), face)) + { + discounted_count = discounted_count.saturating_add(entry.count); + } + } + } + + let commitment = compute_commitment(reducer_count, discounted_count, total_nonland); + + CostReductionFeature { + reducer_count, + total_discount, + discounted_count, + commitment, + } +} + +/// CR 601.2f: total per-application generic discount these statics give to +/// spells YOU cast. `0` means "not a cost-reduction engine". +/// +/// Parts-based so it classifies both a deck-time `CardFace.static_abilities` +/// slice and a live `GameObject.static_definitions` slice — the two carry the +/// same `StaticDefinition` shape under different field names. +pub(crate) fn your_spell_discount_parts<'a>( + statics: impl IntoIterator, +) -> u32 { + statics + .into_iter() + .filter_map(your_spell_discount) + .fold(0u32, u32::saturating_add) +} + +/// CR 601.2f: the generic discount this one static gives to spells you cast, or +/// `None` when it is not a board-wide reduction of your own spells. +/// +/// Mirrors the eligibility tests `casting::collect_cost_modifiers` applies at +/// cast time, so deck classification and the resolver agree by construction: +/// `Reduce` mode only, never a `SelfRef` self-cost reduction, and never an +/// opponent-scoped modifier. +fn your_spell_discount(def: &StaticDefinition) -> Option { + let StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount, + .. + } = &def.mode + else { + // `Raise` (Thalia) and `Minimum` (Trinisphere) are taxes, not discounts. + return None; + }; + + // CR 113.6: a `SelfRef` reduction is "this spell costs {N} less" — resolved + // by `apply_self_spell_cost_modifiers` for the spell being cast, and never + // applied from a battlefield permanent to other spells. It is a property of + // one card, not a deck-wide engine. + if matches!(def.affected, Some(TargetFilter::SelfRef)) { + return None; + } + + // CR 601.2f: caster scope. `Opponent` is a discount handed to the other + // side; only `You` and an unscoped modifier reduce spells you cast. + if let Some(TargetFilter::Typed(typed)) = &def.affected { + if matches!(typed.controller, Some(ControllerRef::Opponent)) { + return None; + } + } + + // CR 118.7a: only the generic component of a cost can be reduced by a + // generic reduction, so the discount magnitude is `generic`. A reduction of + // zero generic mana (a purely colored `amount`) moves no cost here and is + // not counted as an engine. + let ManaCost::Cost { generic, .. } = amount else { + return None; + }; + (*generic > 0).then_some(*generic) +} + +/// The `spell_filter` of a qualifying reducer — `Some(None)` for "discounts +/// every spell you cast", `None` when this static is not a qualifying reducer. +/// +/// Separate from [`your_spell_discount`] because coverage is keyed on WHAT is +/// discounted while magnitude is keyed on HOW MUCH; folding both into one +/// return type would force every caller to destructure a tuple it half-ignores. +fn your_spell_discount_filter(def: &StaticDefinition) -> Option> { + your_spell_discount(def)?; + let StaticMode::ModifyCost { spell_filter, .. } = &def.mode else { + return None; + }; + Some(spell_filter.clone()) +} + +/// CR 601.2f: would this reducer's `spell_filter` admit `face` as a discounted +/// spell? `None` is an unfiltered reducer — it discounts everything you cast. +/// +/// Evaluated on the TYPE axis only, delegating every leaf to the engine's +/// CR 205 authority (`matches_type_filter_against_face`). The caster axis is +/// deliberately not re-checked here: `StaticDefinition.affected` already carried +/// it in [`your_spell_discount`], which is exactly how +/// `casting::collect_cost_modifiers` splits the two. +/// +/// Anything this cannot verify — a `properties` predicate that needs live game +/// state, or a filter variant outside the composition below — reports `false`, +/// so an unreadable filter *undercounts* coverage and drives commitment DOWN. +/// The axis fails off rather than claiming a discount the deck may not get. +fn filter_admits_face(filter: Option<&TargetFilter>, face: &CardFace) -> bool { + let Some(filter) = filter else { + return true; + }; + match filter { + TargetFilter::Any => true, + TargetFilter::Typed(typed) => { + typed.properties.is_empty() + && typed + .type_filters + .iter() + .all(|type_filter| matches_type_filter_against_face(face, type_filter)) + } + TargetFilter::Or { filters } => filters + .iter() + .any(|inner| filter_admits_face(Some(inner), face)), + TargetFilter::And { filters } => filters + .iter() + .all(|inner| filter_admits_face(Some(inner), face)), + _ => false, + } +} + +/// Calibration: an Izzet spells shell with four two-mana reducers (Goblin +/// Electromancer / Baral, "instant and sorcery spells you cast cost {1} less") +/// over ~36 nonland cards, ~20 of which are instants or sorceries → +/// reducer density 6.67/60 → 0.667, coverage 20/36 → 0.556, commitment ≈ 0.61. +/// A full artifact shell (eight reducers, ~29 of 36 nonland discounted) → ≈ 0.94. +/// +/// Anti-calibration: two reducers over the same 36 nonland → ≈ 0.43, below +/// [`COST_REDUCTION_FLOOR`]; a deck with no CR 601.2f reducer → 0.0; a reducer +/// whose filter admits nothing the deck plays (a lone Semblance Anvil in a +/// creature-less shell) → coverage 0.0 → 0.0. +/// +/// Geometric mean over (reducer, coverage): BOTH pillars are mandatory. Reducers +/// that discount nothing this deck casts are blanks, and spells with no reducer +/// are just spells — neither alone is a cost-reduction plan. +fn compute_commitment(reducer_count: u32, discounted_count: u32, total_nonland: u32) -> f32 { + let reducer_density = (commitment::density_per_60(reducer_count, total_nonland) + / REDUCER_SATURATION_PER_60) + .min(1.0); + // Coverage is already a fraction of the deck, so it is its own density. + let coverage = if total_nonland == 0 { + 0.0 + } else { + (discounted_count as f32 / total_nonland as f32).min(1.0) + }; + commitment::geometric_mean(&[reducer_density, coverage]) +} diff --git a/crates/phase-ai/src/features/mana_ramp.rs b/crates/phase-ai/src/features/mana_ramp.rs index 16dbafd90a..7896ad0f56 100644 --- a/crates/phase-ai/src/features/mana_ramp.rs +++ b/crates/phase-ai/src/features/mana_ramp.rs @@ -19,8 +19,10 @@ //! - Controller scoping: `TypedFilter.controller: Option` at //! `ability.rs:815-818`. //! -//! `StaticMode::ModifyCost` is deliberately out of scope — cost reducers are a -//! follow-up feature. +//! `StaticMode::ModifyCost` is deliberately out of scope here — this axis +//! measures mana *added* to the pool. Cost reducers (cost *removed* from spells, +//! CR 601.2f) are the disjoint `features::cost_reduction` axis; a card is never +//! counted by both. use engine::game::DeckEntry; use engine::types::ability::{ diff --git a/crates/phase-ai/src/features/mod.rs b/crates/phase-ai/src/features/mod.rs index 98dc265fd2..41c0a1414f 100644 --- a/crates/phase-ai/src/features/mod.rs +++ b/crates/phase-ai/src/features/mod.rs @@ -12,6 +12,7 @@ pub mod artifacts; pub mod blink; pub mod commitment; pub mod control; +pub mod cost_reduction; pub mod devotion; pub mod draw_matters; pub mod enchantments; @@ -37,6 +38,7 @@ pub use aristocrats::AristocratsFeature; pub use artifacts::ArtifactsFeature; pub use blink::BlinkFeature; pub use control::ControlFeature; +pub use cost_reduction::CostReductionFeature; pub use devotion::DevotionFeature; pub use draw_matters::DrawMattersFeature; pub use enchantments::EnchantmentsFeature; @@ -74,6 +76,9 @@ pub struct DeckFeatures { pub mana_ramp: ManaRampFeature, pub tribal: TribalFeature, pub control: ControlFeature, + /// CR 601.2f: cost-reduction density ("spells you cast cost less") — the + /// acceleration axis `mana_ramp` explicitly defers. + pub cost_reduction: CostReductionFeature, pub enchantments: EnchantmentsFeature, pub equipment: EquipmentFeature, pub blink: BlinkFeature, @@ -125,6 +130,7 @@ impl DeckFeatures { mana_ramp: mana_ramp::detect(deck), tribal: tribal::detect(deck), control: control::detect(deck), + cost_reduction: cost_reduction::detect(deck), enchantments: enchantments::detect(deck), equipment: equipment::detect(deck), blink: blink::detect(deck), diff --git a/crates/phase-ai/src/features/tests/cost_reduction.rs b/crates/phase-ai/src/features/tests/cost_reduction.rs new file mode 100644 index 0000000000..6d9f933713 --- /dev/null +++ b/crates/phase-ai/src/features/tests/cost_reduction.rs @@ -0,0 +1,366 @@ +//! Unit tests for `features::cost_reduction` — CR 601.2f "spells you cast cost +//! less" detection. No `#[cfg(test)]` in SOURCE files; tests live here. + +use engine::game::DeckEntry; +use engine::types::ability::{ + ControllerRef, FilterProp, StaticDefinition, TargetFilter, TypeFilter, TypedFilter, +}; +use engine::types::card::CardFace; +use engine::types::card_type::{CardType, CoreType}; +use engine::types::mana::ManaCost; +use engine::types::statics::{CostModifyMode, StaticMode}; + +use crate::features::cost_reduction::*; + +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 } +} + +fn generic(amount: u32) -> ManaCost { + ManaCost::Cost { + shards: Vec::new(), + generic: amount, + } +} + +/// A CR 601.2f board-wide reducer: "spells you cast cost {amount} less", +/// narrowed by `spell_filter`. +fn reducer( + name: &str, + amount: u32, + mode: CostModifyMode, + spell_filter: Option, +) -> CardFace { + let mut f = face(name, CoreType::Creature); + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode, + amount: generic(amount), + spell_filter, + dynamic_count: None, + }); + def.affected = Some(TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::You), + ..Default::default() + })); + f.static_abilities = vec![def]; + f +} + +/// The Goblin Electromancer shape: instant/sorcery spells you cast cost {1} less. +fn spell_reducer(name: &str) -> CardFace { + reducer( + name, + 1, + CostModifyMode::Reduce, + Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::AnyOf(vec![ + TypeFilter::Instant, + TypeFilter::Sorcery, + ])], + controller: Some(ControllerRef::You), + ..Default::default() + })), + ) +} + +/// Filler the reducers above actually discount. +fn discounted_spell(name: &str) -> CardFace { + face(name, CoreType::Instant) +} + +/// Filler no instant/sorcery-scoped reducer discounts. +fn undiscounted_spell(name: &str) -> CardFace { + face(name, CoreType::Creature) +} + +/// A deck of `reducers` copies of `reducer_face` plus `discounted` discounted +/// spells and `other` undiscounted ones, padded to `nonland` nonland cards. +fn deck(reducer_face: CardFace, reducers: u32, discounted: u32, other: u32) -> Vec { + vec![ + entry(reducer_face, reducers), + entry(discounted_spell("Discounted"), discounted), + entry(undiscounted_spell("Other"), other), + ] +} + +#[test] +fn empty_deck_produces_defaults() { + let f = detect(&[]); + assert_eq!(f.reducer_count, 0); + assert_eq!(f.total_discount, 0); + assert_eq!(f.discounted_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn vanilla_creature_not_registered() { + let f = detect(&[entry(undiscounted_spell("Bear"), 4)]); + assert_eq!(f.reducer_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn detects_board_wide_reducer() { + // 4 reducers, 20 discounted instants, 12 other → 36 nonland. + let f = detect(&deck(spell_reducer("Electromancer"), 4, 20, 12)); + assert_eq!(f.reducer_count, 4); + assert_eq!(f.total_discount, 4); + // The reducer itself is a creature, so only the 20 instants are discounted. + assert_eq!(f.discounted_count, 20); +} + +#[test] +fn unfiltered_reducer_discounts_every_nonland_card() { + // `spell_filter: None` — "spells you cast cost {1} less" (Aang / Medallion + // shape) admits every nonland card in the deck, itself included. + let f = detect(&deck( + reducer("Unfiltered", 1, CostModifyMode::Reduce, None), + 4, + 20, + 12, + )); + assert_eq!(f.discounted_count, 36); +} + +#[test] +fn discount_magnitude_is_the_generic_component() { + // CR 118.7a: only the generic component of a cost is reduced. + let f = detect(&deck( + reducer("Big", 2, CostModifyMode::Reduce, None), + 3, + 20, + 13, + )); + assert_eq!(f.reducer_count, 3); + assert_eq!(f.total_discount, 6); +} + +#[test] +fn raise_mode_does_not_count() { + // Thalia, Guardian of Thraben taxes — it is not a discount engine. + let f = detect(&deck( + reducer("Thalia", 1, CostModifyMode::Raise, None), + 4, + 20, + 12, + )); + assert_eq!(f.reducer_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn minimum_mode_does_not_count() { + // Trinisphere floors a cost (CR 601.2f last step); it discounts nothing. + let f = detect(&deck( + reducer("Trinisphere", 3, CostModifyMode::Minimum, None), + 4, + 20, + 12, + )); + assert_eq!(f.reducer_count, 0); +} + +#[test] +fn self_cost_reduction_does_not_count() { + // CR 113.6: "this spell costs {1} less" is a property of one card, resolved + // by `apply_self_spell_cost_modifiers` — never a board-wide engine. + let mut f_card = face("Affinity Thing", CoreType::Artifact); + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(1), + spell_filter: None, + dynamic_count: None, + }); + def.affected = Some(TargetFilter::SelfRef); + f_card.static_abilities = vec![def]; + + let f = detect(&deck(f_card, 4, 20, 12)); + assert_eq!(f.reducer_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn opponent_scoped_reducer_does_not_count() { + // A discount handed to the other side is not your engine. + let mut f_card = face("Opponent Helper", CoreType::Enchantment); + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(1), + spell_filter: None, + dynamic_count: None, + }); + def.affected = Some(TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::Opponent), + ..Default::default() + })); + f_card.static_abilities = vec![def]; + + let f = detect(&deck(f_card, 4, 20, 12)); + assert_eq!(f.reducer_count, 0); +} + +#[test] +fn zero_generic_reduction_does_not_count() { + // A purely colored `amount` moves no generic cost (CR 118.7a). + let f = detect(&deck( + reducer("Colorless Only", 0, CostModifyMode::Reduce, None), + 4, + 20, + 12, + )); + assert_eq!(f.reducer_count, 0); +} + +#[test] +fn unverifiable_filter_property_yields_no_coverage() { + // A `properties` predicate needs live game state, so coverage fails OFF + // rather than claiming a discount the deck may not get. + let f = detect(&deck( + reducer( + "Stateful", + 1, + CostModifyMode::Reduce, + Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Instant], + properties: vec![FilterProp::Tapped], + ..Default::default() + })), + ), + 4, + 20, + 12, + )); + assert_eq!(f.reducer_count, 4); + assert_eq!(f.discounted_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn reducer_matching_nothing_collapses_commitment() { + // The lone-Semblance-Anvil case: a real reducer whose filter admits nothing + // this deck plays. Geometric mean collapses on the zero coverage pillar. + let f = detect(&[ + entry(spell_reducer("Electromancer"), 4), + entry(undiscounted_spell("Creature"), 32), + ]); + assert_eq!(f.reducer_count, 4); + assert_eq!(f.discounted_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn spells_without_a_reducer_collapse_commitment() { + let f = detect(&[entry(discounted_spell("Bolt"), 36)]); + assert_eq!(f.reducer_count, 0); + assert_eq!(f.discounted_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn izzet_spells_shell_hits_calibration_anchor() { + // Docstring anchor: 4 two-mana reducers, 20 of 36 nonland discounted → ≈0.61. + let f = detect(&deck(spell_reducer("Electromancer"), 4, 20, 12)); + assert!( + f.commitment > 0.58 && f.commitment < 0.64, + "expected ≈0.61, got {}", + f.commitment + ); + assert!(f.commitment >= COST_REDUCTION_FLOOR); +} + +#[test] +fn two_reducers_stay_below_floor() { + // Anti-calibration: two reducers over the same 36 nonland → ≈0.43. + let f = detect(&deck(spell_reducer("Electromancer"), 2, 20, 14)); + assert!( + f.commitment < COST_REDUCTION_FLOOR, + "expected below floor, got {}", + f.commitment + ); +} + +#[test] +fn commitment_is_format_size_neutral() { + // Same density over a 99-card Commander shell reads the same as over 60. + let sixty = detect(&deck(spell_reducer("Electromancer"), 4, 20, 12)); + let commander = detect(&deck(spell_reducer("Electromancer"), 7, 35, 21)); + assert!( + (sixty.commitment - commander.commitment).abs() < 0.03, + "{} vs {}", + sixty.commitment, + commander.commitment + ); +} + +#[test] +fn commitment_clamps_to_one() { + // Every nonland card is both a reducer and discounted by it. + let f = detect(&[entry( + reducer("Everything", 3, CostModifyMode::Reduce, None), + 40, + )]); + assert!(f.commitment <= 1.0); + assert!( + f.commitment > 0.99, + "expected saturation, got {}", + f.commitment + ); +} + +#[test] +fn lands_are_excluded_from_coverage() { + // CR 305.1: playing a land is not casting a spell, so lands never count as + // discounted cards nor toward the nonland denominator. + let with_lands = detect(&[ + entry(spell_reducer("Electromancer"), 4), + entry(discounted_spell("Discounted"), 20), + entry(undiscounted_spell("Other"), 12), + entry(face("Island", CoreType::Land), 24), + ]); + let without_lands = detect(&deck(spell_reducer("Electromancer"), 4, 20, 12)); + assert_eq!(with_lands.commitment, without_lands.commitment); + assert_eq!(with_lands.discounted_count, 20); +} + +#[test] +fn parts_predicate_sums_multiple_reducing_statics() { + // One face carrying two reducers reports their combined discount. + let mut f_card = face("Double", CoreType::Artifact); + let mut first = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(1), + spell_filter: None, + dynamic_count: None, + }); + first.affected = Some(TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::You), + ..Default::default() + })); + let second = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(2), + spell_filter: None, + dynamic_count: None, + }); + f_card.static_abilities = vec![first, second]; + + assert_eq!(your_spell_discount_parts(&f_card.static_abilities), 3); +} + +#[test] +fn parts_predicate_reports_zero_for_non_reducer() { + let f_card = undiscounted_spell("Bear"); + assert_eq!(your_spell_discount_parts(&f_card.static_abilities), 0); +} diff --git a/crates/phase-ai/src/features/tests/mod.rs b/crates/phase-ai/src/features/tests/mod.rs index da91977ba0..00b8926771 100644 --- a/crates/phase-ai/src/features/tests/mod.rs +++ b/crates/phase-ai/src/features/tests/mod.rs @@ -3,6 +3,7 @@ pub mod artifacts; pub mod blink; +pub mod cost_reduction; pub mod devotion; pub mod draw_matters; pub mod enchantments; diff --git a/crates/phase-ai/src/policies/cost_reduction.rs b/crates/phase-ai/src/policies/cost_reduction.rs new file mode 100644 index 0000000000..bcd9140853 --- /dev/null +++ b/crates/phase-ai/src/policies/cost_reduction.rs @@ -0,0 +1,154 @@ +//! `CostReductionPolicy` — makes a cost-reduction permanent a reason the AI can +//! see to deploy the discount BEFORE the spells it discounts. +//! +//! ## The gap this closes +//! +//! CR 601.2f: Goblin Electromancer, Baral, Foundry Inspector and the Medallion +//! cycle are acceleration that never taps for mana — every later spell costs +//! less for as long as the permanent survives. The engine already applies the +//! discount at cast time (`casting::collect_cost_modifiers`), so the AI is never +//! overcharged; what it lacks is any reason to *sequence the reducer first*. +//! `RampTimingPolicy` supplies exactly that signal for permanents that add mana +//! (`Effect::Mana`, land fetch, extra land drops) and structurally cannot see a +//! cost reducer, so a deck whose entire acceleration plan is cost reduction gets +//! no sequencing guidance at all. This policy adds it. +//! +//! ## Performance +//! +//! `verdict()` runs per candidate per search node. The card-local check — do +//! this candidate's OWN statics carry a board-wide reduction of your spells — +//! runs FIRST and rejects every non-reducer candidate after reading one card's +//! AST. Only a confirmed reducer pays for the hand walk, which is bounded by +//! hand size and touches no battlefield sweep, no `find_legal_targets`, and no +//! affordability query. + +use engine::types::card_type::CoreType; +use engine::types::game_state::GameState; +use engine::types::identifiers::ObjectId; +use engine::types::player::PlayerId; + +use crate::features::cost_reduction::{your_spell_discount_parts, COST_REDUCTION_FLOOR}; +use crate::features::DeckFeatures; + +use super::context::PolicyContext; +use super::registry::{DecisionKind, PolicyId, PolicyReason, PolicyVerdict, TacticalPolicy}; + +pub struct CostReductionPolicy; + +/// Cap on how many future casts one deployment is credited for, so a full grip +/// cannot push a single reducer out of the intended band. +/// +/// `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_FUTURE_CASTS: u32 = 4; + +/// Cap on the per-application generic discount credited, so a misparsed or +/// unusually large `amount` cannot dominate the candidate's prior. +pub(crate) const MAX_REWARDED_DISCOUNT: u32 = 3; + +impl TacticalPolicy for CostReductionPolicy { + fn id(&self) -> PolicyId { + PolicyId::CostReduction + } + + fn decision_kinds(&self) -> &'static [DecisionKind] { + &[DecisionKind::CastSpell] + } + + fn activation( + &self, + features: &DeckFeatures, + _state: &GameState, + _player: PlayerId, + ) -> Option { + if features.cost_reduction.commitment < COST_REDUCTION_FLOOR { + None + } else { + Some(features.cost_reduction.commitment) + } + } + + fn verdict(&self, ctx: &PolicyContext<'_>) -> PolicyVerdict { + let Some(facts) = ctx.cast_facts() else { + // CR 601.2 cast-shaped siblings (madness, miracle, foretell, copies) + // do not populate `cast_facts`, so there is no AST to classify. + return PolicyVerdict::neutral(PolicyReason::new("cost_reduction_na")); + }; + + // Card-local first: does THIS candidate carry the discount engine? + let discount = your_spell_discount_parts(facts.object.static_definitions.iter_unchecked()); + if discount > 0 { + // A discount only pays off on spells still to be cast. With an empty + // grip the reducer is a vanilla permanent this turn. + let future_casts = castable_cards_in_hand(ctx, Some(facts.object.id)); + if future_casts == 0 { + return PolicyVerdict::neutral(PolicyReason::new("cost_reduction_no_future_casts")); + } + let rewarded_casts = future_casts.min(MAX_REWARDED_FUTURE_CASTS); + let rewarded_discount = discount.min(MAX_REWARDED_DISCOUNT); + // CR 601.2f: each future cast saves `discount` generic mana; the + // configured weight converts saved mana into card-equivalents. + let saved_mana = f64::from(rewarded_discount * rewarded_casts); + return PolicyVerdict::score( + ctx.config.policy_penalties.cost_reduction_deploy_bonus * saved_mana, + PolicyReason::new("cost_reduction_deploy_engine") + .with_fact("discount", i64::from(discount)) + .with_fact("future_casts", i64::from(future_casts)), + ); + } + + // Otherwise: are we casting past an unplayed reducer that is cheaper than + // this spell? Deploying the discount first is strictly better sequencing + // — the same shape as `RampTimingPolicy`'s `defer_to_ramp`. The mana-value + // gate keeps this to cases where the reducer plausibly fits first. + if hand_holds_cheaper_reducer(ctx, facts.mana_value, facts.object.id) { + return PolicyVerdict::score( + ctx.config.policy_penalties.cost_reduction_defer_penalty, + PolicyReason::new("cost_reduction_defer_to_engine") + .with_fact("mana_value", i64::from(facts.mana_value)), + ); + } + + PolicyVerdict::neutral(PolicyReason::new("cost_reduction_na")) + } +} + +/// Nonland cards in the AI's hand that a discount could still apply to, +/// excluding `exclude` (the candidate itself, which is being spent now). +/// +/// Lands are excluded because CR 305.1 land plays are not spells and are never +/// discounted by a CR 601.2f cost reducer. +fn castable_cards_in_hand(ctx: &PolicyContext<'_>, exclude: Option) -> u32 { + let Some(player) = ctx.state.players.get(ctx.ai_player.0 as usize) else { + return 0; + }; + player + .hand + .iter() + .filter(|id| Some(**id) != exclude) + .filter(|id| { + ctx.state + .objects + .get(id) + .is_some_and(|obj| !obj.card_types.core_types.contains(&CoreType::Land)) + }) + .count() + .try_into() + .unwrap_or(u32::MAX) +} + +/// True when the AI's hand still holds a cost-reduction permanent whose mana +/// value is strictly below `mana_value` — i.e. the engine could have been +/// deployed instead of, and later alongside, this spell. +fn hand_holds_cheaper_reducer(ctx: &PolicyContext<'_>, mana_value: u32, exclude: ObjectId) -> bool { + let Some(player) = ctx.state.players.get(ctx.ai_player.0 as usize) else { + return false; + }; + player.hand.iter().any(|id| { + *id != exclude + && ctx.state.objects.get(id).is_some_and(|obj| { + obj.effective_mana_value() < mana_value + && your_spell_discount_parts(obj.static_definitions.iter_unchecked()) > 0 + }) + }) +} diff --git a/crates/phase-ai/src/policies/mod.rs b/crates/phase-ai/src/policies/mod.rs index 0061f7816e..c6630c1327 100644 --- a/crates/phase-ai/src/policies/mod.rs +++ b/crates/phase-ai/src/policies/mod.rs @@ -14,6 +14,7 @@ mod condition_gated_activation; pub(crate) mod context; mod control_change_awareness; pub(crate) mod copy_value; +mod cost_reduction; mod crew_timing; mod cycling_discipline; mod devotion; diff --git a/crates/phase-ai/src/policies/registry.rs b/crates/phase-ai/src/policies/registry.rs index 46a33a3cee..7ca7e00f59 100644 --- a/crates/phase-ai/src/policies/registry.rs +++ b/crates/phase-ai/src/policies/registry.rs @@ -145,6 +145,7 @@ pub enum PolicyId { /// 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. + CostReduction, DrawPayoff, } @@ -401,6 +402,7 @@ impl Default for PolicyRegistry { Box::new(PayoffPolicy::new(&BLINK_PAYOFF)), Box::new(LoopShortcutPolicy), Box::new(super::self_bounce_target::SelfBounceTargetPolicy), + Box::new(super::cost_reduction::CostReductionPolicy), Box::new(super::draw_payoff::DrawPayoffPolicy), ]; let mut by_kind: HashMap> = HashMap::new(); diff --git a/crates/phase-ai/src/policies/tests/cost_reduction.rs b/crates/phase-ai/src/policies/tests/cost_reduction.rs new file mode 100644 index 0000000000..482247b3de --- /dev/null +++ b/crates/phase-ai/src/policies/tests/cost_reduction.rs @@ -0,0 +1,438 @@ +//! Unit tests for `policies::cost_reduction` — CR 601.2f cost-reduction +//! deployment 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::{ControllerRef, StaticDefinition, TargetFilter, TypedFilter}; +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::mana::ManaCost; +use engine::types::player::PlayerId; +use engine::types::statics::{CostModifyMode, StaticMode}; +use engine::types::zones::Zone; + +use crate::config::AiConfig; +use crate::context::AiContext; +use crate::features::cost_reduction::{CostReductionFeature, COST_REDUCTION_FLOOR}; +use crate::features::DeckFeatures; +use crate::policies::context::{PolicyContext, SearchDepth}; +use crate::policies::cost_reduction::*; +use crate::policies::registry::{ + PolicyId, PolicyReason, PolicyRegistry, PolicyVerdict, TacticalPolicy, +}; +use crate::session::AiSession; + +const AI: PlayerId = PlayerId(0); + +fn state() -> GameState { + GameState::new(FormatConfig::standard(), 2, 42) +} + +fn generic(amount: u32) -> ManaCost { + ManaCost::Cost { + shards: Vec::new(), + generic: amount, + } +} + +/// A CR 601.2f board-wide reducer static scoped to spells YOU cast. +fn reduce_your_spells(amount: u32, mode: CostModifyMode) -> StaticDefinition { + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode, + amount: generic(amount), + spell_filter: None, + dynamic_count: None, + }); + def.affected = Some(TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::You), + ..Default::default() + })); + def +} + +/// A card in the AI's hand. `statics` are attached as live `static_definitions`, +/// which is what the policy classifies at decision time. +fn hand_card( + state: &mut GameState, + name: &str, + core: CoreType, + mana_value: u32, + statics: Vec, +) -> (ObjectId, CardId) { + let card_id = CardId(state.next_object_id); + let id = create_object(state, card_id, AI, name.to_string(), Zone::Hand); + let obj = state.objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(core); + obj.mana_cost = generic(mana_value); + for def in statics { + obj.static_definitions.push(def); + } + state.players[AI.0 as usize].hand.push_back(id); + (id, card_id) +} + +/// The Goblin Electromancer shape: a two-mana creature that discounts your spells. +fn reducer_in_hand(state: &mut GameState, mana_value: u32) -> (ObjectId, CardId) { + hand_card( + state, + "Cost Reducer", + CoreType::Creature, + mana_value, + vec![reduce_your_spells(1, CostModifyMode::Reduce)], + ) +} + +fn plain_spell_in_hand(state: &mut GameState, mana_value: u32) -> (ObjectId, CardId) { + hand_card( + state, + "Plain Spell", + CoreType::Sorcery, + mana_value, + Vec::new(), + ) +} + +fn land_in_hand(state: &mut GameState) -> (ObjectId, CardId) { + hand_card(state, "Island", CoreType::Land, 0, Vec::new()) +} + +fn feature(commitment: f32) -> CostReductionFeature { + CostReductionFeature { + reducer_count: 4, + total_discount: 4, + discounted_count: 20, + commitment, + } +} + +fn session(commitment: f32) -> AiSession { + let features = DeckFeatures { + cost_reduction: feature(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 features = DeckFeatures { + cost_reduction: feature(COST_REDUCTION_FLOOR - 0.01), + ..Default::default() + }; + assert!(CostReductionPolicy + .activation(&features, &state(), AI) + .is_none()); +} + +#[test] +fn activation_opts_in_above_floor() { + let features = DeckFeatures { + cost_reduction: feature(0.8), + ..Default::default() + }; + let activation = CostReductionPolicy.activation(&features, &state(), AI); + assert_eq!(activation, Some(0.8)); +} + +// ─── verdict: deploy the engine ────────────────────────────────────────────── + +#[test] +fn deploying_reducer_with_spells_in_hand_scores_positive() { + let mut st = state(); + let (reducer, card) = reducer_in_hand(&mut st, 2); + plain_spell_in_hand(&mut st, 3); + plain_spell_in_hand(&mut st, 4); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_deploy_engine"); + assert!( + delta > 0.0, + "expected a positive deployment credit, got {delta}" + ); +} + +#[test] +fn deploying_reducer_with_empty_grip_is_neutral() { + let mut st = state(); + let (reducer, card) = reducer_in_hand(&mut st, 2); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_no_future_casts"); + assert_eq!(delta, 0.0); +} + +#[test] +fn lands_in_hand_are_not_future_casts() { + // CR 305.1: playing a land is not casting a spell, so a grip of lands gives + // the reducer nothing to discount. + let mut st = state(); + let (reducer, card) = reducer_in_hand(&mut st, 2); + land_in_hand(&mut st); + land_in_hand(&mut st); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (_, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_no_future_casts"); +} + +#[test] +fn deployment_credit_is_bounded_by_the_caps() { + // A huge grip and a huge discount must stay within the constants' product, + // so a stacked hand cannot push one deployment into the critical band. + let mut st = state(); + let (reducer, card) = hand_card( + &mut st, + "Big Reducer", + CoreType::Creature, + 2, + vec![reduce_your_spells(9, CostModifyMode::Reduce)], + ); + for _ in 0..12 { + plain_spell_in_hand(&mut st, 3); + } + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, _) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + let ceiling = config.policy_penalties.cost_reduction_deploy_bonus + * f64::from(MAX_REWARDED_DISCOUNT * MAX_REWARDED_FUTURE_CASTS); + assert!( + delta <= ceiling + f64::EPSILON, + "delta {delta} exceeded ceiling {ceiling}" + ); +} + +#[test] +fn taxing_permanent_is_not_a_deployment_engine() { + // Thalia raises costs (CR 601.2f); deploying her is not a discount plan. + let mut st = state(); + let (thalia, card) = hand_card( + &mut st, + "Thalia", + CoreType::Creature, + 2, + vec![reduce_your_spells(1, CostModifyMode::Raise)], + ); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(thalia, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +#[test] +fn self_cost_reduction_is_not_a_deployment_engine() { + // CR 113.6: "this spell costs {1} less" discounts only itself. + let mut st = state(); + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(1), + spell_filter: None, + dynamic_count: None, + }); + def.affected = Some(TargetFilter::SelfRef); + let (obj, card) = hand_card(&mut st, "Self Discount", CoreType::Artifact, 4, vec![def]); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(obj, card); + let decision = priority_decision(&candidate); + let (_, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); +} + +// ─── verdict: defer to the engine ──────────────────────────────────────────── + +#[test] +fn casting_past_a_cheaper_reducer_is_penalized() { + let mut st = state(); + reducer_in_hand(&mut st, 2); + let (spell, card) = plain_spell_in_hand(&mut st, 4); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(spell, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_defer_to_engine"); + assert!(delta < 0.0, "expected a sequencing penalty, got {delta}"); +} + +#[test] +fn casting_past_a_more_expensive_reducer_is_neutral() { + // The reducer does not fit ahead of this spell, so there is nothing to defer. + let mut st = state(); + reducer_in_hand(&mut st, 5); + let (spell, card) = plain_spell_in_hand(&mut st, 2); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(spell, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +#[test] +fn casting_with_no_reducer_in_hand_is_neutral() { + let mut st = state(); + let (spell, card) = plain_spell_in_hand(&mut st, 4); + plain_spell_in_hand(&mut st, 2); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(spell, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +// ─── production seam ───────────────────────────────────────────────────────── + +#[test] +fn registry_routes_cast_spell_to_this_policy() { + // Guards the wiring, not just the classifier: registration in + // `PolicyRegistry::default` plus `CastSpell` routing must reach `verdict`. + let mut st = state(); + let (reducer, card) = reducer_in_hand(&mut st, 2); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let verdicts = + PolicyRegistry::default().verdicts(&ctx(&st, &candidate, &decision, &context, &config)); + + let found = verdicts + .iter() + .find(|(id, _)| *id == PolicyId::CostReduction) + .map(|(_, verdict)| verdict.clone()) + .expect("CostReductionPolicy must be registered and routed for CastSpell"); + let (delta, reason) = score_of(found); + assert_eq!(reason.kind, "cost_reduction_deploy_engine"); + assert!(delta > 0.0); +} + +#[test] +fn registry_stays_silent_below_the_activation_floor() { + let mut st = state(); + let (reducer, card) = reducer_in_hand(&mut st, 2); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(COST_REDUCTION_FLOOR - 0.01)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let verdicts = + PolicyRegistry::default().verdicts(&ctx(&st, &candidate, &decision, &context, &config)); + + assert!( + !verdicts + .iter() + .any(|(id, _)| *id == PolicyId::CostReduction), + "policy must not contribute below its activation floor" + ); +} diff --git a/crates/phase-ai/src/policies/tests/mod.rs b/crates/phase-ai/src/policies/tests/mod.rs index e455010e7a..448290563e 100644 --- a/crates/phase-ai/src/policies/tests/mod.rs +++ b/crates/phase-ai/src/policies/tests/mod.rs @@ -3,6 +3,7 @@ pub mod activation_marker_lint; pub mod artifact_synergy; pub mod blink_payoff; +pub mod cost_reduction; pub mod devotion; pub mod draw_payoff; pub mod effect_classify_snapshot; From ef0fa8c9b70fbd1419ce115fd224cf0b8b0c8b03 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 28 Jul 2026 13:32:22 -0700 Subject: [PATCH 2/6] docs(phase-ai): correct the cited cost-modifier authority and record two elided assumptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the cost-reduction axis before final submission: - The doc comments named `casting::collect_cost_modifiers`; the actual engine authority is `casting::collect_battlefield_cost_modifiers` (crates/engine/src/game/casting.rs:7179). A citation that does not resolve is worse than none — it claims a verification that never happened. - `your_spell_discount` silently ignored `dynamic_count`. Record that this is deliberate: a scaling reducer's multiplier is unknowable at deck-analysis time, so the per-unit `generic` is reported, understating rather than inventing a value. - `castable_cards_in_hand` counts remaining casts without narrowing to the reducer's `spell_filter`. Record why that is correct rather than sloppy: the narrowing is applied once per game through `commitment`, which `activation` multiplies in, instead of per candidate in the search inner loop. Documentation only — no behavior change; the 33 tests are untouched and green. Co-Authored-By: Claude Opus 5 (1M context) --- crates/phase-ai/src/features/cost_reduction.rs | 14 +++++++++++--- crates/phase-ai/src/policies/cost_reduction.rs | 11 ++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/crates/phase-ai/src/features/cost_reduction.rs b/crates/phase-ai/src/features/cost_reduction.rs index 9eedaa3719..c0789f7555 100644 --- a/crates/phase-ai/src/features/cost_reduction.rs +++ b/crates/phase-ai/src/features/cost_reduction.rs @@ -9,7 +9,7 @@ //! Trinisphere tax shapes and are NOT this axis. //! - `StaticDefinition.affected: Option` at //! `crates/engine/src/types/ability.rs:20740` — carries the caster scope -//! (`TypedFilter.controller`), exactly as `collect_cost_modifiers` reads it. +//! (`TypedFilter.controller`), exactly as `collect_battlefield_cost_modifiers` reads it. //! - `CardFace.static_abilities: Vec` at `card.rs:162`; //! the runtime counterpart is `GameObject.static_definitions`. //! - `ManaCost::Cost { generic, shards }` at @@ -166,7 +166,7 @@ pub(crate) fn your_spell_discount_parts<'a>( /// CR 601.2f: the generic discount this one static gives to spells you cast, or /// `None` when it is not a board-wide reduction of your own spells. /// -/// Mirrors the eligibility tests `casting::collect_cost_modifiers` applies at +/// Mirrors the eligibility tests `casting::collect_battlefield_cost_modifiers` applies at /// cast time, so deck classification and the resolver agree by construction: /// `Reduce` mode only, never a `SelfRef` self-cost reduction, and never an /// opponent-scoped modifier. @@ -201,6 +201,14 @@ fn your_spell_discount(def: &StaticDefinition) -> Option { // generic reduction, so the discount magnitude is `generic`. A reduction of // zero generic mana (a purely colored `amount`) moves no cost here and is // not counted as an engine. + // + // `dynamic_count` is deliberately NOT resolved here. A scaling reducer + // ("costs {1} less for each artifact you control") multiplies `generic` by a + // game-state quantity that is unknowable at deck-analysis time, so this + // reports the per-unit amount — one application's worth. That understates a + // scaling reducer's ceiling rather than inventing a multiplier, and on the + // live path `CostReductionPolicy` additionally caps the credited discount, + // so a large `amount` whose multiplier is currently zero cannot dominate. let ManaCost::Cost { generic, .. } = amount else { return None; }; @@ -228,7 +236,7 @@ fn your_spell_discount_filter(def: &StaticDefinition) -> Option, exclude: Option) -> u32 { let Some(player) = ctx.state.players.get(ctx.ai_player.0 as usize) else { return 0; From f8d3e4dfd2e7a2eade6a7da5d1ad1fe1c70c9f38 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 28 Jul 2026 17:14:47 -0700 Subject: [PATCH 3/6] fix(phase-ai): make the casting authority own cost-modifier eligibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #6743. Every MED shared one root cause — phase-ai MIRRORED `collect_battlefield_cost_modifiers`'s eligibility instead of sharing it — so this fixes the ownership rather than patching each symptom. Engine (new single authority): - `StaticDefinition::board_wide_cost_modifier()` + `CostModifierCasterScope` (types/ability.rs) is now the one structural definition of "is this a board-wide cost modifier and what are its terms": rejects `Minimum` and `SelfRef` (CR 113.6), and names the caster scope so a caller WITH a PlayerId (the casting pipeline) and one WITHOUT (deck analysis) ask the same question. `collect_battlefield_cost_modifiers` now consumes it, so the two paths cannot drift again. - `matches_target_filter_against_face_scoped` + `context_free_prop_matches_face` (game/filter.rs) extend the CR 205 face authority to every context-free `FilterProp` — mana value (CR 202.3), color (CR 105.2), keyword, supertype, token-ness — and return `Option` so a live-only property is *unanswerable* and fails closed by construction rather than by a caller remembering to. `FaceControllerScope` names the previously-implicit controller assumption. phase-ai (consumes, no longer mirrors): - `your_spell_discount` reads the engine authority; the hand-rolled type-axis matcher is deleted. Color-, mana-value- and keyword-scoped reducers now produce coverage instead of silently reading as discounting nothing. - New `live_your_spell_discounts` resolves what a structural read cannot: `condition` fails off (a card in hand has no truthful "as long as" answer), and `dynamic_count` is resolved through the engine's `resolve_quantity`, so a multiplier of zero earns no credit and a multiplier above one scales it. - The policy counts and penalizes only spells a reducer actually discounts, matched through the engine's live `matches_target_filter` with the reducer as filter source. - `PolicyId::DrawPayoff` gets its CR 121.1 doc comment back; `CostReduction` gets its own verified CR 601.2f one. Tests 33 -> 44. Six new regressions, each proven RED by breaking the specific invariant it guards: color/mana-value/negated-color coverage, live-only property failing closed, controller-scoped filter admitted, filter-narrowed deploy credit and defer penalty, conditional reducer earning nothing, and zero vs positive dynamic multipliers. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/casting.rs | 62 ++--- crates/engine/src/game/filter.rs | 141 +++++++++++- crates/engine/src/types/ability.rs | 106 +++++++++ .../phase-ai/src/features/cost_reduction.rs | 169 ++++++++------ .../src/features/tests/cost_reduction.rs | 138 ++++++++++- .../phase-ai/src/policies/cost_reduction.rs | 120 +++++++--- crates/phase-ai/src/policies/registry.rs | 4 +- .../src/policies/tests/cost_reduction.rs | 215 +++++++++++++++++- 8 files changed, 804 insertions(+), 151 deletions(-) diff --git a/crates/engine/src/game/casting.rs b/crates/engine/src/game/casting.rs index 7b71e6f24a..7d37825edb 100644 --- a/crates/engine/src/game/casting.rs +++ b/crates/engine/src/game/casting.rs @@ -1,8 +1,8 @@ use crate::types::ability::{ is_variable_remove_counter_cost_count, AbilityBlockKind, AbilityBlockReason, AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, AbilityTag, ActivationManaPaymentRestriction, - AdditionalCost, CardPlayMode, CardSelectionMode, CastTimingPermission, CastingPermission, - ChoiceType, ContinuousModification, CostObjectCount, CostPaidObjectSnapshot, + AdditionalCost, BoardWideCostModifier, CardPlayMode, CardSelectionMode, CastTimingPermission, + CastingPermission, ChoiceType, ContinuousModification, CostObjectCount, CostPaidObjectSnapshot, CounterCostSelection, Duration, Effect, EffectKind, FilterProp, GameRestriction, ModalSelectionCondition, ObjectScope, PlayerFilter, PlayerScope, ProhibitedActivity, QuantityExpr, QuantityRef, ResolvedAbility, RestrictionExpiry, RestrictionPlayerScope, @@ -7184,8 +7184,6 @@ fn collect_battlefield_cost_modifiers( target_sensitive_only: bool, casting_variant: Option, ) -> Vec { - use crate::types::ability::ControllerRef; - // CR 202.3d + CR 702.102b: a pre-payment `CastingVariant::Fuse` cast presents // the COMBINED characteristics of both halves to a `ModifyCost` static's // `spell_filter`. The `fused_split_spell` marker is not yet set at this seam. @@ -7219,25 +7217,27 @@ fn collect_battlefield_cost_modifiers( let source_controller = src_obj.controller; { - let (amount, spell_filter, dynamic_count, is_raise) = match &def.mode { - StaticMode::ModifyCost { - mode: CostModifyMode::Reduce, - amount, - spell_filter, - dynamic_count, - } => (amount, spell_filter, dynamic_count, false), - StaticMode::ModifyCost { - mode: CostModifyMode::Raise, - amount, - spell_filter, - dynamic_count, - } => (amount, spell_filter, dynamic_count, true), - _ => continue, + // CR 601.2f + CR 113.6: single structural authority for "is this a + // board-wide cost modifier, and what are its terms" — shared with + // deck-time analysis (`phase-ai`'s `features::cost_reduction`) so the + // two cannot drift. It rejects `Minimum` and `SelfRef` (the latter is + // self-cost-reduction, handled by `apply_self_spell_cost_modifiers` + // for the spell being cast and never applied from a battlefield + // permanent to other spells). + let Some(modifier) = def.board_wide_cost_modifier() else { + continue; }; + let BoardWideCostModifier { + mode, + amount, + spell_filter, + dynamic_count, + caster_scope, + condition: _, + } = modifier; + let is_raise = matches!(mode, CostModifyMode::Raise); - let has_target_filter = spell_filter - .as_ref() - .is_some_and(cost_filter_has_target_ref); + let has_target_filter = spell_filter.is_some_and(cost_filter_has_target_ref); if target_sensitive_only && !has_target_filter { continue; } @@ -7245,14 +7245,6 @@ fn collect_battlefield_cost_modifiers( continue; } - // CR 113.6: SelfRef statics are self-cost-reduction ("this spell costs - // {N} less") — handled by apply_self_spell_cost_modifiers for the spell - // being cast. They must never apply from a battlefield permanent to - // other spells. - if matches!(def.affected, Some(TargetFilter::SelfRef)) { - continue; - } - // CR 113.6 + CR 113.6b: A static functions only in its declared // zones. Empty `active_zones` means battlefield default; non-empty // means restrict to the listed zones. Eminence statics list both @@ -7267,12 +7259,8 @@ fn collect_battlefield_cost_modifiers( // CR 601.2f: Check player scope — does this modifier apply to spells the caster casts? // Must run before condition check so QuantityComparison resolves against the caster. - if let Some(TargetFilter::Typed(ref tf)) = def.affected { - match tf.controller { - Some(ControllerRef::You) if caster != source_controller => continue, - Some(ControllerRef::Opponent) if caster == source_controller => continue, - _ => {} // No controller restriction or matches - } + if !caster_scope.admits(caster, source_controller) { + continue; } // CR 601.2f: Check static condition — "as long as" / "during your turn" @@ -7291,7 +7279,7 @@ fn collect_battlefield_cost_modifiers( } // CR 601.2f: Check spell type filter — does the spell match? - if let Some(ref filter) = spell_filter { + if let Some(filter) = spell_filter { let matches = if let Some(ability) = selected_ability { spell_matches_cost_filter_with_selected_targets_for( state, caster, spell_id, filter, bf_id, ability, fused, @@ -7306,7 +7294,7 @@ fn collect_battlefield_cost_modifiers( // CR 601.2f: Calculate the modification amount. let base_amount = amount.clone(); - let multiplier = if let Some(ref qty_ref) = dynamic_count { + let multiplier = if let Some(qty_ref) = dynamic_count { let qty_expr = crate::types::ability::QuantityExpr::Ref { qty: qty_ref.clone(), }; diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 3244d6a723..479b20d670 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1269,31 +1269,156 @@ pub fn matches_target_filter_including_phased_out( /// face and yields `false`. Use the object-based `matches_target_filter` family /// instead whenever an `ObjectId` exists. pub(crate) fn matches_target_filter_against_face(face: &CardFace, filter: &TargetFilter) -> bool { + matches_target_filter_against_face_scoped(face, filter, FaceControllerScope::Reject) +} + +/// CR 109.5: how a bare-`CardFace` match treats a filter's controller axis. +/// +/// A face has no controller, so a controller-scoped filter is normally +/// unanswerable. Some callers do know the answer out of band — deck analysis +/// asks only about the analyzing player's own list, and a cost modifier's +/// "spells YOU cast" scope is carried on `StaticDefinition.affected` and settled +/// before the spell filter is consulted (see +/// [`crate::types::ability::cost_modifier_caster_scope`]). This names which of +/// those two situations the caller is in instead of leaving it implicit. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FaceControllerScope { + /// The controller is genuinely unknown: any controller-scoped filter fails. + Reject, + /// The face is known to be the querying player's own card, so + /// `ControllerRef::You` is satisfied and `ControllerRef::Opponent` is not. + /// Any other `ControllerRef` remains unanswerable and fails. + AssumeOwn, +} + +/// CR 205: Evaluate a `TargetFilter`'s STATIC characteristics against a bare +/// `CardFace`, resolving the controller axis per `scope`. +/// +/// `pub` so deck-time analysis outside this crate (`phase-ai`'s +/// `features::cost_reduction`) can match a static's `spell_filter` against a +/// bare face through this authority rather than re-deriving CR 205 semantics. +pub fn matches_target_filter_against_face_scoped( + face: &CardFace, + filter: &TargetFilter, + scope: FaceControllerScope, +) -> bool { match filter { TargetFilter::Any => true, TargetFilter::None => false, TargetFilter::Typed(typed) => { - typed.controller.is_none() + controller_ref_admits_face(typed.controller.as_ref(), scope) && typed .type_filters .iter() .all(|type_filter| matches_type_filter_against_face(face, type_filter)) - && typed.properties.iter().all(|property| match property { - FilterProp::HasSupertype { value } => face.card_type.supertypes.contains(value), - _ => false, - }) + && typed + .properties + .iter() + // Fail closed: a property with no context-free reading (live + // combat, counters, zone, chosen-value state) cannot be + // satisfied by a face, so the whole filter does not match. + .all(|property| context_free_prop_matches_face(face, property) == Some(true)) } TargetFilter::Or { filters } => filters .iter() - .any(|inner| matches_target_filter_against_face(face, inner)), + .any(|inner| matches_target_filter_against_face_scoped(face, inner, scope)), TargetFilter::And { filters } => filters .iter() - .all(|inner| matches_target_filter_against_face(face, inner)), - TargetFilter::Not { filter } => !matches_target_filter_against_face(face, filter), + .all(|inner| matches_target_filter_against_face_scoped(face, inner, scope)), + TargetFilter::Not { filter } => { + !matches_target_filter_against_face_scoped(face, filter, scope) + } _ => false, } } +/// CR 109.5: does this filter's controller scope admit a bare face? +fn controller_ref_admits_face( + controller: Option<&ControllerRef>, + scope: FaceControllerScope, +) -> bool { + matches!( + (controller, scope), + (None, _) | (Some(ControllerRef::You), FaceControllerScope::AssumeOwn) + ) +} + +/// CR 205 + CR 202: Evaluate one `FilterProp` against a bare `CardFace`. +/// +/// `Some(true)` / `Some(false)` = the property has a context-free reading and +/// this is it. `None` = the property needs a live object (battlefield state, +/// counters, combat, zone, a value chosen earlier in a resolution) and therefore +/// has NO answer for a face — callers must fail closed rather than guess. +/// +/// Kept as an explicit allowlist, not a wildcard `_ => true`: a newly added +/// `FilterProp` lands in the `None` arm and fails closed until someone decides +/// whether it is context-free, which is the safe direction. +pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Option { + match prop { + // CR 205.4a: printed supertype line. + FilterProp::HasSupertype { value } => Some(face.card_type.supertypes.contains(value)), + FilterProp::NotSupertype { value } => Some(!face.card_type.supertypes.contains(value)), + // CR 202.3: mana value is printed on the face. Only a fixed comparison + // is context-free — a dynamic quantity needs game state. + FilterProp::Cmc { + comparator, + value: QuantityExpr::Fixed { value }, + } => Some(comparator.evaluate( + i32::try_from(face.mana_cost.mana_value()).unwrap_or(i32::MAX), + *value, + )), + // A dynamic mana-value comparison needs game state to resolve. + FilterProp::Cmc { .. } => None, + // CR 105.2 + CR 202.2: printed color, via the face's own color authority. + FilterProp::HasColor { color } => Some(face_colors(face).contains(color)), + FilterProp::NotColor { color } => Some(!face_colors(face).contains(color)), + FilterProp::ColorCount { comparator, count } => Some(comparator.evaluate( + i32::try_from(face_colors(face).len()).unwrap_or(i32::MAX), + i32::from(*count), + )), + // CR 702: printed keyword line. + FilterProp::WithKeyword { value } => Some(face.keywords.contains(value)), + FilterProp::WithoutKeyword { value } => Some(!face.keywords.contains(value)), + // CR 111.1 + CR 108.2: a bare face is a card definition, never a token. + FilterProp::Token => Some(false), + FilterProp::NonToken | FilterProp::RepresentedByCard => Some(true), + // Recursive combinators inherit their operands' answerability. + FilterProp::Not { prop } => context_free_prop_matches_face(face, prop).map(|m| !m), + FilterProp::AnyOf { props } => props.iter().try_fold(false, |acc, inner| { + context_free_prop_matches_face(face, inner).map(|m| acc || m) + }), + // Everything else reads live state and has no face-level answer. + _ => None, + } +} + +/// CR 105.2 + CR 202.2: the colors of a bare face — an explicit color-defining +/// override when present (CR 604.3), otherwise the printed mana cost. +fn face_colors(face: &CardFace) -> Vec { + if let Some(colors) = &face.color_override { + return colors.clone(); + } + [ + ManaColor::White, + ManaColor::Blue, + ManaColor::Black, + ManaColor::Red, + ManaColor::Green, + ] + .into_iter() + .filter(|color| match &face.mana_cost { + ManaCost::Cost { shards, .. } => shards.iter().any(|shard| shard.contributes_to(*color)), + // CR 202.1: no printed mana cost, so no color from one. The `Self*` + // forms are cost REFERENCES resolved against a live object (CR 202.3b), + // not a printed cost, so a bare face has no colors to read from them. + ManaCost::NoCost + | ManaCost::SelfManaCost + | ManaCost::SelfManaValue + | ManaCost::SelfManaCostReduced { .. } => false, + }) + .collect() +} + /// CR 205: Evaluate a single `TypeFilter` against a bare `CardFace`'s printed /// card type line (core types, subtypes, supertypes). Context-free counterpart /// to the object-based type checks in `filter_inner_for_object`. diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 3a8322bce2..d72a39de2b 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -20882,6 +20882,112 @@ pub enum ProtectionDoesNotRemove { ControlledAttachmentsAlreadyAttached, } +/// CR 601.2f: whose spells a cost modifier applies to, read off +/// `StaticDefinition.affected`. +/// +/// The stored form is a `TargetFilter` whose `TypedFilter.controller` carries +/// the scope, so every consumer would otherwise re-destructure it. Naming the +/// three outcomes keeps the decision in one place and lets a caller that has no +/// `PlayerId` (deck-time analysis) ask the same question as one that does +/// (the casting pipeline). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CostModifierCasterScope { + /// "Spells you cast" — applies only when the caster controls the source. + You, + /// "Spells your opponents cast" — applies only when the caster does not. + Opponent, + /// Unscoped — applies to every caster. + Any, +} + +/// CR 601.2f: read the caster scope off a cost modifier's `affected` filter. +pub fn cost_modifier_caster_scope(affected: Option<&TargetFilter>) -> CostModifierCasterScope { + match affected { + Some(TargetFilter::Typed(typed)) => match typed.controller { + Some(ControllerRef::You) => CostModifierCasterScope::You, + Some(ControllerRef::Opponent) => CostModifierCasterScope::Opponent, + _ => CostModifierCasterScope::Any, + }, + _ => CostModifierCasterScope::Any, + } +} + +/// CR 601.2f: the structural facts of a BOARD-WIDE cost modifier — one that a +/// permanent applies to other spells, as opposed to a `SelfRef` "this spell +/// costs {N} less" self-reduction (CR 113.6), which the casting pipeline +/// resolves through a different path entirely. +/// +/// This is the single structural authority for "is this static a cost modifier, +/// and what are its terms". It deliberately answers only the questions that need +/// NO game state, so that deck-time analysis and the live casting pipeline share +/// one definition of eligibility instead of maintaining parallel copies that +/// drift. State-dependent gates — the functioning zone, the `condition`, the +/// `dynamic_count` multiplier, and the spell filter itself — stay with the +/// caller that has the state to evaluate them. +#[derive(Debug, Clone, Copy)] +pub struct BoardWideCostModifier<'a> { + pub mode: crate::types::statics::CostModifyMode, + pub amount: &'a crate::types::mana::ManaCost, + pub spell_filter: Option<&'a TargetFilter>, + pub dynamic_count: Option<&'a QuantityRef>, + pub caster_scope: CostModifierCasterScope, + /// The gate a live caller must still evaluate before applying this modifier + /// (CR 601.2f "as long as" / "during your turn" clauses). `None` is + /// unconditional. + pub condition: Option<&'a StaticCondition>, +} + +impl StaticDefinition { + /// CR 601.2f: view this static as a board-wide cost modifier, or `None` when + /// it is not one. + /// + /// Rejects `Minimum` (a CR 601.2f last-step floor, not a per-spell + /// adjustment) and `SelfRef` self-cost reductions (CR 113.6). + pub fn board_wide_cost_modifier(&self) -> Option> { + let StaticMode::ModifyCost { + mode, + amount, + spell_filter, + dynamic_count, + } = &self.mode + else { + return None; + }; + if matches!(mode, crate::types::statics::CostModifyMode::Minimum) { + return None; + } + if matches!(self.affected, Some(TargetFilter::SelfRef)) { + return None; + } + Some(BoardWideCostModifier { + mode: *mode, + amount, + spell_filter: spell_filter.as_ref(), + dynamic_count: dynamic_count.as_ref(), + caster_scope: cost_modifier_caster_scope(self.affected.as_ref()), + condition: self.condition.as_ref(), + }) + } +} + +impl CostModifierCasterScope { + /// CR 601.2f: does this scope admit `caster`, given who controls the source? + pub fn admits(self, caster: PlayerId, source_controller: PlayerId) -> bool { + match self { + CostModifierCasterScope::You => caster == source_controller, + CostModifierCasterScope::Opponent => caster != source_controller, + CostModifierCasterScope::Any => true, + } + } + + /// CR 601.2f: does this scope admit the source's own controller? The + /// `PlayerId`-free question deck analysis asks — "would this discount the + /// spells of the player who controls it?" + pub fn admits_own_controller(self) -> bool { + !matches!(self, CostModifierCasterScope::Opponent) + } +} + impl StaticDefinition { pub fn new(mode: StaticMode) -> Self { Self { diff --git a/crates/phase-ai/src/features/cost_reduction.rs b/crates/phase-ai/src/features/cost_reduction.rs index c0789f7555..5f7db34b8f 100644 --- a/crates/phase-ai/src/features/cost_reduction.rs +++ b/crates/phase-ai/src/features/cost_reduction.rs @@ -39,13 +39,17 @@ //! read high on both — Sol Ring plus Medallions is a real shell — and the axes //! stay independent. -use engine::game::filter::matches_type_filter_against_face; +use engine::game::filter::{matches_target_filter_against_face_scoped, FaceControllerScope}; +use engine::game::quantity::resolve_quantity; use engine::game::DeckEntry; -use engine::types::ability::{ControllerRef, StaticDefinition, TargetFilter}; +use engine::types::ability::{QuantityExpr, StaticDefinition, TargetFilter}; use engine::types::card::CardFace; use engine::types::card_type::CoreType; +use engine::types::game_state::GameState; +use engine::types::identifiers::ObjectId; use engine::types::mana::ManaCost; -use engine::types::statics::{CostModifyMode, StaticMode}; +use engine::types::player::PlayerId; +use engine::types::statics::CostModifyMode; use crate::features::commitment; @@ -166,55 +170,98 @@ pub(crate) fn your_spell_discount_parts<'a>( /// CR 601.2f: the generic discount this one static gives to spells you cast, or /// `None` when it is not a board-wide reduction of your own spells. /// -/// Mirrors the eligibility tests `casting::collect_battlefield_cost_modifiers` applies at -/// cast time, so deck classification and the resolver agree by construction: -/// `Reduce` mode only, never a `SelfRef` self-cost reduction, and never an -/// opponent-scoped modifier. +/// Eligibility is NOT re-derived here. `StaticDefinition::board_wide_cost_modifier` +/// is the engine's single structural authority — the same one +/// `casting::collect_battlefield_cost_modifiers` consumes at cast time — so +/// `Minimum`, `SelfRef` (CR 113.6) and the caster scope are settled in one place +/// and cannot drift between deck analysis and the resolver. fn your_spell_discount(def: &StaticDefinition) -> Option { - let StaticMode::ModifyCost { - mode: CostModifyMode::Reduce, - amount, - .. - } = &def.mode - else { - // `Raise` (Thalia) and `Minimum` (Trinisphere) are taxes, not discounts. - return None; - }; + let modifier = def.board_wide_cost_modifier()?; - // CR 113.6: a `SelfRef` reduction is "this spell costs {N} less" — resolved - // by `apply_self_spell_cost_modifiers` for the spell being cast, and never - // applied from a battlefield permanent to other spells. It is a property of - // one card, not a deck-wide engine. - if matches!(def.affected, Some(TargetFilter::SelfRef)) { + // CR 601.2f: `Raise` (Thalia) is a tax, not a discount. `Minimum` + // (Trinisphere) never reaches here — the authority rejects it. + if !matches!(modifier.mode, CostModifyMode::Reduce) { return None; } - // CR 601.2f: caster scope. `Opponent` is a discount handed to the other - // side; only `You` and an unscoped modifier reduce spells you cast. - if let Some(TargetFilter::Typed(typed)) = &def.affected { - if matches!(typed.controller, Some(ControllerRef::Opponent)) { - return None; - } + // CR 601.2f: a modifier scoped to opponents' spells is a discount handed to + // the other side, not this deck's engine. + if !modifier.caster_scope.admits_own_controller() { + return None; } - // CR 118.7a: only the generic component of a cost can be reduced by a - // generic reduction, so the discount magnitude is `generic`. A reduction of - // zero generic mana (a purely colored `amount`) moves no cost here and is - // not counted as an engine. - // - // `dynamic_count` is deliberately NOT resolved here. A scaling reducer - // ("costs {1} less for each artifact you control") multiplies `generic` by a - // game-state quantity that is unknowable at deck-analysis time, so this - // reports the per-unit amount — one application's worth. That understates a - // scaling reducer's ceiling rather than inventing a multiplier, and on the - // live path `CostReductionPolicy` additionally caps the credited discount, - // so a large `amount` whose multiplier is currently zero cannot dominate. + generic_discount(modifier.amount) +} + +/// CR 118.7a: only the generic component of a cost can be reduced by a generic +/// reduction, so the magnitude is `generic`. A purely colored `amount` moves no +/// generic cost and is not an engine. +fn generic_discount(amount: &ManaCost) -> Option { let ManaCost::Cost { generic, .. } = amount else { return None; }; (*generic > 0).then_some(*generic) } +/// CR 601.2f: one discount a static would currently apply to its controller's +/// spells, paired with the spells it applies to. +pub(crate) struct LiveDiscount { + /// Generic mana removed per application, `dynamic_count` already resolved. + pub generic: u32, + /// `None` discounts every spell its controller casts. + pub spell_filter: Option, +} + +/// CR 601.2f: the discounts `statics` would apply to their controller's spells +/// **right now**, with every state-dependent term of the casting authority's +/// eligibility resolved rather than assumed. +/// +/// The deck-time [`your_spell_discount`] deliberately answers only the +/// structural half, because a deck list has no game state. This is the live +/// half, and it exists because the casting authority gates each modifier on two +/// things a structural read cannot see (`casting::collect_battlefield_cost_modifiers`): +/// +/// * `condition` — an "as long as" / "during your turn" gate. A candidate still +/// in hand has not entered the battlefield, so a source-relative condition has +/// no truthful answer yet; per CR 601.2f the modifier simply would not apply +/// when it is false. This **fails off**: a conditional reducer earns no +/// deployment credit rather than credit the AI cannot bank on. +/// * `dynamic_count` — "for each [thing]" multiplier, resolved through the +/// engine's `resolve_quantity` authority so this agrees with the resolver by +/// construction. A multiplier of zero means the reducer currently discounts +/// nothing and is skipped entirely. +pub(crate) fn live_your_spell_discounts<'a>( + state: &GameState, + source: ObjectId, + controller: PlayerId, + statics: impl IntoIterator, +) -> Vec { + statics + .into_iter() + .filter_map(|def| { + let per_application = your_spell_discount(def)?; + let modifier = def.board_wide_cost_modifier()?; + // CR 601.2f: an unevaluable gate fails off (see above). + if modifier.condition.is_some() { + return None; + } + let multiplier = match modifier.dynamic_count { + None => 1, + Some(qty) => { + let expr = QuantityExpr::Ref { qty: qty.clone() }; + u32::try_from(resolve_quantity(state, &expr, controller, source).max(0)) + .unwrap_or(0) + } + }; + let generic = per_application.saturating_mul(multiplier); + (generic > 0).then(|| LiveDiscount { + generic, + spell_filter: modifier.spell_filter.cloned(), + }) + }) + .collect() +} + /// The `spell_filter` of a qualifying reducer — `Some(None)` for "discounts /// every spell you cast", `None` when this static is not a qualifying reducer. /// @@ -223,45 +270,29 @@ fn your_spell_discount(def: &StaticDefinition) -> Option { /// return type would force every caller to destructure a tuple it half-ignores. fn your_spell_discount_filter(def: &StaticDefinition) -> Option> { your_spell_discount(def)?; - let StaticMode::ModifyCost { spell_filter, .. } = &def.mode else { - return None; - }; - Some(spell_filter.clone()) + Some(def.board_wide_cost_modifier()?.spell_filter.cloned()) } /// CR 601.2f: would this reducer's `spell_filter` admit `face` as a discounted /// spell? `None` is an unfiltered reducer — it discounts everything you cast. /// -/// Evaluated on the TYPE axis only, delegating every leaf to the engine's -/// CR 205 authority (`matches_type_filter_against_face`). The caster axis is -/// deliberately not re-checked here: `StaticDefinition.affected` already carried -/// it in [`your_spell_discount`], which is exactly how -/// `casting::collect_battlefield_cost_modifiers` splits the two. +/// Delegates wholly to the engine's context-free `CardFace` authority +/// (`matches_target_filter_against_face_scoped`), which owns CR 205 type +/// semantics AND every context-free `FilterProp` (mana value, color, keyword, +/// supertype) — so a color-, mana-value- or keyword-scoped reducer is matched +/// rather than silently discarded, and a property that needs live state fails +/// closed inside that authority instead of here. /// -/// Anything this cannot verify — a `properties` predicate that needs live game -/// state, or a filter variant outside the composition below — reports `false`, -/// so an unreadable filter *undercounts* coverage and drives commitment DOWN. -/// The axis fails off rather than claiming a discount the deck may not get. +/// `FaceControllerScope::AssumeOwn` because the caster axis was already settled +/// by `CostModifierCasterScope` in [`your_spell_discount`] — deck analysis asks +/// only about the analyzing player's own list, which is exactly how +/// `casting::collect_battlefield_cost_modifiers` splits the two checks. fn filter_admits_face(filter: Option<&TargetFilter>, face: &CardFace) -> bool { - let Some(filter) = filter else { - return true; - }; match filter { - TargetFilter::Any => true, - TargetFilter::Typed(typed) => { - typed.properties.is_empty() - && typed - .type_filters - .iter() - .all(|type_filter| matches_type_filter_against_face(face, type_filter)) + None => true, + Some(filter) => { + matches_target_filter_against_face_scoped(face, filter, FaceControllerScope::AssumeOwn) } - TargetFilter::Or { filters } => filters - .iter() - .any(|inner| filter_admits_face(Some(inner), face)), - TargetFilter::And { filters } => filters - .iter() - .all(|inner| filter_admits_face(Some(inner), face)), - _ => false, } } diff --git a/crates/phase-ai/src/features/tests/cost_reduction.rs b/crates/phase-ai/src/features/tests/cost_reduction.rs index 6d9f933713..8959037b25 100644 --- a/crates/phase-ai/src/features/tests/cost_reduction.rs +++ b/crates/phase-ai/src/features/tests/cost_reduction.rs @@ -3,11 +3,12 @@ use engine::game::DeckEntry; use engine::types::ability::{ - ControllerRef, FilterProp, StaticDefinition, TargetFilter, TypeFilter, TypedFilter, + Comparator, ControllerRef, FilterProp, QuantityExpr, StaticDefinition, TargetFilter, + TypeFilter, TypedFilter, }; use engine::types::card::CardFace; use engine::types::card_type::{CardType, CoreType}; -use engine::types::mana::ManaCost; +use engine::types::mana::{ManaColor, ManaCost, ManaCostShard}; use engine::types::statics::{CostModifyMode, StaticMode}; use crate::features::cost_reduction::*; @@ -364,3 +365,136 @@ fn parts_predicate_reports_zero_for_non_reducer() { let f_card = undiscounted_spell("Bear"); assert_eq!(your_spell_discount_parts(&f_card.static_abilities), 0); } + +// ─── review #6743: context-free `TypedFilter.properties` are now honored ────── +// +// Previously any nonempty `properties` list was discarded, so color-, mana-value- +// and keyword-scoped reducers silently discounted nothing. + +/// A face with a real printed mana cost, so color and mana-value props resolve. +fn costed_face(name: &str, core: CoreType, shards: Vec, generic: u32) -> CardFace { + let mut f = face(name, core); + f.mana_cost = ManaCost::Cost { shards, generic }; + f +} + +fn prop_reducer(name: &str, props: Vec) -> CardFace { + reducer( + name, + 1, + CostModifyMode::Reduce, + Some(TargetFilter::Typed(TypedFilter { + properties: props, + controller: Some(ControllerRef::You), + ..Default::default() + })), + ) +} + +#[test] +fn color_scoped_reducer_covers_matching_spells() { + // "White spells you cast cost {1} less" — CR 105.2 printed color. + let f = detect(&[ + entry( + prop_reducer( + "Medallion", + vec![FilterProp::HasColor { + color: ManaColor::White, + }], + ), + 4, + ), + entry( + costed_face( + "White Spell", + CoreType::Instant, + vec![ManaCostShard::White], + 1, + ), + 20, + ), + entry( + costed_face("Red Spell", CoreType::Instant, vec![ManaCostShard::Red], 1), + 12, + ), + ]); + assert_eq!(f.reducer_count, 4); + // Only the 20 white spells — the reducer itself is colorless here. + assert_eq!(f.discounted_count, 20); + assert!( + f.commitment > 0.0, + "color-scoped reducer must produce coverage" + ); +} + +#[test] +fn mana_value_scoped_reducer_covers_matching_spells() { + // "Spells you cast with mana value 3 or greater cost {1} less" — CR 202.3. + let f = detect(&[ + entry( + prop_reducer( + "Big Discount", + vec![FilterProp::Cmc { + comparator: Comparator::GE, + value: QuantityExpr::Fixed { value: 3 }, + }], + ), + 4, + ), + entry(costed_face("Expensive", CoreType::Sorcery, vec![], 4), 20), + entry(costed_face("Cheap", CoreType::Sorcery, vec![], 1), 12), + ]); + assert_eq!(f.reducer_count, 4); + assert_eq!(f.discounted_count, 20); +} + +#[test] +fn negated_color_prop_is_honored() { + let f = detect(&[ + entry( + prop_reducer( + "Anti-White", + vec![FilterProp::NotColor { + color: ManaColor::White, + }], + ), + 4, + ), + entry( + costed_face( + "White Spell", + CoreType::Instant, + vec![ManaCostShard::White], + 1, + ), + 20, + ), + entry( + costed_face("Red Spell", CoreType::Instant, vec![ManaCostShard::Red], 1), + 12, + ), + ]); + // The 12 red spells plus the 4 colorless reducers themselves. + assert_eq!(f.discounted_count, 16); +} + +#[test] +fn live_only_property_still_fails_closed() { + // A property with no context-free reading must NOT be assumed satisfied — + // it yields no coverage, so commitment collapses rather than over-claiming. + let f = detect(&[ + entry(prop_reducer("Stateful", vec![FilterProp::Tapped]), 4), + entry(costed_face("Spell", CoreType::Instant, vec![], 2), 32), + ]); + assert_eq!(f.reducer_count, 4); + assert_eq!(f.discounted_count, 0); + assert_eq!(f.commitment, 0.0); +} + +#[test] +fn controller_scoped_spell_filter_is_admitted_for_own_deck() { + // Regression for the root defect: a `ControllerRef::You` spell filter — the + // shape nearly every real reducer uses — must not be rejected outright. + let f = detect(&deck(spell_reducer("Electromancer"), 4, 20, 12)); + assert_eq!(f.discounted_count, 20); +} diff --git a/crates/phase-ai/src/policies/cost_reduction.rs b/crates/phase-ai/src/policies/cost_reduction.rs index 42abe57a90..1c8eb1c562 100644 --- a/crates/phase-ai/src/policies/cost_reduction.rs +++ b/crates/phase-ai/src/policies/cost_reduction.rs @@ -22,12 +22,17 @@ //! hand size and touches no battlefield sweep, no `find_legal_targets`, and no //! affordability query. +use engine::game::filter::{matches_target_filter, FilterContext}; +use engine::types::ability::TargetFilter; use engine::types::card_type::CoreType; use engine::types::game_state::GameState; use engine::types::identifiers::ObjectId; use engine::types::player::PlayerId; -use crate::features::cost_reduction::{your_spell_discount_parts, COST_REDUCTION_FLOOR}; +use crate::cast_facts::CastFacts; +use crate::features::cost_reduction::{ + live_your_spell_discounts, LiveDiscount, COST_REDUCTION_FLOOR, +}; use crate::features::DeckFeatures; use super::context::PolicyContext; @@ -75,12 +80,21 @@ impl TacticalPolicy for CostReductionPolicy { return PolicyVerdict::neutral(PolicyReason::new("cost_reduction_na")); }; - // Card-local first: does THIS candidate carry the discount engine? - let discount = your_spell_discount_parts(facts.object.static_definitions.iter_unchecked()); - if discount > 0 { - // A discount only pays off on spells still to be cast. With an empty - // grip the reducer is a vanilla permanent this turn. - let future_casts = castable_cards_in_hand(ctx, Some(facts.object.id)); + // Card-local first: does THIS candidate carry a discount engine that + // would actually be applying right now (condition and dynamic multiplier + // resolved, not assumed)? + let discounts = live_your_spell_discounts( + ctx.state, + facts.object.id, + ctx.ai_player, + facts.object.static_definitions.iter_unchecked(), + ); + if !discounts.is_empty() { + // A discount only pays off on the spells it actually reduces, so + // count the grip through each reducer's own `spell_filter` rather + // than crediting every card in hand. + let discount: u32 = discounts.iter().map(|d| d.generic).sum(); + let future_casts = discountable_cards_in_hand(ctx, &discounts, facts.object.id); if future_casts == 0 { return PolicyVerdict::neutral(PolicyReason::new("cost_reduction_no_future_casts")); } @@ -97,11 +111,13 @@ impl TacticalPolicy for CostReductionPolicy { ); } - // Otherwise: are we casting past an unplayed reducer that is cheaper than - // this spell? Deploying the discount first is strictly better sequencing - // — the same shape as `RampTimingPolicy`'s `defer_to_ramp`. The mana-value - // gate keeps this to cases where the reducer plausibly fits first. - if hand_holds_cheaper_reducer(ctx, facts.mana_value, facts.object.id) { + // Otherwise: are we casting past an unplayed, cheaper reducer that would + // actually have discounted THIS spell? Deploying the discount first is + // strictly better sequencing — the same shape as + // `RampTimingPolicy::defer_to_ramp`. Both the mana-value gate and the + // spell-filter match are required, so a narrow reducer never penalizes a + // spell it cannot reduce. + if hand_holds_cheaper_reducer(ctx, &facts) { return PolicyVerdict::score( ctx.config.policy_penalties.cost_reduction_defer_penalty, PolicyReason::new("cost_reduction_defer_to_engine") @@ -113,51 +129,89 @@ impl TacticalPolicy for CostReductionPolicy { } } -/// Nonland cards in the AI's hand that a discount could still apply to, -/// excluding `exclude` (the candidate itself, which is being spent now). +/// CR 601.2f: does `filter` (a reducer's `spell_filter`; `None` = unfiltered) +/// admit the object `id` as a spell it discounts? /// -/// Lands are excluded because CR 305.1 land plays are not spells and are never -/// discounted by a CR 601.2f cost reducer. +/// Delegates to the engine's live object authority `matches_target_filter`, with +/// the reducer as the filter source so a `ControllerRef::You` scope resolves +/// against the reducer's controller exactly as it does at cast time. +fn filter_admits_object( + ctx: &PolicyContext<'_>, + filter: Option<&TargetFilter>, + source: ObjectId, + id: ObjectId, +) -> bool { + match filter { + None => true, + Some(filter) => matches_target_filter( + ctx.state, + id, + filter, + &FilterContext::from_source(ctx.state, source), + ), + } +} + +/// Cards in the AI's hand that at least one of `discounts` would actually +/// reduce, excluding `exclude` (the candidate itself, which is being spent now). /// -/// This counts remaining casts, NOT the subset the reducer's `spell_filter` -/// admits — narrowing per candidate would mean evaluating that filter against -/// every card in hand inside the search inner loop. The narrowing is applied -/// once per game instead: `CostReductionFeature::commitment` folds in the -/// deck-wide fraction of cards the reducers actually discount, and the registry -/// multiplies this verdict by that commitment via `activation`. So a deck whose -/// reducers cover little of its own list is already scaled down here, and a -/// reducer that covers nothing deactivates the policy outright. -fn castable_cards_in_hand(ctx: &PolicyContext<'_>, exclude: Option) -> u32 { +/// Lands are excluded first because CR 305.1 land plays are not spells and are +/// never discounted by a CR 601.2f cost reducer — and that check is far cheaper +/// than a filter evaluation, so it runs before one. +fn discountable_cards_in_hand( + ctx: &PolicyContext<'_>, + discounts: &[LiveDiscount], + exclude: ObjectId, +) -> u32 { let Some(player) = ctx.state.players.get(ctx.ai_player.0 as usize) else { return 0; }; player .hand .iter() - .filter(|id| Some(**id) != exclude) + .filter(|id| **id != exclude) .filter(|id| { ctx.state .objects .get(id) .is_some_and(|obj| !obj.card_types.core_types.contains(&CoreType::Land)) }) + .filter(|id| { + discounts.iter().any(|discount| { + filter_admits_object(ctx, discount.spell_filter.as_ref(), exclude, **id) + }) + }) .count() .try_into() .unwrap_or(u32::MAX) } -/// True when the AI's hand still holds a cost-reduction permanent whose mana -/// value is strictly below `mana_value` — i.e. the engine could have been -/// deployed instead of, and later alongside, this spell. -fn hand_holds_cheaper_reducer(ctx: &PolicyContext<'_>, mana_value: u32, exclude: ObjectId) -> bool { +/// True when the AI's hand still holds a live cost-reduction permanent that is +/// cheaper than this spell AND would actually discount it — i.e. the engine +/// could have been deployed first, to this very spell's benefit. +fn hand_holds_cheaper_reducer(ctx: &PolicyContext<'_>, facts: &CastFacts<'_>) -> bool { let Some(player) = ctx.state.players.get(ctx.ai_player.0 as usize) else { return false; }; player.hand.iter().any(|id| { - *id != exclude + *id != facts.object.id && ctx.state.objects.get(id).is_some_and(|obj| { - obj.effective_mana_value() < mana_value - && your_spell_discount_parts(obj.static_definitions.iter_unchecked()) > 0 + obj.effective_mana_value() < facts.mana_value + && live_your_spell_discounts( + ctx.state, + *id, + ctx.ai_player, + obj.static_definitions.iter_unchecked(), + ) + .iter() + .any(|discount| { + filter_admits_object( + ctx, + discount.spell_filter.as_ref(), + *id, + facts.object.id, + ) + }) }) }) } diff --git a/crates/phase-ai/src/policies/registry.rs b/crates/phase-ai/src/policies/registry.rs index 7ca7e00f59..8f3b79dd45 100644 --- a/crates/phase-ai/src/policies/registry.rs +++ b/crates/phase-ai/src/policies/registry.rs @@ -144,8 +144,10 @@ 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. + /// CR 601.2f: deploy a "spells you cast cost less" engine before the spells + /// it discounts. CostReduction, + /// CR 121.1: reward drawing into an on-battlefield "whenever you draw" engine. DrawPayoff, } diff --git a/crates/phase-ai/src/policies/tests/cost_reduction.rs b/crates/phase-ai/src/policies/tests/cost_reduction.rs index 482247b3de..7c5b2dc53d 100644 --- a/crates/phase-ai/src/policies/tests/cost_reduction.rs +++ b/crates/phase-ai/src/policies/tests/cost_reduction.rs @@ -8,7 +8,10 @@ use std::sync::Arc; use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; use engine::game::zones::create_object; -use engine::types::ability::{ControllerRef, StaticDefinition, TargetFilter, TypedFilter}; +use engine::types::ability::{ + ControllerRef, PlayerScope, QuantityRef, StaticCondition, StaticDefinition, TargetFilter, + TypeFilter, TypedFilter, +}; use engine::types::actions::GameAction; use engine::types::card_type::CoreType; use engine::types::format::FormatConfig; @@ -436,3 +439,213 @@ fn registry_stays_silent_below_the_activation_floor() { "policy must not contribute below its activation floor" ); } + +// ─── review #6743: live gating + spell_filter narrowing ────────────────────── + +/// A reducer whose `spell_filter` restricts it to one core type. +fn typed_reducer_in_hand( + state: &mut GameState, + mana_value: u32, + only: TypeFilter, +) -> (ObjectId, CardId) { + let mut def = StaticDefinition::new(StaticMode::ModifyCost { + mode: CostModifyMode::Reduce, + amount: generic(1), + spell_filter: Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![only], + controller: Some(ControllerRef::You), + ..Default::default() + })), + dynamic_count: None, + }); + def.affected = Some(TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::You), + ..Default::default() + })); + hand_card( + state, + "Typed Reducer", + CoreType::Creature, + mana_value, + vec![def], + ) +} + +#[test] +fn deploy_credit_counts_only_spells_the_reducer_discounts() { + // An artifact-only reducer with a grip of sorceries discounts nothing. + let mut st = state(); + let (reducer, card) = typed_reducer_in_hand(&mut st, 2, TypeFilter::Artifact); + plain_spell_in_hand(&mut st, 3); + plain_spell_in_hand(&mut st, 4); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (_, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_no_future_casts"); +} + +#[test] +fn deploy_credit_counts_matching_spells() { + // Same reducer, but the grip is artifacts — now it pays off. + let mut st = state(); + let (reducer, card) = typed_reducer_in_hand(&mut st, 2, TypeFilter::Artifact); + hand_card(&mut st, "Artifact A", CoreType::Artifact, 3, Vec::new()); + hand_card(&mut st, "Artifact B", CoreType::Artifact, 4, Vec::new()); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_deploy_engine"); + assert!(delta > 0.0); +} + +#[test] +fn defer_penalty_does_not_fire_for_a_spell_the_reducer_cannot_reduce() { + // An artifact-only reducer must not penalize casting a sorcery. + let mut st = state(); + typed_reducer_in_hand(&mut st, 2, TypeFilter::Artifact); + let (spell, card) = plain_spell_in_hand(&mut st, 4); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(spell, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +#[test] +fn conditional_reducer_earns_no_deploy_credit() { + // CR 601.2f: an "as long as" gate has no truthful answer for a card still in + // hand, so the policy fails off rather than banking a discount it cannot + // guarantee. Mirrors the casting authority's condition gate. + let mut st = state(); + let mut def = reduce_your_spells(1, CostModifyMode::Reduce); + def.condition = Some(StaticCondition::IsPresent { + filter: Some(TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Artifact], + controller: Some(ControllerRef::You), + ..Default::default() + })), + }); + let (reducer, card) = hand_card(&mut st, "Conditional", CoreType::Creature, 2, vec![def]); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +#[test] +fn zero_dynamic_multiplier_earns_no_deploy_credit() { + // "costs {1} less for each card in your hand" with the multiplier resolving + // to zero discounts nothing, so it must not be credited. + let mut st = state(); + let mut def = reduce_your_spells(1, CostModifyMode::Reduce); + let StaticMode::ModifyCost { + ref mut dynamic_count, + .. + } = def.mode + else { + unreachable!("constructed as ModifyCost") + }; + *dynamic_count = Some(QuantityRef::GraveyardSize { + player: PlayerScope::Controller, + }); + let (reducer, card) = hand_card(&mut st, "Scaling", CoreType::Creature, 2, vec![def]); + plain_spell_in_hand(&mut st, 3); + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + let candidate = cast(reducer, card); + let decision = priority_decision(&candidate); + let (delta, reason) = + score_of(CostReductionPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); + + // Empty graveyard → multiplier 0 → no live discount at all. + assert_eq!(reason.kind, "cost_reduction_na"); + assert_eq!(delta, 0.0); +} + +#[test] +fn positive_dynamic_multiplier_scales_the_deploy_credit() { + // The same scaling reducer with a stocked graveyard credits more than the + // per-unit amount, matching the multiplier the resolver would apply. + let mut flat = state(); + let (flat_reducer, flat_card) = reducer_in_hand(&mut flat, 2); + plain_spell_in_hand(&mut flat, 3); + + let mut scaled = state(); + let mut def = reduce_your_spells(1, CostModifyMode::Reduce); + let StaticMode::ModifyCost { + ref mut dynamic_count, + .. + } = def.mode + else { + unreachable!("constructed as ModifyCost") + }; + *dynamic_count = Some(QuantityRef::GraveyardSize { + player: PlayerScope::Controller, + }); + let (scaled_reducer, scaled_card) = + hand_card(&mut scaled, "Scaling", CoreType::Creature, 2, vec![def]); + plain_spell_in_hand(&mut scaled, 3); + for _ in 0..3 { + let cid = CardId(scaled.next_object_id); + let id = create_object( + &mut scaled, + cid, + AI, + "Dead Card".to_string(), + Zone::Graveyard, + ); + scaled.players[AI.0 as usize].graveyard.push_back(id); + } + + let config = AiConfig::default(); + let context = context(&config, session(0.8)); + + let flat_candidate = cast(flat_reducer, flat_card); + let flat_decision = priority_decision(&flat_candidate); + let (flat_delta, _) = score_of(CostReductionPolicy.verdict(&ctx( + &flat, + &flat_candidate, + &flat_decision, + &context, + &config, + ))); + + let scaled_candidate = cast(scaled_reducer, scaled_card); + let scaled_decision = priority_decision(&scaled_candidate); + let (scaled_delta, reason) = score_of(CostReductionPolicy.verdict(&ctx( + &scaled, + &scaled_candidate, + &scaled_decision, + &context, + &config, + ))); + + assert_eq!(reason.kind, "cost_reduction_deploy_engine"); + assert!( + scaled_delta > flat_delta, + "multiplier>1 must out-score the flat reducer: {scaled_delta} vs {flat_delta}" + ); +} From cdd72457ee3dc41a3686462b79693a8e38c48021 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 28 Jul 2026 19:29:20 -0700 Subject: [PATCH 4/6] fix(engine): annotate the context-free face keyword reads for the authority gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's `Engine authority gate` (scripts/check-engine-authorities.sh) rejects raw `obj.keywords.contains(..)` in new engine code, because a raw query silently misses off-zone keyword grants that only `keywords::object_has_effective_keyword_kind(state, id, kind)` can see. `context_free_prop_matches_face` is the structural exception the gate documents. Its whole contract is that NO `GameObject` exists — it evaluates a bare `CardFace` outside the game — so there is no zone, no grant, and no `off_zone_characteristics` to consult, and the printed keyword line is the complete truth. The keyword authorities all require a `GameObject` this function by definition cannot have; a caller holding an `ObjectId` is already directed to the object-based `matches_target_filter` family by the module docs. Annotated with `allow-raw-authority:` and the reason, per the gate's own instructions, rather than reshaping the call to defeat the check. Verified: `./scripts/check-engine-authorities.sh upstream/main` exits 0, as do `cargo fmt --all -- --check`, `check-interaction-bindings.sh --check`, `check-prelowered-ratchet.sh`, `check-skill-doc.sh` and `check-resolution-frame-boundaries.sh` — the other gates in the same CI job. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/filter.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 479b20d670..394cbb3391 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1376,8 +1376,16 @@ pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Opt i32::try_from(face_colors(face).len()).unwrap_or(i32::MAX), i32::from(*count), )), - // CR 702: printed keyword line. + // CR 702: printed keyword line. The keyword authorities in + // `game/keywords.rs` all take a `GameObject`; this function's entire + // contract is that NO object exists (a bare `CardFace` outside the + // game), so there is no zone, no grant and no `off_zone_characteristics` + // to consult — the printed line IS the complete truth here. A caller + // holding an `ObjectId` must use the object-based `matches_target_filter` + // family instead, as the module doc says. + // allow-raw-authority: bare CardFace has no object, so no keyword grant can exist to miss FilterProp::WithKeyword { value } => Some(face.keywords.contains(value)), + // allow-raw-authority: bare CardFace has no object, so no keyword grant can exist to miss FilterProp::WithoutKeyword { value } => Some(!face.keywords.contains(value)), // CR 111.1 + CR 108.2: a bare face is a card definition, never a token. FilterProp::Token => Some(false), From d3c45f57fafec1b2b5722babb12f7a1f6021e5d8 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 28 Jul 2026 21:28:14 -0700 Subject: [PATCH 5/6] fix(engine): three-valued OR for FilterProp::AnyOf against a bare face MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-3 blocker on #6743. `context_free_prop_matches_face` returns `Option` where `None` means "this property needs live game state and has no face-level answer". The `AnyOf` arm folded with `try_fold`, which short-circuits on the first `None` — so a disjunction mixing a definite match with an unknowable alternative collapsed to unknown in BOTH orders: [Some(true), None] -> None (the true was already found, then discarded) [None, Some(true)] -> None (stopped before reaching the true) `matches_target_filter_against_face_scoped` admits only `Some(true)`, so a spell filter with a definitely-matching alternative was treated as non-matching purely because a sibling alternative referenced live state. Replaced with a proper CR 608.2b Kleene OR: short-circuit on the first definite `true`; return `None` only when nothing is true AND something is unknown; otherwise `false`. Negation already had the correct behavior — `None.map(!)` is `None` — and now has a regression pinning it. Eight tests covering the full truth table, including both orderings the review called out and an end-to-end case through the typed-filter caller (the production entry point that admits only `Some(true)`). Three are proven RED against the old `try_fold`: `any_of_true_then_unknown_is_true`, `any_of_unknown_then_true_is_true`, and `tri_state_or_reaches_the_typed_filter_caller`. The other five pin the rest of the table and pass either way by design. Verified: engine `game::filter` 149 passed, phase-ai cost_reduction 44 passed, `cargo clippy -p engine -p phase-ai --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/filter.rs | 170 ++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index 394cbb3391..f6ddf90220 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1390,11 +1390,26 @@ pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Opt // CR 111.1 + CR 108.2: a bare face is a card definition, never a token. FilterProp::Token => Some(false), FilterProp::NonToken | FilterProp::RepresentedByCard => Some(true), - // Recursive combinators inherit their operands' answerability. + // Recursive combinators inherit their operands' answerability. Negation + // of an unknown stays unknown. FilterProp::Not { prop } => context_free_prop_matches_face(face, prop).map(|m| !m), - FilterProp::AnyOf { props } => props.iter().try_fold(false, |acc, inner| { - context_free_prop_matches_face(face, inner).map(|m| acc || m) - }), + // CR 608.2b: three-valued (Kleene) OR. A definite `true` in ANY branch + // makes the disjunction true even when a sibling branch is unknowable + // from a face, so this must NOT short-circuit on the first `None` — the + // caller admits only `Some(true)`, and collapsing `[true, unknown]` to + // unknown would reject a spell filter that definitely matches. + // `None` is returned only when nothing is true AND something is unknown. + FilterProp::AnyOf { props } => { + let mut saw_unknown = false; + for inner in props { + match context_free_prop_matches_face(face, inner) { + Some(true) => return Some(true), + Some(false) => {} + None => saw_unknown = true, + } + } + (!saw_unknown).then_some(false) + } // Everything else reads live state and has no face-level answer. _ => None, } @@ -12758,4 +12773,151 @@ mod tests { "ControllerMatches must round-trip through serde" ); } + + // ─── CR 608.2b: three-valued OR in `context_free_prop_matches_face` ────── + // + // Review #6743: `try_fold` short-circuited on the first `None`, so a + // definite `Some(true)` alternative was collapsed to unknown and the typed + // caller (which admits only `Some(true)`) rejected a filter that matches. + + /// A face with a printed white mana cost, so color props are answerable. + fn tri_state_face() -> CardFace { + CardFace { + name: "Tri State Probe".to_string(), + card_type: crate::types::card_type::CardType { + supertypes: Vec::new(), + core_types: vec![CoreType::Instant], + subtypes: Vec::new(), + }, + mana_cost: crate::types::mana::ManaCost::Cost { + shards: vec![crate::types::mana::ManaCostShard::White], + generic: 1, + }, + ..Default::default() + } + } + + /// Context-free and TRUE for `tri_state_face`. + fn known_true() -> FilterProp { + FilterProp::HasColor { + color: crate::types::mana::ManaColor::White, + } + } + + /// Context-free and FALSE for `tri_state_face`. + fn known_false() -> FilterProp { + FilterProp::HasColor { + color: crate::types::mana::ManaColor::Red, + } + } + + /// Live-state-only: unanswerable from a bare face. + fn unknown() -> FilterProp { + FilterProp::Tapped + } + + #[test] + fn any_of_true_then_unknown_is_true() { + let prop = FilterProp::AnyOf { + props: vec![known_true(), unknown()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + Some(true), + "a definite match must survive a later unknowable alternative" + ); + } + + #[test] + fn any_of_unknown_then_true_is_true() { + // Order-sensitive twin: the old `try_fold` stopped at the leading `None`. + let prop = FilterProp::AnyOf { + props: vec![unknown(), known_true()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + Some(true), + "a leading unknown must not mask a definite later match" + ); + } + + #[test] + fn any_of_all_unknown_is_unknown() { + let prop = FilterProp::AnyOf { + props: vec![unknown(), unknown()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + None, + "nothing true and something unknown ⇒ unknown" + ); + } + + #[test] + fn any_of_false_plus_unknown_is_unknown() { + let prop = FilterProp::AnyOf { + props: vec![known_false(), unknown()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + None, + "a definite false does not resolve the unknown alternative" + ); + } + + #[test] + fn any_of_all_false_is_false() { + let prop = FilterProp::AnyOf { + props: vec![known_false(), known_false()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + Some(false), + "every alternative definitely false ⇒ definitely false" + ); + } + + #[test] + fn any_of_true_plus_false_is_true() { + let prop = FilterProp::AnyOf { + props: vec![known_false(), known_true()], + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + Some(true) + ); + } + + #[test] + fn tri_state_or_reaches_the_typed_filter_caller() { + // End-to-end through the production entry point: the typed caller admits + // only `Some(true)`, so the tri-state fix is what makes this match. + let filter = TargetFilter::Typed(TypedFilter { + type_filters: vec![crate::types::ability::TypeFilter::Instant], + properties: vec![FilterProp::AnyOf { + props: vec![known_true(), unknown()], + }], + ..Default::default() + }); + assert!( + matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + ), + "a definitely-matching alternative must admit the face" + ); + } + + #[test] + fn negation_of_unknown_stays_unknown() { + let prop = FilterProp::Not { + prop: Box::new(unknown()), + }; + assert_eq!( + context_free_prop_matches_face(&tri_state_face(), &prop), + None, + "negating an unknowable property cannot invent an answer" + ); + } } From c1a6f040c1ad69432a6366c314d3717091b571eb Mon Sep 17 00:00:00 2001 From: minion1227 Date: Wed, 29 Jul 2026 01:06:10 -0700 Subject: [PATCH 6/6] fix(engine): keep "unknown" unknown through face-filter negation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two round-4 blockers on #6743. [HIGH] `TargetFilter::Not` turned an unanswerable predicate into a match. The `Typed` arm collapsed every non-`Some(true)` property to `false` before returning, so an outer `Not` inverted it: a bare face evaluated against `Not(Tapped)` — whose only predicate needs a battlefield object — was ACCEPTED, admitting the wrong card class through a filter that should have failed closed. The same inversion applied to an unanswerable controller scope, and to the `_ => false` arm covering `SelfRef` and the player-scoped variants. Fixed by threading the third state through the whole walk instead of collapsing early: `target_filter_face_state` returns `Option` across `Typed`/`Or`/`And`/`Not`, combining with `kleene_and` / `kleene_or`, and `matches_target_filter_against_face_scoped` collapses to a bool exactly once at the public boundary. Unanswerable filter variants now yield `None` rather than a definite `false`, so negating them cannot manufacture a match. One asymmetry is deliberate: under `FaceControllerScope::AssumeOwn` the caller has asserted the face is the querying player's own card, so `ControllerRef::Opponent` is definitely FALSE (not unknown) and its negation is a legitimate definite match. Encoding that as `Some(false)` keeps the fix from over-correcting into "every controller scope is unknowable". [MED] Removed the fabricated `CR 608.2b` citations from the Kleene-OR comment and its test heading. Verified against docs/MagicCompRules.txt:2789: 608.2b governs checking target legality as a spell or ability resolves, and has nothing to do with Boolean disjunction. Three-valued logic here is an implementation property of the `Option` answer, not a rules behavior, so it carries no CR annotation now. The four other 608.2b references in this file are pre-existing and untouched. Eight production-entry regressions through `matches_target_filter_against_face_scoped`, covering `Not` around: a live-only property, an unknown controller scope, an unanswerable filter variant, a definite false, and a definite true; plus opponent-scope-under-AssumeOwn, AND with an unknown branch, and OR with a definite true beside an unknown. Proven RED against the old collapsed form: `not_around_live_only_property`, `not_around_unknown_controller_scope`, and `not_around_an_unanswerable_filter_variant` all fail when `Not` is restored to negating the boolean; the two definite-answer cases correctly stay green. Verified: engine `game::filter` 157 passed, phase-ai cost_reduction 44 passed, `cargo clippy -p engine -p phase-ai --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- crates/engine/src/game/filter.rs | 285 +++++++++++++++++++++++++++---- 1 file changed, 253 insertions(+), 32 deletions(-) diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index f6ddf90220..f79c8a8fbc 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -1302,45 +1302,113 @@ pub fn matches_target_filter_against_face_scoped( filter: &TargetFilter, scope: FaceControllerScope, ) -> bool { + // Only a DEFINITE match admits the face. `None` (unknown) collapses to + // `false` here and nowhere earlier — collapsing it inside the recursion is + // what let an outer `Not` invert "unanswerable" into "matches". + target_filter_face_state(face, filter, scope) == Some(true) +} + +/// Three-state evaluation of a `TargetFilter` against a bare `CardFace`. +/// +/// `Some(true)`/`Some(false)` = definitely matches / definitely does not. +/// `None` = the filter's answer depends on a live object that does not exist +/// here, so there IS no answer. +/// +/// The distinction is load-bearing under negation. Collapsing unknown to `false` +/// before recursing means `Not()` reads as `true`, so a face would be +/// admitted by a filter whose only predicate needs a live object. Threading the +/// third state through `Typed`/`Or`/`And`/`Not` and collapsing once, at the +/// boolean boundary, keeps the fail-closed contract intact at every depth. +fn target_filter_face_state( + face: &CardFace, + filter: &TargetFilter, + scope: FaceControllerScope, +) -> Option { match filter { - TargetFilter::Any => true, - TargetFilter::None => false, + TargetFilter::Any => Some(true), + TargetFilter::None => Some(false), TargetFilter::Typed(typed) => { - controller_ref_admits_face(typed.controller.as_ref(), scope) - && typed + let mut terms = vec![controller_ref_face_state(typed.controller.as_ref(), scope)]; + terms.extend( + typed .type_filters .iter() - .all(|type_filter| matches_type_filter_against_face(face, type_filter)) - && typed + // CR 205: the printed type line is always readable from a face. + .map(|tf| Some(matches_type_filter_against_face(face, tf))), + ); + terms.extend( + typed .properties .iter() - // Fail closed: a property with no context-free reading (live - // combat, counters, zone, chosen-value state) cannot be - // satisfied by a face, so the whole filter does not match. - .all(|property| context_free_prop_matches_face(face, property) == Some(true)) + .map(|property| context_free_prop_matches_face(face, property)), + ); + kleene_and(terms) } - TargetFilter::Or { filters } => filters - .iter() - .any(|inner| matches_target_filter_against_face_scoped(face, inner, scope)), - TargetFilter::And { filters } => filters - .iter() - .all(|inner| matches_target_filter_against_face_scoped(face, inner, scope)), + TargetFilter::Or { filters } => kleene_or( + filters + .iter() + .map(|inner| target_filter_face_state(face, inner, scope)), + ), + TargetFilter::And { filters } => kleene_and( + filters + .iter() + .map(|inner| target_filter_face_state(face, inner, scope)), + ), + // Negating an unknown stays unknown — never `true`. TargetFilter::Not { filter } => { - !matches_target_filter_against_face_scoped(face, filter, scope) + target_filter_face_state(face, filter, scope).map(|matched| !matched) + } + // Every remaining variant (`SelfRef`, player-scoped forms, event/LKI + // references) needs a live object or resolution context, so a bare face + // yields no answer rather than a definite `false` — otherwise an outer + // `Not` would turn "cannot tell" into "matches". + _ => None, + } +} + +/// Three-valued AND: any definite `false` wins; otherwise unknown poisons. +fn kleene_and(terms: impl IntoIterator>) -> Option { + let mut saw_unknown = false; + for term in terms { + match term { + Some(false) => return Some(false), + Some(true) => {} + None => saw_unknown = true, } - _ => false, } + (!saw_unknown).then_some(true) } -/// CR 109.5: does this filter's controller scope admit a bare face? -fn controller_ref_admits_face( +/// Three-valued OR: any definite `true` wins; otherwise unknown poisons. +fn kleene_or(terms: impl IntoIterator>) -> Option { + let mut saw_unknown = false; + for term in terms { + match term { + Some(true) => return Some(true), + Some(false) => {} + None => saw_unknown = true, + } + } + (!saw_unknown).then_some(false) +} + +/// CR 109.5: how a filter's controller scope reads against a bare face. +/// +/// Unscoped is a definite match. Under `AssumeOwn` the caller has told us the +/// face is the querying player's own card, so `You` is definitely satisfied and +/// `Opponent` is definitely not. Everything else — a controller scope with no +/// declared answer, or any scope at all under `Reject` — is genuinely unknown, +/// NOT false, so that a surrounding `Not` cannot invert it into a match. +fn controller_ref_face_state( controller: Option<&ControllerRef>, scope: FaceControllerScope, -) -> bool { - matches!( - (controller, scope), - (None, _) | (Some(ControllerRef::You), FaceControllerScope::AssumeOwn) - ) +) -> Option { + match (controller, scope) { + (None, _) => Some(true), + (Some(ControllerRef::You), FaceControllerScope::AssumeOwn) => Some(true), + (Some(ControllerRef::Opponent), FaceControllerScope::AssumeOwn) => Some(false), + _ => None, + } } /// CR 205 + CR 202: Evaluate one `FilterProp` against a bare `CardFace`. @@ -1393,12 +1461,14 @@ pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Opt // Recursive combinators inherit their operands' answerability. Negation // of an unknown stays unknown. FilterProp::Not { prop } => context_free_prop_matches_face(face, prop).map(|m| !m), - // CR 608.2b: three-valued (Kleene) OR. A definite `true` in ANY branch - // makes the disjunction true even when a sibling branch is unknowable - // from a face, so this must NOT short-circuit on the first `None` — the - // caller admits only `Some(true)`, and collapsing `[true, unknown]` to - // unknown would reject a spell filter that definitely matches. - // `None` is returned only when nothing is true AND something is unknown. + // Three-valued (Kleene) OR — ordinary Boolean semantics over the + // `Option` answer, not a rules behavior, so no CR governs it. + // A definite `true` in ANY branch makes the disjunction true even when a + // sibling branch is unknowable from a face, so this must NOT + // short-circuit on the first `None`: the caller admits only `Some(true)`, + // and collapsing `[true, unknown]` to unknown would reject a spell filter + // that definitely matches. `None` is returned only when nothing is true + // AND something is unknown. FilterProp::AnyOf { props } => { let mut saw_unknown = false; for inner in props { @@ -12774,7 +12844,7 @@ mod tests { ); } - // ─── CR 608.2b: three-valued OR in `context_free_prop_matches_face` ────── + // ─── three-valued OR in `context_free_prop_matches_face` ───────────────── // // Review #6743: `try_fold` short-circuited on the first `None`, so a // definite `Some(true)` alternative was collapsed to unknown and the typed @@ -12920,4 +12990,155 @@ mod tests { "negating an unknowable property cannot invent an answer" ); } + + // ─── unknown must survive outer negation (fail-closed at every depth) ──── + // + // Review #6743: the recursion collapsed unknown to `false` inside `Typed`, + // so an outer `Not` inverted "cannot tell" into "matches" and admitted the + // wrong card class. These drive the production entry point. + + fn typed_with(props: Vec) -> TargetFilter { + TargetFilter::Typed(TypedFilter { + type_filters: vec![crate::types::ability::TypeFilter::Instant], + properties: props, + ..Default::default() + }) + } + + #[test] + fn not_around_live_only_property_does_not_admit_a_face() { + // `Tapped` needs a battlefield object; `Not(Tapped)` must NOT be a match. + let filter = TargetFilter::Not { + filter: Box::new(typed_with(vec![unknown()])), + }; + assert!( + !matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + ), + "negating an unanswerable property must not admit the face" + ); + } + + #[test] + fn not_around_unknown_controller_scope_does_not_admit_a_face() { + // Under `Reject` the controller is unknown, so `Not(controller-scoped)` + // is unknown too — not a match. + let filter = TargetFilter::Not { + filter: Box::new(TargetFilter::Typed(TypedFilter { + type_filters: vec![crate::types::ability::TypeFilter::Instant], + controller: Some(ControllerRef::You), + ..Default::default() + })), + }; + assert!( + !matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::Reject + ), + "negating an unanswerable controller scope must not admit the face" + ); + } + + #[test] + fn not_around_a_definite_false_still_admits() { + // The fix must not over-correct: a DEFINITELY false inner filter still + // negates to a match. + let filter = TargetFilter::Not { + filter: Box::new(typed_with(vec![known_false()])), + }; + assert!( + matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + ), + "negating a definite non-match must still admit the face" + ); + } + + #[test] + fn not_around_a_definite_true_rejects() { + let filter = TargetFilter::Not { + filter: Box::new(typed_with(vec![known_true()])), + }; + assert!(!matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + )); + } + + #[test] + fn not_around_an_unanswerable_filter_variant_does_not_admit() { + // `SelfRef` needs a resolution context; unknown, so `Not` is unknown. + let filter = TargetFilter::Not { + filter: Box::new(TargetFilter::SelfRef), + }; + assert!( + !matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + ), + "an unanswerable filter variant must stay unknown under negation" + ); + } + + #[test] + fn opponent_scope_under_assume_own_is_definitely_false_not_unknown() { + // `AssumeOwn` states the face is the querying player's own card, so + // "controlled by an opponent" is definitely FALSE — and its negation is + // therefore a definite match. + let opponent = TargetFilter::Typed(TypedFilter { + type_filters: vec![crate::types::ability::TypeFilter::Instant], + controller: Some(ControllerRef::Opponent), + ..Default::default() + }); + assert!(!matches_target_filter_against_face_scoped( + &tri_state_face(), + &opponent, + FaceControllerScope::AssumeOwn + )); + assert!( + matches_target_filter_against_face_scoped( + &tri_state_face(), + &TargetFilter::Not { + filter: Box::new(opponent) + }, + FaceControllerScope::AssumeOwn + ), + "own-face knowledge makes the opponent scope definitely false, so \ + its negation is a definite match" + ); + } + + #[test] + fn and_with_an_unknown_branch_does_not_admit() { + let filter = TargetFilter::And { + filters: vec![typed_with(vec![known_true()]), typed_with(vec![unknown()])], + }; + assert!(!matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + )); + } + + #[test] + fn or_with_a_definite_true_admits_despite_an_unknown_branch() { + let filter = TargetFilter::Or { + filters: vec![typed_with(vec![unknown()]), typed_with(vec![known_true()])], + }; + assert!( + matches_target_filter_against_face_scoped( + &tri_state_face(), + &filter, + FaceControllerScope::AssumeOwn + ), + "a definitely-matching alternative admits even beside an unknown" + ); + } }