Skip to content

Fix AI target selection for DamageSource::Target to prefer lethal rec… - #7082

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
CodeOptimist:main
Aug 7, 2026
Merged

Fix AI target selection for DamageSource::Target to prefer lethal rec…#7082
matthewevans merged 1 commit into
phase-rs:mainfrom
CodeOptimist:main

Conversation

@CodeOptimist

@CodeOptimist CodeOptimist commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Intro

This intro written by the human (me). Note that the Model/Tier may not be in the Frontier table. My expectation is Matt will override the check.

Also note that we actually did follow the /engine-implementer pipeline for quality, even though it "wasn't necessary". I can gladly provide the reports/output from each of those steps. This may not be a good place to attach them though.

Edit: Maybe I should have changed the implementation method line to say so, with a note that it's technically N/A? 🤷
Edit 2: Occurs to me that I could have moved the Model/Tier to this intro... since that's a guard against fully autonomous PRs? 🤷 But I also wouldn't want this automatically merged, since usage of DeepSeek is under evaluation, so maybe as-is was best.

Summary

Fixes a bug where the AI selects non-lethal targets for self-damage spells like Self-Destruct. It updates the AI policy in removal_lethality.rs to correctly resolve the bound damage source for DamageSource::Target effects, enabling accurate lethality scoring.

Files changed

  • crates/phase-ai/src/policies/removal_lethality.rs
  • crates/phase-ai/src/policies/tests/removal_lethality.rs
  • crates/phase-ai/src/search.rs

Track

Developer

LLM

Model: deepseek-v4-flash-0731
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer
(But technically not-applicable — Changes are strictly AI-policy scoring updates entirely within crates/phase-ai/, touching no crates/engine/ game logic.

CR references

CR 120.1, CR 120.3, CR 120.3d, CR 120.3e, CR 208.1, CR 601.2c, CR 608.2c, CR 702.2b, CR 704.5g, CR 704.5h.

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 test -p phase-ai --lib self_destruct_target_selection_prefers_lethal_over_nonlethal_body — ok

  • cargo test -p phase-ai --lib removal_lethality — 25 passed

  • cargo test -p phase-engine self_destruct_target_power — 3 passed

  • cargo clippy --all-targets -- -D warnings — clean

Gate A

Gate A PASS head=d4ddd2042cf92642e1c5b4fb181a224b8985d226 base=c44a4512e6f91684068c259a028eb44b4d801340

Anchored on

  • crates/engine/src/game/effects/deal_damage.rs:1671 — analogous source resolution authority (source = first object target)
  • crates/engine/src/game/quantity.rs:4940 — analogous amount resolution authority (ObjectScope::Target resolves first object target's power)

Final review-impl

Final review-impl PASS head=d4ddd2042cf92642e1c5b4fb181a224b8985d226

Claimed parse impact

None.

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

Summary by CodeRabbit

  • Bug Fixes

    • Improved AI lethality evaluation for effects that deal damage based on the selected source’s power.
    • AI now correctly evaluates target choices after source targets are selected while remaining neutral when the source is unresolved.
    • Fixed target selection for Self-Destruct-style effects, allowing the AI to choose lethal targets and avoid nonlethal ones.
  • Tests

    • Added coverage for lethal, nonlethal, and unresolved source-target scenarios.

…ipients

### The Bug
* The AI opponent misplayed self-destructive exchange spells (like Self-Destruct) by targeting large bodies it could not kill (e.g., a 3/3) instead of available lethal kills (e.g., 0/1s).

### Root Cause
* The `removal_lethality` policy unconditionally mapped `DamageSource::Target` to `EffectDamageSource::Unresolved`.
* This blinded the AI's lethality scoring to the source's power and keywords, forcing target selection to rely purely on base threat value, which mathematically favors the largest bodies on the board.

### The Fix
* Updated `effect_damage_source` in `removal_lethality.rs` to read the bound source object directly from `TargetSelectionProgress.selected_slots` during interactive selection.
* Added a `bound_target_slice` helper to properly resolve `QuantityRef::Power { scope: Target }` ("X is its power") against the bound source using the existing `resolve_quantity_with_targets_slice` mechanic.
* The implementation strictly adheres to CR 120.3 (establishing the first object target as the damage source) and CR 601.2c (pulling target binding from selected slots).

### Testing and Verification
* Added building-block unit tests to verify that `bound_target_sourced_damage` accurately rewards lethal recipients and penalizes non-lethal recipients with surviving toughness.
* Ensured that unbound (first-slot) decisions, `EachTarget`, and `TriggeringSource` effects correctly fall back to neutral (`Unresolved`) to preserve existing reach-guards.
* Validated through an integration test guard that the AI now correctly chooses the lethal 0/1 Wizard over the 3/3 Cloud of Darkness.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

DamageSource::Target now resolves from bound target selections. Power-based target damage uses the selected source’s power for lethality evaluation. Unit and end-to-end tests cover lethal, nonlethal, and unresolved-source cases.

Changes

Target-sourced lethality

Layer / File(s) Summary
Bound source resolution
crates/phase-ai/src/policies/removal_lethality.rs
Resolves DamageSource::Target from the first selected target when available.
Power-based damage evaluation
crates/phase-ai/src/policies/removal_lethality.rs
Resolves target-sourced quantities against the bound source and target slice. Other damage sources retain existing behavior.
Self-Destruct regression coverage
crates/phase-ai/src/policies/tests/removal_lethality.rs, crates/phase-ai/src/search.rs
Adds configurable source power, bound-source fixtures, lethality cases, unresolved-source coverage, and an end-to-end target-selection test.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AI
  participant TargetSelectionProgress
  participant RemovalLethality
  participant GameState
  AI->>TargetSelectionProgress: select source and recipient
  TargetSelectionProgress->>RemovalLethality: provide bound selection
  RemovalLethality->>GameState: resolve source power
  GameState-->>RemovalLethality: return damage amount
  RemovalLethality-->>AI: score target lethality
Loading

Possibly related PRs

  • phase-rs/phase#6665: Extends removal_lethality with DamageSource::Target handling and power-based source resolution.
  • phase-rs/phase#6826: Modifies AI lethality evaluation for target-sourced, power-based damage.
  • phase-rs/phase#6852: Adds selected-target resolution and Self-Destruct lethality tests.

Suggested labels: bug

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: improving AI target selection for DamageSource::Target effects to prefer lethal targets.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@matthewevans matthewevans self-assigned this Aug 7, 2026
@matthewevans

Copy link
Copy Markdown
Member

Closed without implementation-diff review.

This PR was opened on 2026-08-07, after the 2026-07-24 Frontier-tier cutoff. Its canonical PR-body declaration is Model: deepseek-v4-flash-0731. The policy version already present in this PR's base at creation requires Frontier-tier models only and lists the accepted vendors/models; DeepSeek is not an accepted Frontier model. Tier: Frontier does not override the required accurate Model: declaration.

This is a policy-routing decision, not a judgment on the code. Please rerun the work on an accepted Frontier-tier model and open a fresh PR from current main with an accurate canonical model declaration.

@matthewevans matthewevans removed their assignment Aug 7, 2026
@CodeOptimist

Copy link
Copy Markdown
Contributor Author

Do I ping you @matthewevans (human) or do you get notified of this on your own? 😆

@matthewevans

Copy link
Copy Markdown
Member

Maintainer note: PR is approved for getting a formal review. Do not close.

@matthewevans matthewevans reopened this Aug 7, 2026
@matthewevans matthewevans added the bug Bug fix label Aug 7, 2026
@matthewevans matthewevans self-assigned this Aug 7, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved for the current head d4ddd2042cf92642e1c5b4fb181a224b8985d226.

Review evidence: the independent implementation review found no findings; CodeRabbit has no actionable feedback; and current Rust, frontend, card-data, WASM, paired-seed AI, and decision-cost performance checks are green. The fix is a scoped AI lethality-policy correction with discriminating lethal, nonlethal, and unresolved-source coverage.

@matthewevans
matthewevans added this pull request to the merge queue Aug 7, 2026
@matthewevans matthewevans removed their assignment Aug 7, 2026
Merged via the queue into phase-rs:main with commit 928b003 Aug 7, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants