fix(engine): declined "up to N target" sub-effect no longer inherits parent's target (Cruel Revival) (#5281)#5397
Conversation
…parent's target (phase-rs#5281) Cruel Revival -- "Destroy target non-Zombie creature. It can't be regenerated. Return up to one target Zombie card from your graveyard to your hand." -- returned the destroyed creature to hand instead of destroying it whenever the caster DECLINED the optional "up to one" bounce. Root cause: in resolve_ability_chain, a sub-effect with empty `targets` inherited the parent instruction's targets wholesale (`sub_with_targets .targets = ability.targets.clone()`). The bounce carries its own independent target slot ("target Zombie card from your graveyard"); when its optional target is legally declined (CR 115.6), its `targets` are empty, so it wrongly inherited the parent Destroy's battlefield-creature target and bounced it. Fix: when inheriting parent targets, keep Player targets (shared across the chain -- Paradigm's "that player" draw + lose-life; relative-controller change-zone) but keep an inherited OBJECT target only when it is legal for the sub-effect's own independent filter. Cruel Revival's destroy target (a battlefield creature) is illegal for the bounce's "Zombie card in your graveyard" filter, so it is dropped and the declined bounce resolves as a no-op (CR 608.2c). Anaphoric subs ("It can't be regenerated", ParentTarget refs) keep inheriting unchanged. Adds two runtime cast tests (destroy + return both targeted; and the declined-bounce edge). Full engine lib suite green. CR 115.6 (a spell "up to N targets" is targeted only if >=1 chosen) + CR 608.2c (later text modifies earlier -- "Destroy ... It can't be regenerated").
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #5281 by updating target inheritance logic for sub-effects in crates/engine/src/game/effects/mod.rs to filter out inherited object targets that do not match the sub-effect's independent filter, accompanied by comprehensive integration tests. The review feedback correctly identifies a critical edge case where a declined optional independent target slot could incorrectly inherit and resolve against a parent's target if it matches the filter, and provides an elegant code suggestion to prevent object target inheritance entirely when an independent filter is present.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // CR 115.6 + CR 608.2c (issue #5281): Inherit the parent instruction's | ||
| // targets, but for a sub-effect that carries its OWN independent object | ||
| // filter, keep only the inherited OBJECT targets that are actually legal | ||
| // for that filter. Player targets are shared across the chain (Paradigm's | ||
| // "that player" draw + lose-life; relative-controller change-zone) and | ||
| // are always inherited. Cruel Revival's "Return up to one target Zombie | ||
| // card from your graveyard ..." was given its own target slot, so the | ||
| // parent's destroy target (a battlefield creature) is illegal for it and | ||
| // is dropped — the declined optional bounce resolves as a no-op (CR 115.6) | ||
| // instead of bouncing the creature the spell just destroyed. | ||
| let own_object_filter = | ||
| crate::game::triggers::extract_target_filter_from_effect(&sub.effect) | ||
| .filter(|_| !effect_refs_parent_target(&sub.effect)); | ||
| sub_with_targets.targets = ability | ||
| .targets | ||
| .iter() | ||
| .filter(|target_ref| match (target_ref, own_object_filter) { | ||
| (TargetRef::Player(_), _) => true, | ||
| (TargetRef::Object(_), None) => true, | ||
| (obj @ TargetRef::Object(_), Some(filter)) => { | ||
| !crate::game::targeting::validate_targets_for_ability( | ||
| state, | ||
| std::slice::from_ref(obj), | ||
| filter, | ||
| ability, | ||
| ) | ||
| .is_empty() | ||
| } | ||
| }) | ||
| .cloned() | ||
| .collect(); |
There was a problem hiding this comment.
Potential "Target Stealing" Bug on Multiple Independent Target Slots
[HIGH] Potential target stealing when an optional independent target slot is declined. Evidence: crates/engine/src/game/effects/mod.rs:7817-7847.
Why it matters
If a spell has multiple independent target slots of the same type (for example, "Exile target Zombie card from your graveyard. Return up to one target Zombie card from your graveyard to your hand."), and the player declines the optional target for the second slot, sub.targets will be empty, triggering this branch. Under the current implementation, because the first slot's target (e.g., Zombie A) is valid for the second slot's filter, the second slot will inherit Zombie A and resolve against it, even though the player explicitly declined to target anything for the second slot.
Suggested Fix
If a sub-effect has its own independent target slot (own_object_filter is Some(_)), it should never inherit any object targets from the parent, regardless of whether they are valid for the filter. If the player chose a target for that slot, sub.targets would not be empty, and this branch wouldn't be entered. If they declined it, it should remain empty. This also avoids calling the expensive validate_targets_for_ability function during resolution.
// CR 115.6 + CR 608.2c (issue #5281): Inherit the parent instruction's
// targets, but do not inherit any object targets if the sub-effect has
// its own independent target slot (i.e., own_object_filter is Some).
// Player targets are shared across the chain (e.g., Paradigm's "that player"
// draw + lose-life) and are always inherited.
let own_object_filter =
crate::game::triggers::extract_target_filter_from_effect(&sub.effect)
.filter(|_| !effect_refs_parent_target(&sub.effect));
sub_with_targets.targets = ability
.targets
.iter()
.filter(|target_ref| match (target_ref, own_object_filter) {
(TargetRef::Player(_), _)=> true,
(TargetRef::Object(_), None)=> true,
(TargetRef::Object(_), Some(_))=> false,
})
.cloned()
.collect();
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Independent optional sub-effects can still steal a declined parent target when the parent target happens to satisfy the sub-effect filter. Evidence: crates/engine/src/game/effects/mod.rs:7815 enters the inheritance branch whenever sub.targets.is_empty(), and lines 7830-7847 copy any parent object target that validates against own_object_filter; a spell like “Exile target Zombie card from your graveyard. Return up to one target Zombie card from your graveyard to your hand.” would still return the exiled Zombie when the second slot is declined because the first target passes the second slot’s Zombie-card filter. Why it matters: CR 115.6 optional targets declined at cast time must remain absent at resolution; filtering inherited targets only fixes Cruel Revival because its destroy target is illegal for the return filter, but it does not fix the target-slot class. Suggested fix: when a sub-effect has its own independent target filter and no explicit targets, do not inherit object targets at all; only inherit object targets for parent-target effects, and add a same-filter declined-slot regression.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…rget (phase-rs#5281 review) Addresses the [HIGH] review on phase-rs#5397. The first pass filtered inherited object targets by the sub-effect's own filter legality — a proxy that still let an independent optional sub steal the parent's target when that object happened to satisfy the sub's filter. Tighten it: a sub with its OWN independent target filter never inherits a parent OBJECT target at all (its empty targets mean the optional target was declined, CR 115.6, so it resolves as a no-op). Player targets are still shared across the chain, and anaphoric subs (no filter / ParentTarget ref) still inherit. Full engine lib suite green (15,792 passed); the Cruel Revival, Paradigm-copy, and relative-controller change-zone cases all pass.
|
Good catch — tightened per the [HIGH] finding. Instead of filtering inherited object targets by legality (a proxy that still lets a sub steal a legal parent object), a sub-effect with its own independent target filter now never inherits a parent OBJECT target at all — its empty |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: current head resolves the independent optional sub-effect target slot at the chain target-propagation seam, with runtime coverage for both chosen and declined Cruel Revival return targets. Auto-merge can wait for pending checks.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer fixup: preserved reflexive chain targets while keeping independent optional object target slots from inheriting declined parent targets. Auto-merge can wait for the new CI run.
2 participants
Summary
[[Cruel Revival]] — "Destroy target non-Zombie creature. It can't be regenerated. Return up to one target Zombie card from your graveyard to your hand." — returned the destroyed creature to hand instead of destroying it whenever the caster declined the optional "up to one" bounce.
Fixes #5281
Root cause
The spell parses correctly to
Destroy(non-Zombie creature)→Bounce(Zombie card from graveyard, up to one). The bug is at resolution: inresolve_ability_chain, a sub-effect with emptytargetsinherited the parent instruction's targets wholesale (sub_with_targets.targets = ability.targets.clone()).The bounce has its own independent target slot ("target Zombie card from your graveyard"). When that optional target is legally declined (CR 115.6 — a "up to N target" effect is targeted only if ≥1 target is chosen), its
targetsare empty — so it inherited the parentDestroy's battlefield-creature target and bounced it.Fix
When inheriting parent targets to a chained sub-effect:
ParentTarget/relative-controller ref — keep inheriting unchanged.This is a single, guarded change in the chain-resolution target-propagation; it reuses
targeting::validate_targets_for_abilityfor the legality check. No new engine variant.Implementation method (required)
/engine-implementer— explain why belowCR references
Verification
crates/engine/tests/cruel_revival_destroy_and_return_5281.rs):cruel_revival_destroys_creature_and_returns_graveyard_zombie— both targets: creature destroyed (→ graveyard), graveyard Zombie returned (→ hand).cruel_revival_destroys_creature_when_bounce_declined— the reported bug: bounce declined, creature still destroyed (not bounced).cargo test -p engine --lib— green (no regressions; the Paradigm and change-zone chains that a broader first attempt broke both pass).cargo fmt --all,cargo clippy -p engine— clean.