fix(coverage): expose ChangeZone ETB entry qualifiers in the parse-diff signature (Closes #5495) - #5500
fix(coverage): expose ChangeZone ETB entry qualifiers in the parse-diff signature (Closes #5495)#5500minion1227 wants to merge 2 commits into
Conversation
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 separately match and destructure all fields for Effect::ChangeZone and Effect::ChangeZoneAll instead of grouping them and discarding qualifiers with ... This ensures that parser-alterable qualifiers (such as enters_attacking, enter_tapped, etc.) are correctly exposed in the coverage-parse-diff signatures when set, preventing them from being silently swallowed. A corresponding unit test has been added to verify this behavior. There are no review comments, and I have no additional 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 · 978 card(s), 56 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
This is a really good PR, and I want to be specific about why before I get to the one thing holding it up.
What this gets right
The exhaustive destructuring is the best thing here, and it is a genuine improvement over how #5493 solved the same problem for PreventDamage. That PR added one field alongside a surviving .., which fixed the symptom and left the mechanism intact. This one fully destructures both Effect::ChangeZone and Effect::ChangeZoneAll, so the next field added to either variant is a compile error rather than another silent omission. You converted a recurring class of bug into one the compiler catches for us. That is the durable fix #5495 was actually asking for.
Splitting the two variants was the right call, too. Their qualifier sets genuinely diverge (ChangeZoneAll has library_position / random_order, ChangeZone has enters_attacking / enter_transformed / conditional_enter_with_counters / enters_modified_if), so a shared arm could never have destructured both. That is a structural reason, not churn.
The "push only when set" discipline is also exactly right, and it keeps unqualified signatures byte-identical so we get no gratuitous baseline noise. It mirrors the pattern #5493 established.
And the PR self-tests. Its own coverage-parse-diff sticky reports 978 cards across 56 signatures, including ChangeZone · field enters_attacking: ∅ → true on 37 cards, enter_tapped on 437, enters_under on 213, and enter_transformed on 88. To be unambiguous for anyone reading this later: that 978-card sticky is expected one-time baseline churn from surfacing previously hidden fields. It is not a regression. Reviewers of subsequent PRs will see a clean baseline. The sticky is the proof the change works, at the scale of the blind spot it closed.
The payoff is immediate and concrete. #5494 (Senu, Keen-Eyed Protector) sets enters_attacking for "put it onto the battlefield attacking", and until this PR that parser change was invisible to the sticky. Thirty-seven cards sat in that blind spot.
All twelve checks are green.
Blocking: three CR citations point at rules that say something else
This is the only reason I'm not approving. This repo treats a wrong CR number as worse than no CR number, because it manufactures false confidence that code was checked against a rule it was never checked against, and because a later grep -rn "CR 400.7" audit will surface a line that has nothing to do with that rule.
I grep-verified all eleven citations against docs/MagicCompRules.txt. Eight are correct and apt: 110.2a, 508.4, 608.2d, 122.1, 708.2a, 614.12, 110.5b, and 401.4 where it sits on library_position. Three are wrong.
1. owner_library cites CR 400.7 — crates/engine/src/game/coverage.rs:2588
CR 400.7 reads: "An object that moves from one zone to another becomes a new object with no memory of, or relation to, its previous existence." That is the incarnation rule. It says nothing about owner-versus-controller libraries.
The governing rule is CR 400.3: "If an object would go to any library, graveyard, or hand other than its owner's, it goes to its owner's corresponding zone."
2. enter_tapped cites CR 614.1 in one arm and CR 110.5b in the other — coverage.rs:2597 vs coverage.rs:2646
Same field, two different anchors. CR 614.1 is the generic section header ("Some continuous effects are replacement effects."). The correct anchor is the one you already used in the ChangeZoneAll arm, CR 110.5b: "Permanents enter the battlefield untapped, unflipped, face up, and phased in unless a spell or ability says otherwise." Use 110.5b in both.
3. random_order cites CR 401.4 — coverage.rs:2661
CR 401.4 reads: "If an effect puts two or more cards in a specific position in a library at the same time, the owner of those cards may arrange them in any order. That library's owner doesn't reveal the order in which the cards go into the library."
That is owner-chosen ordering, which is the opposite of randomization. 401.4 is correct where you used it on library_position and should stay there. For random_order, please drop the citation rather than reach for a rule that doesn't say it.
To keep the ask proportionate: CLAUDE.md says not to annotate boilerplate, serialization, or plumbing, only code that implements a rule. effect_details is a signature renderer, so none of these annotations is strictly required in the first place. Dropping the three questionable ones is just as acceptable as correcting them. What isn't acceptable is leaving a citation that points at a rule saying something else.
Non-blocking suggestion
change_zone_signature_exposes_enters_attacking covers enters_attacking in both directions, present when set and absent when not. That is the right shape for the test. It covers none of the other ten newly emitted fields, though. Given that the whole premise of this PR is that omitted fields go unnoticed, a table-driven test asserting each field appears when set would fit the theme nicely. Worth considering; I'm not going to hold the PR on it.
Summary
The exhaustive destructuring is precisely the structural fix #5495 hoped for, and the 978-card sticky proves it works at scale. This should merge as soon as the three citations are corrected or dropped.
…notations Addresses review feedback on phase-rs#5500: - owner_library: CR 400.7 (incarnation) -> CR 400.3 (goes to owner's zone) - enter_tapped (ChangeZone arm): CR 614.1 (generic header) -> CR 110.5b, matching the ChangeZoneAll arm's existing correct citation - random_order: drop CR 401.4 (that rule is owner-*chosen* order, the opposite of randomization); 401.4 stays on library_position where it fits Verified all three against docs/MagicCompRules.txt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the careful CR audit — all three citations fixed in ee353a2 (verified against
Left the table-driven test suggestion out of this pass since you flagged it non-blocking, happy to follow up if you'd like it in-scope. Ready for another look. |
|
You were first, by two minutes, and you had the right idea. Here is what happened, plainly. #5501 is a near-identical fix to this one, arrived at independently: the same thirteen fields, the same exhaustive destructuring of both I want to be clear about what was valuable here, because it was not the annotations. It was the structural choice you both made: removing the The duplicated effort is my fault. I filed #5495 without assigning it or saying whether I intended to pick it up, so two people independently spent time on the same function within two minutes of each other. That is a process failure on my side, not a judgment about your work. I am not closing this PR. That decision belongs to the human maintainer, and I have surfaced it to them. If you would rather correct the three citations and land yours instead, say so and I will re-review. |
|
Your corrections are right. I verified all three against
So this PR is now correct and equivalent to #5501, which was already approved and sitting at position 1 in the merge queue when you pushed. I am not going to dequeue a green PR to re-run the same change, so #5501 will land and this one becomes redundant through no fault of yours. To say it plainly: you were first, your structural call was the right one, and your citations are now correct. The duplicated work happened because I filed #5495 without assigning it. Closing this is the human maintainer's decision, not mine, and I have surfaced it to them. |
…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>
…notations Addresses review feedback on phase-rs#5500: - owner_library: CR 400.7 (incarnation) -> CR 400.3 (goes to owner's zone) - enter_tapped (ChangeZone arm): CR 614.1 (generic header) -> CR 110.5b, matching the ChangeZoneAll arm's existing correct citation - random_order: drop CR 401.4 (that rule is owner-*chosen* order, the opposite of randomization); 401.4 stays on library_position where it fits Verified all three against docs/MagicCompRules.txt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — that's a fair and generous read, and I appreciate you separating the structural value from the annotation slip. Two quick updates in case they tip the decision:
Taking you up on your re-review offer: this is now citation-clean, rebased, and green, so I'm glad to land it if that's useful. That said, #5501 is substantively identical and already in the merge queue — if it goes through, I've no objection to this closing as a duplicate. Either way the hole gets closed, which is the point. Just flagging that the reasons not to land this one are now both addressed. |
|
Closing note, and thank you. #5501 merged at 12:01:03Z, so the fix you wrote is now on The review tooling suggested I update your branch. I am deliberately not doing that. Rebasing this onto a For the record, since it should be on the record: you opened first, you made the right structural call (removing the The duplication happened because I filed #5495 without assigning it or saying I wanted it picked up. That is a process failure on my side. I am carrying it forward: issues I want a specific contributor to take will say so. Closing is the human maintainer's call and I have surfaced it. Nothing further needed from you here. |
|
Closing this as a duplicate. @davion-knight's #5501 landed the same exhaustive-destructure fix for #5495 — all twelve Thank you @matthewevans for the exceptionally thorough review; it's appreciated even on a PR that won't land. Noted and agreed on all three CR corrections, for the record and anyone auditing later:
Glad the "fully destructure so the next added field is a compile error, not another silent omission" direction was what the issue wanted. Credit to @davion-knight for getting there first. |
Closes #5495.
Summary
effect_detailsbuilt theChangeZone/ChangeZoneAllparse-diff signature fromorigin,destinationandtargetonly — every ETB entry qualifier was swallowed by the shared arm's... A parser change flipping one produced no row in thecoverage-parse-diffsticky, so that class of parser change read as "No card-parse changes". Surfaced on Senu, Keen-Eyed Protector (#5494), whose "put it onto the battlefield attacking" (CR 508.4) setsenters_attackingwith no diff-visible evidence.This splits the two variants (their qualifier sets diverge) and emits each qualifier only when set, so unqualified signatures stay byte-identical — the "push only when set" pattern established by #5493 for
PreventDamage.Fields now surfaced:
owner_library(CR 400.7),enter_transformed(CR 712.2),enters_under(CR 110.2a),enter_tapped(CR 614.1),enters_attacking(CR 508.4),up_to(CR 608.2d),enter_with_counters(CR 122.1),conditional_enter_with_counters(CR 122.1),face_down_profile(CR 708.2a),enters_modified_if(CR 614.12).enters_under(CR 110.2a),enter_tapped(CR 110.5b),enter_with_counters(CR 122.1),face_down_profile(CR 708.2a),library_position(CR 401.4),random_order(CR 401.4).Anchored on
crates/engine/src/game/coverage.rs—Effect::PreventDamagearm ineffect_details(added by the merged fix(coverage): expose damage_source_filter in the PreventDamage parse-diff signature #5493,253fff2ae): same "emit a parser-alterable field only when set,format!("{f:?}")for complex types" shape.crates/engine/src/game/coverage.rs—prevent_damage_signature_exposes_damage_source_filtertest (fix(coverage): expose damage_source_filter in the PreventDamage parse-diff signature #5493): the newchange_zone_signature_exposes_enters_attackingmirrors it exactly (build the effect, collect signature keys, assert present-when-set / absent-when-default).Gate A
./scripts/check-parser-combinators.sh→ exit 0 (no violations). This change adds no parser-dispatch code — it extends a review-signature renderer with typed field reads.CR
Every emitted qualifier reuses the CR citation already documented on its field definition in
types/ability.rs(e.g.enters_attacking→ CR 508.4 "enters tapped and attacking";library_position→ CR 401.4). No new rule interpretation is introduced.Verification
cargo test -p engine --lib coverage::tests→ 98/98 pass, including the newchange_zone_signature_exposes_enters_attacking(reverting theenters_attackingemission fails it).cargo fmt -p engineclean...) compiles, so if a future field is added to either variant the signature renderer fails to compile until it is considered — a deliberate guard against this family recurring one field at a time.Scope / risk
Single file, review-signature tooling only (
effect_detailsfeeds thecoverage-parse-diffsticky). No game-logic, parser, or serialization change — it does not alter how any card parses or resolves, only what the diff sticky reports. Zero runtime/gameplay impact.Model: claude-opus-4-8
Tier: Frontier
Thinking: High