fix(ai): evaluate target-powered damage and fights - #6826
Merged
Conversation
matthewevans
enabled auto-merge
July 30, 2026 20:11
Contributor
|
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:
📝 WalkthroughWalkthroughAdds target-aware damage parsing and resolution, a bounded targeted-exchange verdict simulator, AI filtering for harmful cast or activation actions, and parser, unit, and integration tests. ChangesTargeted exchange behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AI Search
participant Candidate Validation
participant Targeted Exchange Verdict
participant Game State
AI Search->>Candidate Validation: re-match cast or activation
Candidate Validation->>Targeted Exchange Verdict: authenticate root candidate
Targeted Exchange Verdict->>Game State: replay targets and resolve preview
Game State-->>Targeted Exchange Verdict: participant status
Targeted Exchange Verdict-->>AI Search: Reject, Allow, or Indeterminate
AI Search-->>AI Search: remove rejected action
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/engine/src/parser/oracle_effect/mod.rs (2)
20699-20704: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two consecutive
DealDamagedestructurings.Both arms re-match the same node; one binding covers both mutations.
♻️ Proposed tidy-up
- if let Effect::DealDamage { amount, .. } = sub_ability.effect.as_mut() { - rebind_target_subject_object_scope(amount); - } - if let Effect::DealDamage { target, .. } = sub_ability.effect.as_mut() { - *target = TargetFilter::ParentTargetSlot { index: 0 }; - } + if let Effect::DealDamage { amount, target, .. } = sub_ability.effect.as_mut() { + rebind_target_subject_object_scope(amount); + *target = TargetFilter::ParentTargetSlot { index: 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/engine/src/parser/oracle_effect/mod.rs` around lines 20699 - 20704, Merge the two consecutive Effect::DealDamage matches in the sub_ability mutation block into a single destructuring arm that binds both amount and target, then apply rebind_target_subject_object_scope to amount and assign the ParentTargetSlot target within that arm.
20426-20441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWildcard arm over
QuantityRefhides future scope-carrying variants.The outer
QuantityExprmatch is exhaustive, but the inner_ => returnmeans any newly addedQuantityRefvariant that carries anObjectScopesilently stops being rebound — the target-subject damage amount would then resolve against the spell source instead of the chosen target, with no compiler error. Consider listing the non-scope variants explicitly (or extracting aQuantityRef::scope_mut()helper on the type so the exhaustiveness lives in one place and every caller benefits).As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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/engine/src/parser/oracle_effect/mod.rs` around lines 20426 - 20441, The inner match over QuantityRef in the QuantityExpr::Ref branch must not use a wildcard that hides future scope-carrying variants. Replace `_ => return` with explicit handling of every current non-scope-carrying QuantityRef variant, or introduce and reuse a QuantityRef::scope_mut() helper that centralizes exhaustive matching while preserving rebinding of anaphoric and source scopes to ObjectScope::Target.Source: Coding guidelines
crates/engine/src/ai_support/targeted_exchange.rs (1)
127-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
_arm defeats the explicitWaitingForenumeration.Every listed variant and the wildcard return the same
Indeterminate, so the enumeration adds no behavior while the wildcard removes the compiler's ability to flag a newly added prompt kind that should be explored (e.g. a future partial/subset target prompt would silently fail open here). Drop the wildcard and make the match exhaustive, or drop the redundant list.
state: &mut GameStateis also stronger than needed — the body only reads and hands&*statetoexplore_target_children.As per coding guidelines: "Use exhaustive
matchexpressions without wildcard fallbacks when matching known enums, allowing the compiler to detect missing variants."🤖 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/engine/src/ai_support/targeted_exchange.rs` around lines 127 - 140, Update the WaitingFor match in the targeted exchange function to remove the wildcard arm and explicitly handle every enum variant, preserving target exploration only for TargetSelection and TriggerTargetSelection. Change the function’s state parameter from &mut GameState to an immutable reference and pass it directly to explore_target_children, since the body does not mutate state.Source: Coding guidelines
crates/phase-ai/src/search.rs (1)
299-330: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-enumerating the full candidate set per action makes the "fast" priority path expensive — and this gate is untested.
root_action_is_allowedrunsvalidated_candidate_actions_for_semantic_owner(candidate generation + fullFilterPipeline) once per cast/activation, andtargeted_exchange_verdictthen enumerates candidates again and clonesGameStateacross up toMAX_WITNESS_BRANCHESchildren per node. On a board with many castable spells this is exactly the workfast_priority_actionexists to avoid. Hoist the candidate set once and look up each action in it:♻️ Hoist the enumeration out of the filter
- let actions: Vec<_> = engine::ai_support::flat_priority_actions(state) - .into_iter() - .filter(|action| root_action_is_allowed(state, ai_player, action)) - .collect(); + let candidates = validated_candidate_actions_for_semantic_owner(state, ai_player); + let actions: Vec<_> = engine::ai_support::flat_priority_actions(state) + .into_iter() + .filter(|action| root_action_is_allowed_in(state, &candidates, action)) + .collect();with
root_action_is_allowed_intaking the pre-computed slice (the existing signature can delegate to it).Separately: every new test drives
score_candidates/choose_action, so neither this filter nor the fallback filter at Line 231 is exercised. A test that reachesfast_priority_actionwith a vetoed cast inflat_priority_actionswould prove this gate actually fires.As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline".
🤖 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/search.rs` around lines 299 - 330, Hoist validated_candidate_actions_for_semantic_owner out of the per-action filter and pass the precomputed candidate slice into a new root_action_is_allowed_in helper, keeping root_action_is_allowed as a delegating compatibility wrapper if needed; ensure targeted_exchange_verdict reuses that slice rather than re-enumerating candidates. Add a production-pipeline test that reaches fast_priority_action with a vetoed cast in flat_priority_actions and verifies the cast is rejected.Source: Path instructions
🤖 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/search.rs`:
- Around line 231-232: Update the fallback branch around fallback_action so
root_action_is_allowed cannot convert the last-resort action into None. Treat
the veto as a preference gate by selecting an allowed fallback escape when
available, while preserving a fallback action so the decision never deadlocks
when score_candidates returns no results.
---
Nitpick comments:
In `@crates/engine/src/ai_support/targeted_exchange.rs`:
- Around line 127-140: Update the WaitingFor match in the targeted exchange
function to remove the wildcard arm and explicitly handle every enum variant,
preserving target exploration only for TargetSelection and
TriggerTargetSelection. Change the function’s state parameter from &mut
GameState to an immutable reference and pass it directly to
explore_target_children, since the body does not mutate state.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 20699-20704: Merge the two consecutive Effect::DealDamage matches
in the sub_ability mutation block into a single destructuring arm that binds
both amount and target, then apply rebind_target_subject_object_scope to amount
and assign the ParentTargetSlot target within that arm.
- Around line 20426-20441: The inner match over QuantityRef in the
QuantityExpr::Ref branch must not use a wildcard that hides future
scope-carrying variants. Replace `_ => return` with explicit handling of every
current non-scope-carrying QuantityRef variant, or introduce and reuse a
QuantityRef::scope_mut() helper that centralizes exhaustive matching while
preserving rebinding of anaphoric and source scopes to ObjectScope::Target.
In `@crates/phase-ai/src/search.rs`:
- Around line 299-330: Hoist validated_candidate_actions_for_semantic_owner out
of the per-action filter and pass the precomputed candidate slice into a new
root_action_is_allowed_in helper, keeping root_action_is_allowed as a delegating
compatibility wrapper if needed; ensure targeted_exchange_verdict reuses that
slice rather than re-enumerating candidates. Add a production-pipeline test that
reaches fast_priority_action with a vetoed cast in flat_priority_actions and
verifies the cast is rejected.
🪄 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: 183ea942-c5db-4549-bc58-c9feb7d8a799
📒 Files selected for processing (8)
crates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/targeted_exchange.rscrates/engine/src/game/effects/deal_damage.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/self_destruct_target_power.rscrates/phase-ai/src/search.rs
Parse changes introduced by this PR · 14 card(s), 12 signature(s) (baseline: main
|
matthewevans
force-pushed
the
ship/targeted-exchange
branch
from
July 31, 2026 01:16
66b26a8 to
88870b3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
16826-16838: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnnotate the target-subject damage helper with a CR citation.
try_parse_target_subject_multi_target_damage_chainimplements rules-related targeting and damage-source binding but has noCR <number>: <description>annotation. Add the applicable citation, consistent withwrap_target_subject_damage(CR 608.2c + CR 120.1).🤖 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/engine/src/parser/oracle_effect/mod.rs` around lines 16826 - 16838, Annotate the try_parse_target_subject_multi_target_damage_chain helper with the applicable “CR <number>: <description>” rules citation, matching the existing citation style and the related wrap_target_subject_damage annotation (CR 608.2c + CR 120.1).Source: Path instructions
🤖 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/engine/src/parser/oracle_effect/mod.rs`:
- Around line 16947-16965: Add a verified “CR <number>: <description>”
annotation to try_parse_target_subject_multi_target_damage_chain, documenting
the target-declaration rule (such as CR 601.2c) and, if applicable, the relevant
damage-source rule that requires isolating the source target from the recipient
chain. Keep the existing parsing logic unchanged and ensure the citation matches
the verified rules reference format used by nearby comments.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 5042-5065: The regression test around the chain traversal must
validate the complete expected structure, not just the root effect. Extend the
assertions following the `node`/`sub_ability` walk to reject `TargetOnly`
effects at every relevant chain node, verify the rider is bound to the second
target creature, and assert the `CantBlock` ability is attached to the final
chain node rather than merely present anywhere in the chain.
In `@crates/phase-ai/src/search.rs`:
- Around line 2113-2121: Update the candidate filtering around
targeted_exchange_verdict so non-exchange candidates are excluded before
targeted_exchange_verdict replays them. Add or reuse an
ability-shape/legality-successor check from validate_candidates to identify
exchange candidates, while preserving the existing Reject filtering for
candidates that require targeted exchange evaluation.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 16826-16838: Annotate the
try_parse_target_subject_multi_target_damage_chain helper with the applicable
“CR <number>: <description>” rules citation, matching the existing citation
style and the related wrap_target_subject_damage annotation (CR 608.2c + CR
120.1).
🪄 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: 201f65e6-77d9-4c29-aeb2-8a898d14152e
📒 Files selected for processing (4)
crates/engine/src/ai_support/targeted_exchange.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/phase-ai/src/search.rs
Comment on lines
+5042
to
+5065
| assert!( | ||
| !matches!(definition.effect.as_ref(), Effect::TargetOnly { .. }), | ||
| "a named/self source must not be wrapped as a targeted damage source: {definition:#?}" | ||
| ); | ||
|
|
||
| let mut node = Some(&definition); | ||
| let mut has_cant_block = false; | ||
| while let Some(ability) = node { | ||
| if let Effect::GenericEffect { | ||
| static_abilities, .. | ||
| } = ability.effect.as_ref() | ||
| { | ||
| has_cant_block |= static_abilities.iter().any(|static_def| { | ||
| static_def.modifications.iter().any(|modification| { | ||
| matches!( | ||
| modification, | ||
| ContinuousModification::AddStaticMode { | ||
| mode: StaticMode::CantBlock | ||
| } | ||
| ) | ||
| }) | ||
| }); | ||
| } | ||
| node = ability.sub_ability.as_deref(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strengthen the target and chain assertions.
The TargetOnly check covers only definition.effect. A nested sub_ability could still be incorrectly wrapped and pass this test.
The has_cant_block scan checks only for the presence of StaticMode::CantBlock. It does not verify that the rider is attached to the second target creature or remains the final chain node. Assert the expected chain structure and target binding so incorrect parser output fails this regression test.
🤖 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/engine/src/parser/oracle_effect/tests.rs` around lines 5042 - 5065,
The regression test around the chain traversal must validate the complete
expected structure, not just the root effect. Extend the assertions following
the `node`/`sub_ability` walk to reject `TargetOnly` effects at every relevant
chain node, verify the rider is bound to the second target creature, and assert
the `CantBlock` ability is attached to the final chain node rather than merely
present anywhere in the chain.
Source: Path instructions
matthewevans
force-pushed
the
ship/targeted-exchange
branch
from
July 31, 2026 03:46
63e6b57 to
8549046
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engine/src/parser/oracle_effect/lower.rs`:
- Around line 10119-10130: Extend the target-source handling in the effect scope
walker to include Effect::DamageEachPlayer when its damage_source is
DamageSource::Target, rebinding its amount through the same target-aware
mechanism used by DealDamage and DamageAll. Ensure the parser and effect
regression coverage exercises a target-subject clause using each-player damage,
and implement this as reusable engine support rather than card-specific logic.
- Around line 10132-10135: The rebind_target_subject_damage_where_x traversal
currently follows only sub_ability; extend it to recursively visit else_ability,
every mode_abilities entry, and Effect::ChooseOneOf branches. Apply the same
target-source rebinding to DealDamage and DamageAll nodes in all conditional and
modal branches while preserving the existing sub_ability traversal.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 19562-19576: Update the subject-target branch around
try_parse_multi_target_damage_chain and wrap_target_subject_damage to parse
using a cloned parser context. Commit the cloned context back to the original
only when wrapping succeeds; preserve the original ctx unchanged when wrapping
returns None so fallback parsing does not inherit partial fields such as
target_chooser.
🪄 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: 19627264-9781-43cf-96f2-c1f9abff882e
📒 Files selected for processing (5)
crates/engine/src/ai_support/targeted_exchange.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/phase-ai/src/search.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/parser/oracle_effect/tests.rs
- crates/engine/src/ai_support/targeted_exchange.rs
- crates/phase-ai/src/search.rs
Comment on lines
+10119
to
+10130
| match def.effect.as_mut() { | ||
| Effect::DealDamage { | ||
| amount, | ||
| damage_source: Some(DamageSource::Target), | ||
| .. | ||
| } | ||
| | Effect::DamageAll { | ||
| amount, | ||
| damage_source: Some(DamageSource::Target), | ||
| .. | ||
| } => super::rebind_target_subject_object_scope(amount), | ||
| _ => {} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add target-source handling for DamageEachPlayer.
The local damage parser lowers damage ... to each player to Effect::DamageEachPlayer at Lines 7344-7354 and Lines 7567-7573. This walker handles only DealDamage and DamageAll with DamageSource::Target.
If a target-subject clause with where X is its power lowers to DamageEachPlayer, its amount keeps Power { scope: Source }. It can then read the ability source instead of the chosen target. Add a target-aware source representation or rebind for this effect. Add a parser and effect regression test.
As per path instructions, target-powered damage must be a reusable engine capability for a class of cards, not a special-case subset.
🤖 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/engine/src/parser/oracle_effect/lower.rs` around lines 10119 - 10130,
Extend the target-source handling in the effect scope walker to include
Effect::DamageEachPlayer when its damage_source is DamageSource::Target,
rebinding its amount through the same target-aware mechanism used by DealDamage
and DamageAll. Ensure the parser and effect regression coverage exercises a
target-subject clause using each-player damage, and implement this as reusable
engine support rather than card-specific logic.
Source: Path instructions
Comment on lines
+10132
to
+10135
| if let Some(sub) = def.sub_ability.as_deref_mut() { | ||
| rebind_target_subject_damage_where_x(sub); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 10 'Effect::TargetOnly|DamageSource::Target|else_ability|mode_abilities' \
crates/engine/src/parser crates/engine/src/gameRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target walker and nearby traversal ---'
rg -n -C 35 'rebind_target_subject_damage_where_x|TargetOnly.*Damage|DamageSource::Target' \
crates/engine/src/parser/oracle_effect/lower.rs
printf '%s\n' '--- branch fields in the relevant definition type ---'
rg -n -C 12 'pub (sub_ability|else_ability|mode_abilities)|sub_ability:|else_ability:|mode_abilities:' \
crates/engine/src/types crates/engine/src/parser/oracle_effect
printf '%s\n' '--- focused callers and tests ---'
rg -n -C 8 'rebind_target_subject_damage_where_x|target subject|TargetOnly' \
crates/engine/src/parser/oracle_effect/lower.rs crates/engine/src/parser/oracle_effect/*test* crates/engine/src/game 2>/dev/null | head -n 1200Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-qUR21u
printf '%s\n' '--- walker definition ---'
rg -n -A 90 -B 25 'rebind_target_subject_damage_where_x' "$log"
printf '%s\n' '--- nearby TargetOnly construction ---'
rg -n -A 35 -B 35 'Effect::TargetOnly' crates/engine/src/parser/oracle_effect/lower.rs | \
rg -n -m 12 'TargetOnly|Damage|sub_ability|else_ability|mode_abilities|^[0-9]+-'
printf '%s\n' '--- exact branch fields for AbilityDefinition ---'
rg -n -A 20 -B 10 'struct AbilityDefinition|pub struct AbilityDefinition|mode_abilities' \
crates/engine/src/types/ability.rs crates/engine/src/parser/oracle_effect/lower.rs | head -n 400Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all TargetOnly sites in lower.rs ---'
rg -n 'Effect::TargetOnly' crates/engine/src/parser/oracle_effect/lower.rs
printf '%s\n' '--- branch assignments in lower.rs ---'
rg -n -C 4 'else_ability\s*=|mode_abilities\s*=|\.else_ability\(|\.mode_abilities' \
crates/engine/src/parser/oracle_effect/lower.rs
printf '%s\n' '--- TargetOnly sites with surrounding function context ---'
python3 - <<'PY'
from pathlib import Path
p = Path("crates/engine/src/parser/oracle_effect/lower.rs")
lines = p.read_text().splitlines()
hits = [i for i, line in enumerate(lines) if "Effect::TargetOnly" in line]
for i in hits:
start = max(0, i - 18)
end = min(len(lines), i + 28)
print(f"\n--- lines {start + 1}-{end} ---")
print("\n".join(f"{n + 1}:{lines[n]}" for n in range(start, end)))
PYRepository: phase-rs/phase
Length of output: 12073
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- modal and conditional construction in oracle_effect ---'
rg -n -C 12 'mode_abilities|else_ability' crates/engine/src/parser/oracle_effect \
| rg -n 'mode_abilities|else_ability|TargetOnly|DealDamage|DamageAll|where X|conditional|modal' \
| head -n 1200
printf '%s\n' '--- TargetOnly references across parser tests and lowering code ---'
rg -n -C 10 'TargetOnly|mode_abilities|else_ability' \
crates/engine/src/parser/oracle_effect/tests.rs \
crates/engine/src/parser/oracle_effect/lower.rs \
crates/engine/src/parser/oracle_effect/mod.rs \
| rg -n 'TargetOnly|mode_abilities|else_ability|DealDamage|DamageAll|where X|Choose one|Otherwise|If you do' \
| head -n 1600
printf '%s\n' '--- relevant parser function names ---'
rg -n '^fn |^pub\(crate\) fn |^pub fn ' crates/engine/src/parser/oracle_effect/{lower.rs,mod.rs} \
| rg 'where|target|modal|condition|else|branch|damage' \
| head -n 500Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target-powered damage parser tests ---'
sed -n '4935,5145p' crates/engine/src/parser/oracle_effect/tests.rs
printf '%s\n' '--- TargetOnly with nested choice/conditional tests ---'
sed -n '13635,13735p' crates/engine/src/parser/oracle_effect/tests.rs
sed -n '45935,46025p' crates/engine/src/parser/oracle_effect/tests.rs
printf '%s\n' '--- modal lowering and branch representation ---'
sed -n '1735,1785p' crates/engine/src/parser/oracle_effect/mod.rs
sed -n '18380,18435p' crates/engine/src/parser/oracle_effect/mod.rsRepository: phase-rs/phase
Length of output: 19036
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target-subject damage wrapper and its callers ---'
rg -n -A 90 -B 25 'wrap_target_subject_damage|target_subject_damage' \
crates/engine/src/parser/oracle_effect
printf '%s\n' '--- conditional/modal damage test cases ---'
rg -n -C 8 'Target.*deals|target.*deals|where X is its power|DamageAll|ChooseOneOf|mode_abilities|else_ability' \
crates/engine/src/parser/oracle_effect/tests.rs \
| rg -n 'where X is its power|Target.*deals|target.*deals|DamageAll|ChooseOneOf|mode_abilities|else_ability|if |otherwise|instead' \
| head -n 1800
printf '%s\n' '--- nested branch traversal helpers ---'
rg -n -A 35 -B 10 'branches|mode_abilities|else_ability' \
crates/engine/src/parser/oracle_effect/lower.rs \
| rg -n 'branches|mode_abilities|else_ability|rebind|where_x' \
| head -n 1000Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- exact wrapper implementation ---'
rg -n 'fn wrap_target_subject_damage' crates/engine/src/parser/oracle_effect
line=$(rg -n 'fn wrap_target_subject_damage' crates/engine/src/parser/oracle_effect | head -n 1 | cut -d: -f2)
start=$((line - 20))
end=$((line + 130))
sed -n "${start},${end}p" crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- assembly paths that attach else branches to parsed clauses ---'
rg -n -A 35 -B 15 'else_ability\s*=\s*Some|else_ability\s*=\s*def|attach.*else|Otherwise' \
crates/engine/src/parser/oracle_effect/{assembly.rs,sequence.rs,mod.rs} \
| head -n 1800
printf '%s\n' '--- modal metadata construction ---'
rg -n -A 45 -B 20 'mode_abilities\s*=' crates/engine/src/parser/oracle_effect \
| head -n 1200Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- where-X application call order ---'
rg -n -A 45 -B 25 'apply_where_x_ability_expression|apply_where_x_to_latest_def|where_x_expression' \
crates/engine/src/parser/oracle_effect/{mod.rs,assembly.rs,sequence.rs,lower.rs} \
| head -n 1800
printf '%s\n' '--- all mutations of a TargetOnly definition after wrapping ---'
rg -n -A 20 -B 20 'TargetOnly|d\.else_ability|def\.else_ability|mode_abilities' \
crates/engine/src/parser/oracle_effect/{assembly.rs,sequence.rs,mod.rs} \
| rg -n 'TargetOnly|else_ability|mode_abilities|condition|where|damage' \
| head -n 1800Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- current_defs construction and branch attachment ---'
sed -n '2140,2260p' crates/engine/src/parser/oracle_effect/assembly.rs
printf '%s\n' '--- final definition assembly and where-X application ---'
sed -n '1860,1945p' crates/engine/src/parser/oracle_effect/assembly.rs
sed -n '2440,2490p' crates/engine/src/parser/oracle_effect/assembly.rs
printf '%s\n' '--- ParsedEffectClause fields and conditional propagation ---'
rg -n -A 35 -B 10 'struct ParsedEffectClause|pub struct ParsedEffectClause|clause_sub' \
crates/engine/src/parser/oracle_effectRepository: phase-rs/phase
Length of output: 29717
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- condition assignment to TargetOnly definitions ---'
sed -n '2084,2145p' crates/engine/src/parser/oracle_effect/assembly.rs
printf '%s\n' '--- Otherwise branch binding target candidates ---'
sed -n '1290,1355p' crates/engine/src/parser/oracle_effect/assembly.rs
printf '%s\n' '--- tests containing TargetOnly and branch fields in the same test ---'
python3 - <<'PY'
from pathlib import Path
p = Path("crates/engine/src/parser/oracle_effect/tests.rs")
lines = p.read_text().splitlines()
starts = [i for i, line in enumerate(lines) if line.startswith("fn ")]
for pos, start in enumerate(starts):
end = starts[pos + 1] if pos + 1 < len(starts) else len(lines)
block = "\n".join(lines[start:end])
if "TargetOnly" in block and ("else_ability" in block or "mode_abilities" in block):
print(f"\n--- lines {start + 1}-{end} ---")
print("\n".join(f"{i + 1}:{lines[i]}" for i in range(start, min(end, start + 180))))
PYRepository: phase-rs/phase
Length of output: 12338
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- nested modal effect representation ---'
rg -n -A 25 -B 12 'ChooseOneOf\s*\{|enum Effect|branches:' \
crates/engine/src/types/ability.rs crates/engine/src/types \
| head -n 900
printf '%s\n' '--- existing recursive walkers for modal branches ---'
rg -n -A 35 -B 10 'ChooseOneOf|branches\.iter|branches\.iter_mut|mode_abilities' \
crates/engine/src/parser/oracle_effect crates/engine/src/types/ability.rs \
| rg -n 'ChooseOneOf|branches|mode_abilities|iter_mut|rebind|where_x' \
| head -n 1400
printf '%s\n' '--- source-order invariant for the reported path ---'
python3 - <<'PY'
from pathlib import Path
src = Path("crates/engine/src/parser/oracle_effect/lower.rs").read_text()
assembly = Path("crates/engine/src/parser/oracle_effect/assembly.rs").read_text()
walker = src[src.index("fn rebind_target_subject_damage_where_x"):src.index("fn apply_where_x_ability_condition")]
assert "def.else_ability" not in walker
assert "def.mode_abilities" not in walker
assert "if let Some(sub) = def.sub_ability" in walker
apply = src[src.index("fn apply_where_x_ability_expression"):src.index("fn rebind_target_subject_damage_where_x")]
assert "def.else_ability" in apply
assert "def.mode_abilities" in apply
assert assembly.index("for current in &mut current_defs") < assembly.index("d.else_ability = Some(else_def)")
print("The rebind walker omits else_ability and mode_abilities, while the broader where-X pass handles them; branch attachment occurs after the per-clause where-X pass.")
PYRepository: phase-rs/phase
Length of output: 50371
Traverse every target-only branch during target-source rebinding.
rebind_target_subject_damage_where_x follows only sub_ability. It must also traverse else_ability, mode_abilities, and Effect::ChooseOneOf branches. Otherwise, target-powered DealDamage or DamageAll nodes retain the wrong object scope in conditional or modal branches.
🤖 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/engine/src/parser/oracle_effect/lower.rs` around lines 10132 - 10135,
The rebind_target_subject_damage_where_x traversal currently follows only
sub_ability; extend it to recursively visit else_ability, every mode_abilities
entry, and Effect::ChooseOneOf branches. Apply the same target-source rebinding
to DealDamage and DamageAll nodes in all conditional and modal branches while
preserving the existing sub_ability traversal.
Source: Path instructions
matthewevans
force-pushed
the
ship/targeted-exchange
branch
from
July 31, 2026 05:30
eb6a21b to
5213d56
Compare
CodeOptimist
added a commit
to CodeOptimist/phase
that referenced
this pull request
Aug 1, 2026
…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 power instead of silently 0 (CR 608.2c + CR 208.1). - 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. - search.rs: a confirmation test showing today's engine targeted_exchange_verdict already rejects the turn-16 Clash of the Eikons fight (AI 3/2 vs opponent 3/5), documenting that the fight-class "bad play" is owned by that separate authority rather than this term. Verification: cargo fmt clean; cargo clippy --all-targets -- -D warnings clean; full phase-ai lib suite 1983 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). 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.
CodeOptimist
added a commit
to CodeOptimist/phase
that referenced
this pull request
Aug 1, 2026
…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. - search.rs: a confirmation test showing today's engine targeted_exchange_verdict already rejects the turn-16 Clash of the Eikons fight (AI 3/2 vs opponent 3/5), documenting that the fight-class "bad play" is owned by that separate authority rather than this term. Verification: cargo fmt clean; cargo clippy --all-targets -- -D warnings clean; full phase-ai lib suite 1983 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.
CodeOptimist
added a commit
to CodeOptimist/phase
that referenced
this pull request
Aug 1, 2026
…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.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests