Fix Animate Dead / Dance of the Dead reanimation (#4767)

[rykerwilliams]

Summary

Fixes the reanimation effect for Animate Dead and Dance of the Dead — the enchanted creature was never actually returned to the battlefield, and (a compounding, separately-discovered prerequisite bug) the Aura could never even attach to its graveyard-zone cast-time target in the first place.

Closes #4767

Files changed

• crates/engine/src/types/ability.rs — new TargetFilter::OriginalSource variant (pre-forward_result-rebind source identity)
• crates/engine/src/game/effects/mod.rs — eager in-place OriginalSource concretization at the forward_result rebind site
• crates/engine/src/game/effects/effect.rs — concretizes Keyword::Enchant(ParentTarget) to a concrete SpecificObject
• crates/engine/src/game/effects/sacrifice.rs — "that creature's controller sacrifices it" now uses the creature's current controller for ParentTarget/ParentTargetSlot
• crates/engine/src/game/effects/delayed_trigger.rs — parent_target_snapshot prefers the node's own propagated targets over the TriggeringSource fallback when the root chain is empty (otherwise the delayed sacrifice snapshotted the Aura instead of the reanimated creature)
• crates/engine/src/parser/oracle_effect/mod.rs, oracle_trigger.rs — new whole-body, fail-closed recognizer building the ChangeZone → GenericEffect → Attach → CreateDelayedTrigger chain directly from Oracle text, generalized over the class's verb/destination axis (Animate Dead's "return … to the battlefield" / Dance of the Dead's "put … onto the battlefield tapped")
• crates/engine/src/game/ability_utils.rs, stack.rs — target re-validation and the Aura-spell-resolution attach guard now consult the Aura's own Keyword::Enchant filter instead of hardcoding battlefield-only, fixing every zone-scoped Enchant Aura (Animate Dead, Dance of the Dead, Spellweaver Volute), not just this card; corrected a mislabeled CR 303.4f citation to CR 608.3c
• crates/engine/src/game/ability_rw.rs, ability_scan.rs, cost_payability.rs, coverage.rs, filter.rs, trigger_matchers.rs — exhaustive-match arms for the new TargetFilter variant
• crates/mtgish-import/src/convert/condition.rs, crates/engine/src/parser/oracle_static/restriction.rs — same, in the mtgish-import name-lookup table and a match added upstream since this branch was cut
• crates/engine/src/game/casting_tests.rs, oracle_trigger_tests.rs — new tests (see Verification)

CR references

• CR 608.2c — resolution-order anaphora / self-modification binding
• CR 613.1f — Layer 6 ability add/remove (the Enchant keyword swap)
• CR 701.3a / CR 701.3b — Attach mechanics
• CR 701.21a — Sacrifice performed by the object's own current controller
• CR 603.7c — delayed triggered ability creation-time snapshot binding
• CR 400.7 — new-object identity on zone change
• CR 303.4a — an Aura spell's target is defined by its enchant ability
• CR 608.3c — Aura-spell-resolution attach (corrected from a mislabeled CR 303.4f, which explicitly excludes resolving as a spell)

Track

Developer

LLM

Model: claude-sonnet-5
Thinking: high

Verification

• cargo fmt --all — clean
• cargo clippy --all-targets -- -D warnings — clean
• ./scripts/check-parser-combinators.sh — clean (exit 0)
• cargo test -p engine — full suite green, 0 failures, both before and after merging origin/main
• New discriminating runtime tests drive the real handle_cast_spell → resolve_top pipeline end-to-end: cast → resolve the Aura spell → resolve the ETB trigger → assert the creature is reanimated, controlled by the caster, correctly attached (not backwards), survives an SBA check, and has -1/-0 applied; a sibling test drives the delayed leaves-battlefield sacrifice; a hostile fixture proves the sacrifice is performed by the creature's current controller after a control change, not the Aura's original controller. New parser tests cover both Animate Dead and Dance of the Dead producing the isomorphic chain shape with zero Effect::Unimplemented.
• Live-revert-and-rerun performed at each layer during development (parser recognizer, runtime concretization, sacrifice guard, parent_target_snapshot fallback) — each confirmed the relevant test fails without its fix and passes with it restored.

Gate A

$ ./scripts/check-parser-combinators.sh
(exit 0, no violations)

Anchored on

• crates/engine/src/game/database/unearth.rs — existing forward_result-chained ETB reanimation pattern (self-return case), traced as the initial analog before determining the reanimator-Aura shape (moved object ≠ ability source) needed the OriginalSource identity channel instead
• crates/engine/src/game/effects/attach.rs:60-61 — existing use of sba::is_valid_attachment_target as the single legality authority at an initial-attach call site, mirrored by the stack.rs fix
• crates/engine/src/parser/oracle_effect/mod.rs (try_parse_grant_graveyard_keyword_to_target and sibling whole-body recognizers) — existing pattern for a fail-closed, whole-effect-body parser recognizer dispatched via .or_else() and wrapped as TriggerBody::PreLowered, mirrored exactly by the new reanimator-Aura recognizer

Tier

Standard

Scope Expansion

This bug had two compounding root causes, not one: the ETB effect parser gap (the issue's literal report) and a separate, previously-undiscovered prerequisite — the Aura could never attach to a graveyard-zone target at spell resolution in the first place, via two resolution-path checks that hardcoded battlefield-only instead of consulting the Aura's own zone-scoped Enchant filter. Both were required to actually fix the reported behavior; the second is a general fix benefiting every zone-scoped Enchant Aura, not scope creep on a single card.

Validation Failures

None.

CI Failures

None.

[gemini-code-assist]

Code Review

This pull request implements support for reanimator-Auras like Animate Dead and Dance of the Dead by introducing the OriginalSource target filter and a dedicated ETB trigger parser, alongside updates to target validation, delayed triggers, and Aura resolution. The review feedback highlights two important improvements: ensuring that sacrifice actions are correctly attributed to the permanent's current controller under CR 701.21a, and resolving a latent target-dropping hazard in the Effect::Attach validation branch to prevent sibling targeted nodes from losing their targets.

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.

[gemini-code-assist]

[HIGH] Ensure sacrifice event/action is attributed to the object's current controller. Evidence: crates/engine/src/game/effects/sacrifice.rs:368-372.\n> Why it matters: Under CR 701.21a, only the permanent's controller can sacrifice it, so bypassing the controller check without updating the acting player in the sacrifice event will attribute the sacrifice to the wrong player.\n> Suggested fix: Update the sacrifice action below this check to use obj.controller instead of ability.controller when performing the sacrifice.

References

1. Strict fidelity to the MTG Comprehensive Rules (CR) is a non-negotiable pillar of the codebase. Every game rule, validation, and computed value must match the CR exactly. ^(link)

[gemini-code-assist]

[MEDIUM] Resolve the latent target-dropping hazard in Effect::Attach validation. Evidence: crates/engine/src/game/ability_utils.rs:1475-1484.\n> Why it matters: Dropping live SelfRef and ParentTarget entries during target validation is a latent bug that will cause sibling targeted nodes in the same chain to lose their targets.\n> Suggested fix: Refactor the Effect::Attach validation branch to preserve existing targets instead of returning an empty list when no explicit target slots are consumed.

References

1. Is this the most architecturally idiomatic approach for this codebase? Surface GAPS (things missing or wrong), not style nits. ^(link)

[github-actions]

Parse changes introduced by this PR

Baseline pending for d61c941877270f323c8d33ee84201ed6824d1d11 — this populates once main publishes its coverage snapshot (a few minutes after that commit landed).

[matthewevans]

[MED] The new battlefield-condition arm corrupts conjunctive intervening-if clauses. Evidence: crates/engine/src/parser/oracle_trigger.rs:4231 adds the "if it's on the battlefield" pattern to try_extract_simple_condition, whose implementation at crates/engine/src/parser/oracle_trigger.rs:5587 uses an unguarded substring find() and strip_condition_clause rather than parsing the whole condition. The PR parse-diff sticky confirms the blast radius: "Name Sticker" Goblin loses its RollDie (sides=20) signature and gains a stray you signature, because its Oracle text is if it's on the battlefield and you control 9 or fewer creatures ... , roll a 20-sided die. Why it matters: this regresses an already-supported trigger by stripping only the first half of a compound condition and feeding the residual and you control... into effect parsing. Suggested fix: route this through the existing condition parser/combinators so the full if A and B, ... predicate is preserved as a composite condition, or make the simple arm fail unless the matched phrase is the entire condition clause (comma/end boundary) so conjunctive forms remain on the existing path.

I checked the existing Gemini comments against this head. The sacrifice attribution concern is already addressed: the resolver now uses obj.controller for the muzzling check and sacrifice_permanent after the relaxed ParentTarget guard. I did not carry the Attach validation note as the blocking finding because this PR's current failing evidence is the parse-diff regression above.

[rykerwilliams]

Fixed in 923e277.

The regression (try_extract_simple_condition matching a substring): Name Sticker Goblin's conjunctive condition ("if it's on the battlefield and you control 9 or fewer creatures named ~, roll a 20-sided die") was matched only on its leading half, corrupting the residual "and ..." text into the effect body and dropping the RollDie effect entirely — confirmed by re-tracing the actual production parse before the fix (condition: None, but the RollDie effect body was gone). Went with your second suggested option: the matched phrase must now be immediately followed by a clause boundary (comma/period/end), so a conjunctive form correctly falls through to the general condition parser instead of half-matching. Added a regression test using the card's verbatim Oracle text (confirmed RollDie parses fully intact), plus a sibling test proving Animate Dead's own non-conjunctive case still matches. Live-revert-and-rerun confirmed the regression test fails without the fix and the sibling passes either way (proving the fix is correctly scoped, not just avoiding the corruption by breaking the whole table entry).

The Attach target-dropping hazard Gemini flagged: also fixed. validate_targets_in_chain's Effect::Attach branch was rebuilding kept from scratch across the attachment/target loop, silently dropping any target entries beyond what this node's own two operands consume. For the shipped Animate Dead/Dance of the Dead shape this was harmless (confirmed: that chain never reaches this function at all, per the existing NOTE comment), but it's a real latent hazard for any future chain where an Attach node propagates extra targets through for a downstream sibling. Now appends any un-claimed target_iter entries to kept so they pass through unchanged. Added a discriminating test and confirmed via live revert-and-rerun.

Full cargo test -p engine green (0 failures), clippy clean (-D warnings), parser combinator gate clean.

[rykerwilliams]

One more note on the updated parse-diff sticky: the `Scourge of the Throne` line (`condition: source is attacking → ∅`, `UntapAll added`, `the removed`) is a bonus fix, not a new regression.

Its real text is "Whenever this creature attacks for the first time each turn, if it's attacking the player with the most life or tied for most life, untap all attacking creatures." The pre-existing `("if it's attacking", TriggerCondition::SourceIsAttacking)` table entry (unrelated to this PR, already present on main) had the identical unguarded-substring bug: it matched the leading "if it's attacking" and fed the residual "the player with the most life or tied for most life, untap all attacking creatures" into effect parsing, silently corrupting the `UntapAll` effect (hence the missing `UntapAll`/stray `the` on main today). Since my clause-boundary fix in 923e277 applies to the whole shared `try_extract_simple_condition` function, not just the two new entries, it also fixes this pre-existing case: the condition now correctly falls through to `∅` (an honest gap for this specific "attacking a particular player" predicate, which has no other parser path today) instead of the wrong bare "is attacking," and the effect body is no longer corrupted, so `UntapAll` now parses correctly.

[matthewevans]

[MED] The boundary guard fixes the Name Sticker regression but now drops Scourge of the Throne's intervening-if condition while marking the effect supported. Evidence: crates/engine/src/parser/oracle_trigger.rs:5598 only accepts "if it's attacking" when the next byte is comma/period/end, so Scourge's "if it's attacking the player with the most life or tied for most life" no longer maps to SourceIsAttacking; the current parse-diff sticky confirms Scourge of the Throne changes from condition: source is attacking to condition: ∅ while adding UntapAll (filter=attacking creature). Why it matters: this turns a conditionally supported trigger into an unconditional one, so the engine would untap attackers and grant the extra combat even when Scourge is attacking the wrong player. Suggested fix: make the simple-condition guard distinguish true conjunctive tails from grammatical object tails like the player..., or add a typed condition for the full attack-target/life-total predicate; if that full condition is not supported yet, keep this card as an honest unsupported/strict-failure parse rather than dropping the condition.

I rechecked the prior blocking Name Sticker finding against the new head and that part is addressed: the new regression test keeps RollDie intact, and the Animate Dead sibling still parses. I also checked the Gemini Attach target-retention note; the current head preserves unclaimed propagated targets and has a focused unit test for that path.

[rykerwilliams]

Fixed in 0ebea2c.

You're right — my earlier defense of the `None` result was wrong for this specific case. Dropping the condition entirely doesn't produce an "honest gap" here, since `None` evaluates as unconditionally true — the ability would fire regardless of who's being attacked, which is strictly worse than the pre-existing bug (which just failed to do anything useful).

Went with your second suggested option ("add a typed condition for the full attack-target/life-total predicate"): CR 702.105a's "attacks the player with the most life or tied for most life" isn't a novel predicate — it's exactly what Dethrone's own synthesized trigger already models (`build_dethrone_trigger`, `database/synthesis.rs`: `TriggerCondition::QuantityComparison(DefendingPlayer LifeTotal >= AllPlayers::Max LifeTotal)`). Added a nom-based recognizer (`scan_preceded` + `tag`/`alt`, per the project's combinator mandate — not the find()-based simple-condition table) that maps the intervening-if phrasing to that identical typed condition, ordered ahead of the plain "if it's attacking" entry so the longer, more specific phrase is never partially shadowed. So Scourge of the Throne now gets the correct, real gating rather than either the original corruption or an unconditional pass-through.

Verified via live revert-and-rerun (new test fails without the recognizer, passes with it restored). Full `cargo test -p engine` green, clippy clean, parser combinator gate clean.

[rykerwilliams]

Flagging one more parse-diff line for transparency, not yet fully investigated: The Invincible Iron Man shows ability/Attach field conditional: "target is Equipment" → "Equipment changed zones this way".

This card's real text is "At the beginning of combat on your turn, you may put an artifact card from your hand onto the battlefield. If it's an Equipment, attach it to The Invincible Iron Man" — the same "moved object = attachment, self = host" polarity as Armored Skyhunter/Quest for the Holy Relic. It's plausible this is a beneficial side-effect of the ability_utils.rs Attach-target-preservation fix (this PR), and it may be related to the currently-open issue #4796 ("The Invincible Iron Man does not attach Equipment put onto the battlefield") — but I haven't verified that end-to-end and don't want to overclaim a fix for an issue outside this PR's scope. Noting it here so it's visible rather than silent; happy to open a dedicated follow-up to verify #4796 separately if useful.

[matthewevans]

[MED] The new "if it's on the battlefield" trigger-condition arm regresses Hex, Kellan's Companion by narrowing an exile-or-battlefield spell-cast trigger to battlefield-only and turning its perpetual buff into a regular pump. Evidence: crates/engine/src/parser/oracle_trigger.rs:4276 adds the broad simple-condition pattern, and the current parse-diff sticky reports Hex, Kellan's Companion changing SpellCast active in: battlefield, exile → battlefield, condition: source is in battlefield or source is in exile → source is in battlefield, removing ApplyPerpetual, and adding Pump (p/t=+1/+1, target=any target). Why it matters: Hex's ability is designed to function from exile as well as the battlefield and to apply a perpetual +1/+1; after this parse change, the exile half is no longer represented and the supported battlefield half has the wrong effect duration. Suggested fix: make the battlefield-condition extraction fail closed for nested/conditional effect bodies like Hex, or route that text through the existing exile-or-battlefield/perpetual parser so Hex remains byte-equivalent while the reanimator-Aura ETB class still gets SourceInZone { Battlefield }.

I rechecked the earlier two findings on the current head: the Name Sticker residual-condition corruption is fixed, and Scourge now maps to a typed defending-player life-total comparison instead of dropping the condition. This new Hex parse-diff regression is the remaining blocker.

[matthewevans]

Maintainer sign-off: reviewed current head f3/final 1809260 after maintainer fixups; CI green; prior requested changes resolved.