fix(engine): granted "leave the battlefield → exile instead" riders must actually fire (Geth, Realmbreaker, Elemental Expressionist)#6580
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds ChangesObject-hosted replacement grants
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleText
participant Parser
participant LayerEngine
participant GameObject
participant ZoneChange
OracleText->>Parser: parse leave-battlefield exile rider
Parser->>LayerEngine: create GrantReplacement
LayerEngine->>GameObject: install deduplicated replacement
ZoneChange->>GameObject: apply hosted replacement
GameObject->>ZoneChange: redirect battlefield exit to exile
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
31d2d44 to
054b54d
Compare
michiot05
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/analysis/ability_graph.rs (1)
1883-1897: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMake the
ContinuousModificationmatch exhaustive.The new arm is correct, but the trailing
_ => {}still hides future variants from ability-graph traversal. Enumerate the remaining intentional no-op variants explicitly so adding another nested ability carrier fails compilation until this visitor is updated.As per path instructions, known enums must use exhaustive matches without wildcard fallbacks.
🤖 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/src/analysis/ability_graph.rs` around lines 1883 - 1897, Update the ContinuousModification match in the ability-graph traversal to remove the wildcard fallback and explicitly list every remaining intentional no-op variant. Preserve the existing GrantAbility, GrantTrigger, and GrantReplacement handling, ensuring future enum variants cause a compilation failure until this visitor is updated.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.
Inline comments:
In `@crates/engine/src/game/coverage.rs`:
- Around line 6934-6937: Update GrantReplacement handling to traverse both
ReplacementDefinition branches: execute and ReplacementMode::Optional { decline
}. In crates/engine/src/game/coverage.rs at lines 6934-6937, 4534-4538,
5912-5915, and 6010-6014, include decline in static support, parsed-child
emission, unimplemented-part checks, and missing-part collection; in
crates/engine/src/game/ability_rw.rs at lines 2698-2703 and 2767-2773, include
decline in legacy-reference traversal and replacement-helper inspection; and in
crates/phase-ai/src/policies/x_reference.rs at lines 170-172, detect X
references in decline as well as execute.
In `@crates/engine/src/parser/oracle_static/tests.rs`:
- Around line 32133-32136: Update
classify_quoted_inner_grants_leave_battlefield_exile_6566 to pass the original
Oracle wording through normalize_card_name_refs or the production parser before
classification, rather than supplying an already normalized "~" string. Assert
the resulting GrantReplacement so the test covers normalization of “this
creature/permanent/land” and the full production path.
In `@crates/engine/src/types/layers.rs`:
- Around line 114-116: Correct the comment above
ContinuousModification::GrantReplacement to cite CR 613.1f for its Layer 6
assignment, replacing the inaccurate CR 614.6 reference. If replacement
semantics are mentioned, use CR 614.1 or CR 614.1a instead.
In `@crates/engine/tests/integration/issue_6566_granted_leave_exile.rs`:
- Around line 174-177: Remove the misleading “Dedup guard” comment and the
redundant evaluate_layers(runner.state_mut()) call in this test, since this
location cannot observe duplicate grants and duplicate behavior is covered by
duplicate_grants_dedup_to_one_replacement. Retain the corpse binding only if it
remains necessary for test clarity or compilation.
---
Outside diff comments:
In `@crates/engine/src/analysis/ability_graph.rs`:
- Around line 1883-1897: Update the ContinuousModification match in the
ability-graph traversal to remove the wildcard fallback and explicitly list
every remaining intentional no-op variant. Preserve the existing GrantAbility,
GrantTrigger, and GrantReplacement handling, ensuring future enum variants cause
a compilation failure until this visitor is updated.
🪄 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: ec1a5b19-0f42-48f4-a769-df770586f97c
📒 Files selected for processing (20)
crates/engine/src/analysis/ability_graph.rscrates/engine/src/database/unearth.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/effect.rscrates/engine/src/game/layers.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_static/keyword_grant.rscrates/engine/src/parser/oracle_static/shared.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/layers.rscrates/engine/tests/integration/issue_6566_granted_leave_exile.rscrates/engine/tests/integration/main.rscrates/phase-ai/src/policies/x_reference.rs
| ContinuousModification::GrantReplacement { replacement } => replacement | ||
| .execute | ||
| .as_deref() | ||
| .is_none_or(is_ability_supported), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Traverse both executable branches of ReplacementDefinition.
All these paths inspect only execute and ignore ReplacementMode::Optional { decline }, causing false-green support/coverage results and incomplete legacy/X analysis.
crates/engine/src/game/coverage.rs#L6934-L6937: include decline support when computingis_static_supported.crates/engine/src/game/coverage.rs#L4534-L4538: emit the decline ability as a parsed child.crates/engine/src/game/coverage.rs#L5912-L5915: inspect decline for unimplemented parts.crates/engine/src/game/coverage.rs#L6010-L6014: collect missing parts from decline.crates/engine/src/game/ability_rw.rs#L2698-L2703: traverse decline for legacy references.crates/engine/src/game/ability_rw.rs#L2767-L2773: extend the replacement helper to inspect decline.crates/phase-ai/src/policies/x_reference.rs#L170-L172: detect X references in decline.
As per path instructions, reusable replacement-definition handling must cover the complete GrantReplacement contract, not only the current mandatory rider.
📍 Affects 3 files
crates/engine/src/game/coverage.rs#L6934-L6937(this comment)crates/engine/src/game/coverage.rs#L4534-L4538crates/engine/src/game/coverage.rs#L5912-L5915crates/engine/src/game/coverage.rs#L6010-L6014crates/engine/src/game/ability_rw.rs#L2698-L2703crates/engine/src/game/ability_rw.rs#L2767-L2773crates/phase-ai/src/policies/x_reference.rs#L170-L172
🤖 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/src/game/coverage.rs` around lines 6934 - 6937, Update
GrantReplacement handling to traverse both ReplacementDefinition branches:
execute and ReplacementMode::Optional { decline }. In
crates/engine/src/game/coverage.rs at lines 6934-6937, 4534-4538, 5912-5915, and
6010-6014, include decline in static support, parsed-child emission,
unimplemented-part checks, and missing-part collection; in
crates/engine/src/game/ability_rw.rs at lines 2698-2703 and 2767-2773, include
decline in legacy-reference traversal and replacement-helper inspection; and in
crates/phase-ai/src/policies/x_reference.rs at lines 170-172, detect X
references in decline as well as execute.
Source: Path instructions
| // CR 613.1f + CR 614.6: granting an object-hosted replacement is an | ||
| // ability-adding effect (Layer 6), beside GrantTrigger. | ||
| | ContinuousModification::GrantReplacement { .. } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the CR citation for the layer mapping.
CR 614.6 describes the event after a replacement is applied; it does not justify assigning this modification to Layer 6. Use CR 613.1f for the layer assignment and CR 614.1/CR 614.1a for replacement-effect semantics instead. (media.wizards.com)
As per path instructions, rules-touching code must use a CR citation whose rule body describes the implementation.
🤖 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/src/types/layers.rs` around lines 114 - 116, Correct the
comment above ContinuousModification::GrantReplacement to cite CR 613.1f for its
Layer 6 assignment, replacing the inaccurate CR 614.6 reference. If replacement
semantics are mentioned, use CR 614.1 or CR 614.1a instead.
Sources: Path instructions, MCP tools
| // Dedup guard: exactly one granted Moved/SelfRef def (a second evaluate_layers | ||
| // pass re-derives from base and must not accumulate). | ||
| evaluate_layers(runner.state_mut()); | ||
| let _ = corpse; // corpse id == reanimated identity; retained for clarity. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The "dedup guard" comment overclaims — this block asserts nothing. The comment states "exactly one granted Moved/SelfRef def … must not accumulate," but no assert follows the second evaluate_layers, and by this point reanimated is already in Zone::Exile, so a re-derivation on the battlefield set can't be observed here anyway. The actual dedup is covered by duplicate_grants_dedup_to_one_replacement. Either drop the misleading comment/evaluate_layers call or move a real leave_exile_replacement_count(...) == 1 assertion to a point where reanimated is still on the battlefield.
🤖 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_6566_granted_leave_exile.rs` around
lines 174 - 177, Remove the misleading “Dedup guard” comment and the redundant
evaluate_layers(runner.state_mut()) call in this test, since this location
cannot observe duplicate grants and duplicate behavior is covered by
duplicate_grants_dedup_to_one_replacement. Retain the corpse binding only if it
remains necessary for test clarity or compilation.
Parse changes introduced by this PR · 5 card(s), 6 signature(s) (baseline: main
|
…ust actually fire A quoted grant of a replacement-shaped ability — Geth, Thane of Contracts' 'It gains "If this creature would leave the battlefield, exile it instead of putting it anywhere else"' — parsed to an inert ContinuousModification::GrantAbility. The concretized body was pushed into obj.abilities and never into replacement_definitions, so the redirect silently did nothing and the reanimated creature went to the graveyard on any battlefield exit. Adds ContinuousModification::GrantReplacement, the replacement-store sibling of GrantAbility/GrantTrigger/GrantStaticAbility (CR 614 is a categorically distinct sink from CR 602/603/604). It is applied at layer 6 with structural dedup and re-derived every layer pass (CR 613.1f), so its lifetime is governed by the granting continuous effect rather than an object-lifetime stamp: a permanent grant survives its source leaving, and a temporary one (Elemental Expressionist's "Until end of turn") lapses at cleanup (CR 611.2a, CR 514.2). A grant carrying other modifications alongside the replacement is never promoted to permanent — Expressionist grants two abilities, so that guard is load-bearing and is pinned by its own regression. Also fixes the parser gap that made the rider unreachable at this seam: normalize_card_name_refs rewrites "this creature|permanent|land" to "~" card-wide (CR 201.5b), but parse_leave_battlefield_rider_ref had no "~" arm, so every quoted grant failed to parse. The standalone rider path (phase-rs#6538) is unaffected — no standalone printing uses a "this <type>" subject. The Moved→Exile ReplacementDefinition is built by one shared, deliberately UNSTAMPED constructor (leave_battlefield_exile_replacement, expiry: None) with three consumers that decide the lifetime themselves: * the standalone parser rider (try_parse_leave_battlefield_exile_ replacement) and unearth's synthesis (CR 702.84a) each compose .expiry(UntilHostLeavesPlay) — reproducing phase-rs#6538's host-lifetime stamp byte-for-byte (CR 400.7), so phase-rs#6538's base-install / non-copiable / host-exit-prune machinery is preserved exactly; and * the granted path (classify_quoted_inner → GrantReplacement) uses the parser fn purely as a DETECTOR and grants the UNSTAMPED constructor output. A granted replacement's lifetime is governed by the grant's duration and re-derived every layer pass (CR 611.2a / CR 613.1f); a host-lifetime stamp would be read by phase-rs#6538's is_runtime_host_lifetime_replacement (base-install + non-copiable + host-exit-prune), base-installing the granted rider so it would outlive the granting continuous effect and Expressionist's until-end-of-turn grant would never lapse. The reconciliation is pinned by granted_replacement_is_not_host_lifetime_stamped (RED if the granted def is ever re-stamped) and the byte-identity test leave_battlefield_exile_replacement_matches_6538_shape (RED if the standalone shape drifts from phase-rs#6538). Closes phase-rs#6566 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
293afbe to
54fa471
Compare
|
Heads up — I force-pushed a clean rebase onto current The CI red here wasn't just a stale test — it's a genuine semantic conflict with our own #6538, which merged after this branch was cut. #6538 now stamps The rebase resolves it by splitting the lifetime decision per consumer:
Pinned by a new revert-failing guard, If you'd rather drive branch updates yourself (or prefer I never force-push over a maintainer commit), just say the word and I'll work to whatever process you want. Model: claude-opus-4-8 |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed at head 54fa471dec40. This is excellent work, and it lands at the right seam. Approving.
What I verified (not just read in the description)
- The seam is correct.
GrantReplacementis applied in the layer-6 continuous-modification loop (game/layers.rs) intoobj.replacement_definitionswith a structural-dedup idempotency guard, exactly mirroring theGrantStaticAbility/AddStaticModesiblings. The "no lifetime stamp needed" argument holds becausereplacement_definitionsis reseeded frombase_replacement_definitionsevery pass (layers.rs:1692, pre-existing) — so the grant's persistence is governed by whether the continuous effect still applies that pass, not an object-lifetime stamp. This is genuinely distinct from #6538's host-lifetime path and correctly leaves it untouched. - The promotion guard is right and well-fenced.
effect.rs::resolvepromotes toDuration::Permanentonly whenduration_from_fallback && !modifications.is_empty() && modifications.iter().all(GrantReplacement). The!is_empty()conjunct stops an empty list from vacuously satisfying.all(), and the.all()predicate is what keeps Expressionist's mixed[GrantReplacement, GrantTrigger]from being promoted. CR 611.2a lifetime semantics, applied correctly. - The tests discriminate.
mixed_replacement_and_trigger_grant_is_not_promoted_to_permanentis genuinely revert-sensitive — swapping.all→.anypromotes the mixed def and reds only that row. The Geth test drives the runtime pipeline (reanimate → destroy → exile) with a paired un-granted control creature going to the graveyard as the reach guard, which is what proves the replacement is doing the work rather than a blanket behavior. This is exactly the discriminating-test bar. - CR citations verified against
docs/MagicCompRules.txt: 611.2a (continuous-effect lifetime), 614.1a ("instead" = replacement), 613.1f (Layer 6 ability-adding), 201.5b (name→gained-ability rewrite, which is what the~blocker fix rests on), 514.2 (EOT cleanup). All accurate. - The parse-diff proves generality. 5 cards flip in lockstep from
grant ability→grant replacement(Geth, Realmbreaker, Llanowar Greenwidow, Spirit-Sister's Call, Elemental Expressionist). This is a class fix, not a card patch.
The ~ blocker writeup (that normalize_card_name_refs rewrites this creature/permanent/land → ~ card-wide, so the rider had to grow a ~ arm to parse at all) is a nice catch, and confronting all 74 "would leave the battlefield" corpus cards to show no standalone rider uses a this <type> subject is the right way to prove the #6538 front door is unaffected.
On sequencing with #5762
Approving and enqueuing this answers your sequencing question: this lands first, and #5762 rebases onto it. Your scoping is correct — The Eighth Doctor is Effect::Unimplemented at the permission seam #5762 fixes, never reaches this classifier, and doesn't appear in your parse-diff, so the two are complementary rather than competing. The only overlap is textual (add_target_replacement.rs, oracle_effect/mod.rs); the queue's own CI will surface any conflict if #5762 is retargeted before this merges. No need to hold.
Nice work — the seam reasoning, the three revert-failing regressions, and parameterizing GrantReplacement as a member of the existing grant family (rather than special-casing the redirect) are exactly what I want to see.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Closes #6566 (filed at your request during the #6538 review).
Summary
A quoted grant of a replacement-shaped ability parsed to an inert
ContinuousModification::GrantAbility: the concretized body was pushed intoobj.abilitiesand never intoreplacement_definitions, so the redirect silently did nothing. Geth, Thane of Contracts — "Return target creature card from your graveyard to the battlefield. It gains "If this creature would leave the battlefield, exile it instead of putting it anywhere else."" — reanimated a creature that then went to the graveyard on any battlefield exit.Adds
ContinuousModification::GrantReplacement, the replacement-store sibling ofGrantAbility/GrantTrigger/GrantStaticAbility(CR 614 is a categorically distinct sink from CR 602/603/604). It is applied at layer 6 with structural dedup and re-derived every layer pass (CR 613.1f), so its lifetime follows the granting continuous effect rather than an object-lifetime stamp:[GrantReplacement, GrantTrigger]), so that guard is load-bearing and has its own revert-failing regression.This is deliberately not #6538's host-lifetime stamp — that path is for standalone (non-granted) riders and is unchanged here; the two now share one
ReplacementDefinitionconstructor.Blocker fixed along the way (the feature is a silent no-op without it):
normalize_card_name_refsrewritesthis creature|permanent|land→~card-wide (CR 201.5b), butparse_leave_battlefield_rider_refhad no~arm, so every quoted grant failed to parse before reaching the classifier. The added arm is purely additive; I confronted all 74 corpus cards containing "would leave the battlefield" and no standalone rider uses athis <type>subject, so the #6538 front door is provably unaffected.Relationship to #5762 (disclosed — please tell me how you'd like this sequenced)
@ntindle's open #5762 (Play historic from graveyard once/turn + granted leave-battlefield replacement rider) addresses The Eighth Doctor at a different seam: it threads an optional
ReplacementDefinitionthroughStaticMode's play-from-graveyard permission. Mine routes quoted grants through the classifier into a layer-6GrantReplacement.They look complementary rather than competing — The Eighth Doctor is explicitly out of scope here precisely because its whole clause is
Effect::Unimplemented { name: "static_structure" }at the permission seam #5762 fixes, so it never reaches my classifier and does not appear in my parse-diff. That said, the two branches touchadd_target_replacement.rsandoracle_effect/mod.rs, so they will conflict textually. Happy to rebase onto whichever lands first, or to hold this until #5762 resolves — your call; I did not want two granted-replacement lifecycle changes sitting in the queue without flagging it.Implementation method (required)
Method: /engine-implementer — plan → /review-engine-plan (3 rounds; round 1 corrected the card set and identified Geth Thane of Contracts as the real card, round 2 caught the
~no-op blocker, round 3 resolved which cards actually flip) → implement → /review-impl (3 rounds; round 1 required the Expressionist EOT-lapse regression, the final pass required the mixed-def guard test) → commit. Each step in a fresh agent context. Final read-only /review-impl: PASS head=054b54d30d76eccd659b71fb01f2763576b3353d.Gate A
./scripts/check-parser-combinators.sh→ Gate A PASS head=054b54d30d76eccd659b71fb01f2763576b3353d (Gate G PASS). The only parser change is onetag("~")arm composed into an existingalt— no string probes.Anchored on
ContinuousModification::GrantTrigger(types/ability.rs) + its Layer-6 apply (game/layers.rs) — the sibling grant that lands in a different store (obj.trigger_definitions);GrantReplacementmirrors it exactly ontoobj.replacement_definitions, with theGrantStaticAbilitystructural-dedup push as the idempotency idiom.game/layers.rsbase→live reseed — the store is reset to base every pass, which is why the layer-native design needs no stamp, no base-install, and no host-exit prune (the grant's own duration governs, viaprune_end_of_turn_effects/prune_affected_object_left_effects).game/mod.rsgame::zones— the documented#[cfg(any(test, feature = "test-support"))] pub/#[cfg(not(…))] pub(crate)precedent reused verbatim to expose the shared constructor to the integration-test crate (the feature is enabled only from[dev-dependencies];cargo tree -e normalshowsengine FEATURES=[]).Verification
cargo fmt --allclean;cargo clippy -p engine --tests0 warnings;cargo build -p engine -p phase-aiclean.crates/engine/tests/integration/issue_6566_granted_leave_exile.rs, registered inmain.rs) — 5 passed: Geth verbatim-Oracle reanimate → destroy → exile (with an un-granted control creature → graveyard as the paired reach guard); permanent grant survives end-of-turn cleanup; duplicate grants dedup to one definition; Expressionist-shaped until-end-of-turn grant exiles same turn and lapses at cleanup (post-cleanup destroy → graveyard); mixed[GrantReplacement, GrantTrigger]def is not promoted to permanent.~rider row, theit-subject no-regression row,classify_quoted_inner→GrantReplacement, a non-rider quoted body → notGrantReplacement, layer classification, and a byte-identity assertion that the shared constructor still emits fix(engine): unearth's leave-battlefield exile replacement must survive layer resets (Rotting Rats + Sacrifice) #6538's exactReplacementDefinition+AddTargetReplacement { target: Any }wrapper.tag("~")arm fails the parser row and all integration rows; dropping the&& duration.is_none()conjunct fails only the Expressionist EOT row (the three Geth rows still pass — which is exactly why that regression was added); swapping the mixed-def.all→.anyfails only the mixed-def row.docs/MagicCompRules.txt: 614.1a, 614.6, 613.1f, 611.2a, 514.2, 201.5b, 702.84a.ContinuousModificationarms added at all 10 wildcard-free match sites (compile-finalized, including 3 the compiler surfaced beyond the planned list).Claimed parse impact
Regenerated
card-data.jsonon this head and counted occurrences of the new variant (brand-new, so every occurrence is a flip): exactly 5 cards —elemental expressionist,geth, thane of contracts,llanowar greenwidow,realmbreaker, the invasion tree,spirit-sister's call. Explicitly verified absent: Magar of the Magic Strings and The Eighth Doctor (both swallowed upstream byEffect::Unimplemented, so untouched by this change) and Geth, Lord of the Vault (no such rider on that printing — my original issue text mis-named it; corrected in #6566).Scope Expansion
None.
🤖 Generated with Claude Code
Summary by CodeRabbit
~/self-reference variants in replacement parsing.