fix(engine): attach Copy Enchantment when it enters as a copy of an Aura (#4782)#5824
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements CR 303.4f and CR 303.4g to correctly handle cases where a permanent (such as Copy Enchantment) enters the battlefield and becomes a copy of an Aura post-entry. It introduces resolve_entering_aura_attachment to handle the attachment of the newly realized Aura after layers are flushed, along with comprehensive tests. The review feedback identifies an important edge case where the object might leave the battlefield during intermediate triggers or replacement effects before attachment resolution, and suggests defensively guarding against this by ensuring the object is still in the battlefield zone.
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✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — copied “enchant player” Auras cannot complete the multi-host attachment choice.
🔴 Blocker
crates/engine/src/game/zone_pipeline.rs:1379-1393deliberately includesTargetRef::Playerin the legal Aura hosts, andresolve_entering_aura_attachmentforwards any multi-host set throughWaitingFor::ReturnAsAuraTargetat:1470-1473. The resume path then rejectsTargetRef::Playeroutright atcrates/engine/src/game/engine.rs:5527-5533. CR 303.4f requires the controller to choose a legal object or player; the current implementation therefore turns a valid copied enchant-player Aura with multiple legal players intoInvalidActioninstead of attaching it. The new tests cover onlyEnchant creatureobject hosts (engine_replacement.rs:5010,:5210).
🟡 Non-blocking
- None.
✅ Clean
- The current head correctly incorporates the prior battlefield-residency guard before it attempts attachment (
zone_pipeline.rs:1442-1449), addressing the earlier review finding. The parse-diff artifact for this head also reports no card-parse changes.
Recommendation: extend the shared ReturnAsAuraTarget resume/finalization path to carry and attach TargetRef::Player, then add a production-pipeline Copy Enchantment regression covering multiple legal player hosts (and retain the existing object-host cases).
|
Addressed matthewevans review blocker in 9650259. Added a production-pipeline Copy Enchantment regression for a copied Focused verification run locally:
The existing generic |
…ura (phase-rs#4782) Copy Enchantment ("You may have this enchantment enter as a copy of any enchantment on the battlefield") enters as a plain enchantment, then its `BecomeCopy` replacement realizes it into an Aura (e.g. Level Up) *after* it is already on the battlefield. The normal aura-attach slot in `move_object` (the `aura_enchant_filter` consult) had already run while the object was still its pre-copy, non-Aura self, so it was skipped — the copy sat unattached and was destroyed by the CR 704.5m unattached-Aura SBA before its copied ability could ever matter. Resolve the entering Aura's attachment once the copy is realized and layers are flushed. New reusable building block `zone_pipeline::resolve_entering_aura_attachment` (reusing the existing `aura_enchant_filter` + `legal_aura_attachment_targets` + `attach_to` primitives) auto-attaches a sole legal host (CR 303.4f), leaves an Aura with no legal host unattached for the CR 303.4g/704.5m SBA, or pauses on `ReturnAsAuraTarget` for a multi-host controller choice. Called at the end of `finish_copy_target_choice_entry` — after entry-trigger replay and batch drain — so a multi-host pause has nothing left to strand. Adds three integration tests covering the 0/1/many-host branches (auto-attach + SBA survival, no-host → graveyard, multi-host prompt → attach). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bjects Address Gemini review: `resolve_entering_aura_attachment` now returns `NotApplicable` if the realized copy is no longer on the battlefield. It runs as the last step of `finish_copy_target_choice_entry`, after entry-trigger replay and batch drain; if an intermediate trigger/replacement moved the Aura off the battlefield, attaching it or prompting for a host of a non-battlefield Aura would be invalid state (CR 303.4 / CR 704.5m attachment applies only to a battlefield Aura). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9650259 to
62339f3
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the copy-entry attachment path now handles object and player Aura hosts without losing the post-entry continuation.
🔴 Blocker
- None.
🟡 Non-blocking
- The contributor-template artifacts are audit-only and incomplete; current-head CI plus the real replacement/choice/SBA regression supply the evidence.
✅ Clean
engine_replacement.rscalls the singleresolve_entering_aura_attachmentauthority after copy realization;zone_pipeline.rshandles zone safety and player/object single hosts.- The regression drives the real Copy Enchantment pipeline, selects
TargetRef::Player, and verifies both attachment and SBA survival. - Current-head CI is green; the parser-diff reports no changes.
Recommendation: approve and enqueue.
Summary
Fixes #4782. Copy Enchantment entering as a copy of an Aura (e.g. Level Up / a Class) never got attached and died to the CR 704.5m unattached-Aura state-based action before its copied ability could function.
Root cause: Copy Enchantment (
You may have this enchantment enter as a copy of any enchantment on the battlefield) enters as a plain enchantment, then itsBecomeCopyreplacement realizes it into an Aura after it is already on the battlefield. The normal aura-attach slot inmove_object(theaura_enchant_filterconsult inzone_pipeline.rs) had already run while the object was still its pre-copy, non-Aura self, so it was skipped — the copy sat unattached andcheck_unattached_auras(CR 704.5m) sent it to the graveyard on the next SBA check.Fix: Resolve the entering Aura's attachment once the copy is realized and layers are flushed. New reusable building block
zone_pipeline::resolve_entering_aura_attachment(reusing the existingaura_enchant_filter+legal_aura_attachment_targets+attach_to/attach_to_playerprimitives):WaitingFor::ReturnAsAuraTargetfor a multi-host controller choice.It is called at the end of
finish_copy_target_choice_entry— after entry-trigger replay and batch drain — so a multi-host pause has nothing left to strand.Tests
Three integration tests covering the building block across the 0/1/many-host branches:
copy_enchantment_becomes_aura_copy_auto_attaches_and_survives_sba— single host → auto-attach + survives SBA.copy_enchantment_becomes_aura_copy_with_no_host_dies_to_sba— no host → graveyard.copy_enchantment_becomes_aura_copy_multi_host_prompts_then_attaches— multi-host →ReturnAsAuraTargetprompt → attach + survives SBA.Verification
cargo fmt --allcleancargo clippy -p engine --all-targetsclean (zero warnings)cargo test -p engine --lib→ 16591 passed, 0 failed, 7 ignoredReviewer notes (non-blocking, no
BUG/RULES BUG)replay_deferred_entry_eventspause); documented inline with a pointer for the next card that does.🤖 Generated with Claude Code