fix(engine): run the base chain when a teamwork-instead swap doesn't fire#5809
Conversation
There was a problem hiding this comment.
Code Review
This pull request resolves issue #4772 where spells with 'instead' overrides (such as Teamwork) would silently drop their trailing effects when cast without paying the additional cost. The fix consolidates the handling of all four 'instead' condition kinds (AdditionalCostPaidInstead, CastVariantPaidInstead, TargetHasKeywordInstead, and ConditionInstead) in resolve_chain_body so they all correctly fall back to executing the base continuation chain when the condition is not met. Additionally, comprehensive integration tests have been added to verify both the teamwork and non-teamwork branches for 'Too Evil to Stay Dead'. I have no feedback to provide as there are no review comments.
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.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…fire Too Evil to Stay Dead's "choose target creature in graveyard w/ mv<=4 ... if cast using teamwork, instead choose target creature in graveyard. Return the chosen card to the battlefield" silently did nothing when cast without paying the teamwork additional cost: the not-swapped path for AdditionalCostPaidInstead/CastVariantPaidInstead/TargetHasKeywordInstead only checked sub.else_ability and returned early when it was None, dropping sub.sub_ability (the trailing ChangeZone) entirely. The sibling ConditionInstead arm already had the correct sub.sub_ability tail-runner fallback (added for Throw from the Saddle / Evil's Thrall / Take the Fall / That's Rough Buddy) -- this merges all four "instead" condition kinds onto that one, already-tested code path, since the swap-decision check above already treats them uniformly. Anchored on: - crates/engine/src/game/effects/mod.rs:7830-7921 (pre-fix) -- the ConditionInstead not-swap tail-runner this merge generalizes, with its own regression coverage (condition_instead_not_swap_tail_runner_honors_gates, condition_instead_runs_base_chain_when_not_met). - crates/engine/src/game/effects/mod.rs:6444-6473 -- the swap-decision block, which already evaluates all four condition kinds symmetrically. Closes phase-rs#4772
f475408 to
46af398
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Approved
🔴 Blocker
- None.
🟡 Non-blocking
- None.
✅ Clean
resolve_chain_bodyis the shared non-swap authority for all fourinsteadcondition kinds; the rebased code retains main's continuation-context capture.- The registered integration test drives both declined and paid Teamwork through casting, target selection, and stack resolution; the decline case fails if the new tail runner is removed.
- The current-head CI is green, Gemini reported no findings, and the parse-diff confirms no parser coverage change.
Recommendation: approve and enqueue.
Summary
Fixes #4772. Too Evil to Stay Dead ("Choose target creature card in your graveyard with
mana value 4 or less. If this spell was cast using teamwork, instead choose target creature
card in your graveyard. Return the chosen card to the battlefield.") silently did nothing
when cast without paying the optional Teamwork additional cost — mana was spent, the
spell resolved, but the targeted creature never returned to the battlefield.
Root cause
The not-swapped path for three of the four "instead" condition kinds
(
AdditionalCostPaidInstead,CastVariantPaidInstead,TargetHasKeywordInstead) only everchecked
sub.else_ability, returning early when it wasNone— silently droppingsub.sub_ability(here, the trailingChangeZonethat returns the card to the battlefield)entirely. The sibling
ConditionInsteadarm already had the correctsub.sub_abilitytail-runner fallback (added for Throw from the Saddle / Evil's Thrall /Take the Fall / That's Rough Buddy) — this merges all four condition kinds onto that single,
already-tested path, since the swap-decision check just above already evaluates all four
uniformly.
Anchored on
crates/engine/src/game/effects/mod.rs:7830-7921(pre-fix) — theConditionInsteadnot-swap tail-runner this change generalizes, with its own existing regression coverage
(
condition_instead_not_swap_tail_runner_honors_gates,condition_instead_runs_base_chain_when_not_met).crates/engine/src/game/effects/mod.rs:6444-6473— the swap-decision block, which alreadyevaluates
AdditionalCostPaidInstead/CastVariantPaidInstead/TargetHasKeywordInstead/ConditionInsteadsymmetrically, establishing that the not-swap path should be symmetrictoo.
Testing
Added
crates/engine/tests/integration/issue_4772_too_evil_to_stay_dead.rs:too_evil_to_stay_dead_without_teamwork_still_reanimates— the actual regression (failson pre-fix code with the exact reported symptom).
too_evil_to_stay_dead_with_teamwork_still_reanimates— differential control, confirmsthe already-working swap path doesn't regress.
Verified the new test is load-bearing by reverting just the
effects/mod.rschange andre-running — it fails with the exact issue #4772 symptom, then passes again with the fix
restored. Both tests pass on a clean, isolated
cargo test -p engine --test integration issue_4772run (not relying on any shared/incremental build cache).cargo fmt --allrun, no diff beyond the change above.Validation Failures (honesty disclosure, per docs/AI-CONTRIBUTOR.md)
This was implemented and verified by a Claude Code agent (diagnosis via real code reading
regression test proven load-bearing by revert/restore,
cargo fmtclean), but notthrough the formal
/engine-implementermulti-agent plan → review-plan → implement →review-impl pipeline — that skill wasn't available in the environment this was built in.
No
Gate Acombinator-purity script output or discriminating-test coverage map /maintainer-simulation matrix is included. Flagging this plainly rather than claiming a
review loop that didn't run.
Model: claude-sonnet-5
Tier: Standard