From 1c828d88879ee52e21e0f16e6f40e4a9c0bcdf35 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 16 Aug 2026 02:43:26 -0700 Subject: [PATCH] test(ai): make the retarget slot-legality rows discriminate in both directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #7477, answering its review. Row 2f proved the per-slot authority REJECTS a pool member legal only for another slot, but could not prove it ADMITS a legal change: its surviving candidate equalled the current slot-0 target, so it passed through `retarget_slot_violation`'s unchanged-position exemption. A generator that dropped every changed proposal would still have passed it. Closing that needs three players, not a third pool member. Slot 0's filter is "an opponent of P0", which at two players admits exactly P1 — and P1 is the current target, so no legal change exists to offer. Row 2g uses a 3-player scenario and, more importantly, keeps the current slot-0 target OUT of the pool, so no exempt non-change is available and the only candidate that can survive is a genuine slot-0-legal change. A drop-guard asserts that absence, so putting the current target back fails the row loudly instead of quietly reverting it to proving half of what it claims. Row 2h pins the `None` contract that #7477 deliberately introduced. Under `Single` scope an empty enumeration means every pool member fails the per-slot check, and `apply_retarget` would reject any submission built from that pool, so the fallback refuses rather than laundering an engine gap into an AI retry loop. Nothing pinned that, so a future change could have silently restored a rejected submission. Its negative assertion carries a positive control that the pool is non-empty, so `None` means "every candidate was filtered out" and never "there was nothing to filter". Verified by perturbation rather than by passing: removing the `slot_legal` filter from the generator's `Single` arm reds all three slot-legality rows and leaves row 2b — which does not test slot legality — green. Also drops a false card attribution the earlier PR left behind because it sat outside that change's frozen scope. Redirect reads "You may choose new targets for target spell", which is the CR 115.7d `RetargetScope::All` template, not "change the target of". Bolt Bend and Misdirection are correct there. --- crates/engine/src/types/ability.rs | 4 +- .../tests/retarget_fallback_action.rs | 181 ++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 4847a14e70..e2889d189b 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -16585,8 +16585,8 @@ impl Effect { | Effect::PutSticker { target, .. } | Effect::ApplySticker { target, .. } | Effect::ProliferateTarget { target, .. } - // CR 115.7 + CR 115.1: "Change the target of target spell or ability" - // (Bolt Bend, Redirect, Misdirection) targets the stack spell/ability + // CR 115.7a + CR 115.1: "Change the target of target spell or ability" + // (Bolt Bend, Misdirection) targets the stack spell/ability // it will retarget. That target is chosen as the spell is cast (CR // 115.1), so it must be surfaced here — both to build the cast-time // target slot and so resolution-time re-validation (CR 608.2b) checks diff --git a/crates/phase-ai/tests/retarget_fallback_action.rs b/crates/phase-ai/tests/retarget_fallback_action.rs index a301422fea..d22fba2367 100644 --- a/crates/phase-ai/tests/retarget_fallback_action.rs +++ b/crates/phase-ai/tests/retarget_fallback_action.rs @@ -29,6 +29,7 @@ use engine::types::game_state::{ }; use engine::types::identifiers::CardId; use engine::types::phase::Phase; +use engine::types::player::PlayerId; use engine::types::zones::Zone; use phase_ai::config::{create_config, AiDifficulty, Platform}; @@ -258,3 +259,183 @@ fn fallback_multi_role_retarget_action_is_slot_legal() { claim of CR-115.7a / CR-115.7b legality; see this row's SCOPE note", ); } + +/// A third player, so slot 0's "an opponent of P0" filter admits MORE THAN ONE +/// player. In a two-player game that filter admits exactly P1, so the only +/// slot-0-legal candidate is necessarily the current target — which survives via +/// the unchanged-position exemption rather than through the admit path. +const P2: PlayerId = PlayerId(2); + +/// Builds the same two-slot mana node row 2f uses, but at an explicit player +/// count and with caller-chosen targets, so a row can put a genuine slot-0 +/// CHANGE in the pool. Slot 0 (recipient) takes an opponent of P0; slot 1 +/// (count source) takes any player. +fn park_multi_role_retarget( + player_count: u8, + current_targets: Vec, + legal_new_targets: Vec, +) -> GameRunner { + let mut runner = GameScenario::new_n_player(player_count, 42).build(); + + let role = ManaTargetRole::Both { + recipient: TargetFilter::Typed(TypedFilter::default().controller(ControllerRef::Opponent)), + count_source: TargetFilter::Player, + }; + let source = create_object( + runner.state_mut(), + CardId(902), + P0, + "Multi-Role Mana Source".to_string(), + Zone::Battlefield, + ); + let entry_id = create_object( + runner.state_mut(), + CardId(902), + P0, + "Multi-Role Mana Ability".to_string(), + Zone::Stack, + ); + let ability = ResolvedAbility::new( + Effect::Mana { + produced: ManaProduction::Colorless { + count: QuantityExpr::Fixed { value: 1 }, + }, + restrictions: vec![], + grants: vec![], + expiry: None, + target: Some(role), + }, + current_targets.clone(), + source, + P0, + ); + runner.state_mut().stack.push_back(StackEntry { + id: entry_id, + source_id: source, + controller: P0, + kind: StackEntryKind::ActivatedAbility { + source_id: source, + ability: Box::new(ability), + }, + }); + + // The same two structural reach-guards row 2f carries: without the entry, + // `retarget_actions`' `is_none_or` passes every candidate unfiltered and + // `apply_retarget` skips its per-slot stage, so any row built here would be + // vacuous in both directions. + assert!( + runner.state().stack[0].ability().is_some(), + "reach guard: stack index 0 must carry the ability under test" + ); + assert!( + mana_multi_role(&runner.state().stack[0].ability().unwrap().effect).is_some(), + "reach guard: the node must be inside the per-slot admitted class" + ); + + runner.state_mut().waiting_for = WaitingFor::RetargetChoice { + player: P0, + stack_entry_index: 0, + scope: RetargetScope::Single, + current_targets, + legal_new_targets, + }; + runner +} + +/// Row 2g — the ADMIT half of the per-slot authority, which row 2f cannot show. +/// +/// Row 2f proves the filter REJECTS a pool member legal only for another slot. +/// It cannot prove the filter ADMITS a legal CHANGE, because its surviving +/// candidate equals the current slot-0 target and therefore passes through +/// `retarget_slot_violation`'s unchanged-position exemption. A generator that +/// dropped every changed proposal would still pass row 2f. +/// +/// This row removes that escape by construction: the current slot-0 target (P1) +/// is deliberately ABSENT from the pool, so no exempt non-change exists and the +/// only candidate that can survive is a genuine, slot-0-legal CHANGE. +#[test] +fn fallback_multi_role_retarget_admits_a_legal_slot_change() { + // Slot 0 currently holds P1. Pool offers P0 (illegal for slot 0 — P0 is not + // its own opponent) and P2 (legal for slot 0, and a real change). + let runner = park_multi_role_retarget( + 3, + vec![TargetRef::Player(P1), TargetRef::Player(P0)], + vec![TargetRef::Player(P0), TargetRef::Player(P2)], + ); + + // Drop-guard: the pool must genuinely exclude the current slot-0 target, or + // the exemption is back and this row degenerates into row 2f. + let WaitingFor::RetargetChoice { + current_targets, + legal_new_targets, + .. + } = runner.state().waiting_for.clone() + else { + panic!("fixture must park a RetargetChoice"); + }; + assert!( + !legal_new_targets.contains(¤t_targets[0]), + "drop-guard: the current slot-0 target must be ABSENT from the pool, or the \ + surviving candidate would be an exempt non-change; got {legal_new_targets:?}" + ); + + let action = fallback_for_prompt(&runner); + + assert!(action.is_some(), "reach guard: the fallback must answer"); + + // Discriminating, ADMIT side: the surviving proposal is the slot-0-legal + // CHANGE. Under a generator that dropped changed proposals this is `None`; + // under one that ignored slot legality it would be `[P0]`. + assert_eq!( + action, + Some(GameAction::RetargetSpell { + new_targets: vec![TargetRef::Player(P2)], + }), + "CR 115.7a: the fallback must propose the slot-0-legal CHANGE, and must not \ + propose the pool member legal only for the count-source slot" + ); +} + +/// Row 2h — the `None` contract this layer deliberately introduces. +/// +/// `fallback_action`'s retarget arm returns `None` when the engine's enumeration +/// is empty, and that refusal is deliberate: under `Single` scope an empty +/// enumeration means every pool member fails the per-slot check, and +/// `apply_retarget`'s `Single` arm would reject any submission built from that +/// pool. Returning a knowingly-rejected action instead would launder an engine +/// gap into an AI retry loop. +/// +/// Nothing pinned that contract, so a future change could silently restore a +/// rejected submission and no test would notice. This row pins it. +#[test] +fn fallback_multi_role_retarget_yields_none_when_no_pool_member_is_slot_legal() { + // Slot 0 holds P1 and the pool offers only P0, which is illegal for slot 0. + // P1 is absent, so there is no exempt non-change to fall back on either. + let runner = park_multi_role_retarget( + 3, + vec![TargetRef::Player(P1), TargetRef::Player(P0)], + vec![TargetRef::Player(P0)], + ); + + // Positive control for a negative assertion: the prompt really is parked and + // its pool really is non-empty, so a `None` below means "every candidate was + // filtered out", never "there was nothing to filter" or "no prompt existed". + let WaitingFor::RetargetChoice { + legal_new_targets, .. + } = runner.state().waiting_for.clone() + else { + panic!("positive control: the fixture must park a RetargetChoice"); + }; + assert!( + !legal_new_targets.is_empty(), + "positive control: the pool must be NON-empty, or `None` proves nothing about \ + the per-slot filter" + ); + + assert_eq!( + fallback_for_prompt(&runner), + None, + "the retarget arm must refuse rather than submit an action `apply_retarget` \ + would reject; see the DEFERRED(out-of-run) note in `phase-ai/src/search.rs`" + ); +}