fix(coverage): expose damage_source_filter in the PreventDamage parse-diff signature#5493
Conversation
…-diff signature `effect_details` built the `PreventDamage` signature from a hand-picked field list that dropped `damage_source_filter` (swallowed by `..`), so any parser change to it was invisible to the `coverage-parse-diff` sticky — the required review evidence for parser-surface PRs. That produced a false `No card-parse changes detected` on phase-rs#5488, whose 13 Circle/Rune of Protection cards genuinely moved from an unqualified `ChosenDamageSource` to `ChosenDamageSource { filter: Some(..) }`. Emit `damage_source_filter` in the signature (via `fmt_target`, which already renders the qualifier). Per the issue's "audit the renderers" note, also emit `CreateDamageReplacement`'s `source_filter` (fed by the same `parse_oneshot_source_filter` binding) and its `target_filter`, which had the same blind spot. Test: `prevent_damage_signature_exposes_damage_source_filter` asserts the field appears when set and is omitted when `None` — reverting the fix fails it. Fixes phase-rs#5492.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the effect_details function in crates/engine/src/game/coverage.rs to include damage_source_filter for Effect::PreventDamage, as well as source_filter and target_filter for Effect::CreateDamageReplacement. This ensures that changes to these filters are visible in the coverage parse-diff signature (addressing issue #5492). A corresponding unit test was also added to verify this behavior. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Parse changes introduced by this PR · 78 card(s), 33 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved. This PR proves itself: its own coverage-parse-diff sticky is the test.
The sticky on this very PR reports:
#### 3 card(s) · ability/PreventDamage · field `damage_source_filter`: `∅` → `chosen damage source matching black`
Those cards are Circle of Protection: Black and Rune of Protection: Black — exactly the cards from #5488 whose qualified source filter the sticky could not see. Before this change, effect_details destructured amount, target, and scope off Effect::PreventDamage and dropped damage_source_filter into the .. rest pattern, so a parse going from unqualified ChosenDamageSource to ChosenDamageSource { filter: Some(..) } produced a byte-identical signature and the sticky reported "No card-parse changes." That inverted the signal: it made a real parser change read as an inert one. This retroactively confirms #5488 was a genuine parse change, and it removes a blind spot that was silently telling reviewers the opposite of the truth.
Good call going past the filed issue. #5492 only named PreventDamage, and you also covered CreateDamageReplacement::source_filter, which shares PreventDamage's parse_oneshot_source_filter binding and therefore had the identical blind spot. The sticky confirms that sibling gap was real, not theoretical:
#### 6 card(s) · ability/CreateDamageReplacement · field `source_filter`: `∅` → `chosen damage source`
That row is Jade Monolith, Beacon of Destiny, and General's Regalia among others. Fixing the reported field and leaving its twin broken would have left the same class of bug live on a different effect. You found the class.
The "only push when Some" design is the right call. Unqualified prevention keeps a byte-identical signature, so this introduces zero gratuitous baseline churn on cards that were already being reported correctly. Worth noting for anyone reading the sticky here: all 33 signature rows are ∅ → <value>. Not one is a value-to-value transition. That is the structural tell that this is surfacing a previously hidden field rather than changing any parse.
On the 78-card / 33-signature sticky on this PR: this is expected one-time baseline churn, not a regression. It is the direct consequence of exposing a field that was previously omitted from the signature. Once this lands, the baseline absorbs it and reviewers of subsequent PRs will see a clean diff. Please don't let anyone read that number as behavior change.
I checked the target_filter field using format!("{f:?}") while its two siblings use fmt_target, and I am not raising it. target_filter is Option<DamageTargetFilter> (types/ability.rs:11041), a distinct type from the Option<TargetFilter> carried by source_filter and damage_source_filter. fmt_target takes &TargetFilter and cannot accept it. The Debug format is correct here, not an inconsistency.
CR annotations verified against docs/MagicCompRules.txt. CR 614.1a ("Effects that use the word 'instead' are replacement effects") and CR 615 (Prevention Effects) both check out and are used consistently with the rest of the codebase.
One non-blocking nit, for a follow-up if you'd rather not touch this PR:
prevent_damage_signature_exposes_damage_source_filter tests the PreventDamage key in both directions — present when Some, absent when None. That is exactly the right shape, because the absence assertion is what guards the no-churn property. There is no equivalent assertion for the two new CreateDamageReplacement keys (source_filter, target_filter), which have the same both-directions contract and the same regression risk if someone later moves a field back into the .. rest pattern. Extending the existing test with a CreateDamageReplacement case, or filing a follow-up, would close that gap. Not a merge blocker.
Merging. Thanks for the clean fix and for chasing the sibling.
…n the parse-diff signature (phase-rs#5495) `effect_details` built the ChangeZone/ChangeZoneAll signature from origin, destination and target only — every ETB entry qualifier (enters_attacking, enter_tapped, enter_transformed, owner_library, enters_under, up_to, enter_with_counters, conditional_enter_with_counters, face_down_profile, enters_modified_if; library_position, random_order for the mass case) was swallowed by `..`. A parser change flipping one produced no row in the coverage-parse-diff sticky, reading as "No card-parse changes" — surfaced on Senu (phase-rs#5494), whose "put it onto the battlefield attacking" (CR 508.4) sets enters_attacking with no diff-visible evidence. Split the two variants (their qualifier sets diverge) and emit each field only when set, so unqualified signatures stay byte-identical — the "push only when set" pattern phase-rs#5493 established for PreventDamage. Adds change_zone_signature_exposes_enters_attacking (appears when set, omitted when false; reverting the emission fails it). Closes phase-rs#5495. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e parse-diff signature (phase-rs#5501) Follow-up to phase-rs#5492/phase-rs#5493. `effect_details` built the `ChangeZone` / `ChangeZoneAll` signature from `origin`/`destination`/`target` only, swallowing every battlefield-entry qualifier via `..`. So a parser change that flips e.g. `enters_attacking` false→true (Senu, Keen-Eyed Protector — "put it onto the battlefield attacking", CR 508.4) produced no row in the coverage-parse-diff sticky, leaving the change unreviewable through the evidence channel reviewers rely on. Split the shared arm and emit every entry qualifier a parser change can alter, each only when active so a plain ChangeZone's signature is unchanged: - ChangeZone: owner_library, enter_transformed, enters_under, enter_tapped, enters_attacking, up_to, enter_with_counters, conditional_enter_with_counters, face_down_profile, enters_modified_if. - ChangeZoneAll: enters_under, enter_tapped, enter_with_counters, face_down_profile, library_position, random_order. Both patterns are now exhaustive (no `..`), so a future entry field fails to compile here rather than being silently dropped — closing this class of gap going forward, not just the reported field. Test: `change_zone_signature_exposes_enters_attacking` asserts the field appears when true and is absent otherwise, via the real `effect_details`. Fixes phase-rs#5495.
…n the parse-diff signature (phase-rs#5507) (phase-rs#5511) `effect_details` rendered only `produced` for `Effect::Mana`, swallowing `restrictions`/`grants`/`expiry`/`target` with `..`. So attaching a `ManaSpellGrant` to produced mana (Hall of the Bandit Lord's creature-spell haste rider, phase-rs#5502) showed only removals in the coverage-parse-diff sticky with no compensating addition — reading as a regression when it was a half-rendered effect. Third instance of the family (phase-rs#5492/phase-rs#5493 PreventDamage, phase-rs#5495/phase-rs#5501 ChangeZone). Following phase-rs#5501, fully destructure `Effect::Mana` (no `..`) so a new field is a compile error not another silent omission, emitting each only when set so unqualified mana signatures stay byte-identical. Adds mana_signature_exposes_grants. Closes phase-rs#5507. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Fixes #5492.
Problem
effect_details(coverage.rs) builds thePreventDamageparse-diff signature from a hand-picked field list that droppeddamage_source_filter(swallowed by..). Thecoverage-parse-diffsticky is the required review evidence for parser-surface PRs, and✓ No card-parse changes detectedis treated as a HIGH inert-fix signal — so omitting a field silently makes a whole class of parser change unreviewable and inverts the signal: a correct fix reads as inert. This just produced a false negative on #5488 (13 Circle/Rune of Protection cards movingChosenDamageSource→ChosenDamageSource { filter: Some(..) }).Fix
damage_source_filterin thePreventDamagesignature (viafmt_target, which already renders the "chosen damage source matching …" qualifier — it just never got called for this field).CreateDamageReplacement'ssource_filter(fed by the sameparse_oneshot_source_filterbinding, same blind spot) and itstarget_filter.These fields only appear when
Some, so unqualified cases' signatures are unchanged (no spurious diffs); the engine-source-hash bump self-heals the baseline, as the issue notes.Test
prevent_damage_signature_exposes_damage_source_filtercalls the productioneffect_detailsand asserts the field is present when set and absent whenNone— reverting the fix fails it (it's the real signature builder, not a re-implementation).cargo check -p engine --tests+cargo fmtclean.