fix(coverage): expose ChangeZone/ChangeZoneAll entry qualifiers in the parse-diff signature - #5501
Conversation
…e parse-diff signature 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.
There was a problem hiding this comment.
Code Review
This pull request updates the coverage signature generator in crates/engine/src/game/coverage.rs to explicitly destructure and format the entry-qualifier fields for both Effect::ChangeZone and Effect::ChangeZoneAll instead of swallowing them with wildcards. It also adds a unit test to verify that enters_attacking is exposed in the ChangeZone signature. The feedback recommends adding a similar coverage test for ChangeZoneAll to ensure its new fields, such as random_order, are also verified.
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.
| .any(|k| k == "enters_attacking"), | ||
| "a plain (non-attacking) ChangeZone must not add the row", | ||
| ); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Add coverage test for ChangeZoneAll's new fields.
Why it matters: While ChangeZone is now covered by change_zone_signature_exposes_enters_attacking, ChangeZoneAll's new fields (like random_order or library_position) are not unit-tested in the coverage signature generator. Adding a corresponding test for ChangeZoneAll ensures these fields are correctly serialized and prevents future regressions.
Suggested fix: Add a test change_zone_all_signature_exposes_random_order right after change_zone_signature_exposes_enters_attacking.
}
#[test]
fn change_zone_all_signature_exposes_random_order() {
let signature_keys = |random: bool| -> Vec<String> {
effect_details(&Effect::ChangeZoneAll {
origin: Some(Zone::Graveyard),
destination: Zone::Battlefield,
target: TargetFilter::None,
enters_under: None,
enter_tapped: EtbTapState::Unspecified,
enter_with_counters: vec![],
face_down_profile: None,
library_position: None,
random_order: random,
})
.into_iter()
.map(|(k, _)| k)
.collect()
};
assert!(
signature_keys(true).iter().any(|k| k == "random_order"),
"random_order=true must appear in the parse-diff signature",
);
assert!(
!signature_keys(false).iter().any(|k| k == "random_order"),
"a plain ChangeZoneAll must not add the random_order row",
);
}
Parse changes introduced by this PR · 978 card(s), 56 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Approved. This closes #5495 the way I hoped it would be closed.
The important part is the exhaustive destructuring. Rather than adding enters_attacking beside a surviving .., you removed the .. from both Effect::ChangeZone and Effect::ChangeZoneAll. The next person who adds a field to either variant now gets a compile error instead of another silently omitted signature. That converts a recurring class of bug into one the compiler catches, which is a strictly better outcome than #5493 achieved for PreventDamage, where the .. survived. Splitting the two arms because their qualifier sets genuinely diverge is right too.
The restraint on CR annotations is also correct, and worth calling out because it is the kind of thing that gets "fixed" by a later reviewer who thinks more citations are better. effect_details is a signature renderer, so CLAUDE.md's rule that boilerplate and plumbing go unannotated applies. You cite exactly one, CR 508.4 for enters_attacking, which I grep-verified against docs/MagicCompRules.txt and which is exact. A wrong CR number is worse than no CR number.
The PR self-tests. Its own coverage-parse-diff sticky reports 978 cards and 56 signatures:
437 card(s) · ChangeZone · field enter_tapped: ∅ → Tapped
213 card(s) · ChangeZone · field enters_under: ∅ → You
88 card(s) · ChangeZone · field enter_transformed: ∅ → true
37 card(s) · ChangeZone · field enters_attacking: ∅ → true
To be explicit for anyone reading later: that 978-card sticky is expected one-time baseline churn from surfacing fields that were previously hidden, not a behavior change. Subsequent PRs will see a clean baseline. The direct payoff is that #5494 (Senu, Keen-Eyed Protector) sets enters_attacking for "put it onto the battlefield attacking", and until now that change produced no row at all. Thirty-seven cards were sitting in that blind spot.
Thirteen checks green, mergeable, clean.
On the duplicate. #5500 is a near-identical fix opened two minutes before yours by another contributor, arrived at independently: same field set, same exhaustive destructuring, same test name. I am landing yours because #5500 carries three misapplied CR citations, and because yours is already green and mergeable. The duplicated effort is my fault. I filed #5495 without assigning it or saying whether I wanted it picked up, so two people did the same work. That is on me, not on either of you.
One non-blocking nit. change_zone_signature_exposes_enters_attacking covers enters_attacking in both directions, present when set and absent when not, which is the right shape. It covers none of the other ten fields you now emit. Given that the entire premise of this PR is that an omitted field goes unnoticed, a table-driven test asserting each field appears when set would suit the subject matter. Worth doing whenever someone next touches this function.
…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>
…hase-rs#5601 tests through the card pipeline The phase-rs#5601 parser fix reaches the shipped card (regenerated card-data.json emits Desperate Gambit's lose branch as ChosenDamageSource, no SelfRef), but the coverage parse-diff reported no change: build_ability_item recurses into sub_ability / else_ability / mode_abilities, but a FlipCoin's win_effect / lose_effect are embedded AbilityDefinitions rendered only as ('win'/'lose', 'yes') presence markers. So the lose-branch PreventDamage node — and its damage_source_filter — never entered the parse signature, making the SelfRef -> ChosenDamageSource flip invisible (same swallowed-structure class as phase-rs#5492/phase-rs#5495/phase-rs#5501). - coverage.rs: build_ability_item now recurses into FlipCoin/FlipCoins/ FlipCoinUntilLose branch effects, so the branch subtree (and its damage_source_filter) enters the parse-diff signature. Regenerated coverage-data now renders the PreventDamage node with damage_source_filter= 'chosen damage source'. +test flip_coin_branch_effects_are_exposed_in_parse_details. - imperative.rs: re-point both phase-rs#5601 regression tests to parse_oracle_text (the full card pipeline synthesis uses) instead of parse_effect_chain, so they exercise the path the shipped card takes and cannot go green while the card is broken.
…ttened-head choose-source chain (phase-rs#5634) * fix(parser): thread ChosenDamageSource through the flattened-head choose-source chain Desperate Gambit's win and lose branches share one anaphor ("a source you control"), but the lose-branch prevention bound SelfRef while the win branch bound ChosenDamageSource. Bound to SelfRef the prevention shields against the Instant on the stack (which deals no damage), so losing the flip prevents nothing (CR 615.1). The repair pass thread_chosen_damage_source_into_oneshot_effects exists to thread SelfRef -> ChosenDamageSource across the flip branches, but its gate keyed solely on a surviving head Effect::ChooseDamageSource. The shipped text "Choose a source you control and flip a coin" (one sentence) lowers the head choice to a bare target selection, so no ChooseDamageSource node survives and the gate never fired (0/35,396 pool-wide). Add a second gate signal: the chain chose a damage source if a ChosenDamageSource anaphor is already bound in any branch (the win branch's "that source"), even when the head was flattened. The existing rewrite recursion then threads the sibling lose-branch SelfRef. Sibling branches cannot co-refer at construction time, which is why the post-hoc pass exists. Blast radius is exactly Desperate Gambit: only 2 pool cards carry a SelfRef damage-source filter, and Mercenaries ("this creature") has no ChosenDamageSource so the gate does not fire (SelfRef preserved). Fixes phase-rs#5601 * style: cargo fmt --all * fix(coverage): expose FlipCoin branch effects in parse-details; drive phase-rs#5601 tests through the card pipeline The phase-rs#5601 parser fix reaches the shipped card (regenerated card-data.json emits Desperate Gambit's lose branch as ChosenDamageSource, no SelfRef), but the coverage parse-diff reported no change: build_ability_item recurses into sub_ability / else_ability / mode_abilities, but a FlipCoin's win_effect / lose_effect are embedded AbilityDefinitions rendered only as ('win'/'lose', 'yes') presence markers. So the lose-branch PreventDamage node — and its damage_source_filter — never entered the parse signature, making the SelfRef -> ChosenDamageSource flip invisible (same swallowed-structure class as phase-rs#5492/phase-rs#5495/phase-rs#5501). - coverage.rs: build_ability_item now recurses into FlipCoin/FlipCoins/ FlipCoinUntilLose branch effects, so the branch subtree (and its damage_source_filter) enters the parse-diff signature. Regenerated coverage-data now renders the PreventDamage node with damage_source_filter= 'chosen damage source'. +test flip_coin_branch_effects_are_exposed_in_parse_details. - imperative.rs: re-point both phase-rs#5601 regression tests to parse_oracle_text (the full card pipeline synthesis uses) instead of parse_effect_chain, so they exercise the path the shipped card takes and cannot go green while the card is broken. --------- Co-authored-by: jaytbarimbao-collab <300663773+jaytbarimbao-collab@users.noreply.github.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Fixes #5495. Follow-up to #5492 / #5493.
Problem
effect_detailsbuilt theChangeZone/ChangeZoneAllparse-diff signature fromorigin/destination/targetonly — every battlefield-entry qualifier was swallowed by... So a parser change that flipsenters_attackingfalse→true (Senu, Keen-Eyed Protector — "put it onto the battlefield attacking", CR 508.4) produces no row in thecoverage-parse-diffsticky, leaving that class of change unreviewable through the evidence channel reviewers are told to rely on.Fix
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 (no spurious diffs; the engine-source-hash bump self-heals the baseline):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.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 blind spot going forward, per the issue's "audit the renderers" intent, not just the one reported field. (The exhaustive destructure already caught two fields I'd have otherwise missed.)Test
change_zone_signature_exposes_enters_attackingasserts the field appears whentrueand is absent for a plain ChangeZone, via the realeffect_details— reverting the fix fails it.cargo check -p engine --tests,clippy,fmtclean.