fix(ai): cast-commit whiff for dynamic damage spells - #7091
Conversation
Problem: The AI was committing non-lethal direct damage spells (like Slash of Light) against opponent creatures, effectively wasting the card. The existing cast-commit whiff detection (`AntiSelfHarmPolicy::score_pre_cast`) failed open for dynamic damage amounts (like `ObjectCount`) because the underlying `lethal_to_creature` helper returns `None` for non-fixed amounts. Solution: Extend `AntiSelfHarmPolicy` with a cast-commit lethality guard that evaluates dynamic damage amounts live. * Introduced `removal_lethality::can_kill_any_legal_target`, which composes the existing `pending_damage_to_object` and `outcome_is_lethal` primitives to determine if the spell can actually kill any of its legal targets. * Applied the established soft penalty (`wasted_cast_penalty`, -8.0) if the spell is harmful, targets only creatures, has opponent targets, but is provably non-lethal against all of them. * Designed the gate to fail open (no veto) for variable-X spells, non-damage removal, or unknown damage sources at cast-commit to prevent over-blocking. Validation: * Flipped pinned reproduction tests for Slash of Light to successfully assert `PassPriority` over `CastSpell`. * Added building-block and pipeline tests to prove the gate only blocks total whiffs (deferring partial whiffs to target selection), and safely ignores non-damage and variable-X spells.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds legal-target lethality analysis for harmful creature-targeting spells. The AI penalizes wasteful non-lethal casts, avoids committing total whiffs, and continues to prefer casts when at least one legal target is lethal. ChangesCreature removal lethality
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AI as anti_self_harm
participant Lethality as removal_lethality::can_kill_any_legal_target
participant Resolver as Legal target resolver
AI->>Lethality: evaluate harmful creature-only spell
Lethality->>Resolver: resolve legal opposing creatures
Resolver-->>Lethality: targets and damage outcomes
Lethality-->>AI: lethality result
AI-->>AI: apply penalty or allow cast
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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/tests/ai_quality.rs (2)
388-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the reach-guard so the test cannot pass for the wrong reason.
!results.is_empty()does not prove the cast was ever offered. If the mana funding or the priority setup stops the engine from generating aCastSpellcandidate for Slash of Light, the AI still produces a pass and unrelated downstream actions, so all three assertions hold and the test goes green without exercising the cast-commit gate.Assert that the engine offers the cast before running the AI. Capture the spell id from
add_spell_to_hand_from_oracle(currently discarded at Line 330) and check the candidate set.💚 Proposed stronger reach-guard
- scenario + let slash = scenario .add_spell_to_hand_from_oracle( P0, "Slash of Light", true, "Slash of Light deals damage equal to the number of creatures you control plus the number of Equipment you control to target creature.", ) .id();+ // Reach-guard: the engine must actually offer the cast, otherwise the + // assertions below would pass without reaching the cast-commit gate. + assert!( + engine::ai_support::candidate_actions(runner.state()) + .iter() + .any(|candidate| matches!( + candidate.action, + GameAction::CastSpell { object_id, .. } if object_id == slash + )), + "the affordable Slash of Light cast must be a generated candidate" + ); + let ai_players = HashSet::from([P0]);- // Reach-guard: the engine's full Very Hard action pipeline gave the AI a - // chance to act (at least one decision was produced), proving the test did - // not short-circuit before the cast-commit gate could be evaluated. A pass - // (or any decision) reaching the arms above means the whiff guard fired. - assert!( - !results.is_empty(), - "Very Hard AI must produce at least one decision at the cast-commit step" - );🤖 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/tests/ai_quality.rs` around lines 388 - 395, Strengthen the reach guard in the test by retaining the spell ID returned from add_spell_to_hand_from_oracle instead of discarding it, then verify the engine’s candidate set contains a CastSpell action for that specific spell before running the AI. Keep the existing results non-empty assertion as a separate downstream guard.
275-286: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe whiff guard does not change behavior at Medium difficulty.
The comment states that at Medium the cast decision routes through the search path, which scores the whiff cast at approximately
WIN_SCOREwhether or not the penalty applies. The reported misplay therefore still reproduces for a Medium AI. The softwasted_cast_penaltycannot outweigh a terminal-eval score of that magnitude.Confirm this is an accepted limitation for the release. Do you want me to open an issue for the Medium search/terminal-eval artifact so the gap is tracked rather than only recorded in a test comment?
🤖 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/tests/ai_quality.rs` around lines 275 - 286, The Medium-difficulty search/terminal-evaluation path still permits the whiff misplay and is currently excluded from the test. Track this limitation explicitly by opening or linking a follow-up issue for the Medium artifact, and update the test comment to reference that tracking issue while preserving the existing assertions.
🤖 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/removal_lethality.rs`:
- Around line 412-456: Update the harmful creature-only effect loop around
pending_damage_to_object to fail open whenever the spell contains any harmful
creature-only effect that is not Effect::DealDamage, preventing non-damage
removal from being evaluated with whole-spell damage. Separately scan every
DealDamage effect in ctx.effects() for amount.contains_x() before applying the
veto, including damage effects whose target filters are not creature-only;
preserve the existing lethal-target evaluation for damage-only spells.
---
Nitpick comments:
In `@crates/phase-ai/tests/ai_quality.rs`:
- Around line 388-395: Strengthen the reach guard in the test by retaining the
spell ID returned from add_spell_to_hand_from_oracle instead of discarding it,
then verify the engine’s candidate set contains a CastSpell action for that
specific spell before running the AI. Keep the existing results non-empty
assertion as a separate downstream guard.
- Around line 275-286: The Medium-difficulty search/terminal-evaluation path
still permits the whiff misplay and is currently excluded from the test. Track
this limitation explicitly by opening or linking a follow-up issue for the
Medium artifact, and update the test comment to reference that tracking issue
while preserving the existing assertions.
🪄 Autofix
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: cdfef3a2-64ed-4b28-a1b3-6a302abcac20
📒 Files selected for processing (3)
crates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/tests/ai_quality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — mixed removal spells are falsely penalized as damage whiffs
Reviewed 6b0329fa2e1765838d07149cc669cdcdf5588615.
crates/phase-ai/src/policies/removal_lethality.rs:412-456 evaluates each harmful creature-only effect, but pending_damage_to_object at :204-301 aggregates all DealDamage effects on the spell. For a mixed spell such as “deal 1 damage to target creature; destroy target creature,” the Destroy iteration receives the sibling one-damage outcome, sees a surviving target, and returns false. anti_self_harm.rs:412-416 then applies the anti-self-harm whiff penalty despite the Destroy effect being useful.
The existing pure-Destroy test at removal_lethality.rs:650-700 only covers PendingDamage::None, not this mixed interaction.
Please:
- Gate the veto to damage-only analyzable spells: fail open when any harmful creature-only effect is non-
DealDamage. - Fail open for variable-X damage found anywhere in the spell, including
DealDamageeffects whose filter is not creature-only. - Add a mixed
DealDamage(1) + Destroyregression proving it is not penalized as a whiff, plus an offeredCastSpellproduction reach guard so the AI assertion cannot pass without exercising the cast path.
Do not seek approval until the mixed-effect behavior is covered and fresh CI is green.
|
@CodeOptimist human Matt here - I don't mind at all that you're trying DeepSeek - if you can have it follow the |
Problem: The newly introduced cast-commit lethality guard (`can_kill_any_legal_target`) over-blocked certain viable spells by falsely identifying them as total damage whiffs. Because the underlying evaluation only aggregates `DealDamage` effects, mixed removal spells (e.g., "deal 1 damage to target creature; destroy target creature") were evaluated solely on their non-lethal damage component, incorrectly ignoring the independent `Destroy` line (CR 701.8a). Additionally, variable-X damage spells with non-creature-only target filters could be incorrectly penalized. Solution: Introduce explicit fail-open guards to `can_kill_any_legal_target` to ensure it only vetoes provable, pure damage whiffs: * Fail open if the spell contains *any* harmful creature-only effect that is not `DealDamage`. This prevents false damage-whiff vetoes on mixed damage+destroy spells, recognizing the non-damage half as an independent, useful removal line. * Fail open if *any* `DealDamage` effect references a variable `X` (CR 107.3a), including those with non-creature-only target filters. Since `X` is chosen at announcement, the damage amount cannot be definitively known at cast-commit. Validation: * Added building-block tests to verify the fail-open semantics for mixed damage+destroy spells and non-creature `DealDamage` `X` spells. * Added a production-path differential test (`mixed_damage_and_destroy_is_not_penalized_as_a_damage_whiff`) proving that mixed spells are successfully evaluated and strictly outrank identical pure-damage whiffs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/removal_lethality.rs`:
- Around line 421-425: Update the guard in the removal-lethality policy to also
accept independently useful contextual effects, including creature-targeting
Effect::GainControl, alongside harmful non-damage effects. Preserve the existing
harmful-effect handling and add a regression test covering nonlethal
Effect::DealDamage combined with Effect::GainControl, expecting the interaction
to fail open.
🪄 Autofix
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: be79c311-da00-4394-bf87-7a8054d81882
📒 Files selected for processing (2)
crates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/tests/ai_quality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested
Reviewed 3169a94f47d5d21ef696471a8dc9d67de7fc3300.
[MED] GainControl can still be falsely treated as a total removal whiff. effect_classify.rs:197-203 classifies Effect::GainControl as Contextual, while the new fail-open at removal_lethality.rs:421-425 only recognizes non-DealDamage effects classified Harmful. A mixed spell with nonlethal DealDamage and creature GainControl therefore reaches the damage-only loop at :443-479, returns false for a surviving target, and anti_self_harm.rs:412-416 applies the wasted-cast penalty to a useful control line. Add the narrow GainControl fail-open and a mixed nonlethal-damage + creature-GainControl regression. This confirms the current-head CodeRabbit thread.
[MED] The existing Very Hard end-to-end regression does not prove its cast path is reached. ai_quality.rs:326-398 discards Slash of Light's object id and only asserts that AI results are nonempty. A pass or unrelated action satisfies that assertion even if candidate_actions never offered Slash's CastSpell. Retain the spell id and assert the generated candidates contain that exact GameAction::CastSpell before running the AI; keep the nonempty-results assertion as the downstream guard.
The earlier mixed Destroy and any-DealDamage-X blockers are resolved at this head. Rust test shards are still pending, which is a separate required-check condition rather than the substantive block above. Keep auto-merge and merge-queue entry disabled until these two current-head gaps are fixed and fresh checks complete.
The cast-commit lethality gate (can_kill_any_legal_target) only failed open on Harmful non-DealDamage effects with creature-only filters, so a mixed 'deal damage to target creature; gain control of target permanent' spell was modelled purely through its non-lethal damage half and wrongly penalized as a total damage whiff. GainControl is classified Contextual (effect_classify.rs:198); the fail-open now covers Harmful or Contextual non-DealDamage effects whose filter can hit an opponent's battlefield permanent (creature-typed, any-target, or any permanent-type filter, CR 613.1b Layer 2), so an independent control-changing half-line keeps the spell castable. Also harden the Very Hard Slash-of-Light reach-guard: pin the spell's object id and assert the scorer offers its exact CastSpell candidate before the AI run, so a pass or unrelated action cannot satisfy the test vacuously. Adds discriminating unit tests (creature and permanent GainControl fail-open) and a production-path differential test proving a mixed damage+GainControl spell outranks the identical pure-damage whiff.
…-open
The cast-commit guard's fail-open helper (targets_creature_or_permanent)
only recognized creature-typed, any-target, and flat permanent-type
filters. Parser-reachable shapes that also name battlefield permanents
fell through the catch-all and re-introduced the false whiff veto the
GainControl fail-open was built to close: TypeFilter::Non ("nonland
permanent", "noncreature"), nested TypeFilter::AnyOf, Kindred
(CR 308.1), and TargetFilter-level Or/And/Not disjunctions.
Restructure into two recursive predicates that fail open on every filter
shape not provably limited to non-permanents: filter_can_match_permanent
(TargetFilter-level: Any/Typed/Or/And/Not) and
type_filter_can_match_permanent (TypeFilter-level: permanent types,
recursive AnyOf, Non excluding Non(Permanent|Card|Any), Kindred). Drop
the mis-cited CR 608.2b (target-legality rule, unrelated to disjunction
semantics) in favor of referencing the engine's own filter-matching.
Adds a shared mixed damage+GainControl fixture and three discriminating
unit tests (AnyOf, Non(Land), TargetFilter::Or): each fails against the
old catch-all, proving the shapes stay fail-open.
The AnyOf discrimination test used a flat AnyOf([Artifact, Creature]) disjunction, which the pre-commit single-level arm already handled — the test passed before the recursive helper existed and did not pin the nested-AnyOf recursion added to close review LOW phase-rs#2. Swap the fixture to a nested AnyOf(AnyOf(artifact, creature)) and add a sibling AnyOf(Non(Land), Non(Creature)) case: both shapes fell through the old flat arm to the catch-all, so each fails against the pre-commit code and pins the recursion (including descent into negated inners) going forward. Correct the doc comment to state the pre-commit limitation accurately.
comments cleanup by Gemini 3.1 Pro Extended
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/removal_lethality.rs`:
- Around line 417-433: Update the non-DealDamage fail-open check in the
removal-lethality logic to determine eligibility from legal game objects rather
than filter shape. Classify non-targeted battlefield-removal effects such as
Effect::DestroyAll explicitly, and for targeted effects use find_legal_targets
to require an actual opposing battlefield permanent, respecting
TypedFilter.controller; add regressions for mixed damage plus DestroyAll and
controller-limited control effects.
🪄 Autofix
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: 1ba8d154-3ef5-4014-ad78-3e55b7b33035
📒 Files selected for processing (3)
crates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/tests/ai_quality.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/policies/anti_self_harm.rs
- crates/phase-ai/tests/ai_quality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking for current head 5c638063dd1e82b3861493cc4c99304e8bea9e18.
The mixed-removal utility classifier still has high-severity false positives. removal_lethality.rs:426–433 and :502–532 infer usefulness from the filter shape but ignore TypedFilter.controller; an own-controller-constrained GainControl branch can therefore be credited as opposing-player removal when it has no legal opposing population. In addition, effect_classify.rs does not extract DestroyAll, so a mixed DestroyAll + damage effect is not classified consistently.
Please make the decision depend on the actual legal opposing target/population after applying the complete typed filter (including controller), rather than a filter-shape proxy. Extend the effect extraction to cover DestroyAll, and add regressions for (1) own-controller-constrained GainControl not being credited as removal and (2) a mixed DestroyAll + damage effect receiving the intended classification.
The earlier requested-changes review was on 3169a94…; this requirement applies to the current head.
…filter shape
The cast-commit gate's fail-open credited any Harmful/Contextual
non-DealDamage effect whose FILTER SHAPE could match a permanent,
ignoring TypedFilter.controller: an own-controller-constrained GainControl
branch ('gain control of target creature you control', CR 108.3/115.2b)
was credited as opposing removal with no legal opposing population, and a
mixed DestroyAll + damage spell was classified only through its damage half
because extract_target_filter never surfaced DestroyAll.
Replace the shape proxy (targets_creature_or_permanent and its
filter/type-filter helpers, deleted) with a population check:
effect_has_legal_opposing_line resolves the COMPLETE typed filter —
controller included — via the engine's find_legal_targets and credits the
effect only when a legal object under an opponent's control exists. A
wipe's real population is resolved the same way.
Surface Effect::DestroyAll's population filter in extract_target_filter:
unlike the SetTapState/Suspect All scopes (which keep a Single sibling and
stay hidden), a wipe is inherently mass (CR 701.8) and its target IS the
population it destroys, so removal classification now sees the wipe line of
a mixed spell consistently.
Regressions: own-controller GainControl veto (with the AI's own creature
present, so the veto proves the controller axis, not the empty set) and
mixed damage+DestroyAll fail-open, plus an extract_target_filter unit test
contrasting the Suspect/SetTapState carve-outs.
…-immunity
The population-based gate justified TypedFilter.controller semantics with
'CR 108.3 / CR 115.2b' — but 108.3 is the OWNER rule and 115.2b does not
exist. Use CR 108.4 (an object's controller) and CR 109.5 ('you' refers to
the object's controller) in all five sites.
Surfacing Effect::DestroyAll in extract_target_filter (previous commit)
made harmful_effect_uses_object_targeting classify a wipe as a
single-target effect, so CantBeTargeted/HexproofFrom/Protection grants were
wrongly credited as answering untargeted mass removal (CR 115.10a). Exclude
DestroyAll there — it is the only surfaced inherently-mass effect, exactly
restoring the pre-surfacing guarantee — and document the CR 115.10a /
702.11 / 702.16 / 702.18 rationale.
by Gemini 3.1 Pro Extended
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/effect_classify.rs`:
- Around line 412-421: Remove Effect::DestroyAll from the targeting-filter
branch used by extract_target_filter, and preserve it through a separate
population-only path for mass effects. Update the helper boundary and its
callers, including find_legal_targets usage, so DestroyAll never supplies a
TargetFilter or receives shroud, hexproof, protection, or source-target legality
checks, while targeted Destroy, DealDamage, and RemoveCounter behavior remains
unchanged.
🪄 Autofix
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: 654a9bac-83be-4273-a5f7-61d3de818980
📒 Files selected for processing (3)
crates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/src/policies/self_protection_classify.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-ai/src/policies/removal_lethality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking for current head c97eeb6884f433644ec001aa1a18beb43b408ceb.
DestroyAll is still evaluated as if it had a legal target: effect_classify.rs:393–421 feeds it through find_legal_targets, and removal_lethality.rs:499–520 uses that result. The actual resolver is non-targeted and instead matches a population (destroy.rs:259–337). This misclassifies default board wipes and wipes whose prospective population is protected/otherwise excluded.
Please give mass removal a separate population-evaluation path that mirrors resolver semantics rather than target legality. Add regressions for both a default DestroyAll population and a protected/excluded population, then re-check the mixed-removal score against those cases.
The prior formal request was on 5c63806…; this requirement applies to the current head.
DestroyAll was fed through find_legal_targets (target legality) even though
the resolver (destroy.rs resolve_all) is NON-targeted and matches a
battlefield population: hexproof/protected opposing creatures were wrongly
excluded from the wipe's usefulness (CR 115.10a — an affected object is not
a target), and a declared TargetFilter::None read as an empty set instead
of the resolver's default all-creatures population (CR 701.8).
Dispatch Effect::DestroyAll in effect_has_legal_opposing_line to a new
resolver-mirroring path: mass_effect_has_opposing_population iterates the
battlefield, substitutes the default creature population for None, skips
indestructible (CR 702.12b), and matches the population via the engine's
matches_target_filter with a from_source_with_controller FilterContext —
the same primitives resolve_all uses, with no targeting exemptions.
Regressions: default-population wipe (mixed deal-1 + DestroyAll{None}
fails open; pre-fix the empty find_legal_targets set vetoed it) and a
hexproof-only opposing population (helper-level assert pins the
resolver-semantics seam; pre-fix target legality credited nothing), plus an
ai_quality differential proving a mixed damage + default-population wipe
outranks the identical pure-burn whiff through the full cast pipeline.
…test The sibling test can_kill_fails_open_on_mixed_damage_and_destroy_all still described the wipe population as resolving 'via find_legal_targets', but that test's typed-creature DestroyAll now dispatches through the resolver-mirroring mass path (mass_effect_has_opposing_population — battlefield population via matches_target_filter, CR 115.10a) introduced in a30eb9c. Update the test doc comment and assert message to name the mass path; behavior and assertion are unchanged. Remaining find_legal_targets mentions in the file are all in genuinely target-legality code or historical pre-fix descriptions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/phase-ai/tests/ai_quality.rs (1)
794-801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the margin so the assertion pins the whiff penalty.
The bug under test applies a full
wasted_cast_penalty(magnitude 8) to the mixed spell. The assertion accepts any gap above 1.0. If a later change applies a partial whiff penalty to the mixed spell, the gap shrinks but stays above 1.0, and this test still passes. The test then stops guarding the fix.Both spells carry the identical
DealDamageeffect, so assert against the penalty magnitude rather than an arbitrary epsilon.♻️ Anchor the bound to the penalty magnitude
assert!( - mixed_score > pure_score + 1.0, + mixed_score >= pure_score + config.penalties().wasted_cast_penalty.abs(), "mixed deal-1 + default-population destroy-all ({mixed_score:.3}) must outrank \ the identical pure burn whiff ({pure_score:.3}): the wipe's default \ all-creatures population (CR 701.8 / destroy.rs `resolve_all`) makes the \ 3/3 a wipe target and the wipe is non-targeted (CR 115.10a), so the gate \ must not penalize the spell as a damage whiff" );Use the accessor that
AiConfigexposes for the penalty band; do not hard-code8.0.🤖 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/tests/ai_quality.rs` around lines 794 - 801, Update the score comparison in the mixed-versus-pure assertion to require a margin tied to the full wasted-cast penalty, using the penalty-band accessor exposed by AiConfig instead of the hard-coded 1.0 threshold. Preserve the existing comparison and diagnostic message while ensuring partial whiff penalties cannot satisfy the test.
🤖 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.
Nitpick comments:
In `@crates/phase-ai/tests/ai_quality.rs`:
- Around line 794-801: Update the score comparison in the mixed-versus-pure
assertion to require a margin tied to the full wasted-cast penalty, using the
penalty-band accessor exposed by AiConfig instead of the hard-coded 1.0
threshold. Preserve the existing comparison and diagnostic message while
ensuring partial whiff penalties cannot satisfy the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 672f3a38-b220-444f-8699-b33d417f815f
📒 Files selected for processing (2)
crates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/tests/ai_quality.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-ai/src/policies/removal_lethality.rs
|
Closed — the contributor model policy is a hard admission gate. This PR's canonical declaration is This is a policy disposition about this PR's declared model, not a judgment about the author or the implementation. Please re-run the work on a listed Frontier-tier model from current |
…guard The hexproof 'can't be targeted' claim recurs at nine sites citing CR 702.11a, which is only 'Hexproof is a static ability.'; the cannot-be-targeted rule is CR 702.11b (verified text). Correct all nine (CR 115.10a non-targeted kept alongside where cited); bare CR 702.11 group refs left as-is. With extract_target_filter target-only again, harmful_effect_uses_object_targeting's leading !DestroyAll conjunct was dead (the .is_some() check already excludes wipes). Remove it; document that target-only extraction excludes mass effects. No behavior change.
…lassify
The any_stack_harmful_answerable_by_grants doc cited CR 702.11a for the
cannot-be-targeted ('targeting immunity') behavior; 702.11a is only 'Hexproof
is a static ability' and 702.11b is the cannot-be-targeted rule. Align with
the same correction applied to the whiff-gate pipeline. The DefensiveGrant
variant comment at line 279 (naming the shroud/he xproof ABILITIES
themselves) keeps 702.11a, which is defensible.
I compared both on the task of responding to the latest review. I actually tested GLM-5.2 twice, because omp+GLM-5.2 compacted the orchestrator's session at only 23% of 1M just before it instructed the implementation agent. (Not clear on why but I have some theories.) A price comparison of that first run: So I re-tested ensuring compaction was disabled: Shocking results honestly. |
Yikes. Super interesting. I don't know anyone else that's tried DeepSeek on this project before :) |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — target-player wipes are still treated as empty at cast commit
Reviewed current head b548a65dc34f376ee2e04d4d07d953e1e2e70515.
mass_effect_has_opposing_population constructs FilterContext::from_source_with_controller at crates/phase-ai/src/policies/removal_lethality.rs:596, which deliberately has no ResolvedAbility. But the engine resolves ControllerRef::TargetPlayer / TargetOpponent by reading the first player target from ability.targets and otherwise fails closed (crates/engine/src/game/filter.rs:2705-2734). DestroyAll population filters of that shape have a companion player target slot (crates/engine/src/game/ability_utils.rs:2800-2805, 3365-3470), and that target is not bound yet when the cast-commit policies score a CastSpell candidate.
Consequently, a mixed spell such as “deal 1 damage to target creature; destroy all creatures target opponent controls” sees no mass population in this helper even with an opponent creature present. Its damage half then reaches the nonlethal path and receives the wasted_cast_penalty at anti_self_harm.rs:420-424; tactical_gate can also classify it redundant. That contradicts this helper’s conservative fail-open contract and does not mirror destroy::resolve_all, which uses FilterContext::from_ability after the companion player has been selected.
Please make an unresolved player-relative mass population an explicit unknown/fail-open result, and thread that through both score_pre_cast and is_redundant_creature_only_removal so it cannot apply a whiff penalty or hard-reject the cast. Add a production-pipeline regression with a dynamic nonlethal damage half plus DestroyAll { target: TypedFilter::creature().controller(TargetOpponent) }, a live opponent creature, and the companion player target still unbound at cast commit. It should prove the mixed cast is offered and is not penalized relative to the target-player wipe baseline.
✅ Clean
The current head correctly keeps DestroyAll out of extract_target_filter; the TargetFilter::None/hexproof regression exercises the non-targeted population distinction. The remaining issue is specifically the missing ability-bound population context for the sibling target-player class.
Recommendation: request changes. Fix the unresolved target-player population seam and add the discriminating regression before approval.
mass_effect_has_opposing_population builds a FilterContext with no
ResolvedAbility, but the engine resolves ControllerRef::TargetPlayer /
TargetOpponent by reading the first TargetRef::Player from ability.targets
and FAILS CLOSED without it (filter.rs TargetPlayer arm). A mixed spell
carrying a companion-player wipe ('destroy all creatures target opponent
controls') therefore read NO mass population at cast-commit even with an
opponent creature present, so its non-lethal damage half was vetoed
(wasted_cast_penalty) and is_redundant_creature_only_removal could
hard-reject the whole cast — contradicting the helper's conservative
fail-open contract and destroy::resolve_all (which resolves the population
via FilterContext::from_ability after the companion player is announced).
Make the mass population TRI-STATE: mass_effect_has_opposing_population now
returns Option<bool> — Some(true) opposing population exists, Some(false)
provably empty, None UNKNOWN when the population filter carries an unbound
player-relative controller scope (TargetPlayer/TargetOpponent/Scoped/
ParentTarget/Chosen/Triggering, via the new
filter_has_unbound_player_controller — conservative: any future
ControllerRef variant fails open). effect_has_legal_opposing_line and the
cast-commit seam has_opposing_mass_population treat None as useful
(!= Some(false)), threading the fail-open through BOTH anti_self_harm's
:397 rescue and tactical_gate::is_redundant_creature_only_removal so an
unresolvable-at-commit wipe can never be penalized or hard-rejected
(CR 109.4 / CR 115.1 / CR 601.2c).
Regressions: unit test (unknown seam + no veto) and two production-pipeline
differentials — mixed TargetOpponent wipe vs target-player-wipe baseline on
a live opponent board (anti_self_harm path) and a hexproof-only board
(tactical-gate thread, M-offered reach-guard discriminating).
…ilterProp boundary filter_has_unbound_player_controller's doc claimed only You/Opponent resolve from the casting source alone, which is inaccurate: ActivePlayer resolves from state.active_player, EnchantedPlayer from source.attached_to, and SourceChosenPlayer from source.chosen_attributes (filter.rs): for a pending spell object those are genuinely absent, but the stated reason was wrong and could mislead a future maintainer into 'correcting' a conservative classification. Reword to the honest criterion: non-You/Opponent scopes are UNBOUND BY CONSERVATIVE DESIGN (an over-approximation so a scoped wipe is never provably-empty at cast-commit), naming the three source/global-state readable variants explicitly. Also document the FilterProp-embedded ControllerRef boundary (Owned/Attacking/ProtectorMatches/HasAttachment/ HasAnyAttachmentOf/MostPrevalentCreatureTypeIn/CanEnchant) as latent with no current parser-emittable wipe population. No behavior change.
Review hold — current head
|
|
(Does it need manual resume? 🤷) |
Fixing the bot! |
|
Maintainer update: I pushed The current-head CI run is still pending, including |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 6de77a7: the cast-commit lethality path, resolver-population boundary, and team-aware opponent relation are clean; current required checks are green.
|
Something noteworthy. Although omp handled the orchestration/sub-agents of engine-implementer like a champ (with my own So, theoretically omp+DeepSeek was a little bit hamstrung during this round. And should now be leveled up. Enjoying this for sure! |
That's super weird.. because when checked out on my system, it is a symlink: But I do see what you're saying in the repo: https://github.com/phase-rs/phase/blob/main/AGENTS.md I did another fresh checkout and it did correctly check it out as a symlink. What platform are you working off of? |
|
You're 100% correct. I use Fork on Windows. I have phase-rs completely inside of WSL. I somehow had All fixed now, thanks for the check. All good! |
2 participants
Intro (human written)
Absolutely followed the
/engine-implementerpipeline even though "not required".This is the last remaining example in my Issue #6582 with the Slash of Light play.
Personal remarks
When I opened that issue I had no idea every example I provided was a separate bug. This has been an experience, my first with agentic coding, and it feels good to "close the loop" whether this PR is approved or not.
This is a second/third data point of whether
deepseek-v4-flash-0731can produce quality results. I would say/engine-implementeris mandatory, after all DS can very cheaply and rapidly execute it. I would hope thatDeepSeek with engine-implementer>Sonnet without engine-implementer???I am very nervous about polluting the codebase with contributions of insufficient quality, so I'm going to stay strictly in the non-engine lane (but using the pipeline), or just step back for a while... probably both. I do have other projects. 😅 (Though I need to publish my minion tweaks. 🤔)
I have the orchestration log of both this and the previous PR, which might be worth looking at.
I did have Gemini 3.1 Pro Extended compare the results of DeepSeek not using engine-implementer, versus using, for my earlier PR #7082 and it was a stark difference. I'm not suggesting DS be approved for any scope of work, but if so that should be mandatory. Maybe I need to open a "Evaluating DeepSeek" thread on the Discord or something and dump some files to collaborate on its evaluation? If I'm not taking a break entirely... that sounds like work. 😅
Cheers for now! 🍻
PR
Summary
Fixed a bug where the AI would waste dynamic damage spells (like Slash of Light) on creatures it couldn't kill by extending
AntiSelfHarmPolicy. A newremoval_lethalityguard was added to evaluate dynamic damage amounts live at the cast-commit decision step, applying a soft penalty to prevent non-lethal whiffs.Files changed
crates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/tests/ai_quality.rsTrack
Developer
LLM
Model: deepseek-v4-flash-0731
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
(But technically not-applicable — Changes are confined entirely to the AI policy layer (
crates/phase-ai), with no modifications to engine game logic, parser, or rules behavior.)CR references
CR 107.3a, CR 120.3, CR 701, CR 704.5f, 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 fmt --all -- --check— clean (exit 0)cargo clippy --all-targets -- -D warnings— cleancargo test -p phase-ai— all suites pass (2033 lib + 23 ai_quality + integration; 0 failures)cargo test -p phase-engine— 18,569 + 4,610 + others pass, 0 failurescargo ai-gate— compare: 0 FAIL, 2 WARN, 1 PASSGate A
Gate A PASS head=6b0329fa2e1765838d07149cc669cdcdf5588615 base=61f559e6aa98270de447de7d26ce503ee4cf7561
Anchored on
crates/phase-ai/src/policies/anti_self_harm.rs:396— analogous soft penalty for whiff detection (harmful-creature-only-no-target)crates/phase-ai/src/policies/removal_lethality.rs:373— analogous target-selection lethality gate primitive (lethality_bonus)Final review-impl
Final review-impl PASS head=6b0329fa2e1765838d07149cc669cdcdf5588615
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes