From 59195514e9cbffbeec0bb602a915fe102677d70d Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 11:35:50 -0700 Subject: [PATCH 1/2] feat(phase-ai): pick the least-costly land for a self-bounce, never the loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #4730. A "return a land you control" ETB (Azorius Chancery and the rest of the Ravnica/MOM bounce-land cycle) forces the controller to return one of their own lands at resolution (CR 608.2c). No policy scored that choice, so the AI kept returning the bounce-land it had just played — replaying and re-bouncing it every turn, the tempo loop reported in #4730. `land_sequencing` fixed the play-sequencing half and explicitly deferred this half to "its own target-selection policy"; this is that policy. New `policies/self_bounce_target.rs` (SelfBounceTargetPolicy): - Fires only on the resolution-time land bounce — a WaitingFor::EffectZoneChoice returning battlefield lands you control to hand — leaving value self-bounce of creatures (blink, save-from- removal) to the value policies. Structural, no card names. - Scores each candidate land by how little returning it costs: the ability's own source (the just-played bounce-land) takes a strong penalty so it loses to any other land (CR 608.2c loop guard); an already-tapped land is preferred (its mana is spent this turn); an untapped land is mildly penalized but still beats looping the source. Covers the whole bounce-land class, not one card. Tests: 4 new self-bounce tests (pure return_desirability ordering + composed verdict over a real EffectZoneChoice/SelectCards context) + full 1537-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/policies/mod.rs | 1 + crates/phase-ai/src/policies/registry.rs | 3 + .../src/policies/self_bounce_target.rs | 146 ++++++++++++ crates/phase-ai/src/policies/tests/mod.rs | 1 + .../src/policies/tests/self_bounce_target.rs | 213 ++++++++++++++++++ 5 files changed, 364 insertions(+) create mode 100644 crates/phase-ai/src/policies/self_bounce_target.rs create mode 100644 crates/phase-ai/src/policies/tests/self_bounce_target.rs diff --git a/crates/phase-ai/src/policies/mod.rs b/crates/phase-ai/src/policies/mod.rs index ca00c1c30d..fc7c97a618 100644 --- a/crates/phase-ai/src/policies/mod.rs +++ b/crates/phase-ai/src/policies/mod.rs @@ -50,6 +50,7 @@ mod redundancy_avoidance; pub mod registry; mod sacrifice_land_protection; mod sacrifice_value; +mod self_bounce_target; mod self_cost; mod self_cost_value; mod self_protection_classify; diff --git a/crates/phase-ai/src/policies/registry.rs b/crates/phase-ai/src/policies/registry.rs index 468e2eac14..164fe97a34 100644 --- a/crates/phase-ai/src/policies/registry.rs +++ b/crates/phase-ai/src/policies/registry.rs @@ -142,6 +142,8 @@ pub enum PolicyId { GraveyardTypes, CrewTiming, CombatWithdrawal, + /// CR 608.2c: "return a land you control" self-bounce target choice. + SelfBounceTarget, } /// Coarse routing kind for a candidate decision. Each policy declares which @@ -396,6 +398,7 @@ impl Default for PolicyRegistry { Box::new(PayoffPolicy::new(&REANIMATOR_PAYOFF)), Box::new(PayoffPolicy::new(&BLINK_PAYOFF)), Box::new(LoopShortcutPolicy), + Box::new(super::self_bounce_target::SelfBounceTargetPolicy), ]; let mut by_kind: HashMap> = HashMap::new(); for (idx, policy) in policies.iter().enumerate() { diff --git a/crates/phase-ai/src/policies/self_bounce_target.rs b/crates/phase-ai/src/policies/self_bounce_target.rs new file mode 100644 index 0000000000..fc0ffb7860 --- /dev/null +++ b/crates/phase-ai/src/policies/self_bounce_target.rs @@ -0,0 +1,146 @@ +//! Self-bounce target selection — when a "return a land you control" ETB forces +//! the AI to return one of its own lands, pick the least-costly one and NEVER +//! the land that just entered (which loops). +//! +//! ## The defect this closes (#4730) +//! +//! Azorius Chancery and the rest of the Ravnica/MOM bounce-land ("Karoo") cycle +//! enter and their ETB returns "a land you control" to hand (CR 608.2c, chosen +//! at resolution). No policy scored that choice, so the AI kept returning the +//! bounce-land it had just played — putting it back in hand to be replayed and +//! bounced again, the tempo loop the reporter observed. `land_sequencing` fixed +//! the *sequencing* half and its own doc-comment deferred this half verbatim: +//! "the AI should return the least-useful land, never the just-played +//! bounce-land — that needs its own target-selection policy." +//! +//! ## Heuristic (building block, not a card fix) +//! +//! The choice surfaces as a `WaitingFor::EffectZoneChoice` returning battlefield +//! lands you control to hand; each candidate is a `SelectCards` selection. +//! Detection is structural (no card names) — it covers every "return a land you +//! control" bouncer. Each land in the selection is scored by how little +//! returning it costs: +//! * the ability's own source — the just-played bounce-land — takes a strong +//! penalty, so it is chosen only when it is the sole eligible land (which +//! breaks the loop); +//! * an already-tapped land is preferred (its mana is spent this turn, so +//! replaying it next turn costs the least tempo); +//! * an untapped land is mildly penalized (returning it forfeits mana still +//! available this turn) but still beats looping the bounce-land. + +use engine::types::ability::EffectKind; +use engine::types::actions::GameAction; +use engine::types::card_type::CoreType; +use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::identifiers::ObjectId; +use engine::types::player::PlayerId; +use engine::types::zones::Zone; + +use crate::features::DeckFeatures; + +use super::context::PolicyContext; +use super::registry::{DecisionKind, PolicyId, PolicyReason, PolicyVerdict, TacticalPolicy}; + +/// Penalty for returning the ability's own source — the just-played bounce-land. +/// Returning it re-buys the land drop for nothing and loops, so it must lose to +/// any other land the pool offers. +pub(crate) const RETURN_SOURCE_PENALTY: f64 = -3.0; +/// Bonus for returning an already-tapped land: its mana is spent this turn, so +/// replaying it next turn costs the least tempo. +pub(crate) const RETURN_TAPPED_BONUS: f64 = 1.0; +/// Penalty for returning an untapped land: doing so forfeits mana still +/// available this turn. Milder than the source penalty — an untapped non-source +/// land is still a better return than looping the bounce-land. +pub(crate) const RETURN_UNTAPPED_PENALTY: f64 = -0.5; + +pub struct SelfBounceTargetPolicy; + +impl SelfBounceTargetPolicy { + /// Desirability of returning `land_id` to hand; higher = better to bounce. + pub(crate) fn return_desirability( + state: &GameState, + land_id: ObjectId, + source_id: ObjectId, + ) -> f64 { + // CR 608.2c: never bounce the permanent that generated the choice — that + // is the tempo loop this policy exists to break. + if land_id == source_id { + return RETURN_SOURCE_PENALTY; + } + match state.objects.get(&land_id) { + Some(land) if land.tapped => RETURN_TAPPED_BONUS, + Some(_) => RETURN_UNTAPPED_PENALTY, + None => 0.0, + } + } +} + +impl TacticalPolicy for SelfBounceTargetPolicy { + fn id(&self) -> PolicyId { + PolicyId::SelfBounceTarget + } + + fn decision_kinds(&self) -> &'static [DecisionKind] { + // `WaitingFor::EffectZoneChoice` routes through the ActivateAbility + // catch-all bucket (see `decision_kind::classify`). + &[DecisionKind::ActivateAbility] + } + + fn activation( + &self, + _features: &DeckFeatures, + _state: &GameState, + _player: PlayerId, + ) -> Option { + // Universal; the verdict's bounce-choice guard self-gates. + // activation-constant: self-bounce target choice, universal. + Some(1.0) + } + + fn verdict(&self, ctx: &PolicyContext<'_>) -> PolicyVerdict { + let na = || PolicyVerdict::neutral(PolicyReason::new("self_bounce_target_na")); + + let GameAction::SelectCards { cards } = &ctx.candidate.action else { + return na(); + }; + // CR 608.2c: a resolution-time "return a land you control to hand" + // choice — from the battlefield, to hand, scoped to the AI. + let WaitingFor::EffectZoneChoice { + player, + source_id, + effect_kind: EffectKind::ChangeZone, + zone: Zone::Battlefield, + destination: Some(Zone::Hand), + cards: pool, + .. + } = &ctx.decision.waiting_for + else { + return na(); + }; + if *player != ctx.ai_player { + return na(); + } + // Restrict to the "return a LAND you control" class: every eligible + // object is a land the AI controls. A value self-bounce of a creature + // (blink, save-from-removal) is intentionally left untouched. + let all_own_lands = pool.iter().all(|id| { + ctx.state.objects.get(id).is_some_and(|o| { + o.controller == ctx.ai_player && o.card_types.core_types.contains(&CoreType::Land) + }) + }); + if pool.is_empty() || !all_own_lands || cards.is_empty() { + return na(); + } + + let delta: f64 = cards + .iter() + .map(|&id| Self::return_desirability(ctx.state, id, *source_id)) + .sum(); + + PolicyVerdict::score( + delta, + PolicyReason::new("self_bounce_target") + .with_fact("returns_source", i64::from(cards.contains(source_id))), + ) + } +} diff --git a/crates/phase-ai/src/policies/tests/mod.rs b/crates/phase-ai/src/policies/tests/mod.rs index e4db8735e5..910fb9df30 100644 --- a/crates/phase-ai/src/policies/tests/mod.rs +++ b/crates/phase-ai/src/policies/tests/mod.rs @@ -15,3 +15,4 @@ pub mod mulligan_input_lint; pub mod poison; pub mod reanimator_payoff; pub mod score_contract_lint; +pub mod self_bounce_target; diff --git a/crates/phase-ai/src/policies/tests/self_bounce_target.rs b/crates/phase-ai/src/policies/tests/self_bounce_target.rs new file mode 100644 index 0000000000..a963d13742 --- /dev/null +++ b/crates/phase-ai/src/policies/tests/self_bounce_target.rs @@ -0,0 +1,213 @@ +//! Unit tests for `policies::self_bounce_target` — CR 608.2c "return a land you +//! control" self-bounce target choice (#4730). No `#[cfg(test)]` in SOURCE +//! files; tests live here. +//! +//! `return_desirability` is the pure core (source-loop guard + tapped/untapped +//! tempo ordering); the composed `verdict` runs against a real `PolicyContext` +//! built over a `WaitingFor::EffectZoneChoice` + `SelectCards` candidate, the +//! seam the engine surfaces for a non-targeted battlefield→hand land bounce. + +use engine::ai_support::{ActionMetadata, AiDecisionContext, CandidateAction, TacticalClass}; +use engine::game::zones::create_object; +use engine::types::ability::EffectKind; +use engine::types::actions::GameAction; +use engine::types::card_type::{CardType, CoreType}; +use engine::types::format::FormatConfig; +use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::identifiers::{CardId, ObjectId}; +use engine::types::player::PlayerId; +use engine::types::zones::{EtbTapState, Zone}; + +use crate::config::AiConfig; +use crate::context::AiContext; +use crate::policies::context::{PolicyContext, SearchDepth}; +use crate::policies::registry::{PolicyVerdict, TacticalPolicy}; +use crate::policies::self_bounce_target::*; + +const AI: PlayerId = PlayerId(0); + +fn state() -> GameState { + GameState::new(FormatConfig::standard(), 2, 42) +} + +// ─── return_desirability (pure core) ──────────────────────────────────────── + +#[test] +fn source_ranks_below_every_other_land() { + let mut st = state(); + let source = land(&mut st, 1, true); // Karoo enters tapped + let tapped = land(&mut st, 2, true); + let untapped = land(&mut st, 3, false); + + let d_source = SelfBounceTargetPolicy::return_desirability(&st, source, source); + let d_tapped = SelfBounceTargetPolicy::return_desirability(&st, tapped, source); + let d_untapped = SelfBounceTargetPolicy::return_desirability(&st, untapped, source); + + assert_eq!(d_source, RETURN_SOURCE_PENALTY); + assert_eq!(d_tapped, RETURN_TAPPED_BONUS); + assert_eq!(d_untapped, RETURN_UNTAPPED_PENALTY); + // The whole point of #4730: the just-played bounce-land is the worst return, + // and a spent (tapped) land is the best. + assert!( + d_source < d_untapped && d_untapped < d_tapped, + "ordering source < untapped < tapped, got {d_source} {d_untapped} {d_tapped}" + ); +} + +// ─── verdict (composed over EffectZoneChoice) ─────────────────────────────── + +#[test] +fn verdict_prefers_a_spent_land_over_the_bounce_source() { + // Pool: the just-played Karoo (source, tapped) + a spent basic + an untapped + // basic. Returning the source must score lowest, the tapped basic highest. + let d_source = verdict_delta(&[(true, true), (false, true), (false, false)], &[0]); + let d_tapped = verdict_delta(&[(true, true), (false, true), (false, false)], &[1]); + let d_untapped = verdict_delta(&[(true, true), (false, true), (false, false)], &[2]); + assert_eq!(d_source, RETURN_SOURCE_PENALTY); + assert_eq!(d_tapped, RETURN_TAPPED_BONUS); + assert_eq!(d_untapped, RETURN_UNTAPPED_PENALTY); + assert!( + d_source < d_untapped && d_untapped < d_tapped, + "AI must rank the just-played bounce-land last" + ); +} + +#[test] +fn verdict_is_neutral_for_a_non_land_pool() { + // A value self-bounce of creatures (blink / save-from-removal) must be left + // to the value policies — this policy only governs land bounces. + assert_eq!( + verdict_delta_kinds( + &[(true, true), (false, true)], + &[1], + false, + Some(Zone::Hand), + EffectKind::ChangeZone + ), + 0.0 + ); +} + +#[test] +fn verdict_is_neutral_for_non_hand_destination() { + // Battlefield→exile / other ChangeZone choices are not the "return to hand" + // bounce class. + assert_eq!( + verdict_delta_kinds( + &[(true, true), (false, true)], + &[1], + true, + Some(Zone::Exile), + EffectKind::ChangeZone + ), + 0.0 + ); +} + +// ─── helpers ──────────────────────────────────────────────────────────────── + +fn land(state: &mut GameState, idx: u64, tapped: bool) -> ObjectId { + make_object(state, idx, true, tapped) +} + +fn make_object(state: &mut GameState, idx: u64, is_land: bool, tapped: bool) -> ObjectId { + let oid = create_object( + state, + CardId(idx), + AI, + format!("Perm {idx}"), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&oid).unwrap(); + obj.card_types = CardType { + supertypes: Vec::new(), + core_types: vec![if is_land { + CoreType::Land + } else { + CoreType::Creature + }], + subtypes: Vec::new(), + }; + obj.tapped = tapped; + oid +} + +/// Build the land pool (index 0 is the bounce source) and score the given +/// selection under a battlefield→hand `ChangeZone` land bounce. +fn verdict_delta(pool_spec: &[(bool, bool)], selection: &[usize]) -> f64 { + verdict_delta_kinds( + pool_spec, + selection, + true, + Some(Zone::Hand), + EffectKind::ChangeZone, + ) +} + +/// `pool_spec[i] = (is_source_placeholder_unused, tapped)`; index 0 is always +/// the ability source. `lands` toggles land vs creature pool objects. +fn verdict_delta_kinds( + pool_spec: &[(bool, bool)], + selection: &[usize], + lands: bool, + destination: Option, + effect_kind: EffectKind, +) -> f64 { + let mut st = state(); + let pool: Vec = pool_spec + .iter() + .enumerate() + .map(|(i, &(_, tapped))| make_object(&mut st, 100 + i as u64, lands, tapped)) + .collect(); + let source_id = pool[0]; + let selected: Vec = selection.iter().map(|&i| pool[i]).collect(); + + let decision = AiDecisionContext { + waiting_for: WaitingFor::EffectZoneChoice { + player: AI, + cards: pool, + count: 1, + min_count: 1, + up_to: false, + source_id, + effect_kind, + zone: Zone::Battlefield, + destination, + enter_tapped: EtbTapState::Unspecified, + enter_transformed: false, + enters_under_player: None, + enters_attacking: false, + owner_library: false, + track_exiled_by_source: false, + face_down_profile: None, + enter_with_counters: Vec::new(), + conditional_enter_with_counters: Vec::new(), + count_param: 0, + library_position: None, + is_cost_payment: false, + enters_modified_if: None, + duration: None, + }, + candidates: Vec::new(), + }; + let candidate = CandidateAction { + action: GameAction::SelectCards { cards: selected }, + metadata: ActionMetadata::for_actor(Some(AI), TacticalClass::Ability), + }; + let config = AiConfig::default(); + let aicontext = AiContext::empty(&config.weights); + let ctx = PolicyContext { + state: &st, + decision: &decision, + candidate: &candidate, + ai_player: AI, + config: &config, + context: &aicontext, + cast_facts: None, + search_depth: SearchDepth::Root, + }; + match SelfBounceTargetPolicy.verdict(&ctx) { + PolicyVerdict::Score { delta, .. } => delta, + PolicyVerdict::Reject { reason } => panic!("unexpected Reject: {reason:?}"), + } +} From 56424aeacd6ce373d435edaa52242182d3e28bf1 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Sun, 26 Jul 2026 12:01:30 -0700 Subject: [PATCH 2/2] test(phase-ai): add registry-routed regression for the self-bounce policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review (#6679): the verdict tests called SelfBounceTargetPolicy directly, so they would stay green if the policy were dropped from PolicyRegistry or the EffectZoneChoice decision-kind routing changed — they did not protect the shipped seam. Adds, following the poison policy's production-seam pattern: - registry_registers_the_policy: PolicyRegistry::default() contains PolicyId::SelfBounceTarget. - registry_routes_the_land_bounce_ordering: routes each SelectCards selection through PolicyRegistry::verdicts (classify → filter by DecisionKind → activation → verdict) and asserts the production ordering tapped-other > untapped-other > source. The shared `eval` helper gained a `route` flag so the direct and routed paths build the identical EffectZoneChoice context. cargo test -p phase-ai --lib self_bounce — 6 pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 --- .../src/policies/tests/self_bounce_target.rs | 76 +++++++++++++++++-- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/crates/phase-ai/src/policies/tests/self_bounce_target.rs b/crates/phase-ai/src/policies/tests/self_bounce_target.rs index a963d13742..bf51f2aab1 100644 --- a/crates/phase-ai/src/policies/tests/self_bounce_target.rs +++ b/crates/phase-ai/src/policies/tests/self_bounce_target.rs @@ -21,7 +21,7 @@ use engine::types::zones::{EtbTapState, Zone}; use crate::config::AiConfig; use crate::context::AiContext; use crate::policies::context::{PolicyContext, SearchDepth}; -use crate::policies::registry::{PolicyVerdict, TacticalPolicy}; +use crate::policies::registry::{PolicyId, PolicyRegistry, PolicyVerdict, TacticalPolicy}; use crate::policies::self_bounce_target::*; const AI: PlayerId = PlayerId(0); @@ -133,7 +133,8 @@ fn make_object(state: &mut GameState, idx: u64, is_land: bool, tapped: bool) -> } /// Build the land pool (index 0 is the bounce source) and score the given -/// selection under a battlefield→hand `ChangeZone` land bounce. +/// selection under a battlefield→hand `ChangeZone` land bounce, via a direct +/// `verdict` call. fn verdict_delta(pool_spec: &[(bool, bool)], selection: &[usize]) -> f64 { verdict_delta_kinds( pool_spec, @@ -144,8 +145,7 @@ fn verdict_delta(pool_spec: &[(bool, bool)], selection: &[usize]) -> f64 { ) } -/// `pool_spec[i] = (is_source_placeholder_unused, tapped)`; index 0 is always -/// the ability source. `lands` toggles land vs creature pool objects. +/// Direct-call variant; always `Some` (panics on `Reject`). fn verdict_delta_kinds( pool_spec: &[(bool, bool)], selection: &[usize], @@ -153,6 +153,36 @@ fn verdict_delta_kinds( destination: Option, effect_kind: EffectKind, ) -> f64 { + eval(pool_spec, selection, lands, destination, effect_kind, false) + .expect("direct verdict always returns a score") +} + +/// The score for `selection` as the **production registry** produces it — +/// classify → filter by `DecisionKind` → run `activation` → `verdict`. `None` +/// when `SelfBounceTargetPolicy` did not run at all (unregistered, or the +/// `EffectZoneChoice` no longer routes to its declared kind). +fn routed_delta(pool_spec: &[(bool, bool)], selection: &[usize]) -> Option { + eval( + pool_spec, + selection, + true, + Some(Zone::Hand), + EffectKind::ChangeZone, + true, + ) +} + +/// `pool_spec[i] = (unused, tapped)`; index 0 is always the ability source. +/// `lands` toggles land vs creature pool objects. `route` runs the full +/// `PolicyRegistry` instead of calling the policy directly. +fn eval( + pool_spec: &[(bool, bool)], + selection: &[usize], + lands: bool, + destination: Option, + effect_kind: EffectKind, + route: bool, +) -> Option { let mut st = state(); let pool: Vec = pool_spec .iter() @@ -206,8 +236,42 @@ fn verdict_delta_kinds( cast_facts: None, search_depth: SearchDepth::Root, }; - match SelfBounceTargetPolicy.verdict(&ctx) { - PolicyVerdict::Score { delta, .. } => delta, + + let verdict = if route { + PolicyRegistry::default() + .verdicts(&ctx) + .into_iter() + .find(|(id, _)| *id == PolicyId::SelfBounceTarget) + .map(|(_, verdict)| verdict)? + } else { + SelfBounceTargetPolicy.verdict(&ctx) + }; + match verdict { + PolicyVerdict::Score { delta, .. } => Some(delta), PolicyVerdict::Reject { reason } => panic!("unexpected Reject: {reason:?}"), } } + +// ─── production seam (registry routing) ───────────────────────────────────── + +#[test] +fn registry_registers_the_policy() { + assert!(PolicyRegistry::default().has_policy(PolicyId::SelfBounceTarget)); +} + +/// End-to-end routing: `WaitingFor::EffectZoneChoice` classifies to +/// `DecisionKind::ActivateAbility`, the policy declares that kind, and the three +/// land selections come out ordered tapped-other > untapped-other > source. A +/// direct-`verdict` probe would stay green even if the policy were dropped from +/// the registry or the routing changed; this asserts the shipped seam. +#[test] +fn registry_routes_the_land_bounce_ordering() { + let pool = &[(true, true), (false, true), (false, false)]; + let source = routed_delta(pool, &[0]).expect("source selection must reach the policy"); + let tapped = routed_delta(pool, &[1]).expect("tapped selection must reach the policy"); + let untapped = routed_delta(pool, &[2]).expect("untapped selection must reach the policy"); + assert!( + source < untapped && untapped < tapped, + "routed ordering source < untapped < tapped, got {source} {untapped} {tapped}" + ); +}