fix(engine): reject stale object targets after zone changes - #7235
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe engine now tracks incarnation pins for ordinary selected object targets. Target assignment captures pins, retargeting updates them, and resolution validates them across target paths. Tests cover stale targets, retargeting, and copied abilities. ChangesSelected target incarnation tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant ResolvedAbility
participant GameState
participant AbilityResolution
Player->>ResolvedAbility: select object target
ResolvedAbility->>GameState: capture current incarnation
GameState-->>ResolvedAbility: store incarnation pin
GameState->>GameState: move object away and return it
AbilityResolution->>ResolvedAbility: validate selected-target pin
ResolvedAbility->>GameState: compare pinned and current incarnations
GameState-->>AbilityResolution: invalidate stale target
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.rs`:
- Around line 11755-11770: Update crates/engine/src/game/engine.rs:11755-11770
to compare target incarnations, not only TargetRef equality, and refresh the
selected-target pin when the existing pin is stale; apply the same
incarnation-aware comparison in crates/engine/src/game/engine.rs:6745-6770 when
finalizing copy retargeting. In
crates/engine/src/game/effects/change_targets.rs:88-103, ensure forced
retargeting rebinds a same-ObjectId target after it leaves and returns, and add
a regression covering that sequence.
In `@crates/engine/src/types/ability.rs`:
- Around line 24713-24715: Update the documentation annotation at the shared
target-selection method to cite CR 115.1, CR 601.2c, CR 602.2b, and CR 603.3d,
reflecting spell, activated-ability, and triggered-ability target selection; do
not leave CR 601.2c as the sole citation unless the method is restricted to
spell targets.
In `@crates/engine/tests/integration/target_incarnation_revalidation.rs`:
- Around line 48-49: Replace the direct zones::move_to_zone calls in the test
with the production ProposedEvent::ZoneChange scenario and priority pipeline,
resolving the intervening leave-and-return ability before the activated ability.
Ensure replacement handling runs and the returned permanent is treated as a new
object, making the original target illegal per the expected rules behavior.
🪄 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: d0655fed-2e7b-4c20-9caf-090be447963d
📒 Files selected for processing (20)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/additional_phase.rscrates/engine/src/game/effects/change_targets.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/extra_turn.rscrates/engine/src/game/effects/grant_extra_loyalty_activations.rscrates/engine/src/game/effects/player_counter.rscrates/engine/src/game/effects/reverse_turn_order.rscrates/engine/src/game/effects/skip_next_step.rscrates/engine/src/game/effects/skip_next_turn.rscrates/engine/src/game/effects/vote.rscrates/engine/src/game/engine.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/stack.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/target_incarnation_revalidation.rscrates/engine/tests/integration/the_chain_veil_loyalty_grants.rs
| move_to_zone(runner.state_mut(), target, Zone::Graveyard, &mut events); | ||
| move_to_zone(runner.state_mut(), target, Zone::Battlefield, &mut events); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use the production zone-change pipeline.
Lines 48-49 call zones::move_to_zone directly. This bypasses ProposedEvent::ZoneChange and replacement handling. The test can pass while a live leave-and-return sequence produces different state or target pins.
Resolve an intervening zone-change ability through the scenario and priority pipeline before resolving the activated ability. CR 400.7 makes the returned permanent a new object, and CR 608.2b requires the original target to be illegal. (media.wizards.com)
🤖 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/target_incarnation_revalidation.rs` around
lines 48 - 49, Replace the direct zones::move_to_zone calls in the test with the
production ProposedEvent::ZoneChange scenario and priority pipeline, resolving
the intervening leave-and-return ability before the activated ability. Ensure
replacement handling runs and the returned permanent is treated as a new object,
making the original target illegal per the expected rules behavior.
Source: Path instructions
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Same-ObjectId retargeting leaves the selected-target incarnation pin stale. Evidence: crates/engine/src/types/ability.rs:24022-24025 represents object targets by ObjectId only, while crates/engine/src/types/identifiers.rs:143-180 defines the distinct ID-plus-epoch identity; all three refresh paths use only old != new — crates/engine/src/game/engine.rs:6745-6770, crates/engine/src/game/engine.rs:11755-11770, and crates/engine/src/game/effects/change_targets.rs:88-103. Why it matters: a legal retarget to an object that left and returned with the same storage ID retains the old epoch pin and then incorrectly fizzles on resolution. Suggested fix: make the changed-target comparison incarnation-aware (including a stale existing pin) and cover copy, interactive, and forced retarget routes.
[HIGH] The regression bypasses the production zone-change pipeline. Evidence: crates/engine/tests/integration/target_incarnation_revalidation.rs:4,48-49 calls zones::move_to_zone directly, whereas production delivery captures PendingZoneChangeDelivery with ProposedEvent::zone_change at crates/engine/src/game/effects/change_zone.rs:289-299 and routes a move through the replacement-aware terminal path at crates/engine/src/game/effects/mod.rs:1260-1276. Why it matters: the test can pass without exercising replacement/delivery state that live leave-and-return events use. Suggested fix: reproduce the intervening leave/return through the scenario and priority pipeline, resolve it before the original activated ability, and retain a reach assertion proving the real delivery path ran.
[MED] The shared selected-target capture method cites spell casting as its only target-selection source. Evidence: crates/engine/src/types/ability.rs:24713-24716 labels a shared announcement/selection method solely with CR 601.2c, while the method serves spell, activated, and triggered ability target selection. Why it matters: the rule annotation misstates the authority for two of its production callers. Suggested fix: retain the CR 400.7 identity citation and add the applicable target-selection sources from CR 115, 601.2c, 602.2b, and 603.3d.
[MED] The PR leaves the Rust producer-census test red from a stale expected coordinate. Evidence: the current head's failing shard reports actual game/engine.rs:11979 versus expected game/engine.rs:11942; the producer is still the intended WaitingFor::OptionalEffectChoice return at crates/engine/src/game/engine.rs:11979, while the census expectation remains at crates/engine/src/game/engine.rs:16395. Why it matters: required CI cannot pass, even though the census producer set has not changed. Suggested fix: mechanically update only that expected location after re-verifying the producer identity.
31ce252 to
7b8b92a
Compare
|
Addressed all review findings in amended commit
Validation: |
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/src/game/engine_keyword_action_stack_tests.rs`:
- Around line 1007-1055: Update the test fixture around ResolvedAbility::new and
finalize_copy_retarget to use an effect whose TargetFilter legally accepts the
TargetRef::Object target, such as a creature- or permanent-targeting effect.
Construct the copied spell and drive retargeting through the normal
WaitingFor::CopyRetarget action flow rather than calling finalize_copy_retarget
directly, while preserving the zone-change setup needed to exercise the
pin-refresh failure path.
🪄 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: 9d1fac48-629f-4b1b-9012-28a68550b9c7
📒 Files selected for processing (5)
crates/engine/src/game/effects/change_targets.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_keyword_action_stack_tests.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/target_incarnation_revalidation.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/game/engine.rs
- crates/engine/src/game/effects/change_targets.rs
- crates/engine/src/types/ability.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head has one blocking test-evidence gap.
🔴 Blocker
[MED] The copy-retarget regression constructs an impossible slot and bypasses the production selection action. Evidence: crates/engine/src/game/engine_keyword_action_stack_tests.rs:1007-1055 pairs Effect::Draw { target: TargetFilter::Controller } with TargetRef::Object, writes that object into legal_alternatives, and directly calls finalize_copy_retarget; the object matcher explicitly rejects TargetFilter::Controller at crates/engine/src/game/filter.rs:2620-2626. Production instead accepts GameAction::ChooseTarget only when the choice is already in an engine-built legal-alternatives slot before it calls finalize_copy_retarget (crates/engine/src/game/engine.rs:11010-11066). Why it matters: this passes even if copied spells that can legally target objects fail to refresh their incarnation pin, so it does not prove the advertised copy-retarget behavior. Suggested fix: exercise a legally object-targeting copied spell through WaitingFor::CopyRetarget and the normal GameAction::ChooseTarget pipeline, retaining the leave/return setup and asserting the refreshed pin after the action completes.
✅ Clean
The current full integration blink regression replaces the earlier direct zone-move fixture, so that previous concern is resolved. CI is green and the current-head parse-diff reports no card-parse change; neither substitutes for a discriminating copy-retarget test.
Recommendation: request changes for the production-flow regression above, then re-review the new head.
7b8b92a to
00ca01b
Compare
|
Addressed the copy-test evidence blocker in amended commit The regression now:
Validation passed: the focused regression, all 22 |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the manual copy-retarget regression is fixed, but the automatic copy path still bypasses the new incarnation authority.
🔴 Blocker
[HIGH] Zada-style automatic copies overwrite object targets without capturing the replacement incarnation. Evidence: crates/engine/src/game/effects/copy_spell.rs:191-212 calls rewrite_copy_spell_object_targets after pushing the copy, while crates/engine/src/game/effects/copy_spell.rs:886-898 only rewrites targets; it never refreshes selected_target_incarnations. Resolution checks a new target only through selected_target_pin_is_current (crates/engine/src/game/ability_utils.rs:1869-1888), which intentionally passes when there is no pin (crates/engine/src/types/ability.rs:24764-24775). Why it matters: if an iteration member leaves and returns after its copy is put on the stack, the copy can affect that new incarnation instead of treating its target as illegal. Suggested fix: make the shared Zada rewrite capture the post-rewrite target incarnations from GameState, then add a regression that lets a retargeted copy's member leave and return before that copy resolves.
✅ Clean
The prior copy-retarget blocker is resolved at this head: the test uses a creature-targeting Effect::Destroy, obtains its slots from open_copy_retarget_choice, and submits GameAction::ChooseTarget through WaitingFor::CopyRetarget (crates/engine/src/game/engine_keyword_action_stack_tests.rs:1007-1071).
Recommendation: retain the new interactive-path test and extend the same incarnation-pinning authority to the automatic RetargetEachCopyToIterationMember path before approval.
00ca01b to
c149607
Compare
|
Addressed the automatic-copy blocker in amended commit The Zada-style Validation passed before push:
|
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/game/effects/copy_spell.rs`:
- Around line 3350-3362: Replace the direct zones::move_to_zone calls in the
regression setup with the replacement-aware ProposedEvent::ZoneChange pipeline,
routing both the Graveyard and Battlefield transitions through normal event
resolution before resolving the copy. Preserve the existing iteration_member
movement sequence and zone targets.
- Around line 911-916: Update rewrite_copy_spell_object_targets to also iterate
through ability.mode_abilities and recursively apply the same target and
incarnation-pin rewrite to each mode ability. Preserve the existing sub_ability
and else_ability traversal, and add a regression test covering automatic copying
of a modal spell with selected object targets.
🪄 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: 6b1c298b-c832-49bc-89bf-3586ecdb42df
📒 Files selected for processing (1)
crates/engine/src/game/effects/copy_spell.rs
| if let Some(sub) = ability.sub_ability.as_mut() { | ||
| rewrite_copy_spell_object_targets(sub, new_target); | ||
| rewrite_copy_spell_object_targets(sub, new_target, new_target_pin); | ||
| } | ||
| if let Some(else_ab) = ability.else_ability.as_mut() { | ||
| rewrite_copy_spell_object_targets(else_ab, new_target); | ||
| rewrite_copy_spell_object_targets(else_ab, new_target, new_target_pin); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Rewrite selected modal ability targets.
ResolvedAbility also stores mode_abilities. This helper only traverses sub_ability and else_ability.
When an automatic copy retargets a modal spell, a selected mode can retain its old object target and old incarnation pin. Traverse every mode_abilities entry with the same rewrite function. Add a modal-copy regression.
As per path instructions, CLAUDE.md requires target-incarnation behavior across modal paths.
🤖 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/copy_spell.rs` around lines 911 - 916, Update
rewrite_copy_spell_object_targets to also iterate through ability.mode_abilities
and recursively apply the same target and incarnation-pin rewrite to each mode
ability. Preserve the existing sub_ability and else_ability traversal, and add a
regression test covering automatic copying of a modal spell with selected object
targets.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the automatic-copy production regression is still not discriminating enough for this rules-sensitive target-incarnation change.
🔴 Blocker
[MED] The new Zada-style regression bypasses the production zone-change delivery path. Evidence: crates/engine/src/game/effects/copy_spell.rs:3351-3362 calls zones::move_to_zone directly for the iteration member, while production ChangeZone resolves through PendingZoneChangeDelivery::new(ProposedEvent::zone_change(...)) at crates/engine/src/game/effects/mod.rs:1260-1268, where replacements and delivery state are applied. Why it matters: the test proves only that a direct mutation bumps the incarnation; it can remain green while the actual leave/return path that the copy will encounter changes delivery/replacement state or target pins differently. Suggested fix: make the regression move the iteration member out and back through the existing ProposedEvent::ZoneChange/resolution pipeline, then resolve the copy and retain the asserted fizzle of the returned incarnation.
✅ Clean
The automatic retarget itself now captures the current iteration-member incarnation before rewriting targets (copy_spell.rs:204-215, 892-916), and the earlier interactive-copy evidence gap is resolved. The current CodeRabbit modal-recursion suggestion does not apply: mode_abilities are AbilityDefinitions; modal casting/selection constructs selected modes as a ResolvedAbility sub-chain before this copy path (game/casting_targets.rs:142-152, game/engine_modes.rs:40-46), which the rewrite already recurses.
Recommendation: request changes for the production-path regression, then re-review the new head.
c149607 to
006e144
Compare
|
Addressed the valid production-path regression blocker in amended commit The automatic-copy regression now moves the iteration member out and back by resolving real I also verified the modal-recursion suggestion is not applicable: Validation: focused automatic-copy regression; all 39 |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head review: selected-target incarnation pins are captured, revalidated, and refreshed through interactive, forced, and automatic-copy retargeting; the regressions exercise the production resolution path.
2 participants
Summary
Fixes stale ordinary object targets after a permanent leaves and returns to the battlefield. The engine now captures selected-target incarnations at announcement/selection, rechecks them on resolution, and refreshes only targets actually changed by retargeting. The regression reproduces the supplied
/home/traemyn/Downloads/earthbend-stripmine.zipstate shape through the real activation and resolution pipeline.Files changed
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/additional_phase.rscrates/engine/src/game/effects/change_targets.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/extra_turn.rscrates/engine/src/game/effects/grant_extra_loyalty_activations.rscrates/engine/src/game/effects/player_counter.rscrates/engine/src/game/effects/reverse_turn_order.rscrates/engine/src/game/effects/skip_next_step.rscrates/engine/src/game/effects/skip_next_turn.rscrates/engine/src/game/effects/vote.rscrates/engine/src/game/engine.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/stack.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/target_incarnation_revalidation.rscrates/engine/tests/integration/the_chain_veil_loyalty_grants.rsTrack
Developer
LLM
Model: GitHub Copilot (traemyn edit: GPT-5.6 Luna)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
The references were verified against
docs/MagicCompRules.txtfetched from the repository's authoritative rules source.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 fmt --all -- --check— passed.cargo check -p phase-engine— passed.cargo test -p phase-engine ability_utils::tests:: --lib— 145 passed, 0 failed.cargo test -p phase-engine --test integration target_incarnation_revalidation— 1 passed, 0 failed.cargo test -p phase-engine --test integration --quiet— 4,801 passed, 2 ignored, 0 failed.git diff --check— passed../scripts/check-parser-combinators.sh upstream/main— Gate G PASS; Gate A PASS for head31ce252b7fc72d95a83a2c036b127efadaf46e06, baseb8185d37bb6a1d49ae5b7fe8e955e2ce54a4357c.Tilt was unavailable in the isolated worktree, so the repository-prescribed direct Cargo checks were used.
Gate A
Gate A PASS head=31ce252b7fc72d95a83a2c036b127efadaf46e06 base=b8185d37bb6a1d49ae5b7fe8e955e2ce54a4357c
Anchored on
crates/engine/src/game/ability_utils.rs:1799—assign_selected_slots_in_chainis the existing interactive target-assignment seam used byGameAction::ChooseTarget.crates/engine/src/game/ability_utils.rs:1891—validate_targets_in_chainis the existing resolution-time target legality seam used by stack resolution.Final review-impl
Final review-impl PASS head=31ce252b7fc72d95a83a2c036b127efadaf46e06
Claimed parse impact
None.
Scope Expansion
None. The supplied Strip Mine/Earthbend reproduction is addressed as a general ordinary selected-object target identity fix across bulk, interactive, automatic/random, activated, triggered, modal/chain, and retargeting paths.
Validation Failures
None. The full implementation-review and validation workflow completed on the immutable candidate
31ce252b7fc72d95a83a2c036b127efadaf46e06. Legacy serialized abilities without the new optional field intentionally remain fail-open for backward compatibility.CI Failures
None. CI has not yet run on the pushed branch; the listed local checks passed and this section will reflect any CI result if one fails.
Pipeline handoff
Pipeline-reviewed head:
31ce252b7fc72d95a83a2c036b127efadaf46e06Current branch head:
31ce252b7fc72d95a83a2c036b127efadaf46e06Pipeline status: current
Current-head review: clean at
31ce252b7fc72d95a83a2c036b127efadaf46e06Checkpoint base:
b8185d37bb6a1d49ae5b7fe8e955e2ce54a4357cSummary by CodeRabbit
Bug Fixes
Tests