From c4616672db58275a612d847ef84ad85062e58467 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 02:17:27 -0700 Subject: [PATCH 1/3] fix(engine): don't affect a blinked referent from a delayed trigger (CR 400.7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/engine/src/game/ability_rw.rs | 1 + crates/engine/src/game/ability_scan.rs | 1 + .../src/game/effects/additional_phase.rs | 1 + crates/engine/src/game/effects/attach.rs | 62 +- .../engine/src/game/effects/cast_from_zone.rs | 37 +- crates/engine/src/game/effects/change_zone.rs | 21 + crates/engine/src/game/effects/copy_spell.rs | 56 +- crates/engine/src/game/effects/counters.rs | 24 +- crates/engine/src/game/effects/deal_damage.rs | 30 +- .../src/game/effects/delayed_trigger.rs | 594 ++++++++++++- crates/engine/src/game/effects/destroy.rs | 9 +- crates/engine/src/game/effects/discard.rs | 31 +- crates/engine/src/game/effects/double.rs | 1 + crates/engine/src/game/effects/effect.rs | 38 +- crates/engine/src/game/effects/extra_turn.rs | 1 + .../engine/src/game/effects/flip_permanent.rs | 25 + crates/engine/src/game/effects/force_block.rs | 13 +- .../engine/src/game/effects/gain_control.rs | 41 +- .../grant_extra_loyalty_activations.rs | 1 + crates/engine/src/game/effects/phase_out.rs | 36 +- .../engine/src/game/effects/player_counter.rs | 2 + .../src/game/effects/remove_from_combat.rs | 42 +- .../src/game/effects/reverse_turn_order.rs | 1 + crates/engine/src/game/effects/sacrifice.rs | 27 +- .../engine/src/game/effects/skip_next_step.rs | 1 + .../engine/src/game/effects/skip_next_turn.rs | 1 + crates/engine/src/game/effects/tap_untap.rs | 17 +- .../src/game/effects/transform_effect.rs | 24 + crates/engine/src/game/effects/vote.rs | 7 + crates/engine/src/game/layers.rs | 6 +- crates/engine/src/game/resolution_prompt.rs | 1 + crates/engine/src/game/stack.rs | 10 + crates/engine/src/game/targeting.rs | 41 +- crates/engine/src/types/ability.rs | 108 +++ crates/engine/src/types/identifiers.rs | 12 + .../delayed_parent_target_incarnation.rs | 833 ++++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + .../the_chain_veil_loyalty_grants.rs | 1 + 38 files changed, 2096 insertions(+), 62 deletions(-) create mode 100644 crates/engine/tests/integration/delayed_parent_target_incarnation.rs diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 44fb97291f..01fcf6e73f 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -3726,6 +3726,7 @@ fn walk_ability( trigger_source: _, // exact triggered-source authority, no read/write effect trigger_definition_ref: _, // exact trigger occurrence, no read/write effect force_block_attacker: _, // exact force-block referent, no read/write effect + target_incarnations: _, // CR 400.7 pins on the referents, no read/write effect controller: _, original_controller: _, scoped_player: _, diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index f41146baf8..79587e1724 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -233,6 +233,7 @@ fn resolved_ability_axes(a: &ResolvedAbility, mode: ScanMode) -> Axes { trigger_source: _, // exact triggered-source authority, no dynamic read trigger_definition_ref: _, // exact trigger occurrence, no dynamic read force_block_attacker: _, // exact force-block referent, no dynamic read + target_incarnations: _, // CR 400.7 referent pins, no dynamic read controller: _, // player id original_controller: _, // player id scoped_player: _, // player id (iteration binding) diff --git a/crates/engine/src/game/effects/additional_phase.rs b/crates/engine/src/game/effects/additional_phase.rs index 77a35200da..d11ce6b811 100644 --- a/crates/engine/src/game/effects/additional_phase.rs +++ b/crates/engine/src/game/effects/additional_phase.rs @@ -271,6 +271,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/attach.rs b/crates/engine/src/game/effects/attach.rs index 738b34b356..5aacc01d7c 100644 --- a/crates/engine/src/game/effects/attach.rs +++ b/crates/engine/src/game/effects/attach.rs @@ -88,6 +88,28 @@ pub fn resolve( return Ok(()); } + // CR 400.7 + CR 603.7c: a delayed attach whose pinned referent became a new + // object attaches nothing. The trigger fired and resolved (CR 603.7b); it + // affected nothing. + // + // PLACEMENT IS LOAD-BEARING — this MUST sit above the two `ok_or_else(..)?` + // conversions below. Letting the substitution in `resolve_object_filter` + // empty the list instead would surface `EffectError::MissingParam` and emit + // NO EffectResolved. Mirrors the CR 303.4j no-op guard further down, which + // is this function's proof that an events sink is in scope here. + // + // Reached by `gift of immortality` and `next of kin`: their ROOT is + // `ChangeZone{SelfRef}`, but `ability_pins_object_anaphor` walks the whole + // chain, so the `Attach` sub-ability's `ParentTarget` earns the pins. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::from(&ability.effect), + source_id, + subject: None, + }); + return Ok(()); + } + // CR 608.2h + CR 608.2k: Typed attachment operands resolve from the // battlefield/LKI unless they are explicit player-chosen targets. // Typed/scan-based attachment filters (e.g. "Equipment attached to ~") resolve @@ -227,7 +249,28 @@ pub fn resolve_unattach_all( attachment_filter, TargetFilter::ParentTarget | TargetFilter::ParentTargetSlot { .. } ) - .then(|| super::effect_object_targets(attachment_filter, &ability.targets)); + .then(|| { + // CR 400.7 + CR 603.7c: substitution only. Population inside the class + // is 0 (zero `UnattachAll` nodes), and the one card that reaches this + // (`stolen uniform`) is denied a pin by + // `condition_names_referent_zone_change`, so `live_object_targets` is + // the identity here today. Applied for uniformity and defence in depth; + // deliberately NO early return, since a population of 0 admits no + // non-vacuous test. + // + // Slot carve-out DOES apply: `attachment_filter` may be + // `ParentTargetSlot`, and `effect_object_targets` indexes it + // positionally. This is the second call site of the standing + // 22-call-site constraint. + let live_targets = ability.live_object_targets(state); + let pool: &[TargetRef] = + if matches!(attachment_filter, TargetFilter::ParentTargetSlot { .. }) { + &ability.targets + } else { + &live_targets + }; + super::effect_object_targets(attachment_filter, pool) + }); for target_id in target_ids { let attachments = state .objects @@ -494,10 +537,19 @@ fn resolve_object_filter<'a>( TargetRef::Player(_) => None, }) } - TargetFilter::ParentTarget => ability.targets.iter().find_map(|target| match target { - TargetRef::Object(id) => Some(*id), - TargetRef::Player(_) => None, - }), + // CR 400.7 + CR 603.7c: a pinned referent that became a new object is + // dropped. The all-stale case is caught by the early return at the top + // of `resolve` (which can emit `EffectResolved`); this substitution + // handles the PARTIAL-stale case. + TargetFilter::ParentTarget => { + ability + .live_object_targets(state) + .into_iter() + .find_map(|target| match target { + TargetRef::Object(id) => Some(id), + TargetRef::Player(_) => None, + }) + } // CR 608.2c: a precise slot anaphor ("Attach it to the chosen creature" // → attachment slot 1, target slot 0) resolves against the whole // resolving chain's accumulated targets. The per-clause `ability.targets` diff --git a/crates/engine/src/game/effects/cast_from_zone.rs b/crates/engine/src/game/effects/cast_from_zone.rs index ca87e5be31..5ffc7c49e1 100644 --- a/crates/engine/src/game/effects/cast_from_zone.rs +++ b/crates/engine/src/game/effects/cast_from_zone.rs @@ -402,8 +402,22 @@ pub fn resolve( TargetFilter::TrackedSet { .. } | TargetFilter::TrackedSetFiltered { .. } => { tracked_set_cast_candidates(state, ability, target_filter) } + // CR 400.7 + CR 603.7c: a delayed cast-from-zone whose pinned referent + // became a new object casts nothing. This `_` arm is THE single read + // through which a pinned referent flows into this resolver. + // + // The plan pre-flagged this file as a possible STOP because of its three + // `scoped_ability.targets = …` assignments (`:112`, `:444`, `:517`) and + // `fallback.targets = ability.targets.clone()` (`:257`). Re-read at the + // source, none of those is a read of the pinned referent: all three + // `scoped_ability` sites WRITE a freshly-derived id list onto a throwaway + // clone purely to scope a `FilterContext`, and their inputs + // (`deduped`, `candidate_ids`, `target_ids`) are already downstream of + // this arm. `:257` is chain-context propagation onto a declined-optional + // fallback ability, not a target resolution. So the flat substitution + // does apply here, at exactly one site. _ => ability - .targets + .live_object_targets(state) .iter() .filter_map(|t| { if let TargetRef::Object(id) = t { @@ -415,6 +429,27 @@ pub fn resolve( .collect(), }; + // CR 400.7 + CR 603.7c + CR 603.7b: the trigger fired and resolved; it cast + // nothing. EARLY RETURN IS MANDATORY, and its placement immediately below + // the read is load-bearing: EVERY branch between here and the end of this + // function keys on `target_ids.is_empty()` and re-binds to a DIFFERENT set + // of objects — the linked-exile scan (`:432`), the `last_revealed` library + // scan (`:467`), and the `SelfRef` source fallback (`:570`). Letting the + // substitution above empty the list without returning would hand the cast to + // one of those pools instead of doing nothing. + // + // Mirrors the existing "No targets resolved — nothing to cast" exit below, + // including its `EffectKind::CastFromZone` literal, so both no-op paths emit + // the same event. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::CastFromZone, + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 701.20e + CR 608.2c: Look-then-cast chains (Kiora) inject the legal // looked-at library cards as targets at the chain seam // (`inject_last_revealed_targets`), already filtered through this cast diff --git a/crates/engine/src/game/effects/change_zone.rs b/crates/engine/src/game/effects/change_zone.rs index a17d70f0c9..31143ec135 100644 --- a/crates/engine/src/game/effects/change_zone.rs +++ b/crates/engine/src/game/effects/change_zone.rs @@ -543,6 +543,27 @@ pub fn resolve( return Ok(()); } + // CR 400.7 + CR 603.7c: a delayed ability whose pinned referent became a + // new object affects nothing. Return before the untargeted zone-scan + // below, which must not rediscover a same-id return — the same reason + // the SelfRef guard above exists. `filter.rs`'s never-match arm makes + // that scan inert for ParentTarget today; this guard does not rely on + // that distant `_ => false` arm. + // + // Emits EffectResolved first, exactly as the three sibling guards in + // this block do (CR 115.6 optional targeting, CR 400.7 SelfRef, + // CR 701.23b fail-to-find): the trigger DID fire and DID resolve + // (CR 603.7b) — it simply affected nothing, and the game log / event + // observers / chain machinery must see that. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::from(&ability.effect), + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 701.23b + CR 401.2: Interactive library-step fail-to-find guard. // The parser emits `origin=Library, target=Any` for the put-step of a // chain where an earlier interactive step selects the card from the diff --git a/crates/engine/src/game/effects/copy_spell.rs b/crates/engine/src/game/effects/copy_spell.rs index 7f6b510651..532b0dd2e3 100644 --- a/crates/engine/src/game/effects/copy_spell.rs +++ b/crates/engine/src/game/effects/copy_spell.rs @@ -22,6 +22,33 @@ pub fn resolve( ability: &ResolvedAbility, events: &mut Vec, ) -> Result<(), EffectError> { + // CR 400.7 + CR 603.7c: a delayed copy whose pinned referent became a new + // object copies nothing. The trigger DID fire and DID resolve (CR 603.7b) — + // it simply affected nothing — so the game log, event observers and the + // chain machinery must see an EffectResolved, exactly as the sibling + // `stack_entry_cant_be_copied` guard below does. + // + // PLACEMENT IS LOAD-BEARING: this MUST sit ABOVE the `ok_or_else(..)?` + // below. Returning `None` from `copy_source_entry` instead converts a + // deliberate no-op into `EffectError::MissingParam` and emits NO + // EffectResolved, because the `?` short-circuits before any events.push. + // The guard belongs at a function that can say "resolved, did nothing", not + // one that can only say "absent". + // + // Inert for every non-pinned caller: `pinned_object_targets_all_stale` + // requires a non-empty `target_incarnations`, which only a pinned delayed + // ParentTarget trigger has. Measured: all 14 in-class CopySpell pairs carry + // `target: ParentTarget`, so Saruman (ExiledBySource) and Isochron Scepter + // (TrackedSet) can never satisfy it and their branches run untouched. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::from(&ability.effect), + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 707.10 / CR 702.153a (Casualty): resolve which stack entry to copy. // The helper handles explicit object targets (Twincast / Gogo), SelfRef // (Casualty triggers whose intermediate stack pushes would make stack.last() @@ -170,10 +197,16 @@ pub fn resolve( .. } ) { - if let Some(member) = ability.targets.iter().find_map(|target| match target { - TargetRef::Object(id) => Some(*id), - TargetRef::Player(_) => None, - }) { + // CR 400.7 + CR 603.7c: sits below the all-stale guard above, so this + // substitution handles the PARTIAL-stale case the guard does not. + if let Some(member) = ability + .live_object_targets(state) + .iter() + .find_map(|target| match target { + TargetRef::Object(id) => Some(*id), + TargetRef::Player(_) => None, + }) + { if let Some(copy_ability) = state.stack.back_mut().and_then(|e| e.ability_mut()) { rewrite_copy_spell_object_targets(copy_ability, member); } @@ -560,10 +593,17 @@ fn copy_source_entry(state: &GameState, ability: &ResolvedAbility) -> Option Some(*id), - TargetRef::Player(_) => None, - }); + // CR 400.7 + CR 603.7c: covers the partial-stale case, and is defence in + // depth for any future caller of `copy_source_entry` that does not pass + // through the guarded `resolve`. + let target_id = + ability + .live_object_targets(state) + .into_iter() + .find_map(|target| match target { + TargetRef::Object(id) => Some(id), + TargetRef::Player(_) => None, + }); if let Some(target_id) = target_id { return state .stack diff --git a/crates/engine/src/game/effects/counters.rs b/crates/engine/src/game/effects/counters.rs index abbc025b74..d83d08279b 100644 --- a/crates/engine/src/game/effects/counters.rs +++ b/crates/engine/src/game/effects/counters.rs @@ -1845,6 +1845,12 @@ fn resolve_defined_or_targets( // local `ability.targets` may have been replaced with the most-recent parent // slot by chain propagation, so resolve against the flattened chain root // (single authority in `targeting`), then keep only the object at `index`. + // + // CR 400.7 + CR 603.7c: deliberately NOT pin-filtered — see the standing + // constraint recorded at the `ParentTargetSlot` arm in + // `targeting::resolved_object_ids_for_filter_with_context`. Slot numbering + // is declared, not live, so filtering here would renumber later slots. Do + // not "complete the pattern" by adding a pin check. if let Some(TargetFilter::ParentTargetSlot { index }) = target_spec { return crate::game::targeting::resolve_parent_slot_from_root(state, ability, *index) .into_iter() @@ -1901,12 +1907,24 @@ fn resolve_defined_or_targets( } } + // CR 400.7 + CR 603.7c: a delayed counter effect's pinned referent that + // became a new object is dropped. Substitution only: the source-fallback + // arms and the attack-batch arm above all key off `has_object_target` / + // `has_choice_bookkeeping_player`, computed from the RAW `ability.targets`, + // so those preconditions stay intact and an emptied list here cannot + // re-bind to a different object. No early return, hence no EffectResolved + // question. + // + // This is `lagrella, the magpie`'s route: her delayed `PutCounter` reaches + // here with the returned card in `targets`. She is correct only because the + // pin is never STAMPED for her (her condition names the referent's own + // entry) — not because this read is exempted. ability - .targets - .iter() + .live_object_targets(state) + .into_iter() .filter_map(|t| { if let TargetRef::Object(id) = t { - Some(*id) + Some(id) } else { None } diff --git a/crates/engine/src/game/effects/deal_damage.rs b/crates/engine/src/game/effects/deal_damage.rs index 34a9645b07..2072056a03 100644 --- a/crates/engine/src/game/effects/deal_damage.rs +++ b/crates/engine/src/game/effects/deal_damage.rs @@ -150,11 +150,37 @@ fn resolve_effect_recipients( .into_iter() .collect(); } + // CR 400.7 + CR 603.7c: a delayed damage effect whose pinned referent became + // a new object deals it no damage. This is a RAW read that never reaches + // `resolved_targets`, so the targeting chokepoint cannot see this pin. + // + // THE `is_empty()` GATE STAYS RAW, AND THAT IS LOAD-BEARING — it is the + // reason no early return is needed here. "Targets were declared" and + // "declared targets are still live" are different questions. Gating on the + // raw list means an all-stale ability still ENTERS this branch and returns + // an empty recipient list, an inert no-op. Substituting the gate itself + // would make it fall through to the `Controller` fallback below and deal + // the damage to a PLAYER instead — precisely the `searing blood` shape the + // Tier C exclusion list warns about (its 14 Controller/Owner-only cards are + // additionally denied pins upstream, so this can only ever fail safe). if !ability.targets.is_empty() { if skip_first_target && ability.targets.len() > 1 { - return ability.targets[1..].to_vec(); + // The positional split runs on the RAW list and the pin filter is + // applied AFTER it — never the other way round. `[1..]` encodes slot + // identity (`[source_0, …, recipient]`), so filtering first could + // renumber a live recipient into the source position. This is the + // same constraint as §5.4(b)'s `ParentTargetSlot` carve-out, applied + // to this file's own positional convention. + return ability.targets[1..] + .iter() + .filter(|target| match target { + TargetRef::Object(id) => ability.target_pin_is_current(*id, state), + TargetRef::Player(_) => true, + }) + .cloned() + .collect(); } - return ability.targets.clone(); + return ability.live_object_targets(state); } match target_filter { TargetFilter::Controller => vec![TargetRef::Player(ability.controller)], diff --git a/crates/engine/src/game/effects/delayed_trigger.rs b/crates/engine/src/game/effects/delayed_trigger.rs index 1412b3dd3e..9bbdf19bbc 100644 --- a/crates/engine/src/game/effects/delayed_trigger.rs +++ b/crates/engine/src/game/effects/delayed_trigger.rs @@ -32,6 +32,34 @@ pub fn resolve( } }; + // CR 603.7c + CR 400.7e: Decide the expected-zone question from the + // PARSER-EMITTED condition, while its `ParentTarget` anaphor is still + // visible. + // + // PLACEMENT IS LOAD-BEARING — DO NOT SINK THIS CALL. Two binders below + // rewrite the exact filter shapes this predicate keys on: + // * `bind_tracked_set_to_condition` — ParentTarget | Any | TrackedSet(0) + // -> TrackedSet { real_id } + // * `bind_contextual_filter_to_condition` — ParentTarget -> SpecificObject + // / Or / Any; ParentTargetSlot likewise + // Evaluated after either of them, this predicate returns `false` for EVERY + // in-class pair — both sides of its discrimination collapse to `false`, the + // pin is always stamped, and Saffi Eriksdotter / Adarkar Valkyrie / Cryptek / + // Together Forever / Whippoorwill / Fatal Fissure / Lagrella go permanently + // inert. (Lagrella is the sharpest case: it is a tracked-set form, so its + // condition is erased before the contextual bind runs at all.) + // + // NOTE the asymmetry with `ability_pins_object_anaphor` below, which + // correctly stays inside the `ability_refs_parent_target` arm: that one + // reads the EFFECT chain and is nested inside a post-bind read of the same + // chain, so the two agree by construction. This one reads the CONDITION and + // has no such nesting. + // + // The shipped `WheneverEvent` empty-parent early return below is the same + // pattern and documents the same hazard: a decision that must be taken + // before a binder erases its discriminator. + let condition_expects_referent_move = condition_names_referent_zone_change(&condition); + // CR 603.7 + CR 608.2c: Resolve the most-recent tracked set once, up front, // so the tracked-set CONDITION rewrite runs BEFORE the single-target // contextual bind below. Genuine "those cards" tracked-set forms (Ugin the @@ -195,30 +223,83 @@ pub fn resolve( condition, crate::types::ability::DelayedTriggerCondition::WheneverEvent { .. } ); - let snapshot_targets = if one_shot && super::ability_refs_triggering_source(&delayed_ability) { + // CR 400.7 + CR 603.7c: Pin each snapshotted ParentTarget referent to the + // incarnation it has right now. CR 400.7j lets the later parts of this same + // effect find an object this effect just moved to a public zone (Goryo's + // Vengeance reanimates, then refers to the creature it returned), so the + // epoch captured here is the post-move one. If that object later changes + // zones, with or without returning, it becomes a new object and this pin + // stops matching. + // + // TWO gates, both mandatory: + // * `ability_pins_object_anaphor` — only genuine ParentTarget/ParentTargetSlot + // OBJECT anaphors. Controller/Owner derive players and are built to + // survive departure under CR 608.2h. Correct to evaluate HERE, inside + // this arm, because the enclosing `ability_refs_parent_target` is a + // post-bind read of the SAME chain (`bind_tracked_set_to_ability_chain` + // already ran), so the two agree by construction. Do NOT hoist it above + // that binder. + // * `!condition_expects_referent_move` — computed at the top of this + // function from the PARSER-EMITTED condition. A delayed trigger whose + // CONDITION is the referent's own zone change expects the referent to + // have moved (CR 603.7c operative test; CR 400.7e). Pinning it would make + // it inert forever. THIS VALUE MUST COME FROM THERE — recomputing it here + // reads a condition both binders have already rewritten and yields + // `false` for every card in the class. + // + // Scoped to the ParentTarget arm: the TriggeringSource arm re-resolves from + // the firing event and already carries a creation-time zone guard + // (`stamp_triggering_source_origins_in_ability_chain`, below); the + // LastCreated arm names tokens, which cease to exist on a zone change + // (CR 111.7) rather than returning as a new incarnation. + let (snapshot_targets, target_pins) = if one_shot + && super::ability_refs_triggering_source(&delayed_ability) + { // CR 603.7c: TriggeringSource always reads the event context (the dying // creature from the ZoneChanged event), not the parent ability's chosen // targets. Bypasses parent_target_snapshot's ability.targets early-return, // which is correct for ParentTarget (Flickerwisp) but wrong here. - crate::game::targeting::resolve_event_context_target( - state, - &crate::types::ability::TargetFilter::TriggeringSource, - ability.source_id, + ( + crate::game::targeting::resolve_event_context_target( + state, + &crate::types::ability::TargetFilter::TriggeringSource, + ability.source_id, + ) + .map(|t| vec![t]) + .unwrap_or_default(), + Vec::new(), ) - .map(|t| vec![t]) - .unwrap_or_default() } else if super::ability_refs_parent_target(&delayed_ability) { - parent_target_snapshot(state, ability) + let targets = parent_target_snapshot(state, ability); + let pins = + if ability_pins_object_anaphor(&delayed_ability) && !condition_expects_referent_move { + targets + .iter() + .filter_map(|target| match target { + TargetRef::Object(id) => state + .objects + .get(id) + .map(crate::types::identifiers::ObjectIncarnationRef::from_object), + TargetRef::Player(_) => None, + }) + .collect() + } else { + Vec::new() + }; + (targets, pins) } else if effect_references_last_created(&delayed_ability.effect) && !state.last_created_token_ids.is_empty() { - state - .last_created_token_ids - .iter() - .map(|&id| TargetRef::Object(id)) - .collect() + ( + state + .last_created_token_ids + .iter() + .map(|&id| TargetRef::Object(id)) + .collect(), + Vec::new(), + ) } else { - vec![] + (vec![], Vec::new()) }; // CR 603.7c: Stamp `ChangeZone.origin` from the CREATION event's @@ -248,6 +329,7 @@ pub fn resolve( rebind_last_created_to_parent_target(&mut delayed_ability.effect); } + delayed_ability.set_target_incarnations_recursive(target_pins); delayed_ability.targets = snapshot_targets; // CR 603.7c: A delayed triggered ability that refers to information from // its creation event keeps that creation-time binding for later resolution. @@ -969,6 +1051,198 @@ fn bind_tracked_set_to_ability_chain(ability: &mut ResolvedAbility, real_id: Tra } } +/// CR 400.7 + CR 603.7c: True when `filter` is an anaphor that names the +/// parent's chosen OBJECT, and is therefore a referent an incarnation pin can +/// govern. +/// +/// Deliberately NARROWER than `effects::filter_refs_parent_target`, which also +/// admits `ParentTargetController` / `ParentTargetOwner`. Those derive a +/// `TargetRef::Player`, not an object: under CR 608.2h they are built to +/// survive the referent's departure (`ability_utils::parent_target_controller` +/// prefers the LKI controller once the object is off-battlefield), and owner is +/// invariant under CR 108.3. Pinning them could only suppress a correct result. +/// +/// Recurses compound filters for the same reason `filter_refs_parent_target` +/// does, so a wrapped anaphor is still found. +/// +/// ALSO USED BY `trigger_names_referent_zone_change` to decide whether an +/// embedded trigger definition names the REFERENT. Keeping `ParentTargetSlot` +/// in the `true` arm is load-bearing there: it is what withholds the pin from +/// `stolen uniform` (`WhenNextEvent { ChangesController, valid_card: +/// ParentTargetSlot }`), the only delayed slot card in the data. Narrowing this +/// arm would silently start pinning it. +/// +/// `_ => false` IS CORRECT HERE. `TargetFilter` is a broad, open enum and the +/// shipped template ends the same way. Do NOT try to exhaust it. +fn filter_refs_parent_object_anaphor(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::ParentTarget | TargetFilter::ParentTargetSlot { .. } => true, + // CR 608.2h + CR 108.3: these derive a PLAYER, not an object. + TargetFilter::ParentTargetController | TargetFilter::ParentTargetOwner => false, + TargetFilter::Typed(typed) => { + // `Typed { controller: ParentTargetController }` selects objects BY + // the parent's controller; the referent being filtered is not the + // parent's object, so it is NOT a parent object anaphor. + typed.properties.iter().any(|prop| { + matches!( + prop, + crate::types::ability::FilterProp::DistinctFrom { reference } + if filter_refs_parent_object_anaphor(reference) + ) + }) + } + TargetFilter::Or { filters } | TargetFilter::And { filters } => { + filters.iter().any(filter_refs_parent_object_anaphor) + } + TargetFilter::Not { filter } => filter_refs_parent_object_anaphor(filter), + _ => false, + } +} + +/// True when any effect in the ability chain references a parent OBJECT anaphor +/// (including nested sub/else abilities). Mirrors `ability_refs_parent_target`'s +/// walk over `effect_parent_ref_slots`; narrower in exactly one respect (above). +fn ability_pins_object_anaphor(ability: &ResolvedAbility) -> bool { + super::effect_parent_ref_slots(&ability.effect) + .iter() + .any(|filter| filter_refs_parent_object_anaphor(filter)) + || ability + .sub_ability + .as_deref() + .is_some_and(ability_pins_object_anaphor) + || ability + .else_ability + .as_deref() + .is_some_and(ability_pins_object_anaphor) +} + +/// CR 400.7 + CR 603.7c: True when this embedded trigger definition names a zone +/// change OF THE REFERENT — i.e. the delayed trigger fires *because* the pinned +/// object moved zones. +/// +/// TWO-STEP, ANAPHOR FIRST. Step 1 asks whether the trigger names the referent +/// at all; measured, that is true for 3 of the 46 embedded trigger definitions +/// in this class, so 43 are answered without inspecting the mode. Step 2 asks +/// whether the named event moves it. +/// +/// Step 1 must read the PARSER-EMITTED filters — `bind_contextual_filter_to_condition` +/// rewrites all three `valid_*` slots. See the call site at the top of `resolve`. +fn trigger_names_referent_zone_change(trigger: &crate::types::ability::TriggerDefinition) -> bool { + let names_referent = [ + &trigger.valid_card, + &trigger.valid_source, + &trigger.valid_target, + ] + .into_iter() + .flatten() + .any(filter_refs_parent_object_anaphor); + + names_referent && !mode_provably_leaves_referent_in_place(&trigger.mode) +} + +/// CR 400.7: Modes VERIFIED not to move the object they name. +/// +/// THE DEFAULT IS DELIBERATELY THE SAFE DIRECTION. `TriggerMode` has 171 +/// variants with no `Enters` and no `Dies` — enters-the-battlefield and dies are +/// BOTH `ChangesZone` — and roughly forty are arguably zone changes +/// (`ChangesZone`, `ChangesZoneAll`, `LeavesBattlefield`, `Exiled`, +/// `Sacrificed`, `Destroyed`, `Milled*`, `Discarded*`, `Drawn`, `Championed`, +/// `Foretell`, `NinjutsuActivated`, `Cycled*`, `Devoured`, `Exploited`, +/// `EntersOr*`, `HauntedCreatureDies`, …). Enumerating the dangerous set means +/// adjudicating each against CR with no card driving it, and a missed one +/// silently PINS a referent the condition expects to have moved. +/// +/// So this allowlist names only what is verified SAFE, and everything else falls +/// to `false` here (=> treated as a zone change => pin withheld => the card +/// keeps its pre-existing behavior). An unrecognized mode can cost coverage on a +/// future card; it can never break one. That asymmetry is the point. +/// +/// Measured at the pinned card-data: the only modes that co-occur with a parent +/// object anaphor in this class are `DamageDone` (long river lurker, niko aris) +/// and `Attacks` (okoye, mighty and adored) — combat/damage events, which per +/// CR 120 and CR 506/508 move nothing between zones. All 3 pairs therefore still +/// pin, so this shape costs ZERO coverage today relative to an exhaustive match. +fn mode_provably_leaves_referent_in_place(mode: &crate::types::triggers::TriggerMode) -> bool { + matches!( + mode, + crate::types::triggers::TriggerMode::DamageDone + | crate::types::triggers::TriggerMode::Attacks + ) +} + +/// CR 603.7c + CR 400.7e: True when this delayed trigger's own condition names a +/// ZONE CHANGE OF THE REFERENT ITSELF — in EITHER direction. +/// +/// CR 603.7c's operative test is whether the object is "no longer in the zone +/// it's expected to be in at the time the delayed triggered ability resolves". +/// For "when that creature dies this turn, return that card…" the expected zone +/// IS the graveyard; for "when an exiled card enters the battlefield this way, +/// put counters on it" (Lagrella) the expected zone IS the battlefield. In both +/// the referent is exactly where it belongs, and CR 400.7e explicitly grants +/// that such an ability "can find the new object that it became in the zone it +/// moved to … if that zone is a public zone". +/// +/// DIRECTION-AGNOSTIC BY DESIGN. An earlier revision named this "…departure" and +/// answered `WhenEntersBattlefield => false` on the strength of the name. That +/// is wrong by this function's own criterion: `zones.rs` bumps the incarnation +/// UNCONDITIONALLY on `to == Zone::Battlefield`, so an entry condition +/// guarantees the creation-time pin is stale at 100% of firings, exactly as a +/// death condition does. Pinning either turns the card into a permanent no-op +/// (Saffi Eriksdotter, Adarkar Valkyrie, Cryptek, Together Forever, +/// Whippoorwill, Fatal Fissure, Lagrella the Magpie). +/// +/// THE DISCRIMINATOR IS THE PARSER-EMITTED CONDITION'S OWN FILTER — not the +/// runtime-bound one. This function MUST be called before +/// `bind_tracked_set_to_condition` and `bind_contextual_filter_to_condition`, +/// which rewrite `ParentTarget` to `TrackedSet` / `SpecificObject` / `Any` and +/// erase the anaphor entirely. See the call site at the top of `resolve`. +/// A condition filtered on `SelfRef` names the SOURCE's departure (Animate +/// Dead's Aura leaving, Golden Guardian's own death), which leaves the +/// referent's expected zone unchanged, so those still pin. +/// +/// EXHAUSTIVE, NO WILDCARD ARM. A new `DelayedTriggerCondition` variant must +/// fail to compile here until someone decides its referent's expected zone. +/// Adding `_ => false` would let a future variant silently inherit the wrong +/// assumption — precisely the defect that produced the `WhenEntersBattlefield` +/// arm above. +fn condition_names_referent_zone_change(condition: &DelayedTriggerCondition) -> bool { + match condition { + // Phase-based: the referent is expected wherever it was at creation. + DelayedTriggerCondition::AtNextPhase { .. } + | DelayedTriggerCondition::AtNextPhaseForPlayer { .. } => false, + + // The condition IS the referent's zone change when it is filtered on the + // referent; filtered on `SelfRef` it is the SOURCE's zone change. + // `WhenEntersBattlefield` gets IDENTICAL treatment to the departure + // family — an entry moves the referent exactly as a departure does. + DelayedTriggerCondition::WhenDies { filter } + | DelayedTriggerCondition::WhenLeavesPlayFiltered { filter } + | DelayedTriggerCondition::WhenDiesOrExiled { filter } + | DelayedTriggerCondition::WhenEntersBattlefield { filter } => { + filter_refs_parent_object_anaphor(filter) + } + + // Names a specific object leaving; that object is the referent. + // (0 in-class pairs today — decided here so it cannot silently appear.) + DelayedTriggerCondition::WhenLeavesPlay { .. } => true, + + // Delegate to the embedded trigger definition(s). + DelayedTriggerCondition::WheneverEvent { trigger, .. } => { + trigger_names_referent_zone_change(trigger) + } + DelayedTriggerCondition::WhenNextEvent { + trigger, + or_trigger, + .. + } => { + trigger_names_referent_zone_change(trigger) + || or_trigger + .as_deref() + .is_some_and(trigger_names_referent_zone_change) + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -3147,4 +3421,296 @@ mod tests { other => panic!("expected ChangeZoneAll, got {other:?}"), } } + + // ================================================================ T-U3 + /// T-U3 — `condition_names_referent_zone_change`, exhaustive over the + /// shipped variants, in BOTH shapes. + /// + /// This is the §5.3(b) predicate's contract table. It is a UNIT test and by + /// construction it CANNOT see placement — it builds pre-bind shapes + /// directly, so it stays fully green even when the production call site is + /// misplaced. That blind spot is exactly why T-D1 exists; do not read a + /// green here as evidence the gate is called in the right place. + #[test] + fn t_u3_condition_names_referent_zone_change_contract() { + use DelayedTriggerCondition as C; + + // ---- (i) PRE-BIND rows: the shapes production actually passes. ---- + assert!(!condition_names_referent_zone_change(&C::AtNextPhase { + phase: Phase::End + })); + assert!(!condition_names_referent_zone_change( + &C::AtNextPhaseForPlayer { + phase: Phase::Upkeep, + player: PlayerId(0), + gate: Default::default(), + } + )); + + // The referent's OWN zone change ⇒ no pin. + assert!(condition_names_referent_zone_change(&C::WhenDies { + filter: TargetFilter::ParentTarget + })); + assert!(condition_names_referent_zone_change( + &C::WhenLeavesPlayFiltered { + filter: TargetFilter::ParentTarget + } + )); + assert!(condition_names_referent_zone_change(&C::WhenDiesOrExiled { + filter: TargetFilter::ParentTarget + })); + // The B-R3-2 arm: entry is a zone change too. `zones.rs` bumps the + // incarnation unconditionally on `to == Battlefield`, so pinning this + // would make the card a permanent no-op at 100% of firings. + assert!(condition_names_referent_zone_change( + &C::WhenEntersBattlefield { + filter: TargetFilter::ParentTarget + } + )); + assert!(condition_names_referent_zone_change(&C::WhenLeavesPlay { + object_id: ObjectId(7) + })); + + // ANTI-VACUITY HALF: a stub returning `true` for every `WhenDies` + // passes every row above and fails every row here. `SelfRef` names the + // SOURCE's departure (Animate Dead's Aura, Golden Guardian's own + // death), which leaves the REFERENT's expected zone unchanged. + assert!(!condition_names_referent_zone_change(&C::WhenDies { + filter: TargetFilter::SelfRef + })); + assert!(!condition_names_referent_zone_change( + &C::WhenLeavesPlayFiltered { + filter: TargetFilter::SelfRef + } + )); + assert!(!condition_names_referent_zone_change( + &C::WhenEntersBattlefield { + filter: TargetFilter::SelfRef + } + )); + + // ---- (ii) POST-BIND rows — the anti-vacuity half B-R3-1 requires. ---- + // + // ⚠️ READ BEFORE "FIXING" ANY OF THESE. + // + // These are NOT shapes production passes. They are the shapes the two + // binders (`bind_tracked_set_to_condition`, + // `bind_contextual_filter_to_condition`) PRODUCE, and the gate is + // called at the top of `resolve` — before both — precisely so it never + // sees them. Their `false` answers are correct only in that light: the + // anaphor has been erased, so the predicate genuinely cannot recognize + // the referent any more. + // + // Turning any of these into `true` would break every pinned card. If + // you got here because a pinned card regressed, the bug is the CALL + // SITE having moved below a binder — see T-D1, which is the test that + // detects that. + assert!(!condition_names_referent_zone_change(&C::WhenDies { + filter: TargetFilter::SpecificObject { id: ObjectId(3) } + })); + assert!(!condition_names_referent_zone_change(&C::WhenDies { + filter: TargetFilter::TrackedSet { + id: TrackedSetId(1) + } + })); + assert!(!condition_names_referent_zone_change(&C::WhenDies { + filter: TargetFilter::Any + })); + // Lagrella's post-bind shape. + assert!(!condition_names_referent_zone_change( + &C::WhenEntersBattlefield { + filter: TargetFilter::TrackedSet { + id: TrackedSetId(1) + } + } + )); + } + + // ================================================================ T-U4 + /// T-U4 — `filter_refs_parent_object_anaphor` is genuinely NARROWER than + /// the shared `filter_refs_parent_target`, in exactly the two intended + /// arms and identical elsewhere. + /// + /// The second half asserts the SHARED function still answers `true` for all + /// four anaphors — i.e. that it was not modified. Widening or narrowing + /// `filter_refs_parent_target` is a hard non-goal, and this is its guard. + #[test] + fn t_u4_parent_object_anaphor_is_narrower_than_parent_target() { + use crate::game::effects::filter_refs_parent_target; + + // The two OBJECT anaphors ⇒ true. + assert!(filter_refs_parent_object_anaphor( + &TargetFilter::ParentTarget + )); + // LOAD-BEARING: this row is what makes T-U3's `stolen uniform` + // (`ChangesController` + `ParentTargetSlot`) row work, and therefore + // what makes §5.4(b)'s slot cut safe. + assert!(filter_refs_parent_object_anaphor( + &TargetFilter::ParentTargetSlot { index: 1 } + )); + + // The two PLAYER anaphors ⇒ false. These are the narrowing, and they + // are why `searing blood` / `touch of moonglove` keep working (CR + // 608.2h / issue #1582): a controller or owner reference must never be + // pinned to an OBJECT incarnation. + assert!(!filter_refs_parent_object_anaphor( + &TargetFilter::ParentTargetController + )); + assert!(!filter_refs_parent_object_anaphor( + &TargetFilter::ParentTargetOwner + )); + + // Recursion is preserved through composite filters. + assert!(filter_refs_parent_object_anaphor(&TargetFilter::Or { + filters: vec![TargetFilter::SelfRef, TargetFilter::ParentTarget], + })); + + // ---- The shared function was NOT modified: all four still true. ---- + assert!(filter_refs_parent_target(&TargetFilter::ParentTarget)); + assert!(filter_refs_parent_target(&TargetFilter::ParentTargetSlot { + index: 1 + })); + assert!(filter_refs_parent_target( + &TargetFilter::ParentTargetController + )); + assert!(filter_refs_parent_target(&TargetFilter::ParentTargetOwner)); + } + + // ================================================================ T-U5 + /// T-U5 — `trigger_names_referent_zone_change`: the anaphor-first two-step + /// order, and the deliberate SAFE DEFAULT. + #[test] + fn t_u5_trigger_names_referent_zone_change_two_step_order() { + use crate::types::triggers::TriggerMode; + + // Step 1 short-circuits: no referent named ⇒ the mode is never + // consulted. Without this ordering every `SpellCast` delayed trigger + // would be classified on its mode alone. + let bare = TriggerDefinition::new(TriggerMode::SpellCast); + assert!(!trigger_names_referent_zone_change(&bare)); + + // Allowlisted modes: the referent is named, but the event provably + // leaves it where it is. `DamageDone` is the `long river lurker` / + // `niko aris` shape; `Attacks` is the `okoye` shape. + let mut damage = TriggerDefinition::new(TriggerMode::DamageDone); + damage.valid_source = Some(TargetFilter::ParentTarget); + assert!(!trigger_names_referent_zone_change(&damage)); + + let mut attacks = TriggerDefinition::new(TriggerMode::Attacks); + attacks.valid_card = Some(TargetFilter::ParentTarget); + assert!(!trigger_names_referent_zone_change(&attacks)); + + // Genuine zone-change modes naming the referent ⇒ true. + let mut changes_zone = TriggerDefinition::new(TriggerMode::ChangesZone); + changes_zone.valid_card = Some(TargetFilter::ParentTarget); + assert!(trigger_names_referent_zone_change(&changes_zone)); + + let mut leaves = TriggerDefinition::new(TriggerMode::LeavesBattlefield); + leaves.valid_card = Some(TargetFilter::ParentTarget); + assert!(trigger_names_referent_zone_change(&leaves)); + + // Anti-vacuity: same mode, SelfRef referent ⇒ false. + let mut leaves_self = TriggerDefinition::new(TriggerMode::LeavesBattlefield); + leaves_self.valid_card = Some(TargetFilter::SelfRef); + assert!(!trigger_names_referent_zone_change(&leaves_self)); + + // The `stolen uniform` shape — the slot anaphor is recognized in step 1 + // and `ChangesController` is not allowlisted in step 2. + let mut stolen = TriggerDefinition::new(TriggerMode::ChangesController); + stolen.valid_card = Some(TargetFilter::ParentTargetSlot { index: 1 }); + assert!(trigger_names_referent_zone_change(&stolen)); + + // THE SAFE DEFAULT, asserted explicitly rather than left implicit: an + // unrecognized mode naming the referent WITHHOLDS the pin. That is a + // deliberate trade — `mode_provably_leaves_referent_in_place` uses a + // closed allowlist with `_ => false`, so a mode nobody has classified + // fails safe. If a future card needs its mode pinned, extend the + // allowlist AND this row together. + let mut unclassified = TriggerDefinition::new(TriggerMode::Cycled); + unclassified.valid_card = Some(TargetFilter::ParentTarget); + assert!(trigger_names_referent_zone_change(&unclassified)); + } + + // ================================================================ T-U6 + /// T-U6 — the slot renumbering carve-out, demonstrated rather than + /// asserted. + /// + /// Assertion (3) is the failure mode §5.5(b)'s carve-out prevents: handing + /// a pin-FILTERED list to `effect_object_targets`, whose + /// `ParentTargetSlot { index }` arm indexes POSITIONALLY, silently + /// renumbers the slots. Delete the `matches!(filter, ParentTargetSlot{..})` + /// carve-out from the guarded handlers and assertion (3)'s behavior becomes + /// the shipped path. + /// + /// The real-card population for a pinned slot filter is **0 today** — + /// `stolen uniform` is denied a pin by §5.3(b), which T-U3's + /// `ChangesController` row asserts. The carve-out exists so that a FUTURE + /// pinned slot card cannot be silently renumbered by this plan's own + /// substitution. Non-vacuous by construction: it needs no card, no + /// pin-stamping path and no `ChangesController` fixture. + #[test] + fn t_u6_slot_filter_must_not_be_handed_a_pin_filtered_list() { + use crate::game::effects::effect_object_targets; + + let mut state = GameState::new_two_player(42); + let a = crate::game::zones::create_object( + &mut state, + CardId(1), + PlayerId(0), + "A".to_string(), + crate::types::zones::Zone::Battlefield, + ); + let b = crate::game::zones::create_object( + &mut state, + CardId(2), + PlayerId(0), + "B".to_string(), + crate::types::zones::Zone::Battlefield, + ); + + let mut ability = ResolvedAbility::new( + Effect::unimplemented("t_u6_slot_carve_out", "unit fixture"), + vec![TargetRef::Object(a), TargetRef::Object(b)], + ObjectId(999), + PlayerId(0), + ); + // A is pinned at a STALE epoch, B at its live one. + ability.target_incarnations = vec![ + crate::types::identifiers::ObjectIncarnationRef { + object_id: a, + incarnation: state.objects[&a].incarnation + 1, + }, + crate::types::identifiers::ObjectIncarnationRef { + object_id: b, + incarnation: state.objects[&b].incarnation, + }, + ]; + + let slot1 = TargetFilter::ParentTargetSlot { index: 1 }; + + // (1) The declared slot resolves correctly from the RAW list. + assert_eq!( + effect_object_targets(&slot1, &ability.targets), + vec![b], + "slot 1 of the raw list is B" + ); + + // (2) The pin filter drops the stale referent. + assert_eq!( + ability.live_object_targets(&state), + vec![TargetRef::Object(b)], + "A is stale and must be dropped" + ); + + // (3) THE DEFECT: the filtered list has only one element, so slot 1 no + // longer exists — the live referent B has been renumbered out of + // existence. This is why the carve-out passes the RAW list for + // `ParentTargetSlot` shapes. + assert_eq!( + effect_object_targets(&slot1, &ability.live_object_targets(&state)), + Vec::::new(), + "filtering BEFORE a positional index silently renumbers the slots — \ + the carve-out exists to prevent exactly this" + ); + } } diff --git a/crates/engine/src/game/effects/destroy.rs b/crates/engine/src/game/effects/destroy.rs index 6781e4f5f3..dd8b1a09a2 100644 --- a/crates/engine/src/game/effects/destroy.rs +++ b/crates/engine/src/game/effects/destroy.rs @@ -214,7 +214,14 @@ pub fn resolve( DestroyOutcome::NeedsChoice => return Ok(()), } } - for target in &ability.targets { + // CR 400.7 + CR 603.7c: a delayed destroy's pinned referent that became a + // new object is dropped. The SelfRef fallback above still reads the RAW + // `ability.targets`, so dropping every element here cannot re-bind the + // destroy to the source, and there is no pool fallback below this loop — + // substitution alone is sufficient and no early return is needed. + // Bound before the loop: `destroy_single_object` takes `&mut GameState`. + let live_targets = ability.live_object_targets(state); + for target in &live_targets { if let TargetRef::Object(obj_id) = target { match destroy_single_object(state, *obj_id, ability.source_id, cant_regenerate, events) { diff --git a/crates/engine/src/game/effects/discard.rs b/crates/engine/src/game/effects/discard.rs index 4a29e0393f..89dfabcb77 100644 --- a/crates/engine/src/game/effects/discard.rs +++ b/crates/engine/src/game/effects/discard.rs @@ -204,6 +204,29 @@ pub fn resolve( _ => (1, false, None, TargetFilter::Any, false), }; + // CR 400.7 + CR 603.7c + CR 603.7b: a delayed discard whose pinned referent + // became a new object discards nothing, and the trigger still resolves. + // + // EARLY RETURN IS MANDATORY HERE — substitution alone would be a live bug, + // not a redundancy. Emptying `specific_targets` below falls through the + // `!specific_targets.is_empty()` gate into the GENERIC hand-choice/random + // path, which picks some OTHER card out of the player's hand. That is a + // fallback re-binding the effect to a different object, so the decision rule + // requires the guard rather than the substitution. + // + // Deliberately placed above the `specific_targets` computation so it fires + // before either gate is evaluated. `EffectKind::from(&ability.effect)` + // (not a literal) because this resolver serves BOTH `DiscardCard` and + // `Discard`, which the Tier C census counts as distinct effect types. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::from(&ability.effect), + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // Check if targets specify specific cards to discard. Parent chain // propagation can inherit non-hand object targets (e.g. Traumatic Critique's // damage recipient) — those must not short-circuit the hand-choice path. @@ -213,8 +236,12 @@ pub fn resolve( // discard targets. Once the bounce moves them to hand they must not bypass the // interactive DiscardChoice path via this fast path — only a *declared* targeted // discard (Oracle uses "target") may consume `ability.targets` here. - let specific_targets: Vec<_> = ability - .targets + // Partially-stale case: the all-stale case already returned above, so this + // substitution only ever drops individual dead referents from a list that + // still has at least one live member — it cannot empty the list and so + // cannot reach the generic-path fallback the guard above protects. + let live_targets = ability.live_object_targets(state); + let specific_targets: Vec<_> = live_targets .iter() .filter_map(|t| { let TargetRef::Object(obj_id) = t else { diff --git a/crates/engine/src/game/effects/double.rs b/crates/engine/src/game/effects/double.rs index 54c941b695..cca9045288 100644 --- a/crates/engine/src/game/effects/double.rs +++ b/crates/engine/src/game/effects/double.rs @@ -331,6 +331,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets, kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/effect.rs b/crates/engine/src/game/effects/effect.rs index 7b21253b0d..d51b3fd282 100644 --- a/crates/engine/src/game/effects/effect.rs +++ b/crates/engine/src/game/effects/effect.rs @@ -523,6 +523,7 @@ fn register_transient_effect( .is_some_and(crate::game::ability_utils::filter_references_target_player) && matches!(ability.targets.first(), Some(TargetRef::Player(_))); for bound_filter in transient_bound_filters( + state, ability, application_filter, skip_companion_player_target, @@ -728,24 +729,55 @@ fn register_transient_effect( } } +// CR 400.7 + CR 603.7c: a delayed `GenericEffect` whose pinned referent became a +// new object must not bind a transient continuous effect to that new object. +// This is the `okoye, mighty and adored` / `garruk, curse breaker` / +// `rediscover the way` shape — an untyped `StaticDefinition` carrying +// `affected: ParentTarget` under a `GenericEffect` root. +// +// The substitution lives HERE rather than at the caller's +// `!ability.targets.is_empty()` gate, and that split is deliberate. The gate +// asks "were targets declared?"; substituting it would make an all-stale +// ability fall through to the BROADCAST filter path, which installs the +// continuous effect against a battlefield-wide population — a fallback binding +// different objects. Keeping the gate raw and filtering here instead means an +// all-stale ability still enters the targeted branch and produces ZERO bound +// filters, so `install_transient` is never called. That is already a clean, +// event-preserving no-op, so no early return is required. fn transient_bound_filters( + state: &GameState, ability: &ResolvedAbility, resolved_filter: Option<&TargetFilter>, skip_companion_player_target: bool, inherited_object_target: bool, ) -> Vec { + let live_targets = ability.live_object_targets(state); + if inherited_object_target { let Some(filter) = resolved_filter else { return Vec::new(); }; - return crate::game::effects::effect_object_targets(filter, &ability.targets) + // Slot carve-out (§5.4b): this hands its list straight to + // `effect_object_targets`, which indexes `ParentTargetSlot` + // POSITIONALLY. A pin-filtered list would renumber the slots, so the + // raw list is passed for that shape only. + let pool: &[TargetRef] = if matches!(filter, TargetFilter::ParentTargetSlot { .. }) { + &ability.targets + } else { + &live_targets + }; + return crate::game::effects::effect_object_targets(filter, pool) .into_iter() .map(|id| TargetFilter::SpecificObject { id }) .collect(); } - ability - .targets + // The `skip` is positional (it drops a companion player slot), but it skips + // from the FRONT of a list whose leading element is a player ref, and + // `live_object_targets` passes every `TargetRef::Player` through unfiltered. + // The skipped position therefore cannot be removed by the filter, so + // filtering before skipping is safe here. + live_targets .iter() .skip(usize::from(skip_companion_player_target)) .map(|target| match target { diff --git a/crates/engine/src/game/effects/extra_turn.rs b/crates/engine/src/game/effects/extra_turn.rs index a871aa0f48..179fa1aa90 100644 --- a/crates/engine/src/game/effects/extra_turn.rs +++ b/crates/engine/src/game/effects/extra_turn.rs @@ -76,6 +76,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/flip_permanent.rs b/crates/engine/src/game/effects/flip_permanent.rs index 0678fdd2fa..11241f497a 100644 --- a/crates/engine/src/game/effects/flip_permanent.rs +++ b/crates/engine/src/game/effects/flip_permanent.rs @@ -24,6 +24,31 @@ pub fn resolve( } } + // CR 400.7 + CR 603.7c: a delayed flip whose pinned referent became a new + // object flips nothing. PLACEMENT IS LOAD-BEARING — this MUST sit ABOVE the + // `as_slice()` match below, and the match itself is deliberately left + // reading the RAW `ability.targets`. + // + // Substituting `live_object_targets` into that match would be actively + // WRONG rather than merely redundant: a filtered-to-empty list matches the + // `[]` arm, which binds `object_id` to `ability.source_id` — a SOURCE + // FALLBACK that flips the ability's own source instead of doing nothing. + // The two states must stay distinguishable: `[]` means "no target was ever + // declared" (the printed "flip this creature" shape), while an all-stale + // pin means "a referent was declared and it is gone". + // + // After this guard, any surviving single target is by definition live, so a + // substitution below would be a no-op. Early return alone is the complete + // shape here. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::FlipPermanent, + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 710.1: if the named permanent isn't represented by a flip card, or // isn't on the battlefield, `flip_permanent` no-ops (CR 710.2). let object_id = match ability.targets.as_slice() { diff --git a/crates/engine/src/game/effects/force_block.rs b/crates/engine/src/game/effects/force_block.rs index d26bce05a9..4ff6b3472b 100644 --- a/crates/engine/src/game/effects/force_block.rs +++ b/crates/engine/src/game/effects/force_block.rs @@ -53,15 +53,10 @@ pub fn resolve( // object cannot be rediscovered from its raw id. Some(attacker) if state.combat.as_ref().is_some_and(|combat| { - combat.attackers.iter().any(|info| { - info.object_id == attacker.object_id - && state - .objects - .get(&attacker.object_id) - .is_some_and(|object| { - ObjectIncarnationRef::from_object(object) == attacker - }) - }) + combat + .attackers + .iter() + .any(|info| info.object_id == attacker.object_id && attacker.is_current(state)) }) => { StaticMode::MustBlockAttacker { attacker } diff --git a/crates/engine/src/game/effects/gain_control.rs b/crates/engine/src/game/effects/gain_control.rs index 15340d7d80..a06bb59790 100644 --- a/crates/engine/src/game/effects/gain_control.rs +++ b/crates/engine/src/game/effects/gain_control.rs @@ -205,7 +205,24 @@ fn gain_control_object_targets( } } - let chosen_objects = super::effect_object_targets(filter, &ability.targets); + // CR 400.7 + CR 603.7c: a delayed gain-control whose pinned referent became + // a new object controls nothing. This read is RAW and returns below before + // `resolved_targets` — the chokepoint the targeting guard covers — is ever + // reached, so the substitution MUST happen here or the pin is never checked + // at all. A delayed ParentTarget trigger's `targets` are non-empty by + // construction, so the early return below always fires for it. + // + // No early return is needed, and that is verified rather than assumed: if + // `chosen_objects` empties, control falls to `resolved_targets` (which also + // yields empty), `resolve` then iterates an empty list, skips the loop body, + // and falls to its UNCONDITIONAL `EffectResolved` push. An emptied list is + // already a clean no-op with the event. + // + // 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); if !chosen_objects.is_empty() { return chosen_objects; @@ -305,7 +322,27 @@ fn give_control_object_targets( return vec![ability.source_id]; } - let chosen_objects = super::effect_object_targets(filter, &ability.targets); + // CR 400.7 + CR 603.7c: identical shape to `gain_control_object_targets` + // above — a RAW read that returns before the chokepoint. `GiveControl` is + // Tier C (1 pinned pair, `burning cinder fury of crimson chaos fire`, whose + // node carries BOTH an object `target` and a player `recipient`; + // `live_object_targets` passes `TargetRef::Player` through by construction, + // so the recipient is untouched). + // + // No early return needed, re-verified at `resolve_give` rather than copied: + // an emptied list skips the loop and reaches the unconditional + // `EffectResolved` push. + // + // Slot carve-out DOES apply here — unlike `gain_control_object_targets`, + // this function has no `ParentTargetSlot` pre-arm, so a slot filter can + // reach the positional indexer. Pass the raw list for that shape. + 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); if !chosen_objects.is_empty() { return chosen_objects; diff --git a/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs b/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs index a0deac3b11..0ef26f735a 100644 --- a/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs +++ b/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs @@ -94,6 +94,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Activated, sub_ability: None, diff --git a/crates/engine/src/game/effects/phase_out.rs b/crates/engine/src/game/effects/phase_out.rs index 2e80fa24fa..718205b85e 100644 --- a/crates/engine/src/game/effects/phase_out.rs +++ b/crates/engine/src/game/effects/phase_out.rs @@ -94,6 +94,29 @@ pub fn resolve_phase_in( phase_in_player(state, *pid, events); } + // CR 400.7 + CR 603.7c + CR 603.7b: the trigger fired and resolved; it + // affected nothing. ALTITUDE IS LOAD-BEARING — this guard cannot live in + // `collect_phase_in_targets`, which returns a bare `Vec` with no + // `events` in scope and, worse, converts an emptied target list into a + // CR 702.26b battlefield-wide sweep for phased-out permanents matching the + // filter. That is a POOL-SCAN FALLBACK binding different objects, so the + // guard is hoisted here, above the call that would perform it. + // + // Placed AFTER the player-phasing branch deliberately: + // `pinned_object_targets_all_stale` is scoped to object refs, and + // `live_object_targets` passes `TargetRef::Player` through by construction. + // Guarding above the player branch would suppress a legitimate player + // phase-in on an ability that carries both a player ref and a stale object + // ref. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::PhaseIn, + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 702.26b: Filter choke point normally excludes phased-out objects, so // we can't rely on the standard target expansion for phase-in. Instead, // enumerate state.battlefield directly and match the filter manually, @@ -168,8 +191,19 @@ fn collect_phase_in_targets( ability: &ResolvedAbility, target: &TargetFilter, ) -> Vec { + // CR 400.7 + CR 603.7c: drop pinned referents that became new objects. This + // is a RAW read — `resolve_phase_in` never calls `resolved_targets`, so the + // targeting chokepoint cannot see this pin. + // + // Substitution here handles the PARTIALLY-stale case (some referents live, + // some not). It deliberately does NOT handle the all-stale case, because + // emptying `from_targets` falls through to the CR 702.26b battlefield sweep + // below, which would phase in a DIFFERENT set of permanents. That case is + // caught by the `pinned_object_targets_all_stale` early return hoisted into + // `resolve_phase_in` — see the comment there for why the guard cannot live + // in this function. let from_targets: Vec = ability - .targets + .live_object_targets(state) .iter() .filter_map(|t| match t { TargetRef::Object(id) => Some(*id), diff --git a/crates/engine/src/game/effects/player_counter.rs b/crates/engine/src/game/effects/player_counter.rs index 07294a2528..79f2a35622 100644 --- a/crates/engine/src/game/effects/player_counter.rs +++ b/crates/engine/src/game/effects/player_counter.rs @@ -337,6 +337,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, @@ -532,6 +533,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/remove_from_combat.rs b/crates/engine/src/game/effects/remove_from_combat.rs index 61d3921c13..c52ec4a236 100644 --- a/crates/engine/src/game/effects/remove_from_combat.rs +++ b/crates/engine/src/game/effects/remove_from_combat.rs @@ -1,5 +1,7 @@ use crate::game::combat::CombatParticipation; -use crate::types::ability::{Effect, EffectError, EffectKind, ResolvedAbility, TargetFilter}; +use crate::types::ability::{ + Effect, EffectError, EffectKind, ResolvedAbility, TargetFilter, TargetRef, +}; use crate::types::events::GameEvent; use crate::types::game_state::GameState; use crate::types::identifiers::ObjectIncarnationRef; @@ -20,12 +22,48 @@ pub fn resolve( } => { vec![ability.source_id] } + // CR 400.7 + CR 603.7c: a delayed combat-removal whose pinned referent + // became a new object removes nothing. This read is RAW — the file + // makes no `resolved_targets` call, so the targeting chokepoint never + // sees this pin. + // + // Slot carve-out applies: the list is passed straight into + // `effect_object_targets`, which indexes `ParentTargetSlot` + // positionally. Population is 0 today (`melee`'s filter is a bare + // `ParentTarget`), but this is the standing 22-call-site constraint, + // not a card-specific judgement. Effect::RemoveFromCombat { target } => { - super::effect_object_targets(target, &ability.targets) + 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) } _ => return Ok(()), }; + // CR 400.7 + CR 603.7c + CR 603.7b: the trigger fired and resolved; it + // affected nothing. PLACEMENT IS LOAD-BEARING — this MUST sit ABOVE the + // source rebind below. Letting the substitution empty the list instead + // falls into `vec![ability.source_id]`, which re-binds the effect to the + // ability's OWN source instead of doing nothing. + // + // NOTE this file has no existing pushing early return to mirror — its only + // other early return (`_ => return Ok(())` above) deliberately pushes + // nothing. The shape mirrored here is `change_zone.rs` / `sacrifice.rs`. + // `EffectKind::RemoveFromCombat` (not `EffectKind::from(&ability.effect)`) + // matches this file's own convention at the unconditional push below. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::RemoveFromCombat, + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // If no explicit targets, apply to source (e.g., "remove it from combat" // where "it" refers to the ability source). let targets = if targets.is_empty() { diff --git a/crates/engine/src/game/effects/reverse_turn_order.rs b/crates/engine/src/game/effects/reverse_turn_order.rs index 199068496a..4b90202ebe 100644 --- a/crates/engine/src/game/effects/reverse_turn_order.rs +++ b/crates/engine/src/game/effects/reverse_turn_order.rs @@ -49,6 +49,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/sacrifice.rs b/crates/engine/src/game/effects/sacrifice.rs index e04ca4ee6e..a41b54845c 100644 --- a/crates/engine/src/game/effects/sacrifice.rs +++ b/crates/engine/src/game/effects/sacrifice.rs @@ -202,13 +202,38 @@ pub fn resolve( }; let count = resolve_quantity_with_targets(state, count_expr, ability).max(0) as usize; + // CR 400.7 + CR 603.7c: a delayed sacrifice whose pinned referent became a + // new object affects nothing. Return before the empty-pool fallback below, + // which resolves a player scope and would make the controller sacrifice a + // DIFFERENT permanent (`resolve_sacrifice_scope`, CR 701.17a). + // + // Emits EffectResolved first, matching the shipped CR 400.7 SelfRef guard + // above, which this guard is the direct extension of. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::from(&ability.effect), + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + + let live_targets = ability.live_object_targets(state); let targeted_objects = if matches!( sacrifice_controller_scope(filter), Some(ControllerRef::ParentTargetController) ) { Vec::new() } else { - crate::game::effects::effect_object_targets(filter, &ability.targets) + // CR 400.7 + CR 603.7c: `effect_object_targets` indexes ParentTargetSlot + // by DECLARED position, so a pin-filtered slice would renumber every + // later slot. Pass the raw list for that filter shape. + let pool: &[TargetRef] = if matches!(filter, TargetFilter::ParentTargetSlot { .. }) { + &ability.targets + } else { + &live_targets + }; + crate::game::effects::effect_object_targets(filter, pool) }; if targeted_objects.is_empty() { diff --git a/crates/engine/src/game/effects/skip_next_step.rs b/crates/engine/src/game/effects/skip_next_step.rs index 55a5469d16..57d063f26c 100644 --- a/crates/engine/src/game/effects/skip_next_step.rs +++ b/crates/engine/src/game/effects/skip_next_step.rs @@ -112,6 +112,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/skip_next_turn.rs b/crates/engine/src/game/effects/skip_next_turn.rs index 8192587e53..9fcb84f597 100644 --- a/crates/engine/src/game/effects/skip_next_turn.rs +++ b/crates/engine/src/game/effects/skip_next_turn.rs @@ -90,6 +90,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Spell, sub_ability: None, diff --git a/crates/engine/src/game/effects/tap_untap.rs b/crates/engine/src/game/effects/tap_untap.rs index b294c775aa..0f29559cff 100644 --- a/crates/engine/src/game/effects/tap_untap.rs +++ b/crates/engine/src/game/effects/tap_untap.rs @@ -54,8 +54,23 @@ fn tap_untap_target_ids( .get(id) .cloned() .unwrap_or_default(), + // CR 400.7 + CR 603.7c: a delayed tap/untap whose pinned referent became + // a new object taps nothing. This arm is a RAW read that never reaches + // `resolved_targets`, so the targeting chokepoint cannot see this pin. + // + // SUBSTITUTION-ONLY, and that is verified rather than assumed against + // the decision rule: there is no source fallback below this arm, an + // empty vector simply skips `resolve_set_tap_state`'s resolution loop, + // and control falls to that function's UNCONDITIONAL + // `EffectResolved` push (`:135-139`). An emptied list is already a + // clean no-op that emits the event, so no early return is needed. + // + // No slot carve-out applies: this arm enumerates every object ref via + // `filter_map` and never hands the list to `effect_object_targets`'s + // positional indexer, so a filtered list cannot renumber a + // `ParentTargetSlot`. _ => ability - .targets + .live_object_targets(state) .iter() .filter_map(|t| match t { TargetRef::Object(id) => Some(*id), diff --git a/crates/engine/src/game/effects/transform_effect.rs b/crates/engine/src/game/effects/transform_effect.rs index edf52e0a9d..6a5572cacf 100644 --- a/crates/engine/src/game/effects/transform_effect.rs +++ b/crates/engine/src/game/effects/transform_effect.rs @@ -36,6 +36,30 @@ pub fn resolve( } } + // CR 400.7 + CR 603.7c: a delayed transform whose pinned referent became a + // new object transforms nothing. Identical shape to `flip_permanent.rs`, and + // guarded the same way for the same reason: PLACEMENT ABOVE the `as_slice()` + // match is load-bearing, and the match keeps reading the RAW + // `ability.targets`. + // + // A `live_object_targets` substitution inside that match would REBIND rather + // than no-op — an emptied list takes the `[]` arm, which resolves to + // `ability.source_id` and would transform the ability's own source. "No + // target declared" (the printed self-transform shape) and "the declared + // referent went stale" must not collapse into the same arm. + // + // Scoped to `EffectScope::Single` by construction: the `All` branch returned + // above into `resolve_all`, which is a non-targeting battlefield sweep and + // carries no `ability.targets` referent to pin. + if ability.pinned_object_targets_all_stale(state) { + events.push(GameEvent::EffectResolved { + kind: EffectKind::Transform, + source_id: ability.source_id, + subject: None, + }); + return Ok(()); + } + // CR 701.27c: If a spell or ability instructs a player to transform a permanent // that isn't represented by a double-faced card, nothing happens. let object_id = match ability.targets.as_slice() { diff --git a/crates/engine/src/game/effects/vote.rs b/crates/engine/src/game/effects/vote.rs index afb2f26433..bb550b6d6e 100644 --- a/crates/engine/src/game/effects/vote.rs +++ b/crates/engine/src/game/effects/vote.rs @@ -356,6 +356,7 @@ pub fn resolve_tally( trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -424,6 +425,7 @@ pub fn resolve_tally( trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -666,6 +668,7 @@ fn resolved_from_def( trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -929,6 +932,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -1038,6 +1042,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -1472,6 +1477,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, @@ -1638,6 +1644,7 @@ mod tests { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), controller, original_controller: None, scoped_player: None, diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index b58d2c33d6..265f6d0195 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -5992,11 +5992,7 @@ pub(crate) fn transient_effect_is_live(state: &GameState, tce: &TransientContinu // CR 400.7: a recipient that has changed zones is a new object, so a // continuous effect tied to its prior incarnation cannot keep applying. if let Some(recipient) = tce.affected_recipient { - if !state - .objects - .get(&recipient.object_id) - .is_some_and(|object| ObjectIncarnationRef::from_object(object) == recipient) - { + if !recipient.is_current(state) { return false; } } diff --git a/crates/engine/src/game/resolution_prompt.rs b/crates/engine/src/game/resolution_prompt.rs index 64c03031f2..5ca32d7897 100644 --- a/crates/engine/src/game/resolution_prompt.rs +++ b/crates/engine/src/game/resolution_prompt.rs @@ -553,6 +553,7 @@ pub(crate) fn chain_offers_choice(a: &ResolvedAbility) -> bool { trigger_source: _, // exact triggered-source authority, no choice trigger_definition_ref: _, // exact trigger occurrence, no choice force_block_attacker: _, // exact force-block referent, no choice + target_incarnations: _, // CR 400.7 referent pins, no choice controller: _, // player id original_controller: _, // player id scoped_player: _, // player id (iteration binding) diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index 3d46b04937..c3db36cc4a 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -2994,6 +2994,7 @@ fn self_counter_ability_is_batch_candidate(ability: &ResolvedAbility) -> bool { trigger_source, trigger_definition_ref, force_block_attacker: _, + target_incarnations: _, // CR 400.7 referent pins; batch candidacy is shape-only controller: _, original_controller, scoped_player, @@ -3205,6 +3206,7 @@ fn fixed_controller_gain_life_ability_is_batch_candidate(ability: &ResolvedAbili trigger_source: _, trigger_definition_ref: _, force_block_attacker: _, + target_incarnations: _, // CR 400.7 referent pins; batch candidacy is shape-only controller: _, original_controller: _, scoped_player, @@ -3395,6 +3397,7 @@ fn fixed_opponent_lose_life_ability_is_batch_candidate(ability: &ResolvedAbility trigger_source: _, trigger_definition_ref: _, force_block_attacker: _, + target_incarnations: _, // CR 400.7 referent pins; batch candidacy is shape-only controller: _, original_controller: _, scoped_player, @@ -4033,6 +4036,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance( trigger_source: _, trigger_definition_ref: _, force_block_attacker: a_force_block_attacker, + target_incarnations: a_target_incarnations, controller: a_controller, original_controller: _, scoped_player: a_scoped_player, @@ -4087,6 +4091,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance( trigger_source: _, trigger_definition_ref: _, force_block_attacker: b_force_block_attacker, + target_incarnations: b_target_incarnations, controller: b_controller, original_controller: _, scoped_player: b_scoped_player, @@ -4137,6 +4142,11 @@ fn inert_trigger_abilities_eq_ignoring_provenance( a_effect == b_effect && a_targets == b_targets && a_force_block_attacker == b_force_block_attacker + // CR 400.7 + CR 603.7c: two otherwise-identical abilities pinned to + // DIFFERENT incarnations are not the same ability. Participating here + // keeps this manual comparison in agreement with the type's derived + // `PartialEq`; disagreeing with the derive would be the actual defect. + && a_target_incarnations == b_target_incarnations && a_controller == b_controller && a_scoped_player == b_scoped_player && a_kind == b_kind diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index 065bd044f0..9795cf91da 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -845,13 +845,27 @@ pub fn resolved_targets( // CR 608.2c: ParentTarget / ParentTargetSlot inherit propagated targets; // StackSpell uses player-chosen stack targets at ETB (issue #2351). // Slot indexing for ParentTargetSlot happens in `effect_object_targets`. + // + // CR 400.7 + CR 603.7c: a delayed ability's pinned referent that has since + // become a new object is dropped here — it "left that zone and then + // returned", so the ability won't affect it. Unpinned targets (every + // non-delayed ability, and every delayed trigger whose condition names a + // zone change of the referent) pass through unchanged. + // + // ORDERING IS LOAD-BEARING: at this line `ability.targets` is non-empty, so + // the `is_empty()` fallbacks above have ALREADY been passed and returning an + // empty vec here cannot re-bind the referent to `ability.source_id`. Do not + // hoist this guard above them. The `matches!` admits only + // `ParentTarget | StackSpell`, never `ParentTargetSlot` (that is the + // separate branch below), so the returned vector is never consumed + // positionally from here and the slot renumbering hazard does not arise. if !ability.targets.is_empty() && matches!( target_filter, TargetFilter::ParentTarget | TargetFilter::StackSpell ) { - return ability.targets.clone(); + return ability.live_object_targets(state); } // CR 608.2c: ParentTargetSlot needs the accumulated targets from the entire // chain, not just the current ability's targets. During normal resolution @@ -1039,7 +1053,30 @@ pub(crate) fn resolved_object_ids_for_filter_with_context( .then_some(ability.source_id) .into_iter() .collect(), - TargetFilter::ParentTarget => object_targets(&ability.targets).collect(), + // CR 400.7 + CR 603.7c: mirror the `resolved_targets` pin check on the + // untargeted-pool path (the second SelfRef chokepoint). + TargetFilter::ParentTarget => object_targets(&ability.live_object_targets(state)).collect(), + // CR 400.7 + CR 603.7c: `ParentTargetSlot` is deliberately NOT + // pin-filtered. Slot numbering is declared, not live: + // `effects::effect_object_targets` indexes `ParentTargetSlot { index }` + // straight into whatever slice it is handed (the single slot-indexing + // authority, 22 call sites), so dropping a stale element anywhere + // upstream would renumber every later slot. + // + // No slot pin-check exists anywhere in the engine, and none is needed + // today: the only delayed-trigger card carrying a `ParentTargetSlot` + // (`stolen uniform`, `WhenNextEvent { ChangesController, valid_card: + // ParentTargetSlot }`) is denied a pin by + // `condition_names_referent_zone_change` — `ChangesController` is not on + // `mode_provably_leaves_referent_in_place`'s allowlist — so + // `target_pin_is_current` is vacuously true for every slot id in + // practice. + // + // THE STANDING CONSTRAINT FOR ALL 22 CALL SITES: never hand + // `effect_object_targets` a pin-filtered slice when the filter may be + // `ParentTargetSlot`. `sacrifice.rs` is the one guarded read that can + // see one, and it passes the raw `ability.targets` for exactly that + // reason. TargetFilter::ParentTargetSlot { index } => { resolve_parent_slot_from_root(state, ability, *index) .and_then(|target| target_ref_object(&target)) diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 15811b55ad..d1bc6cc98a 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -23261,6 +23261,25 @@ pub struct ResolvedAbility { /// deliberately separate from the parsed grammatical selector on `Effect`. #[serde(default, skip_serializing_if = "Option::is_none")] pub force_block_attacker: Option, + /// CR 400.7 + CR 603.7c: Incarnation pins for the object referents in + /// `targets`, captured when a delayed triggered ability snapshotted its + /// `ParentTarget` referent at creation. A delayed ability that refers to a + /// particular object must not affect a later object that merely reuses the + /// same storage `ObjectId` — CR 603.7c: "if that object left that zone and + /// then returned, it's a new object and thus won't be affected." + /// + /// Empty for every ability that is not a pinned `ParentTarget` delayed + /// trigger — including delayed triggers whose own condition names a ZONE + /// CHANGE OF THE REFERENT, in either direction (CR 400.7e; see + /// `condition_names_referent_zone_change`), where the referent is EXPECTED + /// to have moved and must still be affected. An id with no pin here + /// resolves unchanged, so the guard is inert outside the pinned path. + /// + /// Element-level pins are deliberately non-`Option` so an individual + /// referent can never degrade to "always matches" (contrast + /// `source_incarnation`'s `is_none_or` fail-open at `source_is_current`). + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub target_incarnations: Vec, pub controller: PlayerId, /// CR 109.5: The controller of the spell or ability before any /// resolution-time player-scope iteration rebinds the acting player. @@ -23571,6 +23590,7 @@ impl ResolvedAbility { trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), modal: None, mode_abilities: Vec::new(), parent_target_missing_reason: None, @@ -23717,6 +23737,12 @@ impl ResolvedAbility { self.trigger_source = None; self.trigger_definition_ref = None; self.force_block_attacker = None; + // CR 104.4b + CR 400.7: `normalize_for_loop` compares canonicalized + // clones for repeated-position equality, and the all-zone incarnation + // bump advances a pinned referent's epoch on every zone change. A + // pinned delayed trigger riding on a zone-cycling loop would otherwise + // carry a growing epoch into loop equality and never confirm a draw. + self.target_incarnations.clear(); if let Some(sub) = self.sub_ability.as_mut() { sub.clear_trigger_identity_recursive(); } @@ -23748,6 +23774,88 @@ impl ResolvedAbility { } } + /// CR 400.7 + CR 603.7c: Pin this ability's object referents — and every + /// continuation branch's — to the incarnations observed at delayed-trigger + /// creation. Recursive because chain propagation copies `targets` into + /// sub-abilities at fire time, so each link must carry the same pins. + pub fn set_target_incarnations_recursive(&mut self, pins: Vec) { + self.target_incarnations = pins.clone(); + if let Some(sub) = self.sub_ability.as_mut() { + sub.set_target_incarnations_recursive(pins.clone()); + } + if let Some(else_branch) = self.else_ability.as_mut() { + else_branch.set_target_incarnations_recursive(pins); + } + } + + /// CR 400.7 + CR 603.7c: True when `id` may still be affected by this + /// ability. + /// + /// KEYED, not positional: the same predicate applies to an id drawn from + /// `self.targets` or from any handler's local projection — which is why the + /// handler-direct reads share one authority. It also means a wholesale + /// `targets` overwrite (`triggers.rs`, the Stationed/VehicleCrewed/Saddled + /// reseed) degrades safely: the new id has no pin and passes through. + /// + /// FAIL-OPEN AT THE LOOKUP, NEVER AT THE PIN: `is_none_or` here means "no + /// pin was ever recorded for this id, so this is not a pinned + /// delayed-trigger referent and it passes through unchanged". Once a pin + /// exists it is compared by strict equality with no escape hatch. Do not + /// convert this to an `Option` on the element — that reintroduces the + /// `source_is_current` anti-pattern. + pub fn target_pin_is_current( + &self, + id: ObjectId, + state: &crate::types::game_state::GameState, + ) -> bool { + self.target_incarnations + .iter() + .find(|pin| pin.object_id == id) + .is_none_or(|pin| pin.is_current(state)) + } + + /// CR 603.7c + CR 400.7: The subset of `targets` this ability may still + /// affect. An object target whose creation-time pin no longer matches the + /// live object is dropped: it left its zone (and possibly returned), so it + /// is a new object. Player targets and unpinned object targets pass through. + /// + /// POSITIONAL CONSUMERS MUST NOT USE THIS. `effects::effect_object_targets` + /// indexes `ParentTargetSlot { index }` into whatever slice it is handed, + /// so handing it a filtered slice would RENUMBER the declared slots. + /// Callers that may see a `ParentTargetSlot` filter pass the raw + /// `&self.targets` instead — see `sacrifice.rs`. + pub fn live_object_targets( + &self, + state: &crate::types::game_state::GameState, + ) -> Vec { + self.targets + .iter() + .filter(|target| match target { + TargetRef::Object(id) => self.target_pin_is_current(*id, state), + TargetRef::Player(_) => true, + }) + .cloned() + .collect() + } + + /// CR 603.7c: True when this ability pinned at least one object referent and + /// every one of them has gone stale. Distinguishes "the referent became a + /// new object" (the effect must do nothing) from "no referent was ever + /// chosen" (which legitimately falls back elsewhere). + pub fn pinned_object_targets_all_stale( + &self, + state: &crate::types::game_state::GameState, + ) -> bool { + !self.target_incarnations.is_empty() + && self + .targets + .iter() + .any(|t| matches!(t, TargetRef::Object(_))) + && !self.targets.iter().any( + |t| matches!(t, TargetRef::Object(id) if self.target_pin_is_current(*id, state)), + ) + } + /// Test-only fixture helper for a triggered ability whose source has /// already left the object map. Production code must capture a real /// [`TriggerSourceContext`] at collection time instead. diff --git a/crates/engine/src/types/identifiers.rs b/crates/engine/src/types/identifiers.rs index 0367dc556e..20debeb14b 100644 --- a/crates/engine/src/types/identifiers.rs +++ b/crates/engine/src/types/identifiers.rs @@ -166,6 +166,18 @@ impl ObjectIncarnationRef { incarnation: obj.incarnation, } } + + /// CR 400.7: True when this pinned reference still names the live object it + /// was captured from. An object that changed zones became a new object and + /// bumped its incarnation (`GameObject::bump_incarnation`), so a stale pin + /// matches nothing even though the engine reuses the `ObjectId` as storage + /// identity. + pub fn is_current(&self, state: &crate::types::game_state::GameState) -> bool { + state + .objects + .get(&self.object_id) + .is_some_and(|object| Self::from_object(object) == *self) + } } /// Private serde shim mirroring `PhaseStopCompat` (`types/phase.rs`): new writes diff --git a/crates/engine/tests/integration/delayed_parent_target_incarnation.rs b/crates/engine/tests/integration/delayed_parent_target_incarnation.rs new file mode 100644 index 0000000000..047d198c80 --- /dev/null +++ b/crates/engine/tests/integration/delayed_parent_target_incarnation.rs @@ -0,0 +1,833 @@ +//! CR 400.7 / CR 603.7c — a delayed triggered ability's `ParentTarget` referent +//! is pinned to the incarnation it had when the trigger was created. +//! +//! CR 603.7c: "A delayed triggered ability that refers to a particular object +//! still affects it even if the object changes characteristics. However, 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 driver bug: Goryo's Vengeance reanimates a creature and schedules +//! "Exile it at the beginning of the next end step". Blinking that creature with +//! Ephemerate makes it a NEW object (CR 400.7) that merely reuses the same +//! storage `ObjectId`, so the delayed trigger must no longer affect it. +//! +//! Controls in this file assert the OTHER direction of CR 603.7c's operative +//! test: a delayed trigger whose own condition IS the referent's zone change +//! (Saffi Eriksdotter's "when that creature dies", Lagrella's "when an exiled +//! card enters") expects the referent to have moved, and must keep working. + +use engine::game::scenario::{GameScenario, P0, P1}; +use engine::types::actions::GameAction; +use engine::types::events::GameEvent; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::mana::{ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +// ---------------------------------------------------------------- Oracle text +// Verbatim Oracle text (Scryfall). A paraphrase can take a different parser +// branch and go green while the real card stays broken. + +const GORYOS_VENGEANCE: &str = "Return target legendary creature card from your graveyard to the battlefield. That creature gains haste. Exile it at the beginning of the next end step."; + +const EPHEMERATE: &str = + "Exile target creature you control, then return it to the battlefield under its owner's control."; + +const SAFFI_ERIKSDOTTER: &str = "Sacrifice Saffi Eriksdotter: When target creature is put into your graveyard this turn, return that card to the battlefield."; + +/// A plain removal spell used to move a referent to the graveyard through the +/// real cast pipeline rather than by mutating state behind the engine's back. +const DESTROY_TARGET_CREATURE: &str = "Destroy target creature."; + +// -------------------------------------------------------------------- helpers + +fn mana(n: usize) -> Vec { + (0..n) + .map(|_| ManaUnit::new(ManaType::Black, ObjectId(0), false, vec![])) + .collect() +} + +/// Coloured mana for cards whose costs are not generic-payable from `mana()`'s +/// black pool — Whippoorwill's `{G}{G}` activation and Ephemerate's `{W}`. +fn mana_of(kind: ManaType, n: usize) -> Vec { + (0..n) + .map(|_| ManaUnit::new(kind, ObjectId(0), false, vec![])) + .collect() +} + +/// Give both players a library so crossing a turn boundary does not end the +/// game by decking (CR 704.5b). +/// +/// Required by any test whose delayed trigger resolves on a LATER turn — T-Z5's +/// `AtNextPhaseForPlayer { Upkeep }` crosses two draw steps. Without it the +/// advance loop dies with `GameOver`, which would be a harness artifact +/// masquerading as a result. +fn stock_libraries(scenario: &mut GameScenario) { + for i in 0..40 { + scenario.add_card_to_library_top(P0, &format!("Filler {i}")); + scenario.add_card_to_library_top(P1, &format!("Filler {i}")); + } +} + +/// Advance until every delayed trigger has fired and the stack is empty, +/// returning every event the engine emitted along the way. +/// +/// Adapted from `issue_2424_goryos_vengeance.rs:122-154` (declare-attackers / +/// declare-blockers / pass-priority with a 256-iteration guard). Kept local so +/// the existing Goryo's file is not modified. +fn advance_until_delayed_triggers_resolve( + runner: &mut engine::game::scenario::GameRunner, +) -> Vec { + let mut events = Vec::new(); + let mut guard = 0; + while !runner.state().delayed_triggers.is_empty() || !runner.state().stack.is_empty() { + guard += 1; + assert!( + guard < 256, + "delayed trigger never resolved; phase = {:?}, waiting_for = {:?}, dt = {}, stack = {}", + runner.state().phase, + runner.state().waiting_for, + runner.state().delayed_triggers.len(), + runner.state().stack.len(), + ); + let action = match &runner.state().waiting_for { + WaitingFor::DeclareAttackers { .. } => GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }, + WaitingFor::DeclareBlockers { .. } => GameAction::DeclareBlockers { + assignments: vec![], + }, + // CR 514.1: a delayed trigger that resolves on a LATER turn (T-Z5's + // `AtNextPhaseForPlayer { Upkeep }`) makes the loop cross a cleanup + // step, where a stocked library has pushed the player over seven + // cards. Discarding down is required to keep advancing; a + // `PassPriority` here is rejected outright. + WaitingFor::DiscardToHandSize { count, cards, .. } => GameAction::SelectCards { + cards: cards.iter().take(*count).copied().collect(), + }, + _ => GameAction::PassPriority, + }; + match runner.act(action) { + Ok(result) => events.extend(result.events), + Err(e) => panic!( + "advancing to the delayed trigger failed: {e:?} (waiting_for = {:?})", + runner.state().waiting_for + ), + } + } + events +} + +/// Advance to the end of the current turn, regardless of whether any delayed +/// trigger fires. +/// +/// `advance_until_delayed_triggers_resolve` drains the delayed-trigger list and +/// is the right tool when a trigger is EXPECTED to fire. It is the wrong tool +/// for a negative arm whose trigger legitimately never fires — a "this turn" +/// `WhenDies` trigger on a creature that does not die stays installed until +/// cleanup, so draining would spin until the iteration guard trips and report a +/// harness stall as a result. +fn advance_past_end_of_turn(runner: &mut engine::game::scenario::GameRunner) { + let start_turn = runner.state().turn_number; + let mut guard = 0; + while runner.state().turn_number == start_turn { + guard += 1; + assert!( + guard < 256, + "turn never ended; phase = {:?}, waiting_for = {:?}", + runner.state().phase, + runner.state().waiting_for, + ); + let action = match &runner.state().waiting_for { + WaitingFor::DeclareAttackers { .. } => GameAction::DeclareAttackers { + attacks: vec![], + bands: vec![], + }, + WaitingFor::DeclareBlockers { .. } => GameAction::DeclareBlockers { + assignments: vec![], + }, + WaitingFor::DiscardToHandSize { count, cards, .. } => GameAction::SelectCards { + cards: cards.iter().take(*count).copied().collect(), + }, + _ => GameAction::PassPriority, + }; + if runner.act(action).is_err() { + return; + } + } +} + +/// True when an `EffectResolved` event names this source — the observable proof +/// that a trigger DID fire and DID resolve (CR 603.7b), even when it affected +/// nothing. +fn effect_resolved_from(events: &[GameEvent], source: ObjectId) -> bool { + events.iter().any(|e| { + matches!( + e, + GameEvent::EffectResolved { source_id, .. } if *source_id == source + ) + }) +} + +// ============================================================ T-a (control) + +/// T-a — ANTI-VACUITY CONTROL. With no blink, Goryo's delayed trigger really +/// does exile the reanimated creature at the next end step. +/// +/// This must fail if the "fix" merely disables the delayed trigger, which is +/// why it is a control rather than a nice-to-have. +#[test] +fn t_a_goryos_exiles_reanimated_creature_at_end_step() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(8)); + + let legendary = scenario + .add_creature_to_graveyard(P0, "Legendary Bear", 4, 4) + .as_legendary() + .id(); + let goryos = scenario + .add_spell_to_hand_from_oracle(P0, "Goryo's Vengeance", true, GORYOS_VENGEANCE) + .id(); + + let mut runner = scenario.build(); + + let outcome = runner.cast(goryos).target_object(legendary).resolve(); + // Positive reach-guard: the reanimation actually happened, so the end-step + // assertion below is about the delayed trigger and not about a fizzled cast. + assert_eq!( + outcome.zone_of(legendary), + Zone::Battlefield, + "reach-guard: Goryo's must return the creature to the battlefield" + ); + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: Goryo's must install exactly one delayed exile trigger" + ); + + advance_until_delayed_triggers_resolve(&mut runner); + + assert_eq!( + runner.state().objects[&legendary].zone, + Zone::Exile, + "T-a: with no blink the delayed trigger must exile the creature" + ); +} + +// ============================================================ T-b (the bug) + +/// T-b — THE BUG. Blinking the reanimated creature with Ephemerate makes it a +/// new object (CR 400.7); the delayed trigger must no longer affect it. +/// +/// Two independent halves: +/// 1. the creature ends on the battlefield (the CR 603.7c note), and +/// 2. the trigger STILL fired and resolved as a no-op (CR 603.7b) — evidenced +/// by an `EffectResolved` naming Goryo's. +/// +/// Half 2 is the direct test of the early-return event rule: it goes red if the +/// guard returns without pushing `EffectResolved`, while half 1 stays green. +#[test] +fn t_b_blinked_referent_is_not_exiled_but_trigger_still_resolves() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(8)); + + let legendary = scenario + .add_creature_to_graveyard(P0, "Legendary Bear", 4, 4) + .as_legendary() + .id(); + let goryos = scenario + .add_spell_to_hand_from_oracle(P0, "Goryo's Vengeance", true, GORYOS_VENGEANCE) + .id(); + let ephemerate = scenario + .add_spell_to_hand_from_oracle(P0, "Ephemerate", true, EPHEMERATE) + .id(); + + let mut runner = scenario.build(); + + let reanimated = runner.cast(goryos).target_object(legendary).resolve(); + assert_eq!( + reanimated.zone_of(legendary), + Zone::Battlefield, + "reach-guard: Goryo's must return the creature before it can be blinked" + ); + let goryos_source = goryos; + + let blinked = runner.cast(ephemerate).target_object(legendary).resolve(); + // Reach-guard: the blink really happened. Without this, "still on the + // battlefield" at the end step could pass on a creature that never moved. + assert_eq!( + blinked.zone_of(legendary), + Zone::Battlefield, + "reach-guard: Ephemerate must return the creature to the battlefield" + ); + assert!( + blinked.events().iter().any(|e| matches!( + e, + GameEvent::ZoneChanged { object_id, to: Zone::Exile, .. } if *object_id == legendary + )), + "reach-guard: Ephemerate must actually exile the creature (blink leg 1)" + ); + + let events = advance_until_delayed_triggers_resolve(&mut runner); + + // Half 1 — CR 603.7c: it left the zone and returned, so it is a new object. + assert_eq!( + runner.state().objects[&legendary].zone, + Zone::Battlefield, + "T-b: a blinked referent is a NEW object and must not be exiled by the \ + delayed trigger (CR 400.7 / CR 603.7c)" + ); + + // Half 2 — CR 603.7b: the trigger still fired and still resolved. + assert!( + effect_resolved_from(&events, goryos_source), + "T-b: the delayed trigger must still fire and resolve as a no-op, \ + emitting EffectResolved (CR 603.7b)" + ); +} + +// ============================================================ T-c (ruling) + +/// T-c — the official ruling's literal case: "If the returned creature leaves +/// the battlefield before the end step, it will remain in its current zone. It +/// won't be exiled." +/// +/// The referent left and did NOT return, so it must stay in the graveyard — +/// explicitly distinguished from `Zone::Exile`. +#[test] +fn t_c_referent_that_left_and_did_not_return_stays_put() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(12)); + + let legendary = scenario + .add_creature_to_graveyard(P0, "Legendary Bear", 4, 4) + .as_legendary() + .id(); + let goryos = scenario + .add_spell_to_hand_from_oracle(P0, "Goryo's Vengeance", true, GORYOS_VENGEANCE) + .id(); + let removal = scenario + .add_spell_to_hand_from_oracle(P0, "Murder", true, DESTROY_TARGET_CREATURE) + .id(); + + let mut runner = scenario.build(); + + let reanimated = runner.cast(goryos).target_object(legendary).resolve(); + assert_eq!( + reanimated.zone_of(legendary), + Zone::Battlefield, + "reach-guard: Goryo's must return the creature first" + ); + + let killed = runner.cast(removal).target_object(legendary).resolve(); + assert_eq!( + killed.zone_of(legendary), + Zone::Graveyard, + "reach-guard: the removal spell must put the creature in the graveyard" + ); + + let events = advance_until_delayed_triggers_resolve(&mut runner); + + let final_zone = runner.state().objects[&legendary].zone; + // The two zones are asserted SEPARATELY and explicitly. This was a live + // pre-fix red (the engine exiled the creature out of the graveyard, + // contradicting the official ruling), so the `Exile` case is named rather + // than merely implied: a mis-written single assertion could otherwise pass + // against the exact zone this test exists to rule out. + assert_ne!( + final_zone, + Zone::Exile, + "T-c: the delayed trigger must NOT exile a referent that left the \ + battlefield and did not return (CR 603.7c; official ruling: \"If the \ + returned creature leaves the battlefield before the end step, it will \ + remain in its current zone. It won't be exiled.\")" + ); + assert_eq!( + final_zone, + Zone::Graveyard, + "T-c: the referent left the battlefield and did not return, so it stays \ + in its current zone" + ); + // Positive reach-guard so the negatives above cannot pass vacuously on a + // trigger that never fired at all. + assert!( + effect_resolved_from(&events, goryos), + "T-c reach-guard: the delayed trigger must still have fired and resolved" + ); +} + +// ====================================================== T-Z1 (MUST STAY GREEN) + +/// T-Z1 — MUST-STAY-GREEN CONTROL (Saffi Eriksdotter). +/// +/// "Sacrifice Saffi Eriksdotter: When target creature is put into your graveyard +/// this turn, return that card to the battlefield." +/// +/// Saffi's delayed trigger's own CONDITION is the referent's zone change, so the +/// referent is EXPECTED to have moved (CR 603.7c operative test; CR 400.7e +/// affirmatively grants that such an ability can find the object in the zone it +/// moved to). Pinning it would make the card a permanent no-op. +/// +/// This must pass BOTH before and after the fix. A control that was never green +/// pre-fix proves nothing about regression. +#[test] +fn t_z1_saffi_eriksdotter_still_returns_the_creature() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(12)); + + let saffi = scenario + .add_creature_from_oracle(P0, "Saffi Eriksdotter", 2, 2, SAFFI_ERIKSDOTTER) + .id(); + let victim = scenario.add_creature(P0, "Doomed Bear", 2, 2).id(); + let removal = scenario + .add_spell_to_hand_from_oracle(P0, "Murder", true, DESTROY_TARGET_CREATURE) + .id(); + + let mut runner = scenario.build(); + + let activation = runner.activate(saffi, 0).target_object(victim).resolve(); + assert_eq!( + activation.zone_of(saffi), + Zone::Graveyard, + "reach-guard: activating Saffi sacrifices her" + ); + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: Saffi's activation must install exactly one delayed trigger" + ); + + let killed = runner.cast(removal).target_object(victim).resolve(); + // Reach-guard: the referent really is in the graveyard when the delayed + // trigger resolves, so "returned" below cannot pass vacuously. + assert!( + matches!(killed.zone_of(victim), Zone::Graveyard | Zone::Battlefield), + "reach-guard: the victim must have been put into the graveyard" + ); + + advance_until_delayed_triggers_resolve(&mut runner); + + assert_eq!( + runner.state().objects[&victim].zone, + Zone::Battlefield, + "T-Z1: Saffi's delayed trigger names the referent's OWN zone change, so \ + the referent is expected to have moved and must still be affected \ + (CR 603.7c operative test + CR 400.7e)" + ); +} + +// ====================================================== T-Z4 (MUST STAY GREEN) + +/// T-Z4 — MUST-STAY-GREEN CONTROL (Lagrella, the Magpie), the ENTRY direction. +/// +/// Every other control in this file is a departure case. This is the only test +/// that distinguishes the `WhenEntersBattlefield` arm: the referent is expected +/// to have moved ONTO the battlefield, and `zones.rs:816` bumps the incarnation +/// unconditionally on `to == Battlefield`, so pinning this condition would make +/// the card a permanent no-op at 100% of firings. +/// +/// It also exercises the `counters.rs` direct read and the tracked-set condition +/// erasure at the same time: Lagrella's CDT is `uses_tracked_set: true`, so its +/// condition is rewritten at `bind_tracked_set_to_condition` — which is exactly +/// why the expected-zone gate must read the PARSER-EMITTED condition. +/// +/// Must pass BOTH before and after the fix. +#[test] +fn t_z4_lagrella_still_places_counters_on_the_returned_card() { + // The card's own `oracle_text` as card-data stores it (what the engine + // actually parses), not a paraphrase. + const LAGRELLA: &str = "When Lagrella enters, exile any number of other target creatures controlled by different players until Lagrella leaves the battlefield. When an exiled card enters under your control this way, put two +1/+1 counters on it."; + + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(12)); + + let lagrella = scenario + .add_creature_to_hand_from_oracle(P0, "Lagrella, the Magpie", 3, 3, LAGRELLA) + .as_legendary() + .id(); + let opposing = scenario.add_creature(P1, "Opposing Bear", 2, 2).id(); + let removal = scenario + .add_spell_to_hand_from_oracle(P0, "Murder", true, DESTROY_TARGET_CREATURE) + .id(); + + let mut runner = scenario.build(); + + let entered = runner.cast(lagrella).target_object(opposing).resolve(); + // Reach-guard 1: the exile leg actually happened. + assert_eq!( + entered.zone_of(opposing), + Zone::Exile, + "reach-guard: Lagrella's ETB must exile the opposing creature" + ); + + let removed = runner.cast(removal).target_object(lagrella).resolve(); + assert_eq!( + removed.zone_of(lagrella), + Zone::Graveyard, + "reach-guard: Lagrella must leave the battlefield to return the card" + ); + + advance_until_delayed_triggers_resolve(&mut runner); + + // Reach-guard 2: the card really is on the battlefield when the delayed + // trigger resolves, so "counters placed" cannot pass vacuously on a card + // that never moved. + assert_eq!( + runner.state().objects[&opposing].zone, + Zone::Battlefield, + "reach-guard: the exiled card must return to the battlefield" + ); + + let counters = runner.state().objects[&opposing] + .counters + .get(&engine::types::counter::CounterType::Plus1Plus1) + .copied() + .unwrap_or(0); + assert_eq!( + counters, 2, + "T-Z4: the delayed WhenEntersBattlefield trigger names the referent's \ + OWN entry, so the referent is expected to have moved and must still be \ + affected (CR 603.7c operative test + CR 400.7e)" + ); +} + +// ==================================================== T-H2 (inertness control) + +/// T-H2 — SIBLING / NEGATIVE. A `ParentTarget` reference in a NON-delayed chain +/// resolves unchanged, proving the guard is inert outside the delayed path. +/// +/// Goryo's own "That creature gains haste" is a `GenericEffect{ParentTarget}` +/// resolved immediately in the same chain, so it exercises the no-pin arm of the +/// keyed predicate (`find(..).is_none_or(..)`). +#[test] +fn t_h2_non_delayed_parent_target_reference_is_unaffected() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, mana(8)); + + let legendary = scenario + .add_creature_to_graveyard(P0, "Legendary Bear", 4, 4) + .as_legendary() + .id(); + let goryos = scenario + .add_spell_to_hand_from_oracle(P0, "Goryo's Vengeance", true, GORYOS_VENGEANCE) + .id(); + + let mut runner = scenario.build(); + + let outcome = runner.cast(goryos).target_object(legendary).resolve(); + + assert_eq!( + outcome.zone_of(legendary), + Zone::Battlefield, + "T-H2: the immediate ChangeZone link resolves unchanged" + ); + // The haste grant is the immediate `GenericEffect{ParentTarget}` link. It + // must still find its referent — no pin exists for a non-delayed chain. + assert!( + outcome + .state() + .transient_continuous_effects + .iter() + .any(|tce| { + matches!( + tce.affected, + engine::types::ability::TargetFilter::SpecificObject { id } if id == legendary + ) + }), + "T-H2: the immediate ParentTarget haste grant must still reach its \ + referent — the pin must be inert outside the delayed path" + ); +} + +// ================================================ T-D1 (PLACEMENT DETECTOR) + +/// T-D1 — **THE §5.3(b) PLACEMENT DETECTOR.** Role: PLACEMENT DETECTOR, and it +/// is the ONLY test in this file that can detect a §5.3(b) misplacement. +/// +/// # Why this test exists +/// +/// §5.3(b)'s gate must be called BEFORE the two condition binders, because they +/// rewrite `WhenDies { filter: ParentTarget }` into `WhenDies { filter: +/// SpecificObject { id } }` and the gate can no longer recognize the anaphor. +/// That constraint was **unfalsifiable** until this test existed: implementation +/// round 1 sank the call past both binders — reproducing the exact defect the +/// seam exists to prevent — and every then-existing test still passed. +/// +/// A detector for this seam needs BOTH properties, and the two tests previously +/// named here satisfy at most one each: +/// +/// - **(P1) a pin is actually STAMPED** — requires `parent_target_snapshot` to +/// return a NON-EMPTY list. `saffi eriksdotter` and `adarkar valkyrie` FAIL +/// this: despite Oracle text reading "target creature", neither parse declares +/// a target slot at all, so the snapshot is `[]` and the seam is inert in BOTH +/// placements. **Verify by parse, never by Oracle prose.** +/// - **(P2) the pin is actually READ** — requires the delayed effect to reach a +/// guarded terminal read. `lagrella, the magpie` FAILS this: a pin IS stamped, +/// but her `PutCounter` returns at `counters.rs`'s ungated event-context arm +/// before the guarded read, so the counters land either way. +/// +/// `whippoorwill` is the only in-class card clean on both, verified at the card +/// data rather than assumed: +/// - **P1** — its root ability is `kind: Activated`, `effect: GenericEffect` with +/// a real `target: Typed { type_filters: [Creature] }` slot, and the delayed +/// trigger hangs off it via a `SequentialSibling` sub-ability chain +/// (`AddRestriction` → `CreateDelayedTrigger`). `parent_target_snapshot` +/// therefore returns through `parent_chain_targets_from_root` — the branch +/// Saffi and Adarkar Valkyrie never reach. +/// - **P2** — the delayed effect is `ChangeZone { destination: Exile, target: +/// ParentTarget }`, which routes through `resolved_targets` to the guarded +/// terminal read. +/// - `uses_tracked_set: false`, so `bind_contextual_filter_to_condition` is the +/// ONLY rewrite and a red here is single-cause. +/// +/// # How it goes red +/// +/// With the gate misplaced, `condition_expects_referent_move` is `false`, so a +/// pin IS stamped while the creature is on the battlefield. The creature then +/// dies, which bumps its incarnation, so the pin is stale when the trigger +/// resolves — the guard drops the referent and NOTHING is exiled. The card stays +/// in the graveyard and the `Zone::Exile` assertion fails. +/// +/// With the gate correctly placed, no pin is stamped (the condition names the +/// referent's own zone change), the snapshot resolves unfiltered, and the +/// graveyard card is exiled. That is also the shipped behavior today, so this +/// test is green both pre-fix and post-fix — **the red is its job, not the +/// green.** +#[test] +fn t_d1_whippoorwill_exiles_the_dead_referent_from_the_graveyard() { + // Verbatim Oracle text (Scryfall). A paraphrase can take a different parser + // branch and go green while the real card stays broken. + const WHIPPOORWILL: &str = "{G}{G}, {T}: Target creature can't be regenerated this turn. Damage that would be dealt to that creature this turn can't be prevented or dealt instead to another permanent or player. When the creature dies this turn, exile the creature."; + + // ---- Arm 1: the referent dies. The delayed trigger must exile it. ---- + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, [mana_of(ManaType::Green, 4), mana(8)].concat()); + stock_libraries(&mut scenario); + + let bird = scenario + .add_creature_from_oracle(P0, "Whippoorwill", 1, 1, WHIPPOORWILL) + .id(); + let victim = scenario.add_creature(P1, "Opposing Bear", 2, 2).id(); + let removal = scenario + .add_spell_to_hand_from_oracle(P0, "Murder", true, DESTROY_TARGET_CREATURE) + .id(); + + let mut runner = scenario.build(); + + runner.activate(bird, 0).target_object(victim).resolve(); + + // Reach-guard (P1): the activation really did install the delayed trigger. + // Without this, a parse that silently dropped the CDT would make the whole + // test vacuous. + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: activating Whippoorwill must install exactly one delayed trigger" + ); + assert_eq!( + runner.state().objects[&victim].zone, + Zone::Battlefield, + "reach-guard: the referent is on the battlefield when the trigger is created" + ); + + let killed = runner.cast(removal).target_object(victim).resolve(); + + // Paired positive reach-guard: the referent actually DIED. Without it, + // "ends in Exile" could pass on a creature exiled by something else, and + // "not in Graveyard" could pass vacuously on a creature that never died. + // Both `Graveyard` and `Exile` are accepted here because the delayed trigger + // may already have resolved inside `resolve()`; the one zone that would + // falsify the premise is `Battlefield`. + assert_ne!( + killed.zone_of(victim), + Zone::Battlefield, + "reach-guard: the referent must have died for this test to mean anything" + ); + + advance_until_delayed_triggers_resolve(&mut runner); + + let final_zone = runner.state().objects[&victim].zone; + // The two zones are asserted DISTINCTLY: this is a card-moved-out-of-the- + // graveyard test, so `Graveyard` is the failure state, not a neutral one. + assert_ne!( + final_zone, + Zone::Graveyard, + "T-D1: the delayed WhenDies trigger names the referent's OWN zone change, \ + so no pin may be stamped and the dead creature must still be exiled \ + (CR 603.7c operative test + CR 400.7e). Still in the graveyard means a \ + pin WAS stamped — the §5.3(b) gate ran after the condition binders saw \ + the anaphor rewritten" + ); + assert_eq!( + final_zone, + Zone::Exile, + "T-D1: the referent must end in exile, not merely somewhere other than \ + the graveyard" + ); + + // ---- Arm 2: no kill. The P1 reach-guard for the pair. ---- + // A `[]` snapshot could never have produced arm 1's move, so the two arms + // together prove a referent was genuinely captured — the property `saffi` + // silently lacks and the reason round 1's revert-check produced no red. + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, [mana_of(ManaType::Green, 4), mana(8)].concat()); + stock_libraries(&mut scenario); + + let bird = scenario + .add_creature_from_oracle(P0, "Whippoorwill", 1, 1, WHIPPOORWILL) + .id(); + let survivor = scenario.add_creature(P1, "Opposing Bear", 2, 2).id(); + + let mut runner = scenario.build(); + + runner.activate(bird, 0).target_object(survivor).resolve(); + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: the no-kill arm must install the same delayed trigger" + ); + + // NOT the drain helper: this trigger is SUPPOSED never to fire, so waiting + // for the delayed-trigger list to empty would stall rather than measure. + advance_past_end_of_turn(&mut runner); + + assert_eq!( + runner.state().objects[&survivor].zone, + Zone::Battlefield, + "T-D1 arm 2: a referent that never died must be untouched — the trigger's \ + condition was never met" + ); +} + +// ================================================= T-Z5 (PLACEMENT DETECTOR) + +/// T-Z5 — **THE §5.5(e) DETECTOR** for the `counters.rs` guarded terminal read. +/// Role: PLACEMENT DETECTOR. +/// +/// This is the FIRST executed test of that seam. Plan round 8 claimed `T-Z4` +/// (`lagrella`) covered it; round 1 measured that Lagrella never reaches the +/// line — her `PutCounter` returns earlier, at the ungated `resolve_event_ +/// context_targets` arm. The seam was guarded but untested, not mis-tested. +/// +/// `cycle of life` was chosen from the ten pinned counters-family pairs because +/// its `count` is `Fixed(1)`. The rejected alternatives and why: +/// `sacred boon` / `scars of the veteran` use `EventContextAmount` counts, which +/// would make a red ambiguous between "no counter" and "a differently-sized +/// counter"; `side quest` is an Un-set card; `infinite authority` carries an +/// intervening-if that adds a second failure mode. +/// +/// Verified at the card data: root `GenericEffect` with a real +/// `target: Typed { type_filters: [Creature] }` slot, delayed +/// `AtNextPhaseForPlayer { phase: Upkeep }` → `PutCounter { counter_type: P1P1, +/// count: Fixed(1), target: ParentTarget }`, `uses_tracked_set: false`. +/// +/// **Revert-failing:** revert §5.5(e)'s substitution and the blink arm places +/// the counter anyway, turning this test red. +#[test] +fn t_z5_cycle_of_life_places_no_counter_on_a_blinked_referent() { + // Verbatim Oracle text (Scryfall). + const CYCLE_OF_LIFE: &str = "Return this enchantment to its owner's hand: Target creature you cast this turn has base power and toughness 0/1 until your next upkeep. At the beginning of your next upkeep, put a +1/+1 counter on that creature."; + + // The `AtNextPhaseForPlayer { Upkeep }` condition does NOT name the + // referent's zone change, so this card IS pinned — the opposite verdict from + // T-D1's `WhenDies`, through the same gate. That contrast is deliberate. + fn counters_on(runner: &engine::game::scenario::GameRunner, id: ObjectId) -> u32 { + runner.state().objects[&id] + .counters + .get(&engine::types::counter::CounterType::Plus1Plus1) + .copied() + .unwrap_or(0) + } + + // ---- Arm 1 (MANDATORY reach-guard): no blink, the counter IS placed. ---- + // Without this arm, "no counter" in arm 2 would pass vacuously on a trigger + // that never fired at all. + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, [mana_of(ManaType::White, 4), mana(8)].concat()); + stock_libraries(&mut scenario); + + let cycle = scenario + .add_enchantment_from_oracle(P0, "Cycle of Life", CYCLE_OF_LIFE) + .id(); + let subject = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + + let mut runner = scenario.build(); + + runner.activate(cycle, 0).target_object(subject).resolve(); + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: activating Cycle of Life must install exactly one delayed trigger" + ); + + advance_until_delayed_triggers_resolve(&mut runner); + + assert_eq!( + counters_on(&runner, subject), + 1, + "T-Z5 arm 1: with no blink the delayed upkeep trigger must place its \ + +1/+1 counter — this is the reach-guard that makes arm 2 non-vacuous" + ); + + // ---- Arm 2 (the detector): blink the referent, no counter may land. ---- + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool(P0, [mana_of(ManaType::White, 4), mana(8)].concat()); + stock_libraries(&mut scenario); + + let cycle = scenario + .add_enchantment_from_oracle(P0, "Cycle of Life", CYCLE_OF_LIFE) + .id(); + let subject = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + let ephemerate = scenario + .add_spell_to_hand_from_oracle(P0, "Ephemerate", true, EPHEMERATE) + .id(); + + let mut runner = scenario.build(); + + runner.activate(cycle, 0).target_object(subject).resolve(); + assert_eq!( + runner.state().delayed_triggers.len(), + 1, + "reach-guard: the blink arm must install the same delayed trigger" + ); + + let blinked = runner.cast(ephemerate).target_object(subject).resolve(); + // Reach-guard: the blink really happened and the creature really came back, + // so "no counter" cannot pass on a creature that is simply gone. + assert_eq!( + blinked.zone_of(subject), + Zone::Battlefield, + "reach-guard: Ephemerate must return the creature to the battlefield" + ); + + let events = advance_until_delayed_triggers_resolve(&mut runner); + + assert_eq!( + counters_on(&runner, subject), + 0, + "T-Z5 arm 2: the blinked referent is a NEW object (CR 400.7), so the \ + pinned delayed trigger must place no counter on it" + ); + // CR 603.7b: the trigger still fires and still resolves — it just affects + // nothing. Asserting the no-op WITHOUT this would also pass on a fix that + // wrongly suppressed the trigger entirely. + assert!( + effect_resolved_from(&events, cycle), + "T-Z5 arm 2: the delayed trigger must still have fired and resolved as a \ + no-op (CR 603.7b)" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 66fa7d1434..358ffaeb40 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -148,6 +148,7 @@ mod daretti_emblem_simultaneous_death; mod dark_confidant_upkeep; mod dark_depths_thespian_stage; mod death_priest_myrkul_oxford_anthem; +mod delayed_parent_target_incarnation; mod demon_of_fates_design; mod descendants_fury_sacrificed_referent_4795; mod destroy_redirect_to_battlefield_delivery_tail; diff --git a/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs b/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs index b54860f1d5..64cbc42577 100644 --- a/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs +++ b/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs @@ -156,6 +156,7 @@ fn make_grant_ability(controller: PlayerId, source: ObjectId) -> ResolvedAbility trigger_source: None, trigger_definition_ref: None, force_block_attacker: None, + target_incarnations: Vec::new(), targets: vec![], kind: AbilityKind::Activated, sub_ability: None, From 9e3742e151db2359076251ab87603116cba10880 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 02:48:11 -0700 Subject: [PATCH 2/3] fix(engine): scope the combat-removal pin guard to the non-SelfRef arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/game/effects/remove_from_combat.rs | 19 ++++++++++++++++++- crates/engine/src/game/effects/sacrifice.rs | 6 +++++- crates/engine/src/types/ability.rs | 11 +++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/game/effects/remove_from_combat.rs b/crates/engine/src/game/effects/remove_from_combat.rs index c52ec4a236..a0bbad6392 100644 --- a/crates/engine/src/game/effects/remove_from_combat.rs +++ b/crates/engine/src/game/effects/remove_from_combat.rs @@ -55,7 +55,24 @@ pub fn resolve( // nothing. The shape mirrored here is `change_zone.rs` / `sacrifice.rs`. // `EffectKind::RemoveFromCombat` (not `EffectKind::from(&ability.effect)`) // matches this file's own convention at the unconditional push below. - if ability.pinned_object_targets_all_stale(state) { + // + // SCOPED TO THE NON-`SelfRef` ARM. A `SelfRef` removal's subject is the + // source itself, never the snapshot referent, so a stale pin on some other + // object in `ability.targets` must not cancel it. Without this guard the + // predicate and the subject it suppresses are decoupled — 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. Unreachable today (the in-class population + // is `melee`, whose filter is a bare `ParentTarget`, so no `SelfRef` node + // co-occurs with a pin), but the coupling is what makes it correct rather + // than the population. + 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) { events.push(GameEvent::EffectResolved { kind: EffectKind::RemoveFromCombat, source_id: ability.source_id, diff --git a/crates/engine/src/game/effects/sacrifice.rs b/crates/engine/src/game/effects/sacrifice.rs index a41b54845c..ce77d9455d 100644 --- a/crates/engine/src/game/effects/sacrifice.rs +++ b/crates/engine/src/game/effects/sacrifice.rs @@ -205,7 +205,11 @@ pub fn resolve( // CR 400.7 + CR 603.7c: a delayed sacrifice whose pinned referent became a // new object affects nothing. Return before the empty-pool fallback below, // which resolves a player scope and would make the controller sacrifice a - // DIFFERENT permanent (`resolve_sacrifice_scope`, CR 701.17a). + // DIFFERENT permanent (`resolve_sacrifice_scope`, CR 701.21a: "To sacrifice + // a permanent, its controller moves it from the battlefield directly to its + // owner's graveyard"). Note this file elsewhere cites CR 701.17a for + // sacrifice; 701.17a is mill (CR 701.21 is Sacrifice). That pre-existing + // cluster is left alone here so the correction lands as one auditable pass. // // Emits EffectResolved first, matching the shipped CR 400.7 SelfRef guard // above, which this guard is the direct extension of. diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index d1bc6cc98a..43c6bdf522 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -23742,6 +23742,17 @@ impl ResolvedAbility { // bump advances a pinned referent's epoch on every zone change. A // pinned delayed trigger riding on a zone-cycling loop would otherwise // carry a growing epoch into loop equality and never confirm a draw. + // + // CR 603.3b: the same stripping is correct for the other caller, + // `normalize_ability_identity`, which decides whether two triggered + // abilities are genuinely indistinguishable for auto-ordering — a + // per-instance referent epoch is exactly the object identity that + // comparison must ignore. Both callers operate on clones, so no + // production pin is ever cleared. This is deliberately the opposite of + // `inert_trigger_abilities_eq_ignoring_provenance` (`game/stack.rs`), + // which DOES compare `target_incarnations`: there the question is + // whether two abilities would resolve identically, and two pins at + // different epochs would not. Same field, different questions. self.target_incarnations.clear(); if let Some(sub) = self.sub_ability.as_mut() { sub.clear_trigger_identity_recursive(); From f5d4e11cbfd41e486796ec443f378e5cc50760a9 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 03:07:06 -0700 Subject: [PATCH 3/3] test(engine): close two vacuity holes in the delayed-trigger controls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../delayed_parent_target_incarnation.rs | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/crates/engine/tests/integration/delayed_parent_target_incarnation.rs b/crates/engine/tests/integration/delayed_parent_target_incarnation.rs index 047d198c80..a282ed9c37 100644 --- a/crates/engine/tests/integration/delayed_parent_target_incarnation.rs +++ b/crates/engine/tests/integration/delayed_parent_target_incarnation.rs @@ -155,10 +155,23 @@ fn advance_past_end_of_turn(runner: &mut engine::game::scenario::GameRunner) { }, _ => GameAction::PassPriority, }; - if runner.act(action).is_err() { - return; + // Never `return` on an error. A silent early exit means no game time + // passes, and every "the referent survived to end of turn" assertion + // downstream then holds trivially — the negative arm would pass for the + // wrong reason. Panic instead, matching + // `advance_until_delayed_triggers_resolve` above. + if let Err(e) = runner.act(action) { + panic!( + "advancing past end of turn failed: {e:?} (phase = {:?}, waiting_for = {:?})", + runner.state().phase, + runner.state().waiting_for, + ); } } + assert!( + runner.state().turn_number > start_turn, + "reach-guard: the turn must actually have ended for a survival assertion to mean anything" + ); } /// True when an `EffectResolved` event names this source — the observable proof @@ -406,11 +419,24 @@ fn t_z1_saffi_eriksdotter_still_returns_the_creature() { ); let killed = runner.cast(removal).target_object(victim).resolve(); - // Reach-guard: the referent really is in the graveyard when the delayed - // trigger resolves, so "returned" below cannot pass vacuously. + // Reach-guard on the EVENT, not on the final zone. A zone snapshot cannot + // serve here: Saffi's trigger fires on the death and returns the card + // within this same resolution, so the victim legitimately ends on the + // battlefield — which is indistinguishable from "the removal never resolved + // and it never left". Asserting `Battlefield` as both the guard and the + // conclusion is what made this control vacuous. Prove the death happened + // instead. assert!( - matches!(killed.zone_of(victim), Zone::Graveyard | Zone::Battlefield), - "reach-guard: the victim must have been put into the graveyard" + killed.events().iter().any(|e| matches!( + e, + GameEvent::ZoneChanged { + object_id, + to: Zone::Graveyard, + .. + } if *object_id == victim + )), + "reach-guard: the victim must actually have been put into the graveyard \ + for Saffi's delayed trigger to have anything to return" ); advance_until_delayed_triggers_resolve(&mut runner);