fix(coverage): surface an ability's activation zone in the parse signature - #7330
Merged
matthewevans merged 1 commit intoAug 13, 2026
Merged
Conversation
…ature `activation_zone` appeared zero times in `game/coverage.rs`, so an activated ability's parse-diff signature never rendered the zone it functions from. `can_activate_ability_now` gates legality on that field and the candidate enumerators key their hand, graveyard and library loops off it, so a change to it moves cards between "offered" and "not offered" — and the PR gate could not see any of it. Surfaced on phase-rs#7316, which moved 55 cards from `activation_zone: null` to `Graveyard`, withdrawing a battlefield offer and unlocking a graveyard one on each. Its parse-diff sticky reported `No card-parse changes detected.` That is the inverse of phase-rs#5507's failure mode and worse: there, removals with no compensating addition made a correct fix look like a regression — visibly wrong, so a reviewer investigates. Here a rules-behavior change across 55 cards was indistinguishable from a no-op. Fourth instance of the class, after phase-rs#5492, phase-rs#5495, phase-rs#5507 and phase-rs#5673 — and the first in `ability_details` rather than `effect_details`, which is where phase-rs#5507's exhaustive-destructuring recommendation was applied. The ability shell renders `AbilityDefinition`'s own fields and picks them by hand from a struct with over thirty of them. The key is `activates from`, not `from`: `effect_details` already emits `from` for a `ChangeZone` origin and `trigger_details` for a trigger origin, and `build_ability_item` silently drops duplicate keys — reusing `from` would hide this on precisely the abilities it exists to watch. Emitted unconditionally; `None` is the CR 113.6 battlefield default and emits nothing, so the ~12k abilities that default to the battlefield keep byte-identical signatures. Measured, base vs head on the same corpus: 79 clusters, every one `ability | field | activates from | ∅ → <zone>`, 986 card rows (651 hand, 333 graveyard, 1 exile, 1 command zone). Zero SupportFlip, zero added, zero removed, zero oracle_changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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)
📝 WalkthroughWalkthrough
ChangesActivation-Zone Coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized change only exposes an existing activation-zone property in coverage signatures; it does not alter game behavior or card parsing. The PR is merge-ready after normal checks and review, with no actionable merge-blocking risk remaining. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Reviewed at head 96eadbdc224498ca8711f52d0e69854bc545b039: this places the activation-zone projection at the correct coverage-signature seam, uses a non-colliding key, and the focused test covers both the default omission and the ChangeZone-origin collision.
Held for the exact-head required CI, card-data parse-diff artifact, and CodeRabbit review. The branch is mergeable; its behind status alone does not require a noisy rebase because the merge queue rebases speculatively.
|
Generated for head Parse changes introduced by this PR · 977 card(s), 79 signature(s) (baseline: main
|
matthewevans
approved these changes
Aug 13, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Reviewed current head 96eadbdc224498ca8711f52d0e69854bc545b039 in an isolated worktree. The coverage authority now emits the explicit activation_zone under a collision-free activates from key; it preserves from for effect/trigger origins and leaves the battlefield-default None signature unchanged. The current-SHA parse-diff shows exactly this field change across 977 cards, with no support flips or unrelated parse changes. Verified the CR 113.6b/j/m citation against the current Comprehensive Rules. CI is green and there are no unresolved review threads.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Fixes #7317.
Summary
activation_zoneappeared zero times incrates/engine/src/game/coverage.rs, so an activated ability's parse-diff signature never rendered the zone it functions from.can_activate_ability_now_with_restriction_gatesgates legality on that field (casting.rs:18421) and the candidate enumerators key their hand, graveyard and library loops off it, so a change to it moves cards between "offered" and "not offered" — with nothing to show for it at the PR gate.Surfaced on #7316, which moved 55 cards from
activation_zone: nulltoGraveyard, withdrawing a battlefield offer and unlocking a graveyard one on each. Its parse-diff sticky read✓ No card-parse changes detected.That is the inverse of #5507's failure mode and worse. There, removals with no compensating addition made a correct fix look like a regression — visibly wrong, so a reviewer investigates. Here a rules-behavior change across 55 cards was indistinguishable from a no-op, and the only way to tell was to grep
coverage.rsfor the field name.Fourth instance of the class — #5492 (
PreventDamage.damage_source_filter), #5495 (ChangeZoneentry qualifiers), #5507 (ManaSpellGrant), #5673 (ReplacementDefinitionscoping) — and the first inability_detailsrather thaneffect_details, which is where #5501/#5507 applied the exhaustive-destructuring guard. The ability shell rendersAbilityDefinition's own fields and picks nine of them by hand from a struct with roughly forty-five.That is the intended one-time signature migration, not parser blast radius. Every row is
ability | field | activates from | ∅ → <zone>; nothing changes value, nothing gains or loses support. The review check is that the sticky shows zeroadded,removed,oracle_changedand support flips — measured locally against9b8bb91..96eadbd:handgraveyardexilecommand zoneCompleteness cross-check:
client/public/card-data.jsoncarries 991 non-nullactivation_zonevalues (hand 656, graveyard 333, command 1, exile 1); the regeneratedcoverage-data.jsoncarries 991activates fromentries. One-to-one — nothing is lost tobuild_ability_item's dedup on any real card. (Zone::Library, used by Plot, is set at runtime and never persisted, so it correctly produces no rows.)Files changed
crates/engine/src/game/coverage.rs— one unconditionald.pushat the end ofability_details, its rationale comment, and one unit test beside the existing coverage-parse-diff is blind to ManaSpellGrant (third instance, after #5492 and #5495) #5507 test in the same module.Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — no game logic, parser, effect, trigger, targeting or state-machine behavior changes. This is a projection-only change to the coverage/parse-diff instrument: the sole production edit appends a
(String, String)pair to a details vector consumed bycoverage-report. NoAbilityDefinitionvalue, no AST, no runtime path is affected. A final/review-implpass ran against the committed head regardless, per CONTRIBUTING.CR references
CR 113.6b,CR 113.6j,CR 113.6min the new comment, withCR 113.6named as the battlefield default that makesNoneemit nothing — each grepped fromdocs/MagicCompRules.txt(:775,:791,:796,:771) before being written. They are the three rules that put a non-Nonezone on an ability: stated-zone text, a cost unpayable on the battlefield, and a cost or effect that moves the object out of a zone.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p phase-engine— 18922 + 21 + 9 + 4867 passed, 0 failed, 15 ignoredcargo clippy-strict— cleancargo fmt --all— clean./scripts/check-parser-combinators.sh— Gate G PASS, Gate A PASSpre-commit — Gate P PASS
coverage-parse-diffbase→head on one corpus (MTGJSON5.3.0+20260810) — 79 clusters, allkey="activates from", allbefore=∅; 0 SupportFlip, 0 added, 0 removed, 0 oracle_changedTwo-way control — with the
d.pushremoved,activation_zone_reaches_parse_details_without_colliding_with_effect_originfails at its "must render under its own key" assertion and prints the real signature[("from","graveyard"),("to","hand"),("target","self"),("kind","activated")], reproducing the blindness. ItsNonerow independently pins the byte-identity requirement, so the ~12k battlefield-default abilities cannot churn.Tilt is not installed in this environment;
scripts/tilt-wait.shwould return3(cannot answer), never a build result, so the isolated-direct commands above were used.cargo nextestis likewise unavailable.Gate A
Gate A PASS head=96eadbd base=9b8bb919a0c0ccd7a6e2eb1217d25c341d90a14f
Anchored on
crates/engine/src/game/coverage.rs:2742—effect_detailsemitsfromfor aChangeZoneorigin, inside the exhaustively destructured arm coverage-parse-diff sticky is blind to ChangeZone's enters_attacking (same family as #5492) #5495/fix(coverage): expose ChangeZone/ChangeZoneAll entry qualifiers in the parse-diff signature #5501 produced. This is why the new key cannot be calledfrom:build_ability_item(:4851) drops anability_detailskey already present, so reusing it would hide the field on exactly the graveyard-return abilities it exists to watch.crates/engine/src/game/coverage.rs:3818— thetimingpush inability_details, the existing precedent for projecting an activation-gating property of the ability shell (is_sorcery_speed(), i.e.ActivationRestriction::AsSorcery). The new push is its sibling for the zone gate and uses the same shape.Final review-impl
Final review-impl PASS head=96eadbd
Claimed parse impact
No card's parse changes. 977 cards gain a detail key that was previously absent; no existing key changes value, and no card's
supportedflips. See the table above.Scope Expansion
None. Two adjacent gaps were found during review and are deliberately not in this PR — recorded here so they are not mistaken for oversights, and offered as follow-ups:
ability_detailsstill hand-picks its keys with no compile-time parity check, while the house idiom for this exact struct already exists in four places —Serialize for AbilityDefinition(types/ability.rs:19086, "Exhaustive destructure with NO..— this is the field-parity guard"),game/ability_scan.rs:4333,game/ability_rw.rs:3863,ai_support/shortcut_efficacy.rs:635. A destructure guard changes zero bytes of output, so the usual churn argument does not apply; it is separable and belongs in its own diff.activator_filter(CR 602.2a,casting.rs:18357) appears zero times incoverage.rs, and ofActivationRestriction's 17 variants onlyAsSorceryis rendered. A future PR flippingOnlyOnceEachTurn,MaxTimesEachTurn,ClassLevelIs,CounterThreshold, oractivator_filter: None → Some(Opponent)would move cards between offered and not-offered and post✓ No card-parse changes detected.— the fix(parser): derive activation zone from a self-move's origin, not its destination #7316 failure, one field over.Happy to open either as its own PR if maintainers want them.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit