fix(engine): Fight Rigging exiles the targeted creature instead of countering it (#6437) - #6782
Conversation
…untering it (phase-rs#6437) Root cause: the default sequential-sibling target-propagation arm in resolve_chain_body (has_independent_target_slot) treats any effect whose target filter reports no dedicated slot as "inherit the parent's chosen object". TargetFilter::ExiledBySource is a context-ref with no slot of its own (it resolves independently via exile_links), so a chain like "put a +1/+1 counter on target creature you control. Then ... you may play the exiled card" copied the counter's chosen creature into the CastFromZone sub's targets — treating the targeted creature as the card to license and exiling it instead of countering it. Same fix applied to should_propagate_parent_targets (the sibling authority used by the declined-optional-branch dispatcher), refined so a composed filter that ALSO references ParentTarget (Jodah's "put the rest ... except the hit" cleanup) still propagates as before. A second, sibling bug blocked the fix from being observable on Fight Rigging specifically: capture_linked_exile_snapshot (the leaves-the- battlefield ExiledBySource lookup used whenever a TRIGGERED ability resolves ExiledBySource) filtered to ExileLinkKind::TrackedBySource only, dropping Hideaway's own ExileLinkKind::HideawayLookable links even though Hideaway's own doc comment says the kind is meant to be found by the kind-agnostic ExiledBySource lookup, same as the live (non-snapshot) path. Also affects Collector's Cage, which carries the identical counter-then-exiled-card shape on an activated ability. Adds add_enchantment_from_oracle to the scenario test harness (mirrors add_land_from_oracle) so a permanent's own triggered ability can be driven without going through its cast/ETB pipeline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe engine separates bare ChangesFight Rigging exile-target resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GameScenario
participant FightRigging
participant ExiledCard
GameScenario->>FightRigging: create enchantment from Oracle text
FightRigging->>ExiledCard: resolve ExiledBySource subeffect
ExiledCard-->>FightRigging: grant conditional free-cast permission
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/game/zones.rs (1)
1436-1461: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test exercises the widened link-kind filter.
The existing regression test at Lines 3570-3617 only pushes an
ExileLinkKind::TrackedBySourcelink, so it passes identically before and after this change. Nothing in the diff (or the new Fight Rigging integration tests, which never move the enchantment off the battlefield) proves that aHideawayLookable-kind link is now captured inlinked_exile_snapshoton leaves-the-battlefield — the exact behavior this fix claims to add.Consider adding a case (mirroring the existing test) that pushes an
ExileLinkKind::HideawayLookablelink and asserts it now appears inrecord.linked_exile_snapshotaftermove_to_zone— a case that would have failed under the pre-fixTrackedBySource-only filter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/zones.rs` around lines 1436 - 1461, The regression coverage does not verify that HideawayLookable links are captured when the source leaves the battlefield. Extend the existing linked-exile snapshot test around capture_linked_exile_snapshot and move_to_zone to insert an ExileLinkKind::HideawayLookable link, then assert it appears in record.linked_exile_snapshot after the move; preserve the existing TrackedBySource case.crates/engine/src/game/effects/mod.rs (1)
2479-2488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
ExiledBySource-independence predicate.
should_propagate_parent_targets(Lines 2483-2487) and thehas_independent_target_slotcomputation (Lines 10485-10489) both re-implement the identical check:sub.effect.target_filter().is_some_and(TargetFilter::references_exiled_by_source) && !effect_refs_parent_target(&sub.effect). Given how many finely-tuned gating arms already coexist in this function (and how much doc-comment reasoning accompanies each copy), a future edit to one arm risks silently diverging from the other.♻️ Proposed extraction
+fn is_independent_exiled_by_source_filter(effect: &Effect) -> bool { + effect + .target_filter() + .is_some_and(TargetFilter::references_exiled_by_source) + && !effect_refs_parent_target(effect) +} + fn should_propagate_parent_targets(ability: &ResolvedAbility, sub: &ResolvedAbility) -> bool { sub.targets.is_empty() && !ability.targets.is_empty() && sub.target_choice_timing != TargetChoiceTiming::Resolution - && !(sub - .effect - .target_filter() - .is_some_and(TargetFilter::references_exiled_by_source) - && !effect_refs_parent_target(&sub.effect)) + && !is_independent_exiled_by_source_filter(&sub.effect) }let has_independent_target_slot = (crate::game::triggers::extract_target_filter_from_effect(&sub.effect).is_some() && !effect_refs_parent_target(&sub.effect) && !sub_ability_target_belongs_to_reflexive_context(sub)) - || (sub - .effect - .target_filter() - .is_some_and(TargetFilter::references_exiled_by_source) - && !effect_refs_parent_target(&sub.effect)); + || is_independent_exiled_by_source_filter(&sub.effect);Also applies to: 10481-10489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/mod.rs` around lines 2479 - 2488, Extract the repeated ExiledBySource-independence check into a shared helper near should_propagate_parent_targets, then replace the inline predicate in both should_propagate_parent_targets and the has_independent_target_slot computation with that helper. Preserve the existing boolean semantics and keep the helper focused only on target-filter references and effect_refs_parent_target.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 2479-2488: Extract the repeated ExiledBySource-independence check
into a shared helper near should_propagate_parent_targets, then replace the
inline predicate in both should_propagate_parent_targets and the
has_independent_target_slot computation with that helper. Preserve the existing
boolean semantics and keep the helper focused only on target-filter references
and effect_refs_parent_target.
In `@crates/engine/src/game/zones.rs`:
- Around line 1436-1461: The regression coverage does not verify that
HideawayLookable links are captured when the source leaves the battlefield.
Extend the existing linked-exile snapshot test around
capture_linked_exile_snapshot and move_to_zone to insert an
ExileLinkKind::HideawayLookable link, then assert it appears in
record.linked_exile_snapshot after the move; preserve the existing
TrackedBySource case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0a7f4e0-f85d-48ea-b170-864299104e17
📒 Files selected for processing (5)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/scenario.rscrates/engine/src/game/zones.rscrates/engine/tests/integration/issue_6437_fight_rigging_exiled_card_target.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the Fight Rigging target-propagation regression is discriminating, but one separately broadened runtime path remains untested.
🔴 Blocker
[MED] The all-link-kind leave-the-battlefield snapshot has no production-path regression. Evidence: crates/engine/src/game/zones.rs:1445-1460 changes capture_linked_exile_snapshot from TrackedBySource-only to every ExileLinkKind, but crates/engine/tests/integration/issue_6437_fight_rigging_exiled_card_target.rs:101-177 keeps Fight Rigging on the battlefield throughout, so it never invokes that snapshot function. Why it matters: the new behavior governs a different path — an exiled-card lookup after the source has left the battlefield — and a future change could reintroduce kind filtering or snapshot the wrong link set while this PR remains green. Suggested fix: add a runtime leaves-the-battlefield witness that moves the source through the zone pipeline and proves the subsequent ExiledBySource consumer receives the intended Hideaway-linked card (and preserves the stated all-kind contract), or remove this unrelated snapshot widening from this PR.
✅ Clean
The Fight Rigging witness reaches the real begin-combat trigger, proves the counter target stays on the battlefield, and proves the linked hidden card — not that target — receives the permission.
Recommendation: request changes for the missing snapshot-path discriminator; the target-propagation fix itself is correctly scoped.
…ath (phase-rs#6437) Address review: the kind-widening fix in capture_linked_exile_snapshot (TrackedBySource-only -> every ExileLinkKind) had no test that actually drives a source through the real leaves-the-battlefield zone pipeline — the two existing Fight Rigging tests keep the source on the battlefield throughout, exercising a different (still-present) call path. Adds a regression test for Watcher for Tomorrow's real LTB trigger ("When this creature leaves the battlefield, put the exiled card into its owner's hand", Effect::ChangeZoneAll targeting ExiledBySource): casts a real removal spell to destroy it through the actual casting/zone pipeline (zones::move_to_zone), then asserts the HideawayLookable-linked hidden card reaches its owner's hand. Verified as a genuine discriminator by temporarily reverting the kind filter back to TrackedBySource-only and confirming the new test fails (hidden card stranded in exile) before re-applying the fix.
|
Maintainer re-review — held only for current-head checks. At Rust checks are still running for this head. This is not a request for contributor changes; once they settle, the handler will clear the stale changes-requested state and continue approval/enqueue handling. |
|
Addressed — added `watcher_for_tomorrow_leaving_the_battlefield_finds_the_hideaway_linked_card`, which casts a real removal spell to destroy Watcher for Tomorrow through the actual casting/zone pipeline (`zones::move_to_zone`), driving the genuine leaves-the-battlefield path `capture_linked_exile_snapshot` guards, and asserts the `HideawayLookable`-linked hidden card reaches its owner's hand via Watcher's own parsed LTB trigger (`Effect::ChangeZoneAll`). Verified as a real discriminator: temporarily reverted the kind filter back to `TrackedBySource`-only and confirmed this new test fails (hidden card stranded in `Exile` instead of reaching `Hand`) before re-applying the fix. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current head verified.
The Watcher for Tomorrow scenario exercises the real LTB zone-move path and verifies the Hideaway linked-exile card is recovered. The current parse diff is unchanged and required CI is green.
2 participants
Summary
Fixes #6437 — Fight Rigging's begin-of-combat trigger ("put a +1/+1 counter on target creature you control. Then if you control a creature with power 7 or greater, you may play the exiled card without paying its mana cost.") exiled the targeted creature instead of the Hideaway-exiled card. Also affects Collector's Cage, which carries the identical counter-then-exiled-card shape on an activated ability.
Root cause 1 — wrong-target propagation
The default sequential-sibling target-propagation arm in
resolve_chain_body(has_independent_target_slot,crates/engine/src/game/effects/mod.rs) treats any effect whose target filter reports no dedicated stack-time slot as "safe to inherit the parent's already-chosen object."TargetFilter::ExiledBySourceis a context-ref with no slot of its own (it resolves independently viaexile_linksat its own resolution), so it fell through to that "inherit" branch — copying the counter's chosen creature into theCastFromZonesub's targets.cast_from_zone::resolvethen found a non-empty target list and never reached its ownExiledBySource→exile_linksfallback, instead treating the targeted creature as the card to license: since the creature sits on the battlefield (not Hand/Graveyard/Exile),grant_lingering_permissionsrouted it through the exile-delivery batch — exiling it instead of giving it a counter.Applied the same fix to
should_propagate_parent_targets(the sibling authority used by the declined-optional-branch dispatcher and others), refined so a composed filter that ALSO referencesParentTarget(Jodah, the Unifier's "put the rest ... except the hit" cleanup) still propagates as before — a plain "exclude wheneverExiledBySourceappears anywhere in the filter" first pass regressed that case, caught by the existingjodah_hit_decline_leaves_hit_in_exile_returns_missesunit test.Root cause 2 — link-kind filtering
Even with (1) fixed, Fight Rigging still failed to find its own hidden card:
capture_linked_exile_snapshot(the leaves-the-battlefieldExiledBySourcelookup snapshot, read whenever a triggered ability — as opposed to an activated ability like Windbrisk Heights — resolvesExiledBySource) filtered toExileLinkKind::TrackedBySourceonly, dropping Hideaway's ownExileLinkKind::HideawayLookablelinks.HideawayLookable's own doc comment already says the kind is meant to be discoverable by the kind-agnosticExiledBySourcelookup, matching the live (non-snapshot) path, which doesn't filter by kind at all.Test harness
Added
add_enchantment_from_oracleto the scenario builder (mirrors the existingadd_land_from_oracle) so a permanent's own triggered ability can be driven directly without routing through its cast/ETB pipeline.Test plan
issue_6437_fight_rigging_exiled_card_target.rs: targeted creature keeps its counter and stays on the battlefield; the linked hidden card (not the targeted creature) receives the free-cast permission; below the power-7 threshold only the counter is placed.cargo test -p phase-engine --test integration— 4173 passed, 0 failed.cargo test -p phase-engine --lib— 17933 passed, 0 failed (verified the pre-existingbulk_treasure_activation_is_linear_not_factorialperf-counter flake reproduces identically on unmodifiedmainunder parallel execution; unrelated to this change).cargo clippy -p phase-engine --all-targets -- -D warnings— clean.cargo fmt --all.Summary by CodeRabbit