fix(engine): auto-reattach Gift of Immortality at next end step - #6808
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe parser preserves explicit Aura attachment hosts for battlefield returns, while engine resolution propagates parent targets and tracked sets through delayed triggers. Integration tests exercise normal trigger processing and verify deterministic Aura and Equipment reattachment. ChangesDelayed Aura reattachment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant AbilityAssembly
participant DelayedTrigger
participant AbilityResolver
participant ZonePipeline
OracleParser->>AbilityAssembly: parse battlefield return with attach_host
AbilityAssembly->>DelayedTrigger: create forwarded delayed ChangeZone and Attach
DelayedTrigger->>AbilityResolver: snapshot ParentTarget and resolve chain
AbilityResolver->>ZonePipeline: return Aura and apply attachment
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Maintainer hold for current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_effect/mod.rs`:
- Around line 15778-15779: Update the attach-host parsing flow around
parse_search_attach_host so it preserves and threads the parser’s unconsumed
remainder instead of replacing rem with an empty string. Change the parser
result or surrounding logic to distinguish the consumed host rider from trailing
instructions, while still requiring or handling full consumption when
appropriate.
In `@crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs`:
- Around line 447-493: Add a reach-guard assertion immediately after the first
drain_priority in gift_of_immortality_stays_in_graveyard_when_host_gone,
verifying that exactly one delayed trigger was installed before exiling the
host. Keep the existing graveyard and waiting_for assertions unchanged.
🪄 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: 804b67b9-3c37-47c4-b63f-3947105d16f9
📒 Files selected for processing (13)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_util.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR · 43 card(s), 24 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head parser correctness defects.
🔴 Blocker
-
crates/engine/src/parser/oracle_effect/mod.rs:15778-15792parses an attached-return host then replacesremwith"".parse_search_attach_hostaccepts continuation delimiters, so rules-bearing suffixes are silently discarded. Current parse-diff includes Cass, Hand of Vengeance (…, then attach any number of Equipment …) and shows lost Storm Herald/Takklemaggot signatures. Preserve the unconsumed remainder and let normal continuation parsing lower it; add regression coverage for the continued and lost-semantic forms. -
crates/engine/src/game/effects/mod.rs:6491-6508hydrates only the rootChangeZone.target; its matcher at:6546-6565never reaches nestedAttach { target: ParentTarget }.crates/engine/src/game/effects/change_zone.rs:69-87consequently resolvesParentTargetonly from existing ability targets or the Aura's prior attachment. Event-subject return Auras (the 13-card ParentTarget parse-diff cluster, including Dragon Breath and Smoke Shroud) therefore cannot bind to the creature that entered and can fall back to the ordinary Aura choice atzone_pipeline.rs:2924-2950. Carry/bind the trigger event referent to the nested attach host and add a multiple-legal-host runtime regression that proves no prompt and the correct attachment.
🟡 Non-blocking
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:469-492 lacks a reach guard after the first drain_priority; assert the delayed reattach was installed before exiling the host, or the negative assertion can pass without exercising the hostile path.
Recommendation: fix the two parser/runtime blockers, preserve coverage honesty for the full parse-diff class, and strengthen the hostile-path test before re-review.
There was a problem hiding this comment.
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/game/effects/mod.rs (1)
10353-10394: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSkip
ParentTargetin the attach rebind path
Incrates/engine/src/game/effects/mod.rs, the genericAttachbranch still appendsability.source_idon the success path. Sinceeffect_object_targetstreatsParentTargetas the full fallback target list, that rebased source is the returned Aura itself, soAttach { target: ParentTarget }can resolve against the wrong object. ExemptParentTargethere the same wayLastCreatedis exempted.🤖 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/effects/mod.rs` around lines 10353 - 10394, The generic Attach rebind path must exempt ParentTarget from appending ability.source_id, matching the existing LastCreated exemption. Update the Attach handling in the relevant effect resolution logic so ParentTarget preserves the parent target list and does not rebase to the returned Aura, while leaving other target types unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs (1)
751-792: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCass and Storm Herald continuation-preservation are only validated at the parsed-AST level, not through the real resolution pipeline.
cass_preserves_equipment_reattach_continuationandstorm_herald_preserves_delayed_exile_continuationonly assert on the shape ofparsed.triggers[0].execute(viaability_chain_contains_equipment_attach/ability_chain_contains_delayed_exile). Neither drives aGameRunnerto confirm the equipment reattach or delayed-exile continuation actually executes correctly after the new attach-host lowering (e.g., that the trailingAttach{Equipment}sub-ability actually fires post-assembly, or that the delayed exile CDT installs and fires at end step).smoke_shroud_attaches_to_entering_ninja_among_multiple_hostsshows this is achievable for this PR's card classes — Cass/Storm Herald are left without equivalent runtime coverage even though they exercise the same new attach-host parsing/lowering/assembly code this PR touches.As per path instructions, "a parser AST shape test does NOT prove runtime semantics or a coverage-support claim" for
crates/engine/tests/**.🤖 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_4956_gift_of_immortality_reattach.rs` around lines 751 - 792, Extend cass_preserves_equipment_reattach_continuation and storm_herald_preserves_delayed_exile_continuation with GameRunner-based integration scenarios that resolve each card’s ability through the real pipeline. Assert Cass’s trailing Equipment attach continuation executes after assembly, and Storm Herald’s delayed exile continuation installs and fires at end step, using the existing runtime setup pattern from smoke_shroud_attaches_to_entering_ninja_among_multiple_hosts while retaining the AST checks.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.
Outside diff comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 10353-10394: The generic Attach rebind path must exempt
ParentTarget from appending ability.source_id, matching the existing LastCreated
exemption. Update the Attach handling in the relevant effect resolution logic so
ParentTarget preserves the parent target list and does not rebase to the
returned Aura, while leaving other target types unchanged.
---
Nitpick comments:
In `@crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs`:
- Around line 751-792: Extend cass_preserves_equipment_reattach_continuation and
storm_herald_preserves_delayed_exile_continuation with GameRunner-based
integration scenarios that resolve each card’s ability through the real
pipeline. Assert Cass’s trailing Equipment attach continuation executes after
assembly, and Storm Herald’s delayed exile continuation installs and fires at
end step, using the existing runtime setup pattern from
smoke_shroud_attaches_to_entering_ninja_among_multiple_hosts while retaining the
AST checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ee445e2f-90e7-4195-a67c-f07fa058b1f6
📒 Files selected for processing (6)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head attachment regression and failing parser gate.
🔴 Blocker
crates/engine/src/game/effects/mod.rs:6501-6522 always calculates nested_host_ref even when the root ability already has a chosen target. nested_forward_result_attach_parent_target_ref at :6531-6550 then injects the trigger-event referent into the Attach sub; change_zone.rs:37-46 gives that sub target precedence. This overwrites an explicit chosen attachment host. The current ParentTarget parse-diff class includes Necrotic Plague: it must return to the controller's chosen creature, not the creature that died. Hydrate an event referent only when the chain has no bound parent target, and add a runtime regression with distinct dying/chosen creatures.
🟡 Non-blocking
The current Rust lint job is red because sequence.rs:12980 and :12995 use starts_with in parser-gate-covered code. Rewrite those assertions using the approved parser/string convention so the required gate is green.
Recommendation: preserve explicit parent targets, add the discriminating Necrotic Plague test, and clear the parser-gate failure before re-review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/issue_4956_gift_of_immortality_reattach.rs`:
- Around line 892-980: Strengthen the test around the parsed execute chain:
assert that the return clause’s relevant sub-ability, including the
Attach→ParentTarget path found by find_attach_parent, has forward_result
enabled, rather than relying on manually setting return_aura.forward_result.
Drive the parsed ability through the production resolution pipeline instead of
reconstructing return_aura, so a dropped parser-to-resolution flag fails the
test. Replace the vacuous WaitingFor::ReturnAsAuraTarget check with the sibling
tests’ drain_priority flow to observe the actual prompt.
🪄 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: 38034d5b-165a-4d7f-ad8e-28bb3b350633
📒 Files selected for processing (3)
crates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_effect/sequence.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
[P2] Exercise the parsed Necrotic Plague chain
The new regression test proves only that the parsed tree contains an Attach { target: ParentTarget }; it then constructs a separate ResolvedAbility and manually sets forward_result before resolving it. That leaves both parser-to-runtime contracts changed here untested: the parsed ChangeZone.forward_result wiring and propagation of the resolution-time TargetOnly choice into that ChangeZone. Resolve the parsed trigger through the normal target-selection/priority flow with distinct dying and chosen creatures, then assert it returns attached to the chosen one and never opens ReturnAsAuraTarget (or at minimum structurally assert the exact parsed TargetOnly → forward-result ChangeZone → Attach chain).
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head parser gate and broad parse-proof gap
🔴 Blocker
-
crates/engine/src/parser/oracle_effect/sequence.rs:12980-12982,12995-12997still uses addedstrip_prefixparsing dispatch in the parser-combinator-gated surface. The current-head Rust-lint job rejects those exact lines, so the required gate is red. Make the remainder check an exact expected-string assertion or express it throughnomtag(...).parse(...). -
The current-head coverage-parse-diff affects 27 cards / 17 signatures, including removed Storm Herald delayed-trigger and changed Cass attachment signatures, but
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:761-800only inspects parsed ASTs for those continuations. Resolve representative Cass/Storm chains through the production pipeline and reconcile each removed/changed signature against intended semantics; if a clause remains unimplemented, preserve strict failure rather than expanding supported coverage.
✅ Already covered
The existing current-head review’s Necrotic Plague parser-to-runtime test finding remains valid; no need to repost it.
Recommendation: fix the parser gate and provide runtime/coverage evidence for the actual 27-card parser blast radius, then re-review.
…strings — combinator gate passes
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head still lacks required proof for this parser/runtime change.
🔴 Blocker
-
crates/engine/src/parser/oracle_effect/imperative.rs:14732-14733currently fails the required Rust-lint job: Clippy reportsfield_reassign_with_defaultforlet mut ctx = ParseContext::default(); ctx.subject = …. InitializeParseContextwith thesubjectfield in its struct literal so the mandatory lint gate can proceed. -
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:1060-1079(Cass) and:1371-1386(Necrotic Plague) preselect a root target and then manually stamp it into every nestedAttach. That bypasses the parser-to-resolution contract this PR changes;Effect::TargetOnlyitself only consumes already selected targets atcrates/engine/src/game/effects/mod.rs:4158-4196. A broken forward-result/target-propagation path can therefore pass both tests. Drive the parsed triggers through real trigger target selection and priority resolution with distinct dying and chosen hosts; assert the chosen attachment and that noReturnAsAuraTargetprompt occurs, without astamp_hosthelper. -
The required parse-diff sticky is not current-head evidence: comment
5127806035was last updated at2026-07-30T12:10:12Z, before current head18c79f…was created at14:36Z. Its prior 27-card/17-signature result cannot establish this head’s parser blast radius. Publish the current-head full sticky and reconcile the changed/removed signatures before re-review; preserve strict failure for any clause that remains unresolved.
Recommendation: clear the lint failure, replace the manual-host tests with real selection-to-resolution coverage, and publish/reconcile the current-head parse diff before re-review.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/engine/src/game/engine_resolution_choices.rs (1)
6615-6667: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBottom-narrowing logic is now duplicated with
finish_effect_zone_put_at_library_position.Lines 6969-6991 contain the same
chain_tracked_set_id→ filter-out-chosen→ library-zone-filter → insert sequence forLibraryPosition::Bottom. Since this helper is meant to be the single publication authority forEffectZoneChoiceselections, have that completion call it instead so the two cannot drift (they already differ on the empty-set behavior).🤖 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/engine_resolution_choices.rs` around lines 6615 - 6667, Remove the duplicated bottom-narrowing and tracked-set insertion logic from finish_effect_zone_put_at_library_position, and invoke publish_effect_zone_choice_tracked_set there with the completed choice, effect kind, and library position. Preserve the helper’s existing empty-set behavior and make it the sole publication authority for these EffectZoneChoice selections.
🤖 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/engine_resolution_choices.rs`:
- Around line 6660-6662: Update the empty-result handling around the narrowing
logic near the tracked-set publication so a genuinely empty filtered set still
updates chain_tracked_set_id to the empty result. Preserve the early return only
for the mid-pause guard case, and ensure PutAtLibraryPosition with
LibraryPosition::Bottom cannot retain the previous wider set for chained
TargetFilter::TrackedSet resolution.
In `@crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs`:
- Around line 1375-1386: Remove the unconditional local stamp_host helper and
its invocation at
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:1375-1386,
allowing build_resolved_from_def_with_targets and resolve_ability_chain to
propagate the selected target through TargetOnly, ChangeZone, and Attach. Also
remove the Attach-node stamping at
crates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:1064-1079,
relying on the parent ChangeZone result to bind ParentTarget.
- Around line 966-992: Update find_leave_rider to return Some(e) when def.effect
is Effect::Unimplemented, making the existing Some(Effect::Unimplemented { .. })
match arm reachable. Preserve the current AddTargetReplacement and ChangeZone
matching and recursive sub-ability/else-ability traversal.
---
Nitpick comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 6615-6667: Remove the duplicated bottom-narrowing and tracked-set
insertion logic from finish_effect_zone_put_at_library_position, and invoke
publish_effect_zone_choice_tracked_set there with the completed choice, effect
kind, and library position. Preserve the helper’s existing empty-set behavior
and make it the sole publication authority for these EffectZoneChoice
selections.
🪄 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: 17d83fc4-9993-432b-ba13-0e857a7a9bab
📒 Files selected for processing (8)
crates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs
…pipelines Propagate the chosen Attach host when a forward_result ChangeZone returns zero cards so Cass Equipment ParentTarget still resolves, refresh SelfRef at stack resolve after Aura SBAs, keep AttachedTo dies triggers on the battlefield, and drive Cass/Necrotic through real target selection without stamp_host. Also clear the Clippy field_reassign_with_default lint. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/game/stack.rs (1)
4019-4037: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the whole
ResolvedAbilitywhen the trigger-source refresh doesn't apply.
ability.clone()runs unconditionally on every call, even whentrigger_sourceisNoneor its identity doesn't matchsource_id— the common case. SinceResolvedAbilitycan carry a deepsub_ability/else_abilityEffectchain plus targets, this is an avoidable allocation on a per-stack-resolution path.♻️ Proposed refactor
- let mut ability = ability.clone(); - if let Some(mut source_context) = ability.trigger_source.clone() { - if source_context.identity.reference.object_id == ability.source_id { - if let Some(obj) = state.objects.get(&ability.source_id) { - source_context.identity.expected_zone = obj.zone; - source_context.identity.reference.incarnation = obj.incarnation; - ability.set_trigger_source_recursive(source_context); - } - } - } - // Use resolve_ability_chain to support SubAbility/Execute chaining - let _ = effects::resolve_ability_chain(state, &ability, events, 0); + let needs_refresh = ability.trigger_source.as_ref().is_some_and(|source_context| { + source_context.identity.reference.object_id == ability.source_id + }) && state.objects.contains_key(&ability.source_id); + if needs_refresh { + let mut ability = ability.clone(); + if let Some(mut source_context) = ability.trigger_source.clone() { + if let Some(obj) = state.objects.get(&ability.source_id) { + source_context.identity.expected_zone = obj.zone; + source_context.identity.reference.incarnation = obj.incarnation; + ability.set_trigger_source_recursive(source_context); + } + } + // Use resolve_ability_chain to support SubAbility/Execute chaining + let _ = effects::resolve_ability_chain(state, &ability, events, 0); + } else { + let _ = effects::resolve_ability_chain(state, ability, events, 0); + }🤖 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/stack.rs` around lines 4019 - 4037, Refactor the trigger-source refresh in the resolving ability flow to avoid unconditionally cloning the entire ResolvedAbility. Only clone when trigger_source exists, its object_id matches source_id, and the corresponding object is present; otherwise pass the original ability unchanged to resolve_ability_chain. Preserve the existing zone/incarnation refresh and recursive source update when applicable.
🤖 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_trigger.rs`:
- Around line 1882-1889: Update the annotation above the AttachedTo dies-trigger
handling to cite CR 603.6e and briefly describe that an Aura trigger for its
enchanted permanent leaving can find the Aura’s new graveyard object after
state-based actions; preserve the existing CR 603.6c, CR 113.6, and CR 704.5m
references and behavior.
---
Nitpick comments:
In `@crates/engine/src/game/stack.rs`:
- Around line 4019-4037: Refactor the trigger-source refresh in the resolving
ability flow to avoid unconditionally cloning the entire ResolvedAbility. Only
clone when trigger_source exists, its object_id matches source_id, and the
corresponding object is present; otherwise pass the original ability unchanged
to resolve_ability_chain. Preserve the existing zone/incarnation refresh and
recursive source update when applicable.
🪄 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: 28761e81-3e41-467e-bda3-30f3516ddd8d
📒 Files selected for processing (6)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/stack.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs
| // CR 603.6c + CR 113.6 + CR 704.5m: A SelfRef return "from your | ||
| // graveyard" in the effect body implies the source is already in that | ||
| // zone when the trigger fires — EXCEPT for AttachedTo dies triggers | ||
| // (Necrotic Plague: "When enchanted creature dies, … Return this card | ||
| // from its owner's graveyard"). Those fire while the Aura is still on | ||
| // the battlefield; SBAs move it to the GY before the effect resolves. | ||
| // Applying self-recursion here would park the trigger in the GY only, | ||
| // so the enchanted-creature death never matches. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cite the Aura-specific rule.
This behavior is specifically covered by CR 603.6e: an Aura trigger for its enchanted permanent leaving can find the Aura’s new graveyard object after SBAs. Add it to this annotation. (media.wizards.com)
As per path instructions, rules-touching code requires a verified CR <number>: <description> annotation.
🤖 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_trigger.rs` around lines 1882 - 1889, Update
the annotation above the AttachedTo dies-trigger handling to cite CR 603.6e and
briefly describe that an Aura trigger for its enchanted permanent leaving can
find the Aura’s new graveyard object after state-based actions; preserve the
existing CR 603.6c, CR 113.6, and CR 704.5m references and behavior.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
[P1] Publish the empty tracked set instead of retaining a stale chain set
crates/engine/src/game/effects/mod.rs:5301-5309 now returns from publish_tracked_set when affected_ids is empty. That contradicts the downstream contract exercised by the existing empty-set chain tests at mod.rs:17431-17493 and engine_resolution_choices.rs:2093-2173: an empty producer must replace the chain set, rather than leaving a previous non-empty set available to a following TargetFilter::TrackedSet. As written, a chained no-op can reuse stale objects (and the empty chain set is never published).
The current CI run 30558607178 has failures in both required Rust test shards, including game::effects::tests::tracked_set_sentinel_does_not_reuse_prior_non_empty_set_when_current_move_is_empty. Restore generic empty-set publication, and isolate the Storm Herald pause handling narrowly rather than changing the shared publication primitive.
Recommendation: fix the generic tracked-set contract and rerun the required CI before re-review.
Reverting the blanket stack SelfRef refresh — rewriting trigger_source to the post-SBA GY object flipped source_read to ExactLive and broke Persist, Modular, attachment-count LKI, and related dies triggers. Necrotic Plague instead gains a narrow self_ref_is_current path for battlefield-latched SelfRef returns from the graveyard. Completed empty zone moves also publish a fresh empty tracked set (without mid-pause Storm Herald binding). Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the current head still has two stale-identity/state leaks and a parser blast radius far beyond the stated Gift scope.
🔴 Blocker
-
crates/engine/src/game/effects/mod.rs:8569-8570callspublish_tracked_set_with_causeseven whenaffected_with_causesis empty. The shared publisher returns without rebinding on empty input at:5301-5308, while the generic completed-empty path explicitly creates a fresh empty set at:9637-9644. A following tracked-set consumer can therefore observe a prior non-empty set. Mirror the completed-empty rebind here, while preserving the existing resolution-choice pause behavior, and add a regression that proves an empty player-scope completion cannot inherit a prior tracked set. -
crates/engine/src/types/ability.rs:23339-23351allows the post-SBASelfRefexception solely from the source id, expected battlefield zone, return-from-graveyard shape, and current graveyard zone. It carries no captured incarnation/provenance check. The normal source authority validates the captured incarnation at:23156-23166, and the own-departure successor validates the recorded zone-change identity and no later same-id move at:23214-23264. Capture and validate the relevant incarnation/provenance for this exception as well, with a stale same-id/second-zone-change negative regression. -
crates/engine/src/parser/oracle_effect/assembly.rs:1923-1954is a generic parser assembly change, but the current<!-- coverage-parse-diff -->artifact reports 43 cards across 24 signatures beyond the stated Gift peers: it changes Genju and Traveling Plague and removes behavior from Takklemaggot, Hakim, Loreweaver, and Storm Herald. The selected tests incrates/engine/tests/integration/issue_4956_gift_of_immortality_reattach.rs:832-1356do not reconcile that card-level surface, and both required Rust test shards are red on this head. Reconcile every gained/lost/changed parse signature (restore unintended deltas or expand the declared, tested class) and make the current-head Rust shards pass before resubmission.
✅ Clean
The current parse-diff artifact is present and gives the necessary card-level evidence for the requested reconciliation.
Recommendation: request changes — fix the empty tracked-set rebind and SelfRef provenance checks, then reconcile the complete parse-diff surface with discriminating regressions and a green current-head Rust run.
Remove the empty early-return from publish_tracked_set so completed no-op producers still allocate a fresh empty chain set, while mid-chain empty extends leave prior members intact (Beseech). ParentTarget Attach under forward_result falls back to the ability source when no explicit host was chosen (Iron Man / Armored Skyhunter) without overwriting a bound choice. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head has stale-identity/state leaks and lacks current-head parser coverage evidence.
🔴 Blocker
-
crates/engine/src/types/ability.rs:23332-23351permits the post-SBASelfRefexception using only the raw source id, expected battlefield zone, return-from-graveyard shape, and current graveyard zone. It neither validates the captured incarnation nor proves this is the source's own immediate departure successor. The normal guards at:23156-23166and:23216-23265carry that identity/provenance authority. A same-id object that has changed zones again can therefore be rebound as the old source. Extend this exception with the captured incarnation and recorded zone-change provenance, and add a negative regression for a stale same-id/second-zone-change case. -
crates/engine/src/game/engine_resolution_choices.rs:5450-5468publishes the tracked set on terminal completion. However, theup_toselection path accepts zero choices at:4455-4469, whilepublish_effect_zone_choice_tracked_setreturns early for an empty set at:6663-6667. The next chainedTargetFilter::TrackedSetcan then retain a prior non-empty set instead of observing this completed empty result. Publish a fresh empty tracked set on terminal completion; keep any pause-only early return narrowly scoped. -
The required
<!-- coverage-parse-diff -->comment is not current-head evidence: its latest update is2026-07-30T18:38:06Z, before current head51442f4c57e88dfd0d803ae9dc686bdd1cb0569c(PR update2026-07-30T18:56:49Z). Its 43-card/24-signature result cannot establish this head's parser blast radius. Publish the current-head full artifact and reconcile every gained, removed, and changed signature against the intended, tested scope before re-review.
Recommendation: request changes — restore source-incarnation/provenance safety, publish completed empty tracked sets, then provide and reconcile the current-head parse-diff artifact.
…acked sets Require a matching BF→GY zone-change identity with no later same-id move for post-SBA SelfRef graveyard returns, and publish a fresh empty chain set on terminal EffectZoneChoice completion while keeping mid-pause empty skips. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed —
|
| Cluster | Verdict |
|---|---|
| + Attach (parent target) · Dragon Breath / Smoke Shroud class (13) | Intended. Event-subject return Auras bind host via nested Attach { ParentTarget }. Covered by smoke_shroud_* / Dragon Breath pipeline tests. |
| + Attach (you control creature) · Storm Herald, Craving, One Last Job | Intended. Rem-preserving attach-host parse. Storm Herald pipeline: return → attach → delayed exile TrackedSet. |
+ Attach (parent target, 0+) / Aura host triggering source→creature · Cass |
Intended. Chosen Aura host + Equipment ParentTarget. Covered by cass_* production pipeline (no stamp_host). |
| active in GY→BF · Genju×6, Necrotic Plague, Reins, Traveling Plague | Intended. AttachedTo dies / leave latch while Aura still on BF (CR 704.5m / Necrotic class). Necrotic Plague pipeline proves chosen host ≠ dying creature. |
| Next of Kin trigger add+remove same signature | Fingerprint churn around delayed reattach wrap; net AttachedTo dies shape preserved. Covered by next_of_kin_*. |
| − CreateDelayedTrigger (when=at your next end step) · Storm Herald | Not a semantic loss. CDT remains nested after Attach host (uses_tracked_set + exile TrackedSet); sticky fingerprint moved when rem preservation inserted the Attach rider. Covered by storm_herald_exiles_returned_auras_at_end_step_via_pipeline. |
| **− activate / − ~** · Hakim, Craving | Intended cleanup. Leftover Unimplemented tails from discarded rem; activation/Attach hosts now consumed. Attach (self) / Attach (you control) added for those cards. |
| − ChangeZone (conditional=not previous outcome, …) · Takklemaggot | Class of attach-host rem parse. Return+attach now consumes the attached-to host; choose-gate remains Unimplemented (choose a creature…) — strict failure on the choice clause, not a newly supported Takklemaggot. |
| ParentTarget → TrackedSet #0 · Bison Whistle, From Father to Son, Five Doctors, Rootweaver, Soul Exchange | Intentional building-block. Battlefield returns now publish chain tracked sets (is_battlefield_return_effect) so plural/those anaphors (Storm Herald) and singular “that …” follow-ups bind TrackedSet instead of bare ParentTarget. |
| CDT tracked ∅→yes · Rally / Random Encounter / Wake the Dead | Same TrackedSet publish path for delayed “those” follow-ups. |
| Other Attach adds · Accursed Witch, Radiant Grace, Gryff's Boon, Mantle, Forum Filibuster, Vengeful Strangler, Magnetic Snuffler, Reins | Same attach-host class unlocked by rem preservation — not Gift-only, but the general “return … attached to …” building block this PR owns. |
Verification (local + CI)
issue_4956×14, SelfRef provenance ×2, terminal empty tracked set, Beseech, tracked_set_sentinel — pass- CI run
30578785148on2787aec0f— all required checks green
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the prior identity and parse-diff blockers are addressed, but terminal empty EffectZoneChoice still lacks a discriminating production-pipeline regression.
🔴 Blocker
crates/engine/src/game/engine_resolution_choices.rs:9600-9662 calls the private publish_effect_zone_choice_tracked_set helper directly. That proves the helper can rebind an empty set, but it does not exercise the real WaitingFor::EffectZoneChoice completion path that calls it at :5465-5471. A production-path wiring error can therefore leave a prior non-empty chain_tracked_set_id visible to the next TargetFilter::TrackedSet while this test remains green.
Replace or supplement it with a real terminal zero-choice up_to selection driven through handle_resolution_choice; seed a prior non-empty tracked set and assert the following tracked-set consumer observes the new empty set. Preserve the separate pause-path assertion.
✅ Clean
The current head restores SelfRef provenance checks, and the refreshed parse-diff artifact (2026-07-30T20:37:20Z) reconciles the 43-card / 24-signature surface. Required CI checks for this head are green.
Recommendation: add the production-pipeline zero-choice regression, then request re-review.
…e path The empty SelectCards early-return skipped terminal tracked-set publish, so up_to zero-choice could leave a prior non-empty chain set for TrackedSet consumers. Rebind before resume and cover it via handle_resolution_choice. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed — production empty
|
|
Maintainer hold for current head |
|
Maintainer hold for current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for merge queue on b66baa3: the final delta is the verified CR 603.6e annotation correction; current parse-diff and required checks are reconciled.
2 participants
Closes #4956
Summary
Discord report: Gift of Immortality returns at the next end step but prompts to choose a creature to attach to, instead of auto-attaching to “that creature.”
Oracle: Enchant creature. When enchanted creature dies, return that card to the battlefield under its owner's control. Return this card to the battlefield attached to that creature at the beginning of the next end step.
Peers covered in the same building block: Next of Kin (attach to the put creature) and Lynde, Cheerful Tormentor (return Curse attached to you).
Root cause
The delayed body was a bare
ChangeZone{SelfRef→BF}with no nestedAttachand noAbilityDefinition.forward_result, so Aura entry fell through to CR 303.4f host choice (WaitingFor::ReturnAsAuraTarget).Two follow-on runtime bugs also blocked the happy path once the AST was correct:
SelfRefcurrency kept the parent trigger’s battlefield stamp after SBAs moved the Aura to the GY (incarnation bump) —self_ref_is_currentfailed and the end-stepChangeZoneno-oped.OptionalEffectPerformedwas cloned onto both theCreateDelayedTriggerwrapper and the delayed payload; at end step the creation-time signal was gone, so the return was skipped.Changes
ReturnToBattlefield.attach_host; construct + lower emitChangeZone+ nestedAttach; assembly nest-before-wrap setsforward_resultbefore delayed wrapping (one CDT, not two). Skip short-name rewrite of temporal"next"(Next of Kin).Controllerattach host; ParentTarget walk through sub/else; refresh delayed trigger source zone+incarnation at creation; lift (don’t clone) condition/optional onto the CDT wrapper; keep CDTsource_idas the Aura (not the returned host).issue_4956_gift_of_immortality_reattach.rs: shape + Gift/Next/Lynde runtime reattach; Gift stays in GY when host is gone (no Aura choice).CR references
Test plan
cargo fmt --allcargo test -p engine --test integration issue_4956Summary by CodeRabbit