Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions crates/phase-ai/src/policies/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,38 @@ impl<'a> PolicyContext<'a> {
}
}

/// First *already-chosen* object target of an in-flight target selection.
///
/// CR 120.1 + CR 120.3: for a `DamageSource::Target` effect ("Target creature
/// deals X damage to ..."), the first object target IS the damage source. In
/// a cast/activation multi-slot selection that source is committed to
/// `selected_slots` before the later recipient slots are offered, so it is
/// resolvable here while the AI is still choosing the recipient — which is
/// what lets the removal-lethality term use the source's power/keywords
/// instead of bailing to `Unresolved`. Returns `None` when the leading slot
/// has not been picked yet or is not an object.
///
/// SCOPE: this reads only the ordinary `WaitingFor::TargetSelection` path
/// (spells/activated abilities). It deliberately returns `None` for
/// `TriggerTargetSelection` (event-bound source, CR 120.7) and for the bulk
/// `MultiTargetSelection` path, because the source there is not resolvable
/// from a single recipient slot — so `DamageSource::Target` in those flow
/// contexts stays `Unresolved` in the removal-lethality term rather than
/// being silently ranked on a guess.
pub fn first_selected_object_target(&self) -> Option<ObjectId> {
let selected_slots = match &self.decision.waiting_for {
WaitingFor::TargetSelection { selection, .. } => Some(&selection.selected_slots),
WaitingFor::TriggerTargetSelection { .. } => None,
_ => None,
};
selected_slots.and_then(|slots| {
slots.iter().find_map(|slot| match slot {
Some(TargetRef::Object(id)) => Some(*id),
_ => None,
})
})
}

pub fn effects(&self) -> Vec<&'a Effect> {
// If we're casting/activating, get effects from the source object
match &self.candidate.action {
Expand Down
192 changes: 190 additions & 2 deletions crates/phase-ai/src/policies/evasion_removal_priority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,8 @@ mod tests {
use engine::game::scenario::{GameScenario, P0};
use engine::game::zones::create_object;
use engine::types::ability::{
AbilityDefinition, AbilityKind, Effect, EffectKind, PtValue, QuantityExpr, ResolvedAbility,
TargetFilter, TargetRef, TypedFilter,
AbilityDefinition, AbilityKind, DamageSource, Effect, EffectKind, PtValue, QuantityExpr,
ResolvedAbility, TargetFilter, TargetRef, TypedFilter,
};
use engine::types::format::FormatConfig;
use engine::types::game_state::{
Expand Down Expand Up @@ -573,6 +573,194 @@ mod tests {
);
}

/// Self-Destruct-style `DamageSource::Target` regression guard for #6582.
///
/// `Self-Destruct` ("Target creature you control deals X damage to any other
/// target and X damage to itself, where X is its power") parses its two
/// damage effects with `DamageSource::Target` — the *targeted creature* is
/// the damage source, not the spell. The removal-lethality term must resolve
/// that source (from the already-chosen first target) so the recipient slot
/// is scored by whether the damage actually destroys the body, exactly as the
/// #6582 fix does for default-sourced burn.
///
/// Before the fix, `removal_lethality` returned `Unresolved` for
/// `DamageSource::Target`, so lethality was inert and the AI ranked the
/// recipient purely by threat value — repeating the #6582 misplay (pointing
/// non-lethal damage at the biggest body it cannot kill) for
/// `Self-Destruct`-style spells. This test drives the PRODUCTION path — a
/// real Self-Destruct cast through `TargetSelection`, then the registered
/// `EvasionRemovalPriorityPolicy` verdict and the `lethality_bonus` it feeds
/// — and asserts the corrected preference for the killable body, pinning the
/// fix to observable behaviour.
#[test]
fn target_sourced_damage_prefers_the_killable_body() {
const SELF_DESTRUCT_ORACLE: &str =
"Target creature you control deals X damage to any other target and X damage to itself, where X is its power.";

let mut scenario = GameScenario::new_n_player(2, 42);
scenario.at_phase(Phase::PreCombatMain);
let self_destruct = scenario
.add_spell_to_hand_from_oracle(P0, "Self-Destruct", true, SELF_DESTRUCT_ORACLE)
.with_mana_cost(ManaCost::Cost {
shards: vec![ManaCostShard::Red],
generic: 1,
})
.id();
// The damage source: the AI's own 2/2, the only "creature you control"
// (and therefore the forced slot-1 target). Its power (2) is the damage
// amount, so 2 damage reaches the recipient.
let bird = scenario.add_creature(P0, "Bird", 2, 2).id();
// The killable recipient — 2 damage destroys a 2/2. Low threat.
let killable = scenario
.add_creature(PlayerId(1), "Scrappy Skirmisher", 1, 2)
.id();
// The unkillable high-threat recipient — 2 damage cannot destroy a 3/3.
let unkillable = scenario
.add_creature(PlayerId(1), "Cloud of Darkness", 3, 3)
.id();
Comment on lines +613 to +620

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The comments and the assertion message describe a 2/2, but the fixture creates a 1/2.

Line 615 creates Scrappy Skirmisher with power 1 and toughness 2. The comment on line 613 says "2 damage destroys a 2/2", and the failure message on lines 716-718 reports "killable 2/2". The arithmetic still holds, because 2 damage is lethal to a 2-toughness body. The labels are wrong and will mislead the next reader who debugs a failure.

Align the text with the fixture, or create a 2/2.

🐛 Proposed fix to align the labels with the fixture
-        // The killable recipient — 2 damage destroys a 2/2. Low threat.
+        // The killable recipient — 2 damage destroys a 1/2. Low threat.
         let killable = scenario
             .add_creature(PlayerId(1), "Scrappy Skirmisher", 1, 2)
             .id();
@@
             "Self-Destruct recipient ranking must prefer the body the 2 damage kills \
-             (killable 2/2) over the 3/3 it only tickles: \
-             killable 2/2={killable_delta}, unkillable 3/3={unkillable_delta}"
+             (killable 1/2) over the 3/3 that survives: \
+             killable 1/2={killable_delta}, unkillable 3/3={unkillable_delta}"

Also applies to: 714-719

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/phase-ai/src/policies/evasion_removal_priority.rs` around lines 613 -
620, Align the `killable` fixture’s descriptive comment and the assertion
failure message near the `killable`/`unkillable` setup with its actual 1/2
stats, including replacing the reported “2/2” label around the relevant
assertion. Keep the fixture values and damage-lethality behavior unchanged.

scenario.with_mana_pool(
P0,
vec![ManaUnit::new(ManaType::Red, ObjectId(0), false, vec![])],
);

let mut runner = scenario.build();
let card_id = runner.state().objects[&self_destruct].card_id;
runner
.act(GameAction::CastSpell {
object_id: self_destruct,
card_id,
targets: Vec::new(),
payment_mode: CastPaymentMode::Auto,
})
.expect("the real Self-Destruct fixture should reach target selection");

// Self-Destruct has two target slots: slot 1 is "creature you control"
// (the source; here the forced Bird), slot 2 is "any other target" (the
// recipient — where the non-lethal-vs-lethal decision actually lives).
// Drive the runner through slot 1 so `ResolvedAbility.targets` carries the
// already-chosen source and the decision context is at the recipient slot.
let first_slot = match &runner.state().waiting_for {
WaitingFor::TargetSelection { target_slots, .. } => &target_slots[0],
other => panic!("expected Self-Destruct target selection, got {other:?}"),
};
assert!(
first_slot.legal_targets.contains(&TargetRef::Object(bird)),
"slot 1 (creature you control) must legally be the Bird source"
);
runner
.act(GameAction::ChooseTarget {
target: Some(TargetRef::Object(bird)),
})
.expect("choosing the Bird for slot 1 should advance to the recipient slot");

let (pending_cast, target_slots, selection) = match &runner.state().waiting_for {
WaitingFor::TargetSelection {
pending_cast,
target_slots,
selection,
..
} => (pending_cast, target_slots, selection),
other => panic!("expected Self-Destruct recipient slot, got {other:?}"),
};
let effects = crate::policies::context::collect_ability_effects(&pending_cast.ability);
assert!(
effects.iter().any(|effect| matches!(
effect,
Effect::DealDamage {
damage_source: Some(DamageSource::Target),
..
}
)),
"reach guard: Self-Destruct must parse as DamageSource::Target damage"
);
assert!(
effects
.iter()
.all(|effect| !matches!(effect, Effect::Unimplemented { .. })),
"the regression fixture must not silently drop an unsupported clause"
);
// The source (Bird, power 2) is ALREADY chosen in slot 1 and locked into
// the selection progress before the recipient slot is presented — so its
// power, and hence the damage amount, is knowable during recipient choice.
assert!(
selection
.selected_slots
.first()
.is_some_and(|slot| *slot == Some(TargetRef::Object(bird))),
"the Bird source must already be chosen (selected_slots[0]) before the \
recipient slot, so its power is knowable"
);
// The recipient slot (slot 2, "any other target") offers both the killable
// 2/2 and the unkillable 3/3.
assert!(target_slots
.iter()
.any(|slot| slot.legal_targets.contains(&TargetRef::Object(killable))));
assert!(target_slots
.iter()
.any(|slot| slot.legal_targets.contains(&TargetRef::Object(unkillable))));

let state = runner.state();
let decision = build_decision_context(state);
let config = create_config(AiDifficulty::VeryHard, Platform::Native).into_measurement(42);

// The #6582 fix now covers `DamageSource::Target`: the lethality term
// resolves the source (the already-chosen Bird, power 2) and scores a
// recipient by whether that damage destroys it. So the registered removal
// policy must rank the KILLABLE 2/2 above the unkillable 3/3 the 2 damage
// cannot destroy — the exact #6582 preference, now extended to
// Self-Destruct-style spells.
let killable_delta = registry_delta(state, &decision, killable, &config);
let unkillable_delta = registry_delta(state, &decision, unkillable, &config);
assert!(
killable_delta > unkillable_delta,
"Self-Destruct recipient ranking must prefer the body the 2 damage kills \
(killable 2/2) over the 3/3 it only tickles: \
killable 2/2={killable_delta}, unkillable 3/3={unkillable_delta}"
);

// And pin the underlying signal directly: the 2-damage source is provably
// lethal to the 2/2 (+LETHAL_BONUS) and non-lethal to the 3/3 (a negative
// waste penalty). This is the exact arithmetic the #6582 fix added for
// default-sourced burn, now extended to the resolved `DamageSource::Target`
// source.
let state_ref = runner.state();
let kil_obj = state_ref.objects.get(&killable).unwrap();
let unk_obj = state_ref.objects.get(&unkillable).unwrap();
let aicontext = crate::context::AiContext::empty(&config.weights);

let lethal_bonus_for =
|target: ObjectId, target_obj: &engine::game::game_object::GameObject| {
let candidate = CandidateAction {
action: GameAction::ChooseTarget {
target: Some(TargetRef::Object(target)),
},
metadata: ActionMetadata::for_actor(Some(P0), TacticalClass::Target),
};
let ctx = crate::policies::context::PolicyContext {
state: state_ref,
decision: &decision,
candidate: &candidate,
ai_player: P0,
config: &config,
context: &aicontext,
cast_facts: None,
search_depth: crate::policies::context::SearchDepth::Root,
};
crate::policies::removal_lethality::lethality_bonus(&ctx, target, target_obj)
};

let killable_bonus = lethal_bonus_for(killable, kil_obj);
let unkillable_bonus = lethal_bonus_for(unkillable, unk_obj);
assert!(
(killable_bonus - crate::policies::removal_lethality::LETHAL_BONUS).abs() < 1e-9,
"the 2-damage source must read as a clean kill on the 2/2, got {killable_bonus}"
);
assert!(
unkillable_bonus < 0.0,
"the 2-damage source must read as a wasted non-lethal on the 3/3, got {unkillable_bonus}"
);
}

#[test]
fn activated_removal_weights_controller_threat_but_beneficial_activation_is_neutral() {
let destroy = Effect::Destroy {
Expand Down
80 changes: 69 additions & 11 deletions crates/phase-ai/src/policies/removal_lethality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@

use engine::game::game_object::GameObject;
use engine::game::keywords::object_has_effective_keyword_kind;
use engine::game::quantity::resolve_quantity;
use engine::types::ability::{DamageSource, Effect};
use engine::game::quantity::{resolve_quantity, resolve_quantity_with_targets_slice};
use engine::types::ability::{DamageSource, Effect, TargetRef};
use engine::types::card_type::CoreType;
use engine::types::identifiers::ObjectId;
use engine::types::keywords::{Keyword, KeywordKind};
Expand Down Expand Up @@ -77,20 +77,44 @@ enum EffectDamageSource {
Object(ObjectId),
/// The source depends on information this policy does not have yet:
///
/// * [`DamageSource::Target`] — the first object target *is* the source and
/// is excluded from the recipient slice
/// (`effects::deal_damage::resolve_effect_recipients`), so the object
/// being scored may be the source rather than a recipient.
/// * [`DamageSource::EachTarget`] — every leading target is an independent
/// source with its own keywords and its own re-resolved amount.
/// * [`DamageSource::TriggeringSource`] — bound to the triggering event's
/// object; the engine's `targeting::extract_source_from_event` authority
/// is crate-private, and re-deriving that mapping in the AI layer would
/// duplicate engine logic.
///
/// [`DamageSource::Target`] is NOT here: its source is the first *already
/// chosen* object target, which the policy can resolve from the in-flight
/// selection, so it resolves to `Object`.
Unresolved,
}

/// CR 120.3: resolve which object deals one `DealDamage` effect's damage.
///
/// `Target` (CR 120.1 + CR 120.3: "that creature deals damage...") has its
/// source bound to the FIRST object target of the ability — the creature chosen
/// in the leading slot, not the spell. That selection is committed to the
/// ongoing `TargetSelectionProgress` *before* the later (recipient) slots are
/// offered, so while a recipient is being chosen the source is already knowable
/// and the lethality of the damage it will deal can be computed (its power for
/// the amount, plus wither/infect/deathtouch from its keywords). Resolving it
/// here is what lets the #6582 lethality term cover `Self-Destruct`-style
/// spells.
///
/// When the first target has not been chosen yet (e.g. the very first slot of a
/// `DamageSource::Target` spell, or a target the engine has not exposed), the
/// result is `Unresolved` and the caller stays neutral rather than guessing.
///
/// SCOPE: this resolves the source only on the ordinary cast/activation
/// `TargetSelection` path. A `DamageSource::Target` effect reached from a
/// triggered ability (`TriggerTargetSelection`, event-bound source per CR 120.7)
/// or from the bulk `MultiTargetSelection` flow stays `Unresolved` — that source
/// is not resolvable from a single recipient slot — so those card classes are
/// not ranked by this term (not a regression; `Target` was always `Unresolved`
/// before). The boundary is stated here (and on
/// [`PolicyContext::first_selected_object_target`]) so the coverable surface is
/// explicit rather than implied.
fn effect_damage_source(
ctx: &PolicyContext<'_>,
damage_source: Option<&DamageSource>,
Expand All @@ -102,7 +126,17 @@ fn effect_damage_source(
.map_or(EffectDamageSource::Unresolved, |object| {
EffectDamageSource::Object(object.id)
}),
Some(DamageSource::Target | DamageSource::EachTarget | DamageSource::TriggeringSource) => {
// CR 120.1 + CR 120.3: "Target creature deals X damage to ..." — the first
// resolved object target is the damage source (see deal_damage.rs, which
// binds `targets[0]` as the source and damages `targets[1..]`). Resolve it
// from the already-chosen leading slot so its power/keywords are known.
Some(DamageSource::Target) => ctx
.first_selected_object_target()
.map_or(EffectDamageSource::Unresolved, EffectDamageSource::Object),
// CR 120.1: multi-source batches (EachTarget: every leading target is an
// independent source) and event-bound sources are not resolvable from the
// recipient slot alone.
Some(DamageSource::EachTarget | DamageSource::TriggeringSource) => {
EffectDamageSource::Unresolved
}
}
Expand Down Expand Up @@ -172,10 +206,34 @@ pub(crate) fn pending_damage_to_object(
return PendingDamage::Unresolved;
};
found = true;
let dealt = u32::try_from(
resolve_quantity(ctx.state, amount, ctx.ai_player, source_id).max(0),
)
.unwrap_or(u32::MAX);
// CR 608.2h + CR 208.1: the damage amount for a
// `DamageSource::Target` effect ("target creature deals X damage,
// where X is its power") reads the SOURCE creature's current power
// when the effect resolves. That is `targets[0]` at resolution
// (`deal_damage.rs` binds the first object target as the source and
// damages `targets[1..]`), so mirror the engine by resolving the
// amount against a targets slice whose first entry is the source.
// Without this, a `Power { scope: Target }` amount reads an empty
// targets list and resolves to 0 — silently scoring a Self-Destruct
// as dealing no damage at all.
let dealt = if matches!(damage_source, Some(DamageSource::Target)) {
u32::try_from(
resolve_quantity_with_targets_slice(
ctx.state,
amount,
ctx.ai_player,
source_id,
&[TargetRef::Object(source_id)],
)
.max(0),
)
.unwrap_or(u32::MAX)
} else {
u32::try_from(
resolve_quantity(ctx.state, amount, ctx.ai_player, source_id).max(0),
)
.unwrap_or(u32::MAX)
};
// CR 120.3d + CR 702.80a + CR 702.90c: wither/infect damage to a
// creature is dealt as -1/-1 counters and is never marked.
if is_creature
Expand Down
Loading
Loading