fix(parser): derive activation zone from a self-move's origin, not its destination

[cuinhellcat]

🤖 AI text below 🤖

Summary

CR 113.6m restricts an activated ability to the zone its cost or effect moves the source out of. The rule says nothing about where the card goes. activation_zone_from_self_effect additionally required destination: Zone::Battlefield, so a self-move to any other destination derived no zone at all and fell back to the CR 113.6 battlefield default.

Reported from a real game: two Bestial Bloodline Auras on the battlefield offered {4}{G}: Return this card from your graveyard to your hand. Both halves were wrong — the ability was offered where it does not function, and withheld in the graveyard where it does. The 124 Graveyard → Battlefield cards (Reassembling Skeleton class, CR 113.6m's own printed example) worked only because their destination happened to satisfy the extra constraint.

The fix deletes that pattern field. It is a constraint removal, not a new case: the function already generalised correctly along the origin axis — the doc-comment and the origin != Zone::Battlefield guard show the intent — but its author pinned the destination to the one value the motivating card used (#425, Talon Gates of Madara, Hand → Battlefield).

This is the engine defect scoped out of #7296, which named it in prose and deliberately built its fixtures on Cooped Up and Cage of Hands so they stay green once it is fixed. They do.

55 abilities across 55 cards gain activation_zone: Graveyard; no other ability changes in any direction, in either direction of the census. Two constraints were verified rather than assumed:

• Craft (21 cards) is untouched. synthesize_craft (database/synthesis.rs:655) builds its AbilityDefinition directly and never reaches parse_activated_ability_ir. The proof is structural, not incidental: those 21 sit at activation_zone: None despite carrying destination: Battlefield, which the pre-fix pattern would have accepted — so this derivation demonstrably never ran on them. Independently, their Exile{zone: Battlefield, filter: SelfRef} cost component makes activation_zone_from_self_cost yield Battlefield first anyway, and CR 702.167a + CR 113.6m's unless clause say the battlefield is correct.
• Kogla and Yidaro keeps Hand. It is the only parsed ability where the cost-side and effect-side derivations disagree (Discard this card → Hand; Shuffle this card into your library from your graveyard → Graveyard). The .or_else() order already resolved it, but only latently: the destination constraint (Library ≠ Battlefield) kept the effect side silent, so removing it makes that precedence load-bearing for the first time. CR 113.6j and CR 118.3 make a graveyard activation unpayable rather than merely suboptimal, and CR 113.6m's unless clause makes the effect side inapplicable by rule. A new test pins the ordering and asserts both derivations actually fire, so a future refactor cannot "simplify" it away.

Three parts of CR 113.6m are deliberately not implemented because each governs a measurably empty class at this corpus vintage; each has its extension point named in the doc-comment: the unless clause's effect half (0 cards), its Aura half (0 of the 7 Auras in the class qualify — the exception is satisfiable by a cost, effect or trigger condition, not only a trigger condition), and sentence 2's delayed-trigger case (0 operative cards; the 58 abilities carrying that shape are synthesized unearth, whose delayed move is Battlefield → Exile, i.e. the CR 113.6 default).

Files changed

• crates/engine/src/parser/oracle.rs — the fix: one deleted pattern field in activation_zone_from_self_effect. Its doc-comment rewritten (the rule quantifies over origin only; both destinations are live in the corpus; what the origin != Battlefield guard is really for and that its class is currently empty; the sub_ability recursion is kind-agnostic, which Lochmere Serpent depends on). The .or_else() chain's comment replaced with the three-authority statement CR 113.6b ≻ CR 113.6j ≻ CR 113.6m, naming Kogla and Yidaro as the discriminating card.
• crates/engine/src/parser/oracle_tests.rs — seven new rows plus two repairs. Adds the activation_zone assertion that was missing from parses_activate_only_timing_and_only_if_condition, which had parsed Gutterbones since day one while checking only activation_restrictions — that gap is why this survived. Repairs battlefield_self_changezone_leaves_activation_zone_unset, whose subject lowers to Effect::Bounce and so never reached the ChangeZone arm it was meant to control; it now carries an explicit variant reach-guard.
• crates/engine/src/database/synthesis.rs — two assertions on the existing Craft synthesis test pinning activation_zone == None and the battlefield-zoned self-exile cost.
• crates/engine/tests/integration/graveyard_to_hand_activation_zone.rs (new) — five runtime rows: the battlefield negative on a plain creature, the reported Aura case (attached, with CR 704.5m survival asserted rather than assumed), the graveyard positive resolving to hand, the Craft canary, and an over-restriction canary.
• crates/engine/tests/integration/main.rs — one mod line.

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

Plan → review → revise, four rounds to a clean plan review; then implement → committed checkpoint → /review-impl → findings addressed with code → final /review-impl. Each step ran in a fresh agent context; the author never reviewed its own work. Round 1 of plan review returned 7 blockers, round 2 returned 3, round 3 returned 2, round 4 clean. Two of those rounds found defects in test fixtures that would have passed green while proving nothing — a designated primary negative whose card carried two activation restrictions, so its ability was absent from legal_actions with or without the fix; and a byte-identity gate on a JSON artifact that is not deterministic (CoverageSummary.cards iterates a HashMap; three runs at the same SHA gave three digests), which would have stopped a correct run.

CR references

CR 113.6, CR 113.6b, CR 113.6j, CR 113.6m, CR 118.3, CR 207.2c, CR 602.1, CR 603.7, CR 608.2k, CR 702.57a, CR 702.84, CR 702.167a, CR 704.5m — each grepped from docs/MagicCompRules.txt before being written. (CR 702.84 is unearth; an earlier revision of this branch cited 702.83, which is exalted. Caught by review, corrected in 96daf61.)

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 — 18911 + 21 + 9 + 4857 passed, 0 failed, 15 ignored

• cargo clippy-strict — clean

• cargo fmt --all — clean

• ./scripts/check-parser-combinators.sh — Gate G PASS, Gate A PASS

• pre-commit — Gate P PASS (PreLowered ratchet: no producer count increased)

• MTGJSON_SKIP_REFRESH=1 ./scripts/gen-card-data.sh then a full activation_zone census diffed against a pristine baseline at the merge base — exactly 55 rows change, all None → Graveyard on Graveyard → Hand; 0 other abilities move in any direction; 12,927 activated abilities both sides; corpus totals Graveyard 279 → 334, Hand 656 → 656, Exile 1 → 1, Command 1 → 1. Measured against MTGJSON 5.3.0+20260810; CI parses its own, newer corpus, so new printings may add rows — any change to a row that existed at that vintage would not be this change.

• Two-way control, run twice (once per review round): with destination: Zone::Battlefield, restored, the parser rows fail on activation_zone, and the runtime rows fail in both directions — sibling_battlefield_pump_ability_still_offered at the battlefield negative (the reported bug, reproduced through legal_actions), and bestial_bloodline_activatable_from_graveyard_returns_to_hand at the graveyard positive with legal_actions returned [PassPriority]. Every failure is a value mismatch, not a missing fixture; each runtime negative asserts its restrictions are empty and its cost affordable, so the restriction gate cannot be what makes it pass.

Tilt is not installed in this environment; scripts/tilt-wait.sh would return 3 (cannot answer), never a build result, so the isolated-direct commands above were used. cargo nextest is likewise unavailable; cargo test was used.

Gate A

Gate A PASS head=4baa94e7508ce31588a039a84c422e446e9c3ea2 base=479ad396d46338d6cd571da8b6b7fd8f375a307e

Anchored on

• crates/engine/src/parser/oracle.rs:6746 — activation_zone_from_self_cost, the cost-side sibling of the changed function and the other half of the same .or_else() authority chain; unchanged here, and it keeps priority.
• crates/engine/tests/integration/talon_gates_from_hand_activation.rs — the end-to-end pattern for the same derivation in the Hand → Battlefield direction (issue Talon Gates of Madara bug — The last activated ability of [[Talon Gates of Madara]] can't be activated even with enough… #425), which the new integration file mirrors for the graveyard direction.

Final review-impl

Final review-impl PASS head=4baa94e7508ce31588a039a84c422e446e9c3ea2

Claimed parse impact

55 cards, all in one direction (activation_zone absent → Graveyard), all on a ChangeZone{origin: Graveyard, destination: Hand, target: SelfRef} ability. The parse-diff comment on this PR will be empty — see Validation Failures.

A-Earthquake Dragon, Abzan Devotee, Altar of the Wretched, Bestial Bloodline, Brackish Trudge, Chamber Sentry, Clattering Augur, Clay Revenant, Cleaving Reaper, Convenient Target, Crown of Skemfar, Deathless Ancient, Deathless Behemoth, Deathless Pilot, Durable Coilbug, Dutiful Griffin, Earthquake Dragon, Eldrazi Ravager, Eternal Dragon, Evershrike's Gift, Firewing Phoenix, Gangrenous Goliath, Gilded Assault Cart, Gollum the Abandoned, Gollum, Patient Plotter, Grim Reminder, Gutterbones, Hammer of Bogardan, Jarad, Golgari Lich Lord, Jungle Creeper, Kraul Swarm, Lochmere Serpent, Magma Phoenix, Merchant of Many Hats, Metalwork Colossus, Multani, Yavimaya's Avatar, Phantasmagorian, Pilgrim of the Ages, Project Deathlok Soldier, Repeating Barrage, Salvage Titan, Sanitarium Skeleton, Shard Phoenix, Skarrgan Firebird, Summon the School, Summoned Dromedary, Talons of Wildwood, The Sound of Drums, Tymaret, the Murder King, Undead Gladiator, Unshakable Tail, Vineweft, Vivien's Jaguar, Whiteout, World Breaker

50 are permanents (7 of them Auras, which is why this surfaced as an Aura bug) and 5 are instants or sorceries. For the permanents the fix both withdraws the wrong battlefield offer and unlocks the correct graveyard one; for the 5 the ability was simply dead, since an instant is never on the battlefield.

Two of the 55 are card-level supported: false for pre-existing, unrelated reasons — A-Earthquake Dragon's cost-reduction static and Grim Reminder's spell body. The ChangeZone item is supported: true on all 55, and no card's supported flips in either direction.

Scope Expansion

None.

Validation Failures

The CI parse-diff cannot see this change, and its silence is not evidence. ability_details (game/coverage.rs:3759) emits kind, duration, repeat_for, targeting, targets, conditional, timing and modal — but not activation_zone. None of the 158 distinct detail keys in the shipped export contains the substring activat, and coverage-data.json is byte-comparable across this change. So coverage-parse-diff will report zero clusters for a change that moves 55 cards, and every claim above rests on the card-data census and the runtime tests instead.

That blindness is a pre-existing gap in the instrument, not a property of this change: any change to a rules-load-bearing field that decides whether an ability is offered at all is currently invisible to the PR gate. The fix is one unconditional key in ability_details ("activates from" — verified free; "from" is already taken at coverage.rs:2743, :2803 and :3832, and build_ability_item silently drops duplicate keys at :4832). It is deliberately not in this PR: it is a 937-ability additive schema migration with a different risk profile, and bundling it would collapse its rows and this fix's rows into the same parse-diff clusters, making both unreadable. Happy to open it as a follow-up if maintainers want it, in which case this PR's own rows become legible for the first time.

cargo ai-gate was not run — crates/phase-ai/ is outside this change's scope. Measured instead: none of the 55 card names appears in crates/phase-ai/duel_decks/{standard,pioneer,modern,legacy,pauper} or crates/phase-ai/fixtures/, so no baseline is expected to move.

cargo engine-inventory ran clean but had no committed baseline to diff against (the artifact is gitignored and absent in a fresh worktree). The change adds, removes and renames no enum or variant and touches no file under types/, so an inventory delta is structurally impossible.

An attempt to verify the fix against the reporter's saved game file was made and abandoned as unsound, rather than reported as a pass: the ability text occurs 60 times across that save (zone lists, card definitions, five turn checkpoints), so editing one occurrence proves nothing about what the engine reads. Runtime evidence rests on the integration tests. Note also that a saved game legitimately keeps its serialized abilities, so an in-progress save carries the old behaviour forward; only new games pick up the corrected parse.

CI Failures

None.

Summary by CodeRabbit

• Bug Fixes

  • Corrected activation-zone detection for abilities that return themselves from the graveyard, including attached Auras.
  • Ensured cost-derived activation zones take precedence over effect-derived zones.
  • Preserved normal battlefield activation and timing restrictions for unaffected abilities.

• Tests

  • Added comprehensive coverage for parsing, offering, resolving, and rejecting abilities based on their activation zones.
  • Added regression checks for synthesized Craft abilities.

[coderabbitai]

Warning

Review limit reached

@matthewevans, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 312173ff-5ee8-473b-bde7-6d514e7dca4e

📥 Commits

Reviewing files that changed from the base of the PR and between a7494d1 and 5b4cd79.

📒 Files selected for processing (1)

• crates/engine/src/parser/oracle.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 68ef2bc2-4fd8-4063-8609-2e8166411366

📥 Commits

Reviewing files that changed from the base of the PR and between ff7a54a and a7494d1.

📒 Files selected for processing (2)

• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_tests.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_tests.rs

---

📝 Walkthrough

Walkthrough

Activation-zone derivation now recognizes self-sacrifice costs and traverses complete own-resolution ability trees for non-battlefield self-moves. Parser, synthesis, and integration tests cover precedence, branch handling, action availability, Aura behavior, and Craft activation.

Changes

Activation-zone behavior

Layer / File(s)	Summary
Activation-zone derivation and precedence crates/engine/src/parser/oracle.rs	The parser derives zones from self-sacrifice costs and non-battlefield self-targeted ChangeZone effects. Cost-derived zones take precedence over effect-derived zones.
Parser and synthesis regression coverage crates/engine/src/parser/oracle_tests.rs, crates/engine/src/database/synthesis.rs	Tests cover destination variants, recursive branches, negative cases, per-ability isolation, near-miss costs, and cost/effect precedence. Craft synthesis coverage confirms battlefield activation.
Activation-zone integration behavior crates/engine/tests/integration/graveyard_to_hand_activation_zone.rs, crates/engine/tests/integration/main.rs	Integration tests cover action availability, graveyard resolution, Aura handling, self-sacrifice costs, ordinary battlefield abilities, Craft activation, and module registration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Mergeability Score: ⚪ Minimal · up to a7494

This localized parser correction is supported by passing tests and targeted census/runtime validation, with no actionable merge-blocking risk remaining after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant IntegrationTests
  participant OracleParser
  participant ActionEngine
  participant GameState
  IntegrationTests->>OracleParser: parse self-return ability
  OracleParser-->>IntegrationTests: assign Graveyard activation zone
  IntegrationTests->>ActionEngine: submit activation from Graveyard
  ActionEngine->>GameState: resolve Graveyard to Hand change
  GameState-->>IntegrationTests: return updated game state

Loading

Possibly related issues

• coverage-parse-diff is blind to activation_zone (fourth instance, in ability_details rather than effect_details) #7317 — Both changes concern AbilityDefinition::activation_zone derivation and its effect on activation legality.

Possibly related PRs

• phase-rs/phase#6792 — Both PRs modify activation-zone derivation and precedence in crates/engine/src/parser/oracle.rs.
• phase-rs/phase#7113 — Both PRs use own-resolution ability-tree traversal in the parser.

Suggested labels: quality, needs-maintainer

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main parser fix: deriving the activation zone from a self-move's origin.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/engine/src/parser/oracle.rs (1)

6805-6822: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Traverse all ability branches when deriving the activation zone.

activation_zone_from_self_effect only follows sub_ability. A self-ChangeZone in else_ability or mode_abilities is therefore ignored. Extend the traversal across sub_ability, else_ability, and mode_abilities, matching the existing ability visitors.

🤖 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/parser/oracle.rs` around lines 6805 - 6822, The
activation-zone traversal in activation_zone_from_self_effect currently follows
only sub_ability; extend it to visit else_ability and every mode_abilities
branch as well. Reuse the existing ability-visitor traversal behavior and
preserve the current written-order handling while deriving zones from
self-ChangeZone effects.

Source: Learnings

🤖 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/parser/oracle.rs`:
- Around line 6909-6923: Update activation_zone_from_self_cost to recognize
Sacrifice costs whose target is TargetFilter::SelfRef and return
Some(Zone::Battlefield). Preserve the existing derivation precedence so
self-sacrifice costs select the battlefield activation zone instead of allowing
effect-side derivation to select the graveyard.

---

Nitpick comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 6805-6822: The activation-zone traversal in
activation_zone_from_self_effect currently follows only sub_ability; extend it
to visit else_ability and every mode_abilities branch as well. Reuse the
existing ability-visitor traversal behavior and preserve the current
written-order handling while deriving zones from self-ChangeZone effects.

🪄 Autofix

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: ae6c7ec8-3474-4917-9c9a-691ffed79841

📥 Commits

Reviewing files that changed from the base of the PR and between 479ad39 and 4baa94e.

📒 Files selected for processing (5)

• crates/engine/src/database/synthesis.rs
• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/tests/integration/graveyard_to_hand_activation_zone.rs
• crates/engine/tests/integration/main.rs

[github-actions]

Generated for head 5b4cd79d19beaa423cbdbd337b9a2ff6ec079e41.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Maintainer fixup pushed for current head ff7a54a: self-sacrifice cost authority now derives Battlefield; effect-side source-zone discovery uses the canonical own-resolution traversal, including sub/otherwise/modal branches; focused parser and runtime regressions added. Formatter and repository parser gates passed locally. Holding for required CI, exact-head parse-diff, and refreshed automated feedback before approval/enqueue.

[matthewevans]

Current-head recheck: required CI is now failing, so this PR remains held. The maintainer self-sacrifice inference adds serialized activation_zone: Battlefield to ordinary battlefield abilities, breaking the barbarian_ring_activated_ir and bomat_courier_ir snapshots in Rust test shards 3/4 and 4/4. The current fix must preserve the default None representation except where cost-side authority needs to beat a conflicting non-battlefield effect origin. Exact-head parse-diff is present but reports no changes because it does not surface activation_zone; it is therefore not sufficient to clear this regression. Refreshed CodeRabbit feedback has no actionable comments.

[matthewevans]

Maintainer follow-up pushed for current head a7494d1. It narrows the self-sacrifice source-zone authority so ordinary battlefield activations retain the default activation_zone: None; Battlefield is serialized only when it must override a conflicting non-battlefield self-move origin. A focused parser regression covers that representation boundary. Holding for fresh required CI, exact-head parse-diff, and automated feedback before approval/enqueue.

[matthewevans]

Current-head recheck: all Rust test shards, card-data, WASM/Tauri, frontend, security, exact-head parse-diff, and refreshed CodeRabbit feedback are clean. This PR remains held because the required Rust lint job fails at crates/engine/src/parser/oracle.rs:6928 on Clippy unnecessary_lazy_evaluations: the conflict-only activation-zone match uses Option::or_else where the value is already computed and must use Option::or. No approval/enqueue until that required lint gate is green.

[matthewevans]

Maintainer follow-up pushed for current head 5b4cd79d1900eae0924945e82109036316d030a8: the only change replaces the eagerly evaluated activation-zone or_else with or, satisfying the required Clippy diagnostic. Formatter and repository parser gates passed. Holding for fresh required CI, exact-head parse-diff, and refreshed automated feedback before approval/enqueue.

[matthewevans]

Correction: the exact current head is 5b4cd79d19beaa423cbdbd337b9a2ff6ec079e41.

[matthewevans]

Approved: current-head activation-zone derivation preserves the default battlefield representation while honoring self-sacrifice cost authority; required CI and exact-head parse evidence are green.