Zone pipeline: bucket-B/C tail migration — graveyard redirects + pause-safe battlefield entries - #2829
Merged
Merged
Conversation
… losers to graveyard through zone pipeline (CR 608.2n / 702.35a / 704.5j / 614.6) These three graveyard-destination moves delivered via raw move_to_zone, never proposing the inner ZoneChange, so board-wide Moved graveyard->exile redirects (Rest in Peace / Leyline of the Void) silently dropped: - cipher decline (CR 608.2n): the resolving cipher card -> owner's graveyard now routes through SpellResolutionDefault; handle_encode_choice returns the ZoneMoveResult so the caller surfaces a parked CR 616.1 prompt instead of clobbering it with Priority. - madness decline (CR 702.35a): the exiled card -> owner's graveyard now routes through move_object; the arm evaluates to the parked WaitingFor on pause so the post-action pipeline is skipped. - legend rule (CR 704.5j): the losing legends -> graveyards now route through move_objects_simultaneously (CR 603.10a co-departure stamp); a mid-batch CR 616.1 choice parks the prompt and stashes the tail. CR grep (docs/MagicCompRules.txt): 608.2n spell goes to graveyard on resolution; 702.35a madness; 704.5j legend rule; 614.6 replaced event never happens; 603.10a simultaneous leaves-battlefield.
…hrough zone pipeline (CR 614.1c) Both battlefield entries delivered via raw move_to_zone, skipping the pipeline delivery tail that applies enters-with-counters statics (StaticMode::EntersWithAdditionalCounters — Hardened Scales / Conclave Mentor 'creatures you control enter with an additional +1/+1 counter' class). The raw mover applies those statics nowhere, so an opened Attraction or a ninja entering via ninjutsu silently missed them. Route both through zone_pipeline::move_object. A battlefield-entry pause (CR 616.1 multi-redirect ordering / CR 303.4f aura host / counter- replacement) is not reachable for these object classes in the supported pool: an Attraction is a non-Aura artifact, a ninja is a non-Aura creature, and no supported entry surfaces a choice. The bail is a safety guard, not a resume path — ninjutsu's post-entry combat placement (CR 702.49c) cannot resume across a pause, so on the unreachable pause we stop with the prompt parked rather than act over parked state. Attribution self-anchors (CR 400.7; the raw move recorded no source). CR grep (docs/MagicCompRules.txt): 614.1c enters-with statics; 702.49c ninjutsu combat placement; 616.1 multi-replacement ordering.
…kept pause limitation Two comment-only corrections from the d5a12b8c6 review verdict: 1. zone_pipeline.rs deliver_batch aura arm: the note claiming a tail stashed on NeedsAuraAttachmentChoice would be 'silently drained by the NEXT unrelated replacement-choice resume' is stale. As of d5a12b8c6 the ReturnAsAuraTarget handler (engine.rs:3608-3611) and its chain-resume sibling (engine.rs:3572) both drain pending_batch_deliveries, so the aura-attachment resume finishes the parked batch correctly. 2. engine_resolution_choices.rs DigChoice kept-loop pause: document the multi-kept limitation — if kept card #1 pauses, kept #2+ stay in the library (the for-loop return exits before they move), and publish_tracked_set: Some(kept) publishes ALL kept including the unmoved ones, so a downstream sub-ability can be wired to cards still in the library. Pre-existing, strictly no-worse-than the old raw path; revisit with a kept-loop continuation if a 2+-kept-to-battlefield dig that pauses on the first is ever added. No behavior change.
…zone pipeline so Moved redirects fire (CR 614.6 / 701.20a / 603.10a) The reveal-until and dig 'put the rest into your graveyard' piles delivered via raw move_to_zone, never proposing the inner ZoneChange, so a board-wide Moved graveyard->exile redirect (Rest in Peace / Leyline of the Void) silently dropped on the rest cards. 12 card-data cards carry RevealUntil with rest_destination: Graveyard (Mind Funeral class), plus dig's no-kept-zone 'rest to graveyard' branch — all affected. move_rest is now the single authority: a Graveyard (or any non-library) rest pile routes through move_objects_simultaneously (CR 603.10a co-departure stamp), so each rest card consults the redirect; a Library rest pile keeps the random-order shuffle_to_bottom (no Moved-redirect class targets Library, and the placement arm would lose the shuffle). The new move_rest_then carries an optional BatchCompletion for callers that defer cleanup across a pause. Synchronous completion runs the resolver's own marker-clear + EffectResolved inline (the dispatching chain processor still owns priority/continuation). On a mid-pile CR 616.1 ordering pause, the prompt is parked and the cleanup is deferred onto a cleanup-only RevealRestPile completion (empty rest_cards — the pile IS the batch) so the drain runs it once and EffectResolved never lands over the parked prompt. The dig unkept->graveyard branch mirrors this, deferring finish_with_continuation via the same completion. Discriminating test reveal_until_graveyard_rest_redirected_to_exile_by_rest_in_peace: a graveyard-rest reveal-until with a RIP graveyard->exile Moved redirect on the battlefield now exiles the rest pile (old raw path: graveyard'd). CR grep (docs/MagicCompRules.txt): 614.6 replaced event never happens; 701.20a reveal until / rest pile; 603.10a simultaneous leaves-battlefield.
…pipeline (CR 708.3 / 614.1c)
Both face-down battlefield entries (cast face-down via morph/disguise, and
manifest) delivered via raw move_to_zone followed by a manual
apply_face_down_creature_characteristics + back_face snapshot — bypassing the
pipeline delivery tail, so a face-down 2/2 never received enters-with-counters
statics ('creatures you control enter with an additional +1/+1 counter' —
Hardened Scales / Conclave Mentor class).
Route both through zone_pipeline::move_object with the new
ZoneMoveRequest::face_down(profile) mod. The delivery tail is already the
canonical face-down authority (it snapshots the real face into back_face and
applies the vanilla-2/2 profile BEFORE the entry per CR 708.3, identical to
change_zone's face-down path) AND seeds enters-with-counters statics, so the
manual post-move override is dropped — morph/manifest now match the rest of
the engine's face-down entries. A battlefield-entry pause is unreachable for a
vanilla 2/2 (not an Aura, no Moved redirect / counter-replacement choice); the
bail keeps the helpers safe by construction.
CR grep (docs/MagicCompRules.txt): 708.2a face-down characteristics; 708.3
turned face down before it enters; 614.1c enters-with statics; 701.40a
manifest.
…CR 610.3a / 614.1c) check_exile_returns (Banisher Priest / Fiend Hunter / Oblivion Ring class — 'exile until ~ leaves') returned each exiled card via raw move_to_zone, skipping the pipeline delivery tail: a battlefield return missed enters-with- counters statics (Hardened Scales class), and a non-battlefield return dropped any Moved redirect. Group the returns by destination zone (first-seen order for determinism — Zone isn't Ord) and route each group through move_objects_simultaneously_then (CR 603.10a co-departure). A returned creature can pause on an as-enters / aura-host choice (CR 303.4f / 616.1), so the spent UntilSourceLeaves link cleanup rides a new BatchCompletion::RemoveExileLinks per group, drained once the group's pile lands (synchronously or via the replacement-choice / aura-attachment resume) — never before a paused card finished returning. Links for cards that already left exile by other means are dropped immediately; only the in-flight group ids ride their completion. CR grep (docs/MagicCompRules.txt): 610.3a return to previous zone; 614.1c enters-with statics; 603.10a simultaneous; 303.4f aura host on entry.
…pipeline; document dig rest-partition gap (CR 614.6 / 701.20a) A reveal-until KEPT card sent to the graveyard (4 cards, kept_destination: Graveyard — Mind Funeral-style 'put it into your graveyard') was delivered raw, skipping a Moved graveyard->exile redirect (Rest in Peace / Leyline of the Void). Route it through move_object on both the synchronous resolve path (the non-Hand/Battlefield kept branch; Library stays raw as a placement) and the RevealUntilKeptChoice handler (accept_zone / decline_zone via the new route_kept_card_or_defer helper). On a CR 616.1 pause the rest-pile move + marker clear defer onto a RevealRestPile completion; the kept-choice handler's rest pile now flows through move_rest_then so its completion (marker clear + finish_with_continuation) runs once on either path — closing the latent unhandled-pause the move_rest migration introduced here. Documents the remaining route_rest_partition gap (dig 'rest into graveyard', 65 Dig cards + the null->Graveyard default): it has a caller INSIDE run_batch_completion, so migrating it needs a re-pause-from-completion contract plus pause handling in both synchronous callers — a cross-cutting change tracked for a follow-up rather than a partial migration. CR grep (docs/MagicCompRules.txt): 614.6 replaced event never happens; 701.20a reveal until / rest pile.
…fallback consults Moved redirects (CR 708.3 / 608.3e / 614.6) Round-1 review findings 1 (HIGH) + 3 (MEDIUM) on the zone-pipeline tail migration — both in handle_replacement_choice: 1. The ZoneChange resume arm destructured the approved event with '..', DISCARDING face_down_profile, and delivered via the raw mover — a morph / manifest entry parked on a CR 616.1 ordering prompt resumed FACE UP, violating CR 708.3 and leaking the morpher's hidden card. The prompt is REACHABLE: two co-played external enter-tapped Moved effects (Authority of the Consuls + Imposing Sovereign class) collide on the entry's tap field (no same-value dedupe), surfacing the ordering choice — empirically proven by the new test's parked-prompt assertion. Fix: extract the tail's CR 708.3 block into zone_pipeline::apply_face_down_entry_profile (single authority, not a mirrored copy) and call it from the resume arm immediately after the move, before the tap/controller/counter blocks (tail ordering). Full tail-routing via approve_post_replacement + deliver was assessed and deferred to the Phase-B token migration with a flagged TODO: the arm's epilogue drains post_replacement_continuation with spell-resolution ctx + post_replacement_source clearing, orders pending_spell_resolution differently, and carries the bespoke played_from_zone preservation (PLAN Open Question #3) — three behavioral divergences that need their own reconciliation, not a drive-by. Morph/manifest 'pause unreachable' comments replaced with honest reachability documentation (the bail is complete: the profile rides the parked event; the resume applies it). 2. The CR 608.3e prevented-ETB graveyard fallback delivered raw. The consulted (prevented) event was the battlefield ENTRY; the fallback is a fresh, never-consulted event, so routing it through move_object (SpellResolutionDefault, mirroring stack.rs's C2 prevented-permanent site) cannot double-apply — the prevention def is Battlefield-scoped and cannot re-match a Graveyard move. RIP/Leyline redirects now fire on the discarded spell. The dead pending_continuation is cleared before the move so a CR 616.1 pause cannot leave it for the next resume's epilogue. Fail-first evidence (both red before the fix, green after): - paused_face_down_morph_entry_resumes_face_down: panicked 'resumed morph entry must be FACE DOWN (CR 708.3)' - prevented_etb_graveyard_fallback_consults_moved_redirects: left: Graveyard, right: Exile (staging note in-test: no ZoneChange applier can yield Prevented, so the parked choice is staged as a regeneration-shield Destroy prevention; the resume is driven through the real GameAction entry) CR grep (docs/MagicCompRules.txt): 708.2a face-down characteristics; 708.3 turned face down before it enters; 608.3e prevented ETB to graveyard; 614.6 replaced event never happens; 616.1 multiple replacement ordering.
…ions instead of dropping post-entry work (CR 702.49 / 701.51 / 616.1) Round-1 review finding 2 (MEDIUM): both 3d1497411 bails were wrong because the battlefield-entry prompt IS reachable — two co-played external enter-tapped Moved effects (Authority of the Consuls + Imposing Sovereign for creatures; Kismet / Frozen Aether class for artifacts) produce a material same-field collision on the entry's tap state (no same-value dedupe) and surface a CR 616.1 ordering prompt. On that board: - ninjutsu: the bail skipped the cast-variant provenance tag AND the CR 702.49c tapped-and-attacking combat placement — the resumed ninja entered untagged and non-attacking. - Attraction open: the bail left in_attraction_deck set, never emitted AttractionOpened, and dropped every remaining open of the instruction. Adjudication: continuations, not documented limitation — the BatchCompletion infrastructure already exists (RevealRestPile / RemoveExileLinks shape). Two new variants: NinjutsuPlacement (defers finish_ninjutsu_entry: tag + combat placement + NinjutsuActivated + layers) and AttractionOpenRemainder (defers finish_attraction_open + the remaining opens, which may themselves re-park and re-defer through the same completion — the drain takes the old record before running it, so a fresh park is preserved). Both finish helpers are shared single authorities between the synchronous and resumed paths. The 'pause unreachable' comments are replaced with honest reachability documentation. Bonus CR-correctness in the shared helper: AttractionOpened now fires only when the card actually entered the battlefield (CR 701.51c — prevented or replaced entries must not trigger 'opens an Attraction'; Done also covers prevented/redirected deliveries). Also corrects the CR 610.3a cite to CR 610.3 proper on the RemoveExileLinks doc + completion arm (610.3a is the already-occurred-event timing subpart; 610.3 is the return rule). Fail-first evidence (both red before, green after): - paused_ninjutsu_entry_resumes_with_combat_placement_and_tag: panicked 'resumed ninja must be placed attacking (CR 702.49c)' - paused_attraction_open_resumes_bookkeeping_and_remaining_opens: panicked 'open bookkeeping must run on the resumed Attraction (old bail left the flag set)' Both tests passed their parked-prompt assertions pre-fix, empirically confirming the reviewer's reachability claim for both object classes. CR grep (docs/MagicCompRules.txt): 702.49/49a/49c ninjutsu + placement; 701.51/51b/51c open an Attraction + trigger gate; 616.1 ordering; 610.3 return-to-previous-zone.
…urn cite to CR 610.3 (review finding 4 + nit) Round-1 review finding 4 (MEDIUM-LOW, comment-only): execute_zone_move's replacement consult runs the matcher pass against the object's PRINTED characteristics, but CR 614.12 (docs/MagicCompRules.txt:3094) requires checking 'the characteristics of the permanent as it would exist on the battlefield' — for a face-down (morph/manifest) entry that is the 2/2 with no name/types/subtypes (CR 708.2a), so a type- or name-keyed entry replacement wrongly matches a face-down printed Wizard. Narrow class today (the common enter-tapped/counter statics are type-agnostic or creature-scoped, which the face-down 2/2 satisfies); the fix is profile-projected characteristics in the matcher pass when face_down_profile is present. Documented at the consult with the CR cite. Also corrects check_exile_returns' CR 610.3a cite to CR 610.3 proper (610.3a is the already-occurred-event timing subpart; 610.3 is the rule that creates the return one-shot). CR grep: 614.12 'check the characteristics of the permanent as it would exist on the battlefield'; 610.3 'A second one-shot effect ... returns the object to its previous zone.' No behavior change.
…ction CR 701.51c gate (CR 702.49c) Round-2 review LOW: finish_ninjutsu_entry ran the cast-variant tag and the CR 702.49c combat placement unconditionally — ZoneMoveResult::Done also covers prevented/redirected deliveries, so a redirected resumed entry would tag a non-battlefield object and place it into combat.attackers. Gate both behind the same zone == Battlefield arrival check as finish_attraction_open. Unreachable today (no supported Moved redirect retargets a battlefield entry away from the battlefield), but the gate makes the helper correct by construction rather than by census, matching the twin's standard. NinjutsuActivated stays deliberately OUTSIDE the gate, unlike the Attraction twin's AttractionOpened: CR 701.51c explicitly suppresses the 'opens an Attraction' trigger when the entry is prevented/replaced, but ninjutsu's activation event occurred when the ability was activated (cost paid, attacker returned) — a redirected entry does not un-activate it. Asymmetry documented inline. No new test per the review (unreachable class, identical behavior for every reachable entry — existing paused_ninjutsu_entry_resumes_with_combat_placement _and_tag still green). CR grep (docs/MagicCompRules.txt): 702.49c enters attacking; 701.51c trigger suppression on prevented/replaced entry.
matthewevans
enabled auto-merge
June 10, 2026 18:07
matthewevans
disabled auto-merge
June 10, 2026 18:08
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates several manual zone-change operations to route through the authoritative zone-change pipeline (zone_pipeline), ensuring strict fidelity to the MTG Comprehensive Rules (CR 614.6) regarding Moved redirects (such as Rest in Peace or Leyline of the Void). Specifically, it updates Attractions, cipher choices, reveal-until effects, declined madness cards, exile-until-leaves returns, prevented ETB fallbacks, legend-rule SBAs, and ninjutsu entries to use the pipeline. It also fixes a critical bug where face-down entries (morph/manifest) that paused on replacement-ordering prompts resumed face-up by preserving the face_down_profile in the resume path. Since there are no review comments, I have no feedback to provide.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Zone-change pipeline bucket-B/C tail migration: the remaining raw
zones::move_to_zoneeffect-site callers that skip replacement consults are now routed through the pipeline. 11 commits squashed; follows up #2824.Bug class fixed
Movedredirects (Rest in Peace / Leyline of the Void — 127 live replacement definitions): declined cipher/madness, legend-rule losers (CR 704.5j), reveal-until/dig rest piles and kept-to-graveyard cards. Discriminating test pins the RIP redirect on a reveal-until rest pile.Pause-correctness (CR 616.1 prompts are reachable on any entry via two co-played enter-tapped effects)
apply_face_down_entry_profileused by both the delivery tail and the replacement-choice resume arm (previously the resume dropped the face-down profile — hidden-information leak).BatchCompletioncontinuations (NinjutsuPlacement,AttractionOpenRemainder) instead of dropping combat placement / open bookkeeping on pause.AttractionOpenedis now gated on actual battlefield arrival (CR 701.51c); ninjutsu post-entry work got the symmetric arrival gate (CR 702.49c), withNinjutsuActivateddeliberately ungated (activation already happened at cost time).BatchCompletion::RemoveExileLinksfor exile-until-leaves return batches.Documented gaps (flagged, not fixed here)
route_rest_partitionre-pause-from-completion contract; bucket-A post-replacement deliveries (Phase-B token migration); stack.rs permanent-resolution site (PLAN OQ#3).Verification
cargo check --workspace --all-targetsgreen in the ship worktree against current origin/main.🤖 Generated with Claude Code