fix(engine): prevent-damage recipient scope no longer collapses to Any - #6793
Conversation
phase-rs#6682) Mutational Advantage's "Prevent all damage that would be dealt to those permanents this turn" parsed its recipient to a blanket TargetFilter::Any, protecting every permanent in the game instead of only the caster's own countered ones (Blinding Fog, Defend the Hearth, and Energy Arc hit the same fallback via different recipient shapes). The dispatch chain in parse_prevent_effect never called into parse_target's tracked-set/typed-noun grammar for the recipient position, and a bare "players" noun had no grammar arm at all. Adds a shared resolve_prevent_recipient tier ladder (chosen-target anaphor -> clause-derived population via a widened chain_prior_mass_population -> any other parse_target-recognized filter), a "players" bare-noun arm in parse_target, and the corresponding runtime plumbing: prevent_damage::resolve now freezes a TrackedSet sentinel to a concrete id at shield-creation time (avoiding drift onto whatever a later, unrelated chain publishes), and find_applicable_replacements now enforces valid_card against object damage targets on the global/stack-sourced shield path, which it previously ignored entirely for that recipient class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPrevent-damage parsing now resolves typed, player, chosen-target, and tracked-set recipients. Population binding and shield resolution preserve concrete tracked-set identities, while global object-target prevention enforces ChangesPrevent damage scope handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant ChainPopulation
participant PreventShield
participant ReplacementPlanner
OracleParser->>ChainPopulation: resolve prior population
ChainPopulation-->>OracleParser: return tracked-set filter
OracleParser->>PreventShield: create scoped prevention shield
PreventShield->>PreventShield: freeze tracked-set id
ReplacementPlanner->>PreventShield: evaluate damage event
PreventShield-->>ReplacementPlanner: apply recipient and valid-card filters
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head 408b068f0f48f6c7364e8dccac43f8e39af7f1b6.
-
[HIGH] Snapshot the clause-derived Mutational Advantage population at resolution.
crates/engine/src/parser/oracle_effect/mod.rs:30012carries the population as a rawTargetFilter;crates/engine/src/game/effects/prevent_damage.rs:289binds only the pre-existingTrackedSet; andcrates/engine/src/game/engine_replacement.rs:7218live-re-evaluates the filter. The official ruling says the affected set is determined on resolution. Snapshot the clause-derived population while bare typed recipients stay live, and add gained-counter and lost-counter runtime tests. -
[MED] Exercise the Energy Arc cast pipeline. The parser-only tests at
crates/engine/src/parser/oracle_effect/tests.rs:49555and:49644, plus runtime fixtures that hand-construct atcrates/engine/src/game/effects/prevent_damage.rs:1528and directly push atcrates/engine/src/game/engine_replacement.rs:19523, bypass the Energy Arc cast pipeline. Add selected/nonselected cast-pipeline damage-to and damage-by coverage. -
[MED] Provide the required current-head parse-diff artifact. This PR changes parser code, including
crates/engine/src/parser/oracle_effect/imperative.rs:5958andcrates/engine/src/parser/oracle_target.rs:1638, but the required parse-diff sticky comment is missing. Add the full artifact before re-review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/engine/src/game/replacement.rs (1)
7206-7222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the sentinel
FilterContextinstead of rebuilding it.This is the same 6-line construction as the
damage_source_filterblock at Lines 7148-7153. Build it once next tosource_controller(Line 7137) and share it; both gates want identical semantics, so a future divergence would be silent.♻️ Suggested consolidation
let source_controller = repl_def.source_controller.unwrap_or(state.active_player); + // CR 109.4 + CR 614.1a: the pending host `ObjectId(0)` has + // no entry in `state.objects`, so a controller-relative + // filter needs the install-time anchor when present. + let sentinel_ctx = match repl_def.source_controller { + Some(pid) => FilterContext::from_source_with_controller(ObjectId(0), pid), + None => FilterContext::from_source(state, ObjectId(0)), + };then use
&sentinel_ctxin both thedamage_source_filterandvalid_cardgates.🤖 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/game/replacement.rs` around lines 7206 - 7222, Hoist the sentinel FilterContext construction beside source_controller in the surrounding replacement logic, creating it once with the same controller-dependent semantics. Reuse the shared sentinel context in both the damage_source_filter gate and the valid_card gate, including the matches_target_filter call, instead of rebuilding it locally.crates/engine/src/parser/oracle_effect/imperative.rs (1)
5918-5927: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a terminal-boundary guard on
parse_those_population_anaphor.The combinator is a bare prefix
tagwith no check that the phrase ends at a word/clause boundary, so a longer phrase sharing the same prefix (e.g. "those permanents'" or an unanticipated suffix) would still satisfy.is_ok()and route into the clause-derived-population tier. Not exploitable by any card in the targeted class today, but the repo's own parser convention for recipient/anaphor predicates is to enforce a terminal boundary (seeparse_recipient_is_filter_conditioninoracle_nom/condition.rs).♻️ Illustrative fix
-fn parse_those_population_anaphor(input: &str) -> OracleResult<'_, ()> { - value((), alt((tag("those permanents"), tag("those creatures")))).parse(input) -} +fn parse_those_population_anaphor(input: &str) -> OracleResult<'_, ()> { + value( + (), + terminated( + alt((tag("those permanents"), tag("those creatures"))), + peek(alt((eof, tag(" "), tag(".")))), + ), + ) + .parse(input) +}As per path instructions, "Use word-boundary scanning with nom combinators for timing or keyword phrases that may occur at arbitrary positions; do not use scattered
contains()checks" and the referencedcondition.rsguidance to "enforce terminal boundaries so longer phrases don't get accidentally consumed."🤖 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/imperative.rs` around lines 5918 - 5927, Update parse_those_population_anaphor to require a terminal word or clause boundary after either “those permanents” or “those creatures,” following the boundary-enforcement pattern used by parse_recipient_is_filter_condition. Preserve the existing anchored prefix matching and successful recognition of the exact phrases while rejecting longer suffixes such as possessives or other attached text.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/tests.rs`:
- Around line 49639-49685: Update
energy_arc_bidirectional_prevent_binds_to_untapped_targets so it extracts the
TrackedSet id from both the recipient target and damage_source_filter, then
asserts the two ids are equal. Preserve the existing checks that both shields
use TrackedSet and that the recipient is not TargetFilter::Any.
---
Nitpick comments:
In `@crates/engine/src/game/replacement.rs`:
- Around line 7206-7222: Hoist the sentinel FilterContext construction beside
source_controller in the surrounding replacement logic, creating it once with
the same controller-dependent semantics. Reuse the shared sentinel context in
both the damage_source_filter gate and the valid_card gate, including the
matches_target_filter call, instead of rebuilding it locally.
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 5918-5927: Update parse_those_population_anaphor to require a
terminal word or clause boundary after either “those permanents” or “those
creatures,” following the boundary-enforcement pattern used by
parse_recipient_is_filter_condition. Preserve the existing anchored prefix
matching and successful recognition of the exact phrases while rejecting longer
suffixes such as possessives or other attached text.
🪄 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: 556cee02-a4b1-4023-b8d3-81fbba407144
📒 Files selected for processing (8)
crates/engine/src/game/effects/prevent_damage.rscrates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_target.rs
| /// CR 608.2c + CR 615 (issue #6682): Energy Arc's bidirectional "dealt to and | ||
| /// dealt by those creatures" must bind BOTH shields to the untapped creatures | ||
| /// selected by the preceding clause — a target-derived tracked-set anaphor — | ||
| /// not the `Any` fallback that silently protected/exposed every creature. | ||
| #[test] | ||
| fn energy_arc_bidirectional_prevent_binds_to_untapped_targets() { | ||
| let def = parse_effect_chain( | ||
| "Untap any number of target creatures. Prevent all combat damage that would be \ | ||
| dealt to and dealt by those creatures this turn.", | ||
| AbilityKind::Spell, | ||
| ); | ||
| let prevent = def | ||
| .sub_ability | ||
| .as_deref() | ||
| .expect("the prevent clause must chain after the untap"); | ||
| let Effect::PreventDamage { target, .. } = &*prevent.effect else { | ||
| panic!( | ||
| "expected the recipient (\"to\") shield, got {:?}", | ||
| prevent.effect | ||
| ); | ||
| }; | ||
| assert!( | ||
| matches!(target, TargetFilter::TrackedSet { .. }), | ||
| "the recipient shield must bind to the untapped-creatures tracked set, got {target:?}" | ||
| ); | ||
| assert_ne!(*target, TargetFilter::Any); | ||
|
|
||
| let by_ability = prevent | ||
| .sub_ability | ||
| .as_deref() | ||
| .expect("the source (\"by\") shield must chain as a sequential sibling"); | ||
| let Effect::PreventDamage { | ||
| damage_source_filter, | ||
| .. | ||
| } = &*by_ability.effect | ||
| else { | ||
| panic!( | ||
| "expected the source (\"by\") shield, got {:?}", | ||
| by_ability.effect | ||
| ); | ||
| }; | ||
| assert!( | ||
| matches!(damage_source_filter, Some(TargetFilter::TrackedSet { .. })), | ||
| "the source shield must also bind to the same tracked set, got {damage_source_filter:?}" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Energy Arc test doesn't verify both shields bind the same tracked set.
The test only checks that the recipient's target and the source's damage_source_filter are each some TargetFilter::TrackedSet, but never compares their ids. The PR's own claim — "binds both the recipient and the damage-source filter to the same tracked-set" — is therefore not actually verified: a regression that produced two independently-created tracked sets (e.g. TrackedSetId(0) vs TrackedSetId(1)) would still pass this test.
🐛 Proposed fix to assert the same tracked-set id on both shields
- assert!(
- matches!(target, TargetFilter::TrackedSet { .. }),
- "the recipient shield must bind to the untapped-creatures tracked set, got {target:?}"
- );
+ let TargetFilter::TrackedSet { id: recipient_id } = target else {
+ panic!("the recipient shield must bind to a tracked set, got {target:?}");
+ };
assert_ne!(*target, TargetFilter::Any);
let by_ability = prevent
.sub_ability
.as_deref()
.expect("the source (\"by\") shield must chain as a sequential sibling");
let Effect::PreventDamage {
damage_source_filter,
..
} = &*by_ability.effect
else {
panic!(
"expected the source (\"by\") shield, got {:?}",
by_ability.effect
);
};
- assert!(
- matches!(damage_source_filter, Some(TargetFilter::TrackedSet { .. })),
- "the source shield must also bind to the same tracked set, got {damage_source_filter:?}"
- );
+ let Some(TargetFilter::TrackedSet { id: source_id }) = damage_source_filter else {
+ panic!("the source shield must bind to a tracked set, got {damage_source_filter:?}");
+ };
+ assert_eq!(
+ recipient_id, source_id,
+ "both shields must bind to the SAME tracked set, not independently-created sets"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// CR 608.2c + CR 615 (issue #6682): Energy Arc's bidirectional "dealt to and | |
| /// dealt by those creatures" must bind BOTH shields to the untapped creatures | |
| /// selected by the preceding clause — a target-derived tracked-set anaphor — | |
| /// not the `Any` fallback that silently protected/exposed every creature. | |
| #[test] | |
| fn energy_arc_bidirectional_prevent_binds_to_untapped_targets() { | |
| let def = parse_effect_chain( | |
| "Untap any number of target creatures. Prevent all combat damage that would be \ | |
| dealt to and dealt by those creatures this turn.", | |
| AbilityKind::Spell, | |
| ); | |
| let prevent = def | |
| .sub_ability | |
| .as_deref() | |
| .expect("the prevent clause must chain after the untap"); | |
| let Effect::PreventDamage { target, .. } = &*prevent.effect else { | |
| panic!( | |
| "expected the recipient (\"to\") shield, got {:?}", | |
| prevent.effect | |
| ); | |
| }; | |
| assert!( | |
| matches!(target, TargetFilter::TrackedSet { .. }), | |
| "the recipient shield must bind to the untapped-creatures tracked set, got {target:?}" | |
| ); | |
| assert_ne!(*target, TargetFilter::Any); | |
| let by_ability = prevent | |
| .sub_ability | |
| .as_deref() | |
| .expect("the source (\"by\") shield must chain as a sequential sibling"); | |
| let Effect::PreventDamage { | |
| damage_source_filter, | |
| .. | |
| } = &*by_ability.effect | |
| else { | |
| panic!( | |
| "expected the source (\"by\") shield, got {:?}", | |
| by_ability.effect | |
| ); | |
| }; | |
| assert!( | |
| matches!(damage_source_filter, Some(TargetFilter::TrackedSet { .. })), | |
| "the source shield must also bind to the same tracked set, got {damage_source_filter:?}" | |
| ); | |
| } | |
| /// CR 608.2c + CR 615 (issue `#6682`): Energy Arc's bidirectional "dealt to and | |
| /// dealt by those creatures" must bind BOTH shields to the untapped creatures | |
| /// selected by the preceding clause — a target-derived tracked-set anaphor — | |
| /// not the `Any` fallback that silently protected/exposed every creature. | |
| #[test] | |
| fn energy_arc_bidirectional_prevent_binds_to_untapped_targets() { | |
| let def = parse_effect_chain( | |
| "Untap any number of target creatures. Prevent all combat damage that would be \ | |
| dealt to and dealt by those creatures this turn.", | |
| AbilityKind::Spell, | |
| ); | |
| let prevent = def | |
| .sub_ability | |
| .as_deref() | |
| .expect("the prevent clause must chain after the untap"); | |
| let Effect::PreventDamage { target, .. } = &*prevent.effect else { | |
| panic!( | |
| "expected the recipient (\"to\") shield, got {:?}", | |
| prevent.effect | |
| ); | |
| }; | |
| let TargetFilter::TrackedSet { id: recipient_id } = target else { | |
| panic!("the recipient shield must bind to a tracked set, got {target:?}"); | |
| }; | |
| assert_ne!(*target, TargetFilter::Any); | |
| let by_ability = prevent | |
| .sub_ability | |
| .as_deref() | |
| .expect("the source (\"by\") shield must chain as a sequential sibling"); | |
| let Effect::PreventDamage { | |
| damage_source_filter, | |
| .. | |
| } = &*by_ability.effect | |
| else { | |
| panic!( | |
| "expected the source (\"by\") shield, got {:?}", | |
| by_ability.effect | |
| ); | |
| }; | |
| let Some(TargetFilter::TrackedSet { id: source_id }) = damage_source_filter else { | |
| panic!("the source shield must bind to a tracked set, got {damage_source_filter:?}"); | |
| }; | |
| assert_eq!( | |
| recipient_id, source_id, | |
| "both shields must bind to the SAME tracked set, not independently-created sets" | |
| ); | |
| } |
🤖 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 49639 - 49685,
Update energy_arc_bidirectional_prevent_binds_to_untapped_targets so it extracts
the TrackedSet id from both the recipient target and damage_source_filter, then
asserts the two ids are equal. Preserve the existing checks that both shields
use TrackedSet and that the recipient is not TargetFilter::Any.
Parse changes introduced by this PR · 26 card(s), 14 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reasserted on current head a63ffeb11ef897a95bfe8c48014ed5bbbd425a2f: the prior substantive findings remain unresolved.
- [HIGH] Snapshot Mutational Advantage's clause-derived population at resolution, per the official ruling that determines the affected set on resolution; keep bare typed recipients live and add gained-counter/lost-counter runtime coverage.
- [MED] Add selected/nonselected Energy Arc cast-pipeline coverage for damage-to and damage-by paths; parser-only tests and hand-built replacement fixtures bypass that pipeline.
- [MED] Provide the required full current-head parse-diff artifact for the parser changes.
…solution (phase-rs#6682) Addresses code review on the prevent-damage recipient-scope fix: - Snapshot semantics (HIGH): Mutational Advantage's "those permanents" now resolves through parse_target's existing TrackedSet dispatch instead of binding directly to a live copy of the grant's filter, which would have re-checked "has a counter" at every future damage event. The chain_prior_mass_population widening (from the prior commit) still feeds the runtime: affected_objects_from_events's GenericEffect arm now also recognizes Continuous-mode static grants (not just MustAttack coercion), so the population gets enumerated once and frozen into the tracked set at resolution, matching the official ruling that gained/lost counters after resolution don't change who's protected. - Runtime bug surfaced by the Energy Arc cast-pipeline test (MED): the bidirectional "by" shield's ability.targets inherited the preceding Untap clause's targets via the chain walker's generic target-propagation, and since TargetFilter::Any isn't classified as a context-ref, the shield got installed on the untapped creature as a recipient (valid_card: SelfRef) instead of routing through the source-scoped path. Extended source_scoped_prevent to also recognize TrackedSet/TrackedSetFiltered damage_source_filter shapes, mirroring the existing ParentTarget carve-out for the Maze of Ith class of bidirectional shields. - Added a cast-pipeline test for Energy Arc covering selected/nonselected damage-to and damage-by, and a cast-pipeline runtime test for Mutational Advantage's gained/lost-counter freeze semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all three findings: [HIGH] Clause-derived population must snapshot at resolution, not live-filter. Fixed by routing "those permanents"/"those creatures" uniformly through [MED] Energy Arc cast-pipeline coverage. Added [MED] Parse-diff artifact. Checked the CI run directly — the sticky comment is present but shows "baseline pending" (main's coverage snapshot for the base commit hasn't published yet, a CI-timing dependency independent of this PR's content). Pushing this update triggers a fresh CI run, which should produce a populated diff once the baseline catches up. Full engine test suite ( |
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current head snapshots the resolved population and covers both selected/nonselected cast-pipeline prevention paths.
Summary
Fixes #6682. Mutational Advantage's second clause ("Prevent all damage that would be dealt to those permanents this turn") was parsing its recipient to a blanket
TargetFilter::Any, protecting every permanent in the game instead of only the caster's own countered permanents. The same fallback silently over-broadened Blinding Fog ("creatures"), Defend the Hearth ("players"), and Energy Arc ("dealt to and dealt by those creatures") via three different recipient shapes.Root cause
parse_prevent_effect's recipient dispatch never routed the "dealt to<recipient>" phrase through the sharedparse_targetgrammar — it only handled a closed set of literal phrases (any target,target creature/target permanent,to you, a couple of compounds) plus a single-object anaphor gated on a prior chosen target. Anything else — mass typed nouns ("creatures", "players") and the "those permanents"/"those creatures" population anaphor — fell through toAny.Fix
resolve_prevent_recipienttier ladder (used by bothparse_prevent_effectand the bidirectional "dealt to and dealt by" path) tries, in order: (1) a singular chosen-target anaphor, (2) "those permanents"/"those creatures" bound to the population an earlier same-chain Continuous-mode static grant locked onto (a widenedchain_prior_mass_population, verified against Mutational Advantage's official ruling that the affected set is fixed at resolution), (3) any other recipientparse_targetrecognizes. Also adds aplayersbare-noun grammar arm (Playerhas noTypeFilterrepresentation, so it was never reachable before).prevent_damage::resolvenow freezes aTrackedSetsentinel to a concrete id at shield-creation time, so a persisting shield can't drift onto whatever an unrelated later chain happens to publish.find_applicable_replacementsnow enforcesvalid_cardagainst object damage targets on the global/stack-sourced shield path — previously ignored entirely for that path (proven by a pre-existing test explicitly asserting the old behavior, now split to cover the player-target case it actually applies to), which meant even a correctly-parsed typed/tracked-set recipient from an instant/sorcery source had no runtime effect.CR references
Test plan
cargo test -p phase-engine --lib— 17,983 passed, 0 failedcargo clippy -p phase-engine --lib -- -D warnings— cleancargo fmt --allAnyTrackedSetsentinel resolves to a concrete id immune to a later unrelated chain overwriting it;valid_cardis now enforced against object damage targets on the global shield path🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Parser Improvements