fix(engine): support "prevent N of that damage" static prevention shields#6307
Conversation
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change introduces ChangesDamage prevention flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleReplacementParser
participant ReplacementDefinition
participant DamageResolution
participant ReplacementApplication
participant GameEvent
OracleReplacementParser->>ReplacementDefinition: construct PreventionMinus
ReplacementDefinition-->>DamageResolution: provide damage replacement
DamageResolution->>ReplacementApplication: apply replacement
ReplacementApplication->>ReplacementApplication: compute prevented amount
ReplacementApplication->>GameEvent: emit DamagePrevented outside batching
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_replacement.rs`:
- Around line 9802-9828: Update the after_prevent parsing branch around
parse_number to use composed nom combinators for the literal digit amount
followed immediately by the “of that damage” suffix. Remove the starts_with
filter and replace scan_contains(rest.trim_start(), "of that damage") with
anchored parsing that rejects malformed or extended clauses, while preserving
the existing dynamic “X” handling and EachEvent result.
In `@crates/engine/tests/integration/issue_5902_heart_shaped_herb.rs`:
- Around line 85-128: Extend the integration scenario around
deal_damage::resolve to create an EachEvent(2) prevention effect and apply it to
multiple opponent-source damage events. Assert each event’s damage is reduced by
exactly 2 and that the effect remains available for subsequent events, covering
both parameterized per-event arithmetic and non-depletion rather than only the
existing EachEvent(1) behavior.
🪄 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: 6c167dab-b487-4cb0-9668-719f54818551
📒 Files selected for processing (5)
crates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_5902_heart_shaped_herb.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR · 18 card(s), 17 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review complete: rules, parser architecture, runtime discrimination, parse impact, and current-head CI verified.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: changes requested
[HIGH] The new PreventionAmount::EachEvent duplicates the existing parameterized per-event damage-subtraction authority. Evidence: crates/engine/src/database/synthesis.rs:3704-3715 already models CR 615 / CR 702.64 “prevent N of that damage” as DamageModification::Minus { value: n } and explicitly documents “No new variant”; crates/engine/src/types/ability.rs:19711 defines that slot, and crates/engine/src/game/replacement.rs:1609 applies its saturating subtraction to each event. This PR instead adds the same concept to the separate shield-amount axis and a parallel resolver arm.
Why it matters: the mandatory add-engine-variant Stage 1 result is REFUSE_WITH_EXISTING_SLOT. Keeping two semantic representations for the same continuous per-event subtraction splits parser and resolver authority and makes future prevention cards depend on which path happened to create them.
Suggested fix: reuse or generalize the existing DamageModification::Minus replacement pathway for this Oracle class and remove PreventionAmount::EachEvent. If that shared pathway does not currently emit the required DamagePrevented bookkeeping, fix the bookkeeping at that shared authority instead of introducing a parallel semantic representation.
Current-head CI is green, but that does not contradict the architecture finding: it verifies the duplicated path works, not that the duplicate should exist.
…elds (phase-rs#5902) Bare "prevent N of that damage" prevention statics (Heart-Shaped Herb — phase-rs#5902, Sphere of Purity, Orbs of Warding, Urza's Armor, Guardian Seraph, Daunting Defender, ...) matched none of `parse_damage_prevention_replacement`'s amount branches, so the whole static ability failed to parse into a `ReplacementDefinition` — the reported symptom ("isn't affecting it at all"). Per the maintainer's CR review (add-engine-variant Stage 1 = REFUSE_WITH_EXISTING_SLOT), this reuses the existing shared per-event damage-subtraction authority rather than introducing a new `PreventionAmount` variant: - Parser: "prevent N of that damage" now emits a continuous `DamageModification::Minus { value: N }` `DamageDone` replacement — the same non-consumed, re-firing representation CR 702.64 Absorb already synthesizes (`database::synthesis::build_absorb_replacement`) — carrying the same recipient/source/combat filters. Shield-style prevention (all / all-but / next / redirection) keeps `ShieldKind::Prevention`. - Applier: the shared `DamageModification::Minus` path (Branch 1 of `damage_done_applier`) now emits the `DamagePrevented` bookkeeping the prevention shields emit — per-event outside a combat batch, aggregated into the per-shield combat-damage tally inside one (fixing the bookkeeping at the shared authority, not a parallel path). Absorb and the `Minus { value: u32::MAX }` prevent-all sentinel now emit it too. CR 615.1a + CR 702.64b + CR 609.7b. Tests: parser unit tests for the Minus representation (opponent-scoped and generalized N>1 recipient cases) + a discriminating integration scenario driving `deal_damage::resolve` (reduces N per event, non-consumed across events, opponent-source-scoped, emits DamagePrevented). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1bac4a5 to
00cf1a7
Compare
|
Reworked per the 23:08Z review. Removed The one gap you flagged — the shared Local parity on an isolated target dir, all green: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/game/replacement.rs`:
- Around line 1562-1564: Replace the is_minus_prevention classification in the
replacement handling flow with explicit typed prevention provenance supplied by
the parser. Ensure only genuinely prevention-originated
DamageModification::Minus values emit DamagePrevented and activate prevention
triggers, while arithmetic reductions such as “damage minus 1” do not; preserve
the existing handling for other modification variants.
- Around line 1653-1679: Update the is_minus_prevention bookkeeping to preserve
EventContextAmount by setting state.last_effect_count to the prevented amount
for non-batched events, and ensure the combat_prevention_tally aggregate is
stamped into state.last_effect_count before fire_combat_prevention_riders runs.
Mirror the existing ShieldKind::Prevention bookkeeping while retaining the
current DamagePrevented emission and tally behavior.
🪄 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: eee5344c-373b-4828-9ab5-6b076bc5fa88
📒 Files selected for processing (2)
crates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_replacement.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the revised parser now uses the correct shared DamageModification::Minus seam, but its bookkeeping treats ordinary arithmetic reductions as prevention.
🔴 Blocker
[HIGH] DamageModification::Minus is not exclusively prevention. Evidence: crates/engine/src/game/replacement.rs:1564 classifies every Minus as prevention, while crates/engine/src/parser/oracle_replacement.rs:17403 deliberately parses Benevolent Unicorn's “damage minus 1” arithmetic replacement into the same variant. Why it matters: that card now emits DamagePrevented and can fire prevention triggers even though it prevents no damage. Suggested fix: carry typed prevention provenance from the parser/synthesis producer through the shared Minus path, and add a Benevolent-Unicorn-style negative runtime regression.
[HIGH] The new Minus prevention path omits the replacement-follow-up amount binding. Evidence: crates/engine/src/game/replacement.rs:1666 emits/tallies prevention without the last_effect_count handoff that the shield path performs at replacement.rs:1911. Why it matters: a valid “damage prevented this way” continuation can read stale or missing EventContextAmount; the combat aggregate needs the same binding before its rider fires. Suggested fix: mirror the shield path's per-event and post-batch binding semantics, with a discriminating continuation regression.
[MED] The new Oracle grammar is not an anchored nom parser. Evidence: crates/engine/src/parser/oracle_replacement.rs:9834 adds starts_with plus scan_contains, allowing a later, non-adjacent “of that damage” phrase to satisfy the branch. Why it matters: it violates the parser's combinator gate and can bind an unrelated suffix. Suggested fix: compose the existing numeric parser with an immediately following tag("of that damage") through the lower-case nom bridge.
🟡 Non-blocking
The required parse-diff evidence is stale for this head: its sticky comment is from 2026-07-21 23:01 UTC and still reports the removed EachEvent representation, while this head was committed at 2026-07-23 02:15 UTC. Please let the current card-data job publish a fresh artifact before re-review.
✅ Clean
The prior duplicate PreventionAmount::EachEvent was removed, and the Heart-Shaped Herb scenario now exercises repeated opponent-source damage plus the own-source boundary.
Recommendation: address the typed provenance and continuation-binding gaps at the shared authority, replace the ad hoc parser scan with composable nom parsing, then push a fresh head for review.
…nus path Review round 3 on phase-rs#6307 flagged that classifying every DamageModification::Minus as prevention made plain arithmetic replacements (Benevolent Unicorn, that much damage minus 1) emit phantom DamagePrevented bookkeeping. - Provenance: split the shared subtraction into typed provenance — Minus stays plain CR 614.1a arithmetic (no prevention bookkeeping); the new PreventionMinus variant carries CR 615/702.64 prevention provenance (bare prevent-N-of-that-damage statics, Absorb synthesis) through the SAME applier subtraction arm, so there is one subtraction authority with the provenance in the variant tag. A provenance field on Minus itself would break the dormant, contributor-frozen crates/mtgish-import construction sites. - Amount binding: mirror the Branch 2 shield path at the PreventionMinus site. Outside a combat batch the per-event prevented amount is stamped into last_effect_count so a damage-prevented-this-way continuation resolves EventContextAmount against this event (CR 615.5); inside a batch both the DamagePrevented emission and the stamp defer to the post-batch aggregate via the per-replacement tally (CR 510.2 + CR 615.13), with the execute-template per-event exception and the per-source-reflecting rider exclusion mirrored. - Anchored grammar: the prevent-N-of-that-damage amount is now a composed nom sequence — parse_number immediately followed by tag(" of that damage") via nom_parse_lower — so a non-adjacent of-that-damage phrase can no longer satisfy the branch. Regressions: arithmetic-Minus negative (applier unit + Benevolent Unicorn runtime), per-event continuation binding (stale-count discriminating), in-batch deferral to the aggregate tally, and the parser adjacency anchor.
|
Pushed e3960d9 addressing all three findings. Typed provenance. The shared subtraction now carries typed provenance: Producer audit — every non-test
Amount binding. The Anchored grammar. The amount branch is now Each regression was verified red-first: with the provenance split reverted, arithmetic |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/tests/integration/issue_5902_heart_shaped_herb.rs (1)
58-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBatch-aggregation path for
PreventionMinusis only unit-tested, not exercised through real combat.This scenario drives
deal_damage::resolvedirectly for non-combat damage; thecombat_prevention_tally/fire_combat_prevention_ridersaggregation path forPreventionMinus(unit-tested inreplacement.rswith a hand-builtcombat_prevention_tally) is never driven through an actual combat-damage step here. Since Heart-Shaped Herb-class prevention plausibly applies to combat damage from multiple simultaneous attackers, a scenario-runner test (declare attackers → block →combat_damage()) asserting a single aggregatedDamagePreventedwould close the gap between the unit test's synthetic state and production wiring.As per path instructions, "Test adequacy is the highest-frequency contributor finding" for
crates/engine/tests/**, and a test should "drive the engine through its production pipeline (...combat declaration/replacement/scenario runner)".🤖 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/tests/integration/issue_5902_heart_shaped_herb.rs` around lines 58 - 166, Extend heart_shaped_herb_prevents_one_from_opponent_sources_not_own_and_never_depletes to exercise combat through the scenario runner: declare multiple opponent attackers, assign blocks as needed, and resolve combat via combat_damage(). Assert that the Heart-Shaped Herb reduces the simultaneous combat damage and emits one aggregated DamagePrevented event, while preserving the existing non-combat, ownership, and non-depletion assertions.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.
Nitpick comments:
In `@crates/engine/tests/integration/issue_5902_heart_shaped_herb.rs`:
- Around line 58-166: Extend
heart_shaped_herb_prevents_one_from_opponent_sources_not_own_and_never_depletes
to exercise combat through the scenario runner: declare multiple opponent
attackers, assign blocks as needed, and resolve combat via combat_damage().
Assert that the Heart-Shaped Herb reduces the simultaneous combat damage and
emits one aggregated DamagePrevented event, while preserving the existing
non-combat, ownership, and non-depletion assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 66a9da75-d24e-4da0-b2ca-e432e130dc56
📒 Files selected for processing (5)
crates/engine/src/database/synthesis.rscrates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_5902_heart_shaped_herb.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_replacement.rs
|
Held — current-head implementation review is clean; enqueue remains gated on current CI evidence. ✅ CleanAt 🟡 External gateThe required Rust/card-data checks are still running, so the parse-diff sticky artifact is not yet current for this head. No contributor action is requested. Recommendation: wait for the current check run and fresh parse-diff, then route the clean head to the authorized handler. |
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/mtgish-import/src/convert/replacement.rs`:
- Around line 424-426: Update the fixed-value guard in the QuantityExpr handling
for DamageModification to use u32::try_from(value) rather than the invalid i32
range cast, preserving successful conversion for nonnegative values and the
existing error paths for values that cannot convert.
🪄 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: df054150-5a7f-4e81-876f-fedc38240207
📒 Files selected for processing (4)
crates/engine/src/game/replacement.rscrates/engine/tests/integration/issue_5902_heart_shaped_herb.rscrates/mtgish-import/src/convert/mod.rscrates/mtgish-import/src/convert/replacement.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/tests/integration/issue_5902_heart_shaped_herb.rs
- crates/engine/src/game/replacement.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved at 4e46120: current CI, parse-diff, and resolved review threads verified; the fixed conversion regression remains covered by the production conversion entry test.
2 participants
Summary
Heart-Shaped Herb's own text ("If a source an opponent controls would deal
damage to you, prevent 1 of that damage.") never installed a replacement
effect at all:
parse_damage_prevention_replacement's amount extractiononly recognized
prevent all,prevent all but N,prevent the next N damage, and the literalprevent that damage— a bareprevent N of that damage(no "all"/"all but"/"the next" qualifier) matched none of thosebranches, so the whole function returned
Noneand the ability silentlyfell through. This matches the reported symptom exactly ("isn't affecting
it at all").
This idiom is shared by several other real cards beyond Heart-Shaped Herb
(Sphere of Purity, Orbs of Warding, Urza's Armor, Guardian Seraph, Djeru —
With Eyes Open, Protection of the Hekma, Daunting Defender, Shield of the
Realm, ...), so the fix is generic rather than card-specific.
Fix
Reworked per @matthewevans's review (add-engine-variant Stage 1 =
REFUSE_WITH_EXISTING_SLOT): no newPreventionAmountvariant. Theclass reuses the existing shared per-event damage-subtraction authority —
DamageModification::Minus { value: N }on aDamageDonereplacement, thesame continuous, non-consumed representation CR 702.64 Absorb already
synthesizes (
database::synthesis::build_absorb_replacement) and appliesin Branch 1 of
damage_done_applier.Parser (
parser/oracle_replacement.rs): "prevent N of that damage"now builds the replacement with
damage_modification: Minus { value: N }(leaving
shield_kindas the defaultNone), carrying the samerecipient/source/combat filters as before. Shield-style prevention
(all / all-but / depleting next / redirection-context "prevent that
damage") still routes through
ShieldKind::Prevention. Only a literaldigit after "prevent " qualifies — not the pronoun "X" — so the unrelated
dynamic "prevent X … where X is …" idiom (Cover of Winter class) is
untouched, and the match is anchored on the trailing "of that damage".
Applier bookkeeping (
game/replacement.rs): the sharedDamageModification::Minuspath did not previously emitDamagePrevented.Fixed at that shared authority (per the review) rather than in a
parallel arm: when a
Minusmodification actually reduces an event it nowemits
DamagePrevented— per-event outside a combat batch, and aggregatedinto the per-shield
combat_prevention_tallyinside one (a singlepost-batch
DamagePreventedviafire_combat_prevention_riders, matchingthe
ShieldKind::Preventionshields). CR 702.64 Absorb and theMinus { value: u32::MAX }prevent-all sentinel now emit it too.CR 615.1a (prevention shield) + CR 702.64b (continuous, non-depleting) +
CR 609.7b (controller-scoped source restriction).
Closes #5902
Testing
(
heart_shaped_herb_prevent_n_of_that_damage_is_minus_modification,bare_prevent_n_of_that_damage_generalizes_without_source_controller_clause)pin the parsed
damage_modification/shield_kind/damage_source_filtershape, including a sibling card with no controller qualifier to confirm
the fix generalizes.
issue_5902_heart_shaped_herb.rs)drives the real
deal_damage::resolvepipeline: damage from anopponent-controlled source is reduced by 1, the shield re-fires on a
SECOND opponent-source event (non-consumed — proving it is not a depleting
shield), damage from the controller's own source is unaffected, and
DamagePreventedfires for the prevented point.Local parity (isolated target dir):
cargo fmt --checkclean;cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warningsclean; full engine lib suite 17567 passed /0 failed; full integration suite 3836 passed / 0 failed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests