Skip to content

fix(engine): Saruman of Many Colors copies the card it exiled (Fixes #5576)#5644

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
davion-knight:fix/saruman-copy-exiled-by-source
Jul 12, 2026
Merged

fix(engine): Saruman of Many Colors copies the card it exiled (Fixes #5576)#5644
matthewevans merged 1 commit into
phase-rs:mainfrom
davion-knight:fix/saruman-copy-exiled-by-source

Conversation

@davion-knight

Copy link
Copy Markdown
Contributor

Summary

Saruman of Many Colors (#5576): the same-chain "exile target … card … Copy the exiled card" copied the wrong card, or nothing. The AST was fine (CopySpell { TrackedSet(0) }, correct for a same-chain reference) — the failure was at runtime. There turned out to be two root causes; both are fixed here.

1. Parser — publish the exile link (oracle_effect/imperative.rs)

The "copy the exiled card" reference bound the copy to the chain-local TrackedSet(0) sentinel whenever an earlier same-chain clause was an exile producer. But a plain-targeted ChangeZone{Exile} never publishes a TrackedSet, so copy_source_from_tracked_set fell back to latest_tracked_set_id — a global scan — and copied an unrelated set or nothing.

Bind "copy the exiled card" to ExiledBySource in both the same-chain and cross-resolution cases (matching the issue's recommended fix (b)). This also makes the exile a linked consumer, so should_track_exiled_by_source publishes the ExileLink that copy_source_from_exiled_by_source reads — reusing the machinery from the earlier cross-resolution fix.

2. Engine — don't let a propagated target hijack the copy source (game/effects/copy_spell.rs)

Fix (b) alone is not sufficient. copy_source_entry short-circuits to a generic Object-target-on-stack lookup whenever ability.targets holds an Object — and resolve_ability_chain propagates the parent exile clause's chosen target ("exile target … card") onto the CopySpell sub-ability. That object is now in exile, not on the stack, so the lookup finds nothing and the ExiledBySource branch is never reached — the copy silently no-ops.

Dispatch the ExiledBySource / TrackedSet exile-linked sources before the propagated-Object stack lookup, so an exile-linked copy binds from the durable link regardless of a spuriously-propagated concrete target. Twincast-style "copy target spell" (a genuine on-stack Object target with a non-exile filter) is unaffected.

Blast radius — narrower than fix (b)

The issue flagged Evolving Door and Beseech the Mirror as collateral needing regression tests. My seam is narrower: it only touches the CopySpell "copy the exiled card" site, not the shared resolve_singular_exiled_card_target body. Those two cards use the CastFromZone "cast the exiled card" path and are provably unchanged — verified their parse still lowers to CastFromZone { TrackedSet(0) }.

Among all 8 "copy the exiled card" cards, only Saruman (the sole same-chain one) changes; the imprint-then-copy cards (Isochron Scepter, Elite Arcanist, Spellbinder, …) were already ExiledBySource and are unchanged.

Tests

  • Parser (oracle_trigger_tests.rs): Saruman's copy binds ExiledBySource (on main: TrackedSet(0)).
  • Runtime (copy_spell.rs): drives the real parsed ChangeZone{Graveyard→Exile}CopySpell{ExiledBySource} chain end-to-end — the targeted card is exiled and a copy of that card lands on the stack via the published link. Reverting either half flips it.
  • Full engine lib suite green: 16211 passed, 0 failed (all 36 existing copy_spell tests — Isochron/Twincast/Casualty — still pass).

Fixes #5576

…rs#5576)

Saruman's same-chain "exile target … card … Copy the exiled card" copied
the wrong card (or nothing). Two root causes, both fixed:

1. Parser (oracle_effect/imperative.rs): the "copy the exiled card"
   reference bound the copy to the chain-local TrackedSet(0) sentinel when
   an earlier same-chain clause was an exile producer. A plain-targeted
   ChangeZone{Exile} never publishes a TrackedSet, so copy_source_from_tracked_set
   fell back to a global scan (latest_tracked_set_id) and copied an unrelated
   set or nothing. Bind to ExiledBySource instead — in BOTH the same-chain and
   cross-resolution cases — which also makes the exile a linked consumer, so
   should_track_exiled_by_source publishes the ExileLink the copy reads. The
   distinct CastFromZone "cast the exiled card" sites (Evolving Door, Beseech
   the Mirror) keep their own bindings and are provably unchanged.

2. Engine (game/effects/copy_spell.rs): copy_source_entry short-circuited to a
   generic Object-target-on-stack lookup whenever ability.targets held an
   Object — but resolve_ability_chain propagates the parent exile clause's
   chosen target onto the CopySpell sub-ability, and that object is now in
   exile, not on the stack. The lookup found nothing and the exile-linked
   branch was never reached. Dispatch the ExiledBySource / TrackedSet
   exile-linked sources BEFORE the propagated-Object stack lookup so the copy
   binds from the durable link. Twincast-style "copy target spell" (a real
   on-stack Object target, non-exile filter) is unaffected.

Tests: parser assertion that Saruman's copy binds ExiledBySource; a runtime
discriminator driving the real parsed exile→copy chain end-to-end (copies the
exiled card via the published link). Full engine lib suite green (16211).

Fixes phase-rs#5576
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main 9c8de549a93a)

1 card(s) · ability/CopySpell · field target: tracked set #0cards exiled by source

Examples: Saruman of Many Colors

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@matthewevans

Copy link
Copy Markdown
Member

Right seam, real bug — held on one piece of missing evidence: the parse-diff.

✅ What's right

  • The root cause is correctly identified and the reorder is the correct fix. resolve_ability_chain propagates the parent exile clause's chosen target onto the CopySpell sub-ability — but by then that object is in exile, not on the stack. So the generic ability.targets → stack lookup in copy_source_entry found nothing and returned, meaning the exile-linked branch below it was never reached and the copy silently no-op'd. Hoisting the ExiledBySource / TrackedSet check above the stack lookup is the right repair, at the right seam.
  • The TrackedSet{0}ExiledBySource binding is correct. A plain-targeted ChangeZone{Exile} never publishes a chain-local TrackedSet, so the old sentinel resolved to nothing and copy_source_from_tracked_set fell back to a global scan. Binding to ExiledBySource makes the exile a linked consumer (should_track_exiled_by_source), publishing the ExileLink the copy actually reads.
  • CR 707.10 + CR 607.2a cited and annotated; the new resolver test is driven through the real parser and documented to flip on revert.

🔴 Blocker — required evidence is missing

The parse-diff sticky (<!-- coverage-parse-diff -->) is absent, and this PR changes a parser binding for a whole class. Card data (generate, validate, coverage) is still running, so the sticky hasn't been generated for head 6b4b155053.

This is not a formality. The change flips the binding for every "copy the exiled card" site from TrackedSet{0} to ExiledBySource, and the resolver reorder changes precedence for every CopySpell whose target references a tracked set — previously an Object target on the stack won; now the durable link wins. I can reason that Isochron Scepter (cross-resolution, no stack target) is unaffected, but "I can reason it's fine" is not the bar — the parse-diff is what shows which cards actually moved. Until it renders I can't confirm the blast radius is the one card you intended.

🟡 Non-blocking

  • resolve_singular_exiled_card_target(ctx.chain_has_prior_exile_producer, TargetFilter::ExiledBySource) now passes ExiledBySource for both the same-chain and cross-resolution arms, so at this call site the helper no longer discriminates on chain_has_prior_exile_producer — it's a pass-through. It still earns its keep at the CastFromZone sites, so leave it, but the dead branch at this site is worth a comment.

Recommendation: no action needed from you right now — this is waiting on CI, not on a fix. When Card data lands, I'll read the sticky. If it shows only Saruman (plus any genuinely-correct siblings), this is a straight approve.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved. The parse-diff rendered and it clears the bar I set — and I went one step further than the bar, because the parse-diff alone doesn't cover this change.

The evidence

Parse axis — tight. Sticky on 6b4b155053, baseline main 9c8de549a93a (current tip, not stale): 1 card, 1 signatureability/CopySpell field target: tracked set #0cards exiled by source, Saruman only. Zero collateral.

Resolver axis — the parse-diff cannot see this, so I bounded it by hand. Hoisting the exile/tracked check above the stack lookup changes precedence for every CopySpell whose target references a tracked set or exile link. The hoist is guarded (if let Effect::CopySpell { target, .. } + references_exiled_by_source() / references_tracked_set()), so from card-data.json on main the affected population is provably 9 cards and no others:

arcane bombardment · elite arcanist · isochron scepter · myra the magnificent · saruman of many colors · spellbinder · spellweaver helix · surge to victory · wizard's spellbook

Every other CopySpell target type (TriggeringSource ×134, SelfRef ×77, ParentTarget ×48, Or, StackAbility, And, Any, Typed, StackSpell) never enters the hoisted block and resolves exactly as before.

And the precedence semantics are already pinned by tests you didn't have to write. copy_spell.rs — the very file you reordered — carries two pre-existing guards:

  • copy_spell_tracked_set_copies_exiled_imprint_not_top_of_stack (:3035)
  • copy_spell_tracked_set_without_spell_ability_does_not_fallback_to_stack_top (:3114)

Both assert exile-link-beats-stack-top, both green on this head. The reorder makes the code match what those tests already demanded.

What's right

  • Root cause correctly identified, fixed at the right seam. resolve_ability_chain propagates the parent exile clause's chosen target onto the CopySpell sub-ability, but that object is in exile. The generic ability.targets → stack lookup found nothing and early-returned None, so the exile-linked branch below was never reached and the copy silently no-op'd.
  • CR 607.2a verified as governing, not merely real — it is precisely the linked-ability rule ("cards exiled with [this object]"), which is what makes ExiledBySource the correct binding rather than a convenient one. CR 707.10 correct for the copy itself.
  • The test drives parse_oracle_text — the entry point database::synthesis actually uses — with verbatim shipped text, and flips back to TrackedSet on revert. That is the discriminating form, not the vacuous one.

Non-blocking (do not respin)

resolve_singular_exiled_card_target(ctx.chain_has_prior_exile_producer, TargetFilter::ExiledBySource) now passes ExiledBySource on both arms, so at this call site the helper no longer discriminates on chain_has_prior_exile_producer — it's a pass-through. It still earns its keep at the CastFromZone sites; leave it.

No quality label — the PR needed a rework round. Labeled bug: this changes behavior from wrong to right.

@matthewevans matthewevans added the bug Bug fix label Jul 12, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 12, 2026
Merged via the queue into phase-rs:main with commit 3c43af9 Jul 12, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saruman of Many Colors: CopySpell reads a stale tracked set (same-chain targeted exile never publishes one)

2 participants