fix(ai): rank DamageSource::Target removal by the source's lethality … - #6852
fix(ai): rank DamageSource::Target removal by the source's lethality …#6852CodeOptimist wants to merge 3 commits into
Conversation
…hase-rs#6582 class) The phase-rs#6582 burn-lethality term in removal_lethality stopped the AI wasting default-sourced burn on bodies it cannot kill, but it returned Unresolved for DamageSource::Target damage — Self-Destruct and every "Target creature deals X damage to ..." card. During the recipient slot of a multi-slot selection the damage source is already committed to selected_slots, so its power and wither/infect/deathtouch keywords are knowable. Resolving it there lets the recipient be ranked by whether the damage actually destroys the body instead of by raw threat value. Changes (all in phase-ai): - policies/context.rs: PolicyContext::first_selected_object_target() reads the already-chosen first object target (the damage source) from the in-flight TargetSelectionProgress (CR 120.1 + CR 120.3). - policies/removal_lethality.rs: DamageSource::Target now resolves its source from that leading slot (EachTarget / TriggeringSource stay Unresolved — genuinely not resolvable from one recipient slot), and the amount is resolved against a [source] target slice so a Power{Target} amount reads the source's current power instead of silently 0 (CR 608.2h + CR 208.1). Scope boundary is explicit: a DamageSource::Target reached from a triggered ability or the bulk MultiTargetSelection flow stays Unresolved (source not resolvable from a single recipient slot). - policies/tests/removal_lethality.rs: unit tests resolving a pre-chosen power source and a pre-chosen deathtouch source. - policies/evasion_removal_priority.rs: an end-to-end regression driving real Self-Destruct through cast -> TargetSelection -> the registered EvasionRemovalPriorityPolicy verdict, asserting the killable recipient outranks the unkillable one and pinning the lethality arithmetic. Verification: cargo fmt clean; cargo clippy --all-targets -- -D warnings clean; full phase-ai lib suite 1982 passed / 0 failed / 8 ignored. The Self-Destruct e2e test was confirmed discriminating: reverting the production change flips it to fail with the exact phase-rs#6582 misplay (killable 2/2=0.644 < unkillable 3/3=1.38). An independent fresh-context review-impl pass returned no HIGH/MED findings and two LOW findings, both addressed with code: the damage-amount CR citation corrected from 608.2c to CR 608.2h, and the coverable surface made explicit for triggered-ability / MultiTargetSelection DamageSource::Target cases. Comment-only, re-verified fmt/clippy/tests clean. Note: this is scoped to recipient ranking for DamageSource::Target spells (the precise phase-rs#6582 scope). It does not address the separate question of whether the AI should cast a self-harming spell at all, which is handled by engine::ai_support::targeted_exchange_verdict (phase-rs#6826) at the root-cast gate.
📝 WalkthroughWalkthroughThe AI removal-lethality policies now resolve ChangesTarget-sourced damage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/phase-ai/src/policies/tests/removal_lethality.rs (1)
660-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the wither/infect source and for the unresolved-source boundary.
The new tests cover marked damage and deathtouch. Two branches that this change touches stay untested:
pending_damage_to_objectroutes damage intominus_counterswhen the RESOLVED source has wither or infect. Before this change the source of aDamageSource::Targeteffect was never resolved, so this branch was unreachable for that class. A test with a wither or infect source and a non-zerominus_countersexpectation would pin it.- The documented
Unresolvedboundary has no test. Add cases whereselected_slotsis empty, where the leading slot isSome(TargetRef::Player(_)), and wherewaiting_forisTriggerTargetSelection. Each must producePendingDamage::Unresolvedand alethality_bonusof0.0.Case 2 also pins the contract that
first_selected_object_targetcurrently does not honor. See the comment oncrates/phase-ai/src/policies/context.rs.
with_pending_sourcealready acceptssource_deathtouch: bool. Prefer a keyword list parameter over adding a secondbool, so the fixture stays a parameterized building block.♻️ Proposed fixture signature change
fn with_pending_source<R>( source_power: i32, - source_deathtouch: bool, + source_keywords: &[Keyword], body: Body, probe: impl FnOnce(&PolicyContext<'_>, ObjectId, &GameObject) -> R, ) -> R { @@ obj.power = Some(source_power); obj.toughness = Some(3); - if source_deathtouch { - obj.keywords.push(Keyword::Deathtouch); - obj.base_keywords.push(Keyword::Deathtouch); - } + obj.keywords.extend(source_keywords.iter().cloned()); + obj.base_keywords.extend(source_keywords.iter().cloned()); }As per path instructions for
crates/phase-ai/**and the CLAUDE.md guidance to "add focused building-block and regression tests for source selection, target-slice quantity resolution, power, wither/infect, and deathtouch lethality".🤖 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/tests/removal_lethality.rs` around lines 660 - 731, Extend the tests around with_pending_source, changing its source keyword input to a parameterized keyword list, and add coverage for wither/infect sources asserting non-zero minus_counters. Add unresolved-source cases for empty selected_slots, a leading player target, and TriggerTargetSelection waiting_for; each must return PendingDamage::Unresolved with lethality_bonus(ctx, id, target) equal to 0.0, while preserving the existing marked-damage and deathtouch assertions.Source: Path instructions
crates/phase-ai/src/policies/evasion_removal_priority.rs (1)
695-700: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert legality for the active recipient slot.
After selecting
bird, assertselection.current_slot == 1and assert both bodies are inselection.current_legal_targets.target_slots.iter().any(...)does not prove legality for the live slot.🤖 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 695 - 700, Update the assertions after selecting bird to verify selection.current_slot == 1, then assert both killable and unkillable bodies are present in selection.current_legal_targets. Replace the target_slots.iter().any checks so legality is validated specifically for the active recipient slot.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/phase-ai/src/policies/evasion_removal_priority.rs`:
- Around line 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.
---
Nitpick comments:
In `@crates/phase-ai/src/policies/evasion_removal_priority.rs`:
- Around line 695-700: Update the assertions after selecting bird to verify
selection.current_slot == 1, then assert both killable and unkillable bodies are
present in selection.current_legal_targets. Replace the target_slots.iter().any
checks so legality is validated specifically for the active recipient slot.
In `@crates/phase-ai/src/policies/tests/removal_lethality.rs`:
- Around line 660-731: Extend the tests around with_pending_source, changing its
source keyword input to a parameterized keyword list, and add coverage for
wither/infect sources asserting non-zero minus_counters. Add unresolved-source
cases for empty selected_slots, a leading player target, and
TriggerTargetSelection waiting_for; each must return PendingDamage::Unresolved
with lethality_bonus(ctx, id, target) equal to 0.0, while preserving the
existing marked-damage and deathtouch assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 77cf35cc-01a4-4a9c-ac55-1cfc59d0897a
📒 Files selected for processing (4)
crates/phase-ai/src/policies/context.rscrates/phase-ai/src/policies/evasion_removal_priority.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/src/policies/tests/removal_lethality.rs
| // 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(); |
There was a problem hiding this comment.
📐 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.
|
I merged current The source review of the policy change found the target-source binding and target-slice quantity resolution aligned with the engine's |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head d1c1a03a027cc9255b73aa1b83d096ffe9232641.
[MED] Target-sourced damage loses the engine's anaphoric quantity binding. Evidence: crates/phase-ai/src/policies/removal_lethality.rs:219-227 calls resolve_quantity_with_targets_slice, which has no ResolvedAbility; crates/engine/src/game/quantity.rs:5448-5499 needs that ability to resolve ObjectScope::Anaphoric for one-sided-fight damage. Why it matters: DamageSource::Target recipient selection with an anaphoric amount, such as the distinct-recipient one-sided-fight class preserved by crates/engine/src/parser/oracle_effect/mod.rs:19363-19427, resolves to zero in this policy and remains silently unranked. Suggested fix: carry the pending ResolvedAbility into the policy quantity resolution, or otherwise preserve the engine's quantity context, and add a production target-selection regression that would fail if that binding is removed.
[LOW] The Self-Destruct regression prose does not match its fixture. Evidence: crates/phase-ai/src/policies/evasion_removal_priority.rs:613-718 creates a 1/2 Scrappy Skirmisher but describes and reports it as a 2/2. Why it matters: failure output misstates the scenario being debugged. Suggested fix: align the comments and assertion message with the fixture (or change the fixture deliberately).
2 participants
Human intro
My first agentic coding experience... 🍾
Kind of a test of sentdex's recent assertion that DeepSeek is actually really quite good and that its speed and cost makes it more useful than slower more expensive maximum intelligence models. I don't have enough experience to answer that, but this PR is a data point! (Note that even though it's called DeepSeek "Flash", this modern one is definitely a thinking model.)
Some interesting metrics from minion,
The code-review session was
54K/1.0M ctx.The code-authoring session was
230K/1.0M ctx.(During that, the diagnostic tests were finished around about 150K, proposed fix mostly finished around about 200K.)
That included installing the Rust toolchain, troubleshooting, and some missteps.
Entire cost (OpenRouter hosting) was $1.64, I reason probably more like <= $1 if I knew what I was doing from the jump.
No problem if this is terrible / all wrong, the only reason I had my hand on a lot of this one was to learn how these tools work.
Cheers. Lovely project. 🍻
LLM PR
Summary
The removal-lethality term (the #6582 fix) ranked damage-removal recipients by whether the damage kills them, but it bailed to
UnresolvedforDamageSource::Targetspells such as Self-Destruct, so the AI again spent removal on bodies it cannot kill. This extends the term to resolve the already-chosen damage source during the recipient slot, so those spells' recipients are scored by whether the source's power (and wither/infect/deathtouch) actually destroys the body.Files changed
crates/phase-ai/src/policies/context.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/src/policies/tests/removal_lethality.rscrates/phase-ai/src/policies/evasion_removal_priority.rsTrack
Developer
LLM
Model: deepseek-v4-flash-0731
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — phase-ai policy change only; no crates/engine game logic, parser, resolver, or targeting code changed.
CR references
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— clean (no diff)cargo clippy -p phase-ai --all-targets -- -D warnings— clean (0 warnings)cargo test -p phase-ai --lib— 1982 passed / 0 failed / 8 ignoredDiscriminating gate (revert probe): with
Some(DamageSource::Target)reverted toUnresolved, the e2e testtarget_sourced_damage_prefers_the_killable_bodyfails with killable 2/2=0.644 < unkillable 3/3=1.38 (the exact AI targets creatures with damage spells without considering their toughness? #6582 misplay).Gate A
Gate A PASS head=7bd318d6287919ae98602ce55f7ebb867fc54101 base=ed0a8e55cbc63c45825bfe38dd79a8c05d72875a
Anchored on
DamageSource::Targetthe first resolved object target is the damage source (readstargets[0]).resolve_quantity_with_targets_sliceresolvesObjectScope::Target(Power{Target}) against the passed[source]target slice; our fix calls this exact engine building block.Final review-impl
Final review-impl PASS head=7bd318d6287919ae98602ce55f7ebb867fc54101
An independent, fresh-context
/review-implpass was run.After an inline authoring-session review:
Some(DamageSource::Target)arm to
Unresolvedfailstarget_sourced_damage_prefers_the_killable_bodywithkillable 2/2=0.644 < unkillable 3/3=1.38), and confirmed the e2e drives the real
production path (engine runner cast → TargetSelection → build_decision_context →
production PolicyRegistry verdict).
608.2c→608.2h(the rule that actually describesre-reading the source's current power at resolution) in removal_lethality.rs.
TriggerTargetSelection) andbulk
MultiTargetSelectionDamageSource::Targetcases remainUnresolved;documented on
first_selected_object_target()andeffect_damage_sourceso thecoverable surface is stated rather than implied-covered.
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests