Skip to content

fix(engine): don't affect a blinked referent from a delayed trigger (CR 400.7) - #7099

Merged
matthewevans merged 3 commits into
mainfrom
ship/delayed-trigger-parenttarget-incarnation-pin
Aug 8, 2026
Merged

fix(engine): don't affect a blinked referent from a delayed trigger (CR 400.7)#7099
matthewevans merged 3 commits into
mainfrom
ship/delayed-trigger-parenttarget-incarnation-pin

Conversation

@matthewevans

@matthewevans matthewevans commented Aug 8, 2026

Copy link
Copy Markdown
Member

Fixes a user-reported Modern Goryo's Vengeance interaction: casting Goryo's Vengeance and then Ephemerate on the reanimated creature still exiled it at the beginning of the next end step.

Per CR 603.7c, a delayed triggered ability that refers to a particular object won't affect it if the object left its zone and returned — it's a new object (CR 400.7). The trigger itself still triggers and still goes on the stack (CR 603.7b), and this change preserves that; only its effect on a stale referent changes.

Also fixes a second violation of the same card's official ruling, found while testing: a creature that died before the end step was being exiled out of the graveyard. The ruling says it "will remain in its current zone. It won't be exiled."

Approach

A delayed trigger snapshotted its ParentTarget referent as a bare ObjectId and re-resolved it at firing with no incarnation comparison. Each snapshotted object referent is now pinned to its ObjectIncarnationRef at creation (ResolvedAbility.target_incarnations) and stale elements are dropped at read time via the id-keyed target_pin_is_current. This reuses the shipped ObjectIncarnationRef primitive rather than introducing a new one.

Two constraints are load-bearing:

  • Stale elements are dropped, never the whole list. An emptied list re-binds ParentTarget to ability.source_id, which would make Goryo's exile itself from the graveyard.
  • The expected-zone verdict is computed from the parser-emitted condition, before bind_tracked_set_to_condition and bind_contextual_filter_to_condition rewrite the anaphor away. A post-bind read is vacuously false for every pair.

The predicate is scoped by expected zone, derived as a total function of DelayedTriggerCondition via an exhaustive match with no wildcard arm — a condition naming the referent's own zone change (either direction) must not pin, or "when it dies, return that card" cards (Saffi Eriksdotter, Adarkar Valkyrie, Cryptek, Together Forever) would go permanently inert.

Guards are applied at both targeting.rs chokepoints plus every handler that reads ability.targets directly: sacrifice, destroy, copy_spell, counters, gain_control, attach, remove_from_combat, and the Tier C set. Every early return emits EffectResolved so the no-op stays observable.

Evidence

Both bugs were watched go red before the fix and green after, through the real cast pipeline:

test role pre-fix post-fix
blinked referent not exiled the reported bug RED (left: Exile) GREEN
died before end step second bug RED (left: Exile) GREEN
no blink → exiled anti-vacuity control GREEN GREEN
saffi eriksdotter regression control GREEN GREEN
lagrella regression control GREEN GREEN
whippoorwill placement detector RED when the pre-bind seam is reverted
  • Full engine suite: 23,249 passed / 0 failed
  • Designed-red command (A) 8/8; command (B) 42/42
  • CR gate: zero unverified

Known limitations (disclosed, not hidden)

  • 33 tracked-set cards are NOT covered — Eerie Interlude, Ghostway, Yorion Sky Nomad, Venser the Sojourner, Touch the Spirit Realm and others lose the anaphor to bind_tracked_set_to_ability_chain before the gate sees them. Runtime coverage is 267/317 pairs (84.2%), not the ~95% a pre-bind count suggests. Filed as follow-up.
  • The event-context authority is deliberately unguarded per CR 400.7e: it re-derives the referent from the firing event rather than reading the creation-time snapshot, so there is nothing stale to invalidate. Guarding it would break Lagrella/Saffi/Valkyrie/Whippoorwill to protect zero cards.
  • Ten planned tests remain unwritten, and six revert-checks are unrun because their anchors don't exist. Six Tier C handlers rest on the full-suite regression signal rather than a dedicated production-path test. cast_from_zone.rs is the highest-risk edit: large function, one in-class card (power pack), no test.
  • Two pre-existing clusters left for separate auditable passes: a parser misparse ("sacrifice this creature" parsed as ParentTarget instead of SelfRef) and a CR 701.17aCR 701.21a mis-citation (701.17a is mill; sacrifice is 701.21).

Process

9 plan-review rounds (round 7 returned no blocking gaps), 2 implementation rounds, 1 implementation review — verdict "the implementation is sound", nothing blocking. Its three findings are addressed in the second commit.

Summary by CodeRabbit

  • Bug Fixes
    • Delayed effects now ignore targets that have left play or been replaced, preventing actions from affecting the wrong objects.
    • Effects with entirely stale targets now resolve safely as no-ops instead of failing or selecting replacement objects.
    • Preserved positional target behavior where slot-based effects depend on original ordering.
    • Improved delayed-trigger handling so valid triggers continue resolving while stale referents are correctly skipped.
  • Tests
    • Added comprehensive coverage for delayed target and object-replacement scenarios.

…CR 400.7)

Goryo's Vengeance followed by Ephemerate on the reanimated creature still
exiled it at the beginning of the next end step. A delayed triggered ability
snapshots its `ParentTarget` referent as a bare `ObjectId` and re-resolved it
at firing with no incarnation comparison, so a creature that left and returned
was still affected even though CR 400.7 makes it a new object.

CR 603.7c: "if that object is no longer in the zone it's expected to be in at
the time the delayed triggered ability resolves, the ability won't affect it.
(Note that if that object left that zone and then returned, it's a new object
and thus won't be affected. See rule 400.7.)"

The trigger still triggers and still goes on the stack (CR 603.7b) — the
trigger event occurred. Only its effect on a stale referent changes.

Pin each snapshotted object referent to its `ObjectIncarnationRef` at
delayed-trigger creation (`ResolvedAbility.target_incarnations`), and filter
stale elements at read time via the id-keyed `target_pin_is_current`. Reuses
the shipped `ObjectIncarnationRef` primitive rather than introducing a new one.

The predicate is scoped by expected zone, derived as a total function of
`DelayedTriggerCondition` via an exhaustive match with no wildcard arm: a
condition that names the referent's own zone change (either direction) must
not pin, or "when it dies, return that card" cards would go permanently inert.
That verdict is computed from the parser-emitted condition before
`bind_tracked_set_to_condition` and `bind_contextual_filter_to_condition`
rewrite the anaphor away — a post-bind read is vacuously false.

Guards are applied at both `targeting.rs` chokepoints plus each handler that
reads `ability.targets` directly (sacrifice, destroy, copy_spell, counters,
gain_control, attach, remove_from_combat, and the Tier C set). Stale elements
are dropped, never the whole list: an emptied list re-binds `ParentTarget` to
`ability.source_id`, which would make Goryo's exile itself from the graveyard.
Every early return emits `EffectResolved` so the no-op stays observable.

Also fixes a second violation of the same card's ruling: a creature that died
before the end step was exiled out of the graveyard. It now stays put.

Not covered: 33 tracked-set cards (Eerie Interlude, Ghostway, Yorion, Venser,
Touch the Spirit Realm) lose the anaphor to `bind_tracked_set_to_ability_chain`
before the gate sees them. Filed as follow-up. The event-context authority is
deliberately unguarded per CR 400.7e — it re-derives the referent from the
firing event rather than reading the snapshot, so there is nothing stale to
invalidate.

Tests: `crates/engine/tests/integration/delayed_parent_target_incarnation.rs`
plus inline predicate units. The blink case and the died-before-end-step case
were both watched go red before the fix and green after; `saffi eriksdotter`
and `lagrella` are regression controls green in both runs; `whippoorwill` is
the placement detector that goes red when the pre-bind read is moved.
Review follow-ups on the CR 400.7 delayed-trigger referent pin.

`remove_from_combat.rs`'s all-stale guard sat below the `SelfRef` match arm,
whose subject is `ability.source_id` and never the snapshot referent. The guard
reads `ability.targets`, so a stale pin on an unrelated object could cancel a
self-removal — predicate decoupled from the subject it suppresses. Unreachable
today (the in-class population is `melee`, whose filter is a bare
`ParentTarget`, so no `SelfRef` node co-occurs with a pin), but it is the same
collapse of "no target declared" into "declared referent went stale" that
`flip_permanent.rs` and `transform_effect.rs` preserve their raw `as_slice()`
match to avoid. Now scoped, keeping the guard above the `targets.is_empty() =>
vec![ability.source_id]` rebind, which is load-bearing.

`sacrifice.rs`: the guard comment cited CR 701.17a, which is mill; sacrifice is
CR 701.21a. Corrects only the line this change added — the file carries 13
pre-existing occurrences of the same mis-citation, left for one auditable pass
rather than migrated piecemeal here.

`ability.rs`: `clear_trigger_identity_recursive` documented the new
`target_incarnations.clear()` against CR 104.4b loop detection only. It is also
the CR 603.3b auto-ordering identity stripper, and deliberately the opposite of
`inert_trigger_abilities_eq_ignoring_provenance`, which compares
`target_incarnations`. Same field, different questions; both callers operate on
clones so no production pin is cleared. Documented so the two sites do not read
as an accidental disagreement.

Engine suite: 23,249 passed / 0 failed.
@matthewevans
matthewevans enabled auto-merge August 8, 2026 09:53
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 3a173769-634f-46b7-b355-3c32ddafb81c

📥 Commits

Reviewing files that changed from the base of the PR and between 9e3742e and f5d4e11.

📒 Files selected for processing (1)
  • crates/engine/tests/integration/delayed_parent_target_incarnation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/engine/tests/integration/delayed_parent_target_incarnation.rs

📝 Walkthrough

Walkthrough

Delayed abilities now capture object-incarnation pins for parent targets. Resolution filters stale referents, preserves positional target slots, and completes fully stale effects as no-ops. New integration tests cover delayed trigger and zone-change cases.

Changes

Delayed target incarnation handling

Layer / File(s) Summary
Pin capture and ability identity
crates/engine/src/types/ability.rs, crates/engine/src/types/identifiers.rs, crates/engine/src/game/effects/delayed_trigger.rs, crates/engine/src/game/stack.rs
Delayed triggers classify referents, capture incarnation pins, propagate them through ability branches, and validate them against current objects.
Live target binding and filter resolution
crates/engine/src/game/effects/effect.rs, crates/engine/src/game/targeting.rs, crates/engine/src/game/effects/counters.rs
Target binding uses live object targets. ParentTargetSlot retains raw positional indexing.
Stale-target effect resolution
crates/engine/src/game/effects/*
Effect resolvers exclude stale object targets and emit resolved no-ops when all pinned object targets are stale.
Compatibility fixtures and integration coverage
crates/engine/src/game/ability_rw.rs, crates/engine/src/game/ability_scan.rs, crates/engine/src/game/effects/*, crates/engine/tests/integration/*
ResolvedAbility fixtures initialize the new field. Integration tests cover unchanged, moved, replaced, and non-delayed referents.

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

Sequence Diagram(s)

sequenceDiagram
  participant DelayedTrigger
  participant Ability
  participant GameState
  participant EffectResolver
  participant ResolutionEvent
  DelayedTrigger->>Ability: Capture targets and incarnation pins
  Ability->>GameState: Validate pinned object references
  GameState-->>Ability: Return live targets
  Ability->>EffectResolver: Resolve filtered targets
  alt All pinned targets are stale
    EffectResolver->>ResolutionEvent: Emit EffectResolved no-op
  else Live targets remain
    EffectResolver->>ResolutionEvent: Apply effect to live targets
  end
Loading

Possibly related PRs

Suggested labels: bug

🚥 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 summarizes the main change: preventing delayed triggers from affecting blinked referents under CR 400.7.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ship/delayed-trigger-parenttarget-incarnation-pin

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

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/gain_control.rs (1)

200-225: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Apply the ParentTargetSlot carve-out here too; the pre-arm does not always return.

The comment at Line 221 states that ParentTargetSlot "is handled above by resolve_parent_slot_from_root and never reaches this read", and that a matches! guard "would be dead code". The pre-arm at Line 200 does not guarantee that.

resolve_parent_slot_from_root returns an Option<TargetRef>. The early return at Line 204 fires only for Some(TargetRef::Object(id)). For None, and for Some(TargetRef::Player(_)), control falls through to Line 224. The slot filter is then still active, and effect_object_targets receives the filtered live_targets list.

effect_object_targets indexes ParentTargetSlot positionally. Dropping a stale element renumbers every later slot. With targets = [stale_a, live_b] and index: 1, the filtered list is [live_b], so slot 1 is out of range. Per the comment at Line 199, an out-of-range index falls through to "all inherited targets". The effect then takes control of the wrong permanent.

The sibling give_control_object_targets at Line 340 applies exactly this carve-out. resolve_give has no ParentTargetSlot pre-arm, and the code correctly compensates. Here the pre-arm exists but is partial, so the carve-out is still required.

Control-change effects default to Duration::Permanent at Line 23, so a mis-bound target is not self-correcting.

🐛 Proposed fix: mirror the give-control carve-out
-    // Slot carve-out does NOT apply here: `ParentTargetSlot` is handled above by
-    // `resolve_parent_slot_from_root` and never reaches this read. Adding a
-    // `matches!` guard would be dead code.
-    let live_targets = ability.live_object_targets(state);
-    let chosen_objects = super::effect_object_targets(filter, &live_targets);
+    // Slot carve-out DOES apply: the `ParentTargetSlot` pre-arm above returns
+    // only for `Some(TargetRef::Object(_))`. A `None` or player-valued slot
+    // resolution falls through to here with the slot filter intact, and
+    // `effect_object_targets` indexes it positionally. Pass the raw list for
+    // that shape so a dropped stale element cannot renumber the slots.
+    let live_targets = ability.live_object_targets(state);
+    let pool: &[TargetRef] = if matches!(filter, TargetFilter::ParentTargetSlot { .. }) {
+        &ability.targets
+    } else {
+        &live_targets
+    };
+    let chosen_objects = super::effect_object_targets(filter, pool);
🤖 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/gain_control.rs` around lines 200 - 225,
Update the target preparation in the gain-control effect around
effect_object_targets and chosen_objects to preserve the ParentTargetSlot
carve-out even when resolve_parent_slot_from_root returns None or a Player.
Exclude ParentTargetSlot from the pre-arm substitution path, mirroring
give_control_object_targets, so positional slot indices are resolved against the
unfiltered live object targets and existing non-slot behavior remains unchanged.
🧹 Nitpick comments (3)
crates/engine/src/game/effects/transform_effect.rs (1)

39-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The empty-pin case is covered here, but not on the sibling call site.

transform_effect_uses_source_when_no_explicit_target at line 230 constructs an ability with empty targets and empty target_incarnations, then asserts the source transforms. That test passes through this guard and would fail if pinned_object_targets_all_stale returned true for an empty pin list. This file therefore pins the helper's empty-list semantics.

The same helper is called in crates/engine/src/game/effects/change_zone.rs at lines 558-565 on the untargeted zone-scan path, which has no equivalent discriminating test. Add one there so the contract is guarded at both call sites.

The placement reasoning above is correct: the [] arm at line 67 resolves to ability.source_id, so substituting inside the as_slice() match would rebind instead of no-op.

🤖 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/transform_effect.rs` around lines 39 - 62, Add
a regression test in the change-zone untargeted zone-scan path near the call to
pinned_object_targets_all_stale, constructing an ability with empty targets and
empty target_incarnations and asserting the source is transformed or otherwise
retains the existing no-explicit-target behavior. Ensure the test confirms an
empty pin list returns false and does not trigger the stale-target early return,
while preserving the helper placement and raw ability.targets matching logic.

Source: Path instructions

crates/engine/src/game/effects/remove_from_combat.rs (1)

19-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Carry the SelfRef discrimination out of the first match instead of re-deriving it.

The match at Line 19 already separates the SelfRef shape from the other RemoveFromCombat shape. Line 69 re-matches &ability.effect to recover the same bit.

Two independent matches on the same value can drift. If a third RemoveFromCombat shape is added, the compiler forces an update at Line 19 but not at Line 69, and the guard would silently apply to the new shape.

Bind the discrimination once.

♻️ Proposed refactor
-    let targets: Vec<_> = match &ability.effect {
+    let (targets, subject_is_self_ref): (Vec<_>, bool) = match &ability.effect {
         Effect::RemoveFromCombat {
             target: TargetFilter::SelfRef,
         } => {
-            vec![ability.source_id]
+            (vec![ability.source_id], true)
         }
         Effect::RemoveFromCombat { target } => {
             let live_targets = ability.live_object_targets(state);
             let pool: &[TargetRef] = if matches!(target, TargetFilter::ParentTargetSlot { .. }) {
                 &ability.targets
             } else {
                 &live_targets
             };
-            super::effect_object_targets(target, pool)
+            (super::effect_object_targets(target, pool), false)
         }
         _ => return Ok(()),
     };

Then delete the re-derivation:

-    let subject_is_self_ref = matches!(
-        &ability.effect,
-        Effect::RemoveFromCombat {
-            target: TargetFilter::SelfRef
-        }
-    );
     if !subject_is_self_ref && ability.pinned_object_targets_all_stale(state) {

Also applies to: 69-75

🤖 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/remove_from_combat.rs` around lines 19 - 45,
Bind whether the effect uses TargetFilter::SelfRef while handling ability.effect
in the existing targets match, then reuse that value at the later guard instead
of re-matching ability.effect. Remove the duplicate discrimination so any future
RemoveFromCombat shape is covered by the compiler-enforced match update.
crates/engine/src/game/effects/cast_from_zone.rs (1)

405-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the review-process narrative in the new comments with durable rules documentation.

The new guards across this cohort carry comment blocks that record the reasoning of the change process rather than the behavior of the code. Examples in the current text: "The plan pre-flagged this file as a possible STOP", "the Tier C census counts as distinct effect types", "No early return needed, re-verified at resolve_give rather than copied", "Population is 0 today".

Three concrete costs:

  1. The statements are unverifiable at read time. A future reader cannot check "Tier C" or "the plan" against anything in the repository.
  2. The population claims go stale silently. "Population is 0 today" and "Unreachable today" become wrong the moment a card is added, and nothing fails.
  3. One narrative asserts a false invariant. The gain_control.rs block states a slot guard "would be dead code". It is reachable. That is raised separately at crates/engine/src/game/effects/gain_control.rs Line 200-225.

CLAUDE.md requires rules-touching code to carry a verified CR <number>: <description> annotation. Keep the CR citation and the one-sentence rule statement. Move the placement rationale to a single short sentence. Drop the census and plan references.

Per site:

  • crates/engine/src/game/effects/cast_from_zone.rs#L405-L418: remove the paragraph about the plan, the three scoped_ability.targets assignments, and :257. Keep the CR 400.7 + CR 603.7c sentence and one sentence naming the fallback pools the guard protects.
  • crates/engine/src/game/effects/discard.rs#L207-L220: remove the "Tier C census" clause. Keep the CR citation and the reason EffectKind::from(&ability.effect) is used instead of a literal.
  • crates/engine/src/game/effects/flip_permanent.rs#L27-L42: keep the CR citation and the sentence explaining that [] means "no target declared" while an all-stale pin means "declared referent is gone". Drop the rest.
  • crates/engine/src/game/effects/gain_control.rs#L208-L223: replace the "dead code" claim with the corrected carve-out from the separate comment on that file.
  • crates/engine/src/game/effects/phase_out.rs#L97-L110: keep the CR citation and the sentence on why the guard sits after the player branch. Drop "ALTITUDE IS LOAD-BEARING".
  • crates/engine/src/game/effects/remove_from_combat.rs#L47-L68: keep the CR citation and the sentence scoping the guard to non-SelfRef. Drop the population claim and the cross-file mirroring notes.
  • crates/engine/src/game/effects/tap_untap.rs#L57-L71: keep the CR citation and the sentence stating there is no source fallback. Drop "verified rather than assumed".

As per path instructions: "CLAUDE.md at the repo root is the authoritative design document", and it requires that "Delayed-trigger and object-incarnation behavior must be annotated with verified CR citations and descriptions".

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.

Inline comments:
In `@crates/engine/src/game/ability_rw.rs`:
- Line 3729: Update the target_incarnations comment to remove the unsupported CR
603.7c reference and retain only the supported CR 400.7 citation and its
object-pin explanation; do not change the target_incarnations behavior.

In `@crates/engine/src/game/effects/flip_permanent.rs`:
- Around line 43-50: Add a production-pipeline test for the stale pinned-target
guard in the delayed flip ability flow, using a non-SelfRef ParentTarget with
populated target_incarnations; bump the referent’s incarnation before
resolution, then assert the referent remains unflipped and an EffectResolved
event is emitted. Do not reuse self_flip_does_not_follow_a_blinked_source, since
it only covers stale_self_flip.

In `@crates/engine/src/types/ability.rs`:
- Around line 23828-23850: The current target filtering can preserve slot
indexes incorrectly by allowing stale object IDs to resolve to re-entered
objects. In crates/engine/src/types/ability.rs:23828-23850, add a reusable
slot-aware lookup that indexes self.targets directly and rejects a stale object
at the requested slot. In crates/engine/src/game/effects/effect.rs:760-772,
route ParentTargetSlot through it without binding transient effects from raw
targets; in counters.rs:1848-1853, attach.rs:252-273 and 560-567, and
sacrifice.rs:232-240, validate each selected slot/object before use, retaining
declared positional indexes and rejecting stale incarnations.

In `@crates/engine/tests/integration/delayed_parent_target_incarnation.rs`:
- Around line 134-162: Update advance_past_end_of_turn to panic on an error from
runner.act, matching advance_until_delayed_triggers_resolve instead of returning
early. After the loop, assert that runner.state().turn_number is greater than
start_turn so callers such as T-D1 arm 2 cannot pass without advancing the turn.
- Around line 408-424: Update the T-Z1 reach guard around the resolved removal
to reject Zone::Battlefield, matching T-D1’s assert_ne! pattern so the victim
must have left the battlefield. Add a positive assertion after
advance_until_delayed_triggers_resolve confirming the victim returned to
Zone::Battlefield, while preserving the existing delayed-trigger outcome
assertion and diagnostics.

---

Outside diff comments:
In `@crates/engine/src/game/effects/gain_control.rs`:
- Around line 200-225: Update the target preparation in the gain-control effect
around effect_object_targets and chosen_objects to preserve the ParentTargetSlot
carve-out even when resolve_parent_slot_from_root returns None or a Player.
Exclude ParentTargetSlot from the pre-arm substitution path, mirroring
give_control_object_targets, so positional slot indices are resolved against the
unfiltered live object targets and existing non-slot behavior remains unchanged.

---

Nitpick comments:
In `@crates/engine/src/game/effects/remove_from_combat.rs`:
- Around line 19-45: Bind whether the effect uses TargetFilter::SelfRef while
handling ability.effect in the existing targets match, then reuse that value at
the later guard instead of re-matching ability.effect. Remove the duplicate
discrimination so any future RemoveFromCombat shape is covered by the
compiler-enforced match update.

In `@crates/engine/src/game/effects/transform_effect.rs`:
- Around line 39-62: Add a regression test in the change-zone untargeted
zone-scan path near the call to pinned_object_targets_all_stale, constructing an
ability with empty targets and empty target_incarnations and asserting the
source is transformed or otherwise retains the existing no-explicit-target
behavior. Ensure the test confirms an empty pin list returns false and does not
trigger the stale-target early return, while preserving the helper placement and
raw ability.targets matching logic.
🪄 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: 7a351d31-02b6-4266-9519-b7c55559f2ef

📥 Commits

Reviewing files that changed from the base of the PR and between 11cb159 and 9e3742e.

📒 Files selected for processing (38)
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/effects/additional_phase.rs
  • crates/engine/src/game/effects/attach.rs
  • crates/engine/src/game/effects/cast_from_zone.rs
  • crates/engine/src/game/effects/change_zone.rs
  • crates/engine/src/game/effects/copy_spell.rs
  • crates/engine/src/game/effects/counters.rs
  • crates/engine/src/game/effects/deal_damage.rs
  • crates/engine/src/game/effects/delayed_trigger.rs
  • crates/engine/src/game/effects/destroy.rs
  • crates/engine/src/game/effects/discard.rs
  • crates/engine/src/game/effects/double.rs
  • crates/engine/src/game/effects/effect.rs
  • crates/engine/src/game/effects/extra_turn.rs
  • crates/engine/src/game/effects/flip_permanent.rs
  • crates/engine/src/game/effects/force_block.rs
  • crates/engine/src/game/effects/gain_control.rs
  • crates/engine/src/game/effects/grant_extra_loyalty_activations.rs
  • crates/engine/src/game/effects/phase_out.rs
  • crates/engine/src/game/effects/player_counter.rs
  • crates/engine/src/game/effects/remove_from_combat.rs
  • crates/engine/src/game/effects/reverse_turn_order.rs
  • crates/engine/src/game/effects/sacrifice.rs
  • crates/engine/src/game/effects/skip_next_step.rs
  • crates/engine/src/game/effects/skip_next_turn.rs
  • crates/engine/src/game/effects/tap_untap.rs
  • crates/engine/src/game/effects/transform_effect.rs
  • crates/engine/src/game/effects/vote.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/resolution_prompt.rs
  • crates/engine/src/game/stack.rs
  • crates/engine/src/game/targeting.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/identifiers.rs
  • crates/engine/tests/integration/delayed_parent_target_incarnation.rs
  • crates/engine/tests/integration/main.rs
  • crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs

Comment thread crates/engine/src/game/ability_rw.rs
Comment thread crates/engine/src/game/effects/flip_permanent.rs
Comment thread crates/engine/src/types/ability.rs
Both found by CodeRabbit on #7099, both real.

`advance_past_end_of_turn` returned silently when `runner.act` errored, so no
game time passed and every downstream "the referent survived to end of turn"
assertion held trivially. It now panics with the phase and `waiting_for`, and
asserts the turn actually advanced.

T-Z1's reach-guard accepted `Zone::Battlefield`, which is also the state where
the removal never resolved and the victim never died — and the test's conclusion
is also `Battlefield`, so both held while proving nothing about Saffi's delayed
trigger. Tightening it to `assert_ne!(Battlefield)` made the test FAIL, which
confirmed the control had been vacuous.

The final zone cannot serve as the guard here: Saffi's trigger fires on the
death and returns the card within the same resolution, so the victim
legitimately ends on the battlefield — indistinguishable from never having left.
The guard now asserts the `ZoneChanged { to: Graveyard }` event for the victim,
proving the death happened.

This matters because T-Z1 is the regression control the CR 400.7 predicate
design rests on: it is the evidence that "when it dies, return that card" cards
still work. It now carries that weight.

8/8 green in the delayed-trigger file.
@matthewevans

Copy link
Copy Markdown
Member Author

Thanks — two of these were real and are now fixed, and one of them was a genuinely valuable catch.

Fixed

advance_past_end_of_turn could exit before the turn ends — correct, and fixed in f5d4e11. The helper returned silently on runner.act error, so no game time passed and every downstream "survived to end of turn" assertion held trivially. It now panics with the phase and waiting_for, and asserts the turn actually advanced.

T-Z1's reach-guard admitted the "never died" case — correct, and this one mattered. Tightening it to assert_ne!(Battlefield) made the test fail, which confirmed the control had been vacuous.

The fix isn't the one suggested, though, and the reason is worth recording: the final zone can't serve as the guard here at all. Saffi's trigger fires on the death and returns the card within the same resolution, so the victim legitimately ends on the battlefield — indistinguishable from never having left. The guard now asserts the ZoneChanged { to: Graveyard } event for the victim, which proves the death happened regardless of where the object ends up.

This matters beyond the one test: T-Z1 is the regression control the whole CR 400.7 predicate design rests on — it is the evidence that "when it dies, return that card" cards (Saffi, Adarkar Valkyrie, Cryptek, Together Forever) still work under the pin. It now actually carries that weight.

Not changing — with evidence

ability_rw.rs:3729 — "remove the unverified CR 603.7c reference". CR 603.7c is exactly the governing rule, and your own web query in the analysis confirms its text verbatim: "if that object left that zone and then returned, it is considered a new object and thus will not be affected (see rule 400.7)". That parenthetical is the bug being fixed — Goryo's Vengeance + Ephemerate.

The "unverified" verdict came from rg -n '400\.7|603\.7c' docs/MagicCompRules.txt returning 152 bytes. That file is gitignored in this repo (it isn't redistributed); it's fetched locally via ./scripts/fetch-comp-rules.sh. So the grep found nothing and the rule read as unsupported. Verified locally at docs/MagicCompRules.txt:2620.

ability.rs:23850 — slot-index validation. The concern is sound in principle: with slot 0 stale and another pin current, the all-stale guard doesn't fire and a raw slot read could resolve to the re-entered object.

Measured population: zero. ParentTargetSlot appears in 14 cards, exactly one of which (stolen uniform) sits inside a delayed trigger — and that one is not in the 317-pair class at all, because its delayed effect carries ParentTargetSlot and never ParentTarget. Its condition is WhenNextEvent { mode: ChangesController }, which isn't allowlisted, so no pin is stamped for it in the first place.

The affirmative slot pin-checks were deliberately cut for that reason, per this repo's "don't design for hypothetical future requirements" rule, and filed as a follow-up. What was kept is the slot renumbering carve-out on the substitutions this change introduces — that one isn't speculative, it's a correctness constraint on the edit itself, and it has a unit test.

Acknowledged, tracked

flip_permanent.rs needs a test for the stale pinned-target guard — agreed, and it's in the PR's disclosed residual rather than silently omitted. flip_permanent and transform_effect take early-return-only (substituting there would hit their [] => ability.source_id arm and rebind the effect to its own source), and neither has a dedicated production-path test; they rest on the full-suite regression signal. Ten planned tests remain unwritten and are listed in the PR body.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Generated for head f5d4e11cbfd41e486796ec443f378e5cc50760a9.

Parse changes introduced by this PR

✓ No card-parse changes detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant