Zone pipeline Phase B: bucket-A deliveries onto the ApprovedZoneChange proof token - #2837
Merged
Conversation
…e fork sync Add .github/PULL_REQUEST_TEMPLATE.md with a required check-one block recording whether the change went through /engine-implementer (and if not, why — frontend, docs/CI, or out-of-scope fix). Heading set deliberately omits the ## CR references + ## Track pair so it doesn't trip the AI-contributor labeler. Add AI-CONTRIBUTOR.md §2.1: sync the fork with upstream on every run (ff-only merge of upstream/main, best-effort push), and align the self-contained Appendix B.3 prompt to fetch upstream and cut its branch from upstream/main.
…m into token delivery (CR 614.12a / 614.1c / 616.1) The handle_replacement_choice ZoneChange arm was a divergent partial copy of deliver_replaced_zone_change (the pipeline-copy smell this project exists to kill — it had already dropped played_from_zone and face_down_profile, and still skipped the devour snapshot, EntersWithAdditionalCounters statics, attach_to, entered_via_ability_source, exile-link tracking, and the CR 701.24a library-shuffle arm). It is now dissolved: the post-choice event is a ReplacementResult::Execute payload, sealed through the third mint path (ApprovedZoneChange::approve_post_replacement, PLAN §6.2) and delivered by the shared zone_pipeline::deliver — the consult-once/deliver-once contract is structural for the resume path. Divergence reconciliation (PLAN §7 Phase B), by parameterizing the shared tail rather than keeping two copies: 1. New PostReplacementDrainOwner (types/game_state.rs) rides DeliveryCtx and the ContinueZoneDeliveryTail deferred-tail record (#[serde(default)] = DeliveryTail for old saves): DeliveryTail = the tail drains post_replacement_continuation ctx-less (every direct delivery, unchanged); CallerEpilogue = the tail skips it and the resume epilogue keeps draining WITH the spell-resolution ctx and post_replacement_source cleared for zone changes. A counter-pause mid-resume stashes CallerEpilogue in the deferred record, so the NEXT resume's epilogue still owns the drain — matching the pre-token double-resume behavior. 2. pending_spell_resolution ordering untouched: apply_pending_spell_resolution still runs in the epilogue before that drain. 3. played_from_zone (PLAN OQ#3, UNDECIDED — no adjudication): captured pre-move by the resume caller and ridden on DeliveryCtx; deliver re-stamps it after a battlefield delivery (including the counter-pause outcome, where the pre-token arm restored before the counter application). Both OQ#3 resolutions slot into this one site. Resumed entries now receive the full delivery tail. Fail-first evidence: resumed_entry_receives_enters_with_additional_counters_static — a battlefield entry parked on the two-enter-tapped CR 616.1 prompt resumed WITHOUT the 'other creatures you control enter with an additional +1/+1 counter' static (left: 0, right: 1); green after. Regression net all green: paused face-down morph, ninjutsu, attraction, prevented-ETB, mill/seek/bounce batch resumes (full test-engine + test-ai). CR grep (docs/MagicCompRules.txt): 614.12a entry-replacement application; 614.1c enters-with; 616.1 ordering; 701.24a shuffle; 303.4f aura host; 603.6a ETB trigger provenance.
… through the token path (CR 614.1c / 614.12a) The land-play `Execute` arm (handle_play_land) was a divergent partial copy of deliver_replaced_zone_change: it raw-moved via zones::move_to_zone and hand-applied only enter_tapped / controller_override / the event's own enter_with_counters / pending_etb_counters, SKIPPING the shared delivery tail's CR 614.1c EntersWithAdditionalCounters statics snapshot, CR 303.4f attach_to host, entered_via_ability_source provenance (CR 603.6a), and the CR 701.24a library-shuffle arm. Dissolved per PLAN §6.2/§7: the post-replace_event event is a ReplacementResult::Execute payload, sealed through approve_post_replacement and delivered by zone_pipeline::deliver. drain = CallerEpilogue: the land-play epilogue owns the post_replacement_continuation drain (clears post_replacement_source, runs the land-specific accounting), so the shared tail must not also drain it — mirroring the d0a593d0e resume arm. played_from_zone is set FRESH after delivery via mark_land_played_from_zone (recording a brand-new origin, not preserving a pre-move value), so the ctx re-stamp knob stays None; the counter-pause NeedsChoice arm stamps it before parking, matching the pre-token arm that stamped before the apply_etb_counters early-return. Fail-first evidence: played_land_receives_enters_with_additional_counters_static — a Forest played through the real PlayLand action while an active "other permanents you control enter with an additional +1/+1 counter" static is functioning. Old land-play arm applied only the event's own enter_with_counters and dropped the statics snapshot (counter = 0); through the shared tail the played land now receives it (counter = 1). CR grep (docs/MagicCompRules.txt): 614.1c enters-with seeds (line 3056); 614.6 replaced event never happens (line 3072); 305.1 playing a land (line 1688).
…rough the token path (CR 614.1 / 614.6) pay_top_library_exile_cost (Thought Lash cumulative-upkeep class) consulted replace_event per card then DEGRADED each survivor to an (object_id, to) pair, delivering via raw zones::move_to_zone — discarding the full post-replacement ProposedEvent payload (the event's applied: HashSet<ReplacementId> and every field the delivery tail reads). Per PLAN §6.2: stash the full ProposedEvents and seal each through ApprovedZoneChange::approve_post_replacement, then deliver via zone_pipeline::deliver — a consult-skipping approved delivery. Never re-proposes through move_object (which would discard `applied` and double-apply the Moved definitions already applied this pass). Structural fix (consult-once/deliver-once), not a behavior change: a plain Library → Exile cost has no battlefield-entry mods to apply, the redirected destination was already honored pre-migration (the `to` field was captured from the Execute event), and the delivery tail's post_replacement_continuation drain early-returns for the Exile destination (apply_zone_delivery_tail: `to == Exile` with a source attribution and no exile-link returns Done before the drain) — so no enter-mod or continuation behavior is newly observable here. ctx: source_id = the event's cause (preserving the proposal's Some(source_id)); played_from_zone = None (not a battlefield entry); drain = DeliveryTail (this synchronous cost path has no caller epilogue). The Library → Exile destination cannot surface a CR 614.1c counter-pause, so the NeedsChoice arm is a debug_assert guard. Regression test: top_library_exile_cost_exiles_top_card_through_sealed_delivery pins the observable outcome (top card exiled, left the library) across both the old raw delivery and the new sealed one. CR grep (docs/MagicCompRules.txt): 614.1 replacement effects (line 3050); 614.6 a replaced event never happens (line 3072).
matthewevans
enabled auto-merge
June 10, 2026 19:58
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements Phase B of the zone-pipeline migration, unifying land play, top-library exile cost payment, and replacement-choice resume paths with the shared zone_pipeline::deliver machinery to ensure they receive the full delivery tail. Feedback on the changes highlights a potential state corruption issue in release builds where a debug assertion is used instead of returning an error during cost payment. Additionally, several missing mandatory CR 305.1 annotations for land play origin bookkeeping were identified, violating repository style guide rule R6.
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.
Comment on lines
+5288
to
+5292
| // Stamp the play origin (engine bookkeeping for "where it was | ||
| // played from"). Set fresh AFTER delivery — the ctx re-stamp | ||
| // knob is for preserving a pre-move value, which this site does | ||
| // not have (it is recording a brand-new origin). | ||
| mark_land_played_from_zone(state, object_id, origin_zone); |
Contributor
There was a problem hiding this comment.
[MED] Add mandatory CR 305.1 annotation for land play origin bookkeeping. Evidence: crates/engine/src/game/engine.rs:5288.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number> comment to adhere to R6 of the repository style guide. Suggested fix: Add CR 305.1 annotation to the mark_land_played_from_zone call.
Suggested change
| // Stamp the play origin (engine bookkeeping for "where it was | |
| // played from"). Set fresh AFTER delivery — the ctx re-stamp | |
| // knob is for preserving a pre-move value, which this site does | |
| // not have (it is recording a brand-new origin). | |
| mark_land_played_from_zone(state, object_id, origin_zone); | |
| // CR 305.1: Stamp the play origin (engine bookkeeping for "where it was | |
| // played from") after putting the land onto the battlefield. Set fresh | |
| // AFTER delivery — the ctx re-stamp knob is for preserving a pre-move | |
| // value, which this site does not have (it is recording a brand-new origin). | |
| mark_land_played_from_zone(state, object_id, origin_zone); |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Member
Author
There was a problem hiding this comment.
Added in 004db8d (same fix as the sibling comment — both the parked-prompt and synchronous stamp sites are annotated with CR 305.1 + CR 400.7i).
…d-play provenance (CR 305.1 + CR 400.7i) — Gemini review
matthewevans
enabled auto-merge
June 10, 2026 20:40
matthewevans
disabled auto-merge
June 10, 2026 20:42
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 Phase B: all production bucket-A post-replacement delivery sites now route through the
ApprovedZoneChangeproof token + sharedzone_pipeline::delivertail — consult-once/deliver-once is structural, not conventional. Follows #2824/#2829. Also adds the PR template mandating engine-implementer attestation.The centerpiece
handle_replacement_choice's ZoneChange resume arm — previously a divergent partial copy of the delivery tail that had already silently dropped two fields (face-down profile, batch attribution) — is dissolved. It now seals the post-replacement event viaApprovedZoneChange::approve_post_replacement(private fields +_seal; re-consult impossible by construction) and delivers through the shared tail. Resumed entries now receive the full tail: devour snapshot,EntersWithAdditionalCountersstatics (CR 614.1c),attach_to,entered_via_ability_source, exile-link tracking, library auto-shuffle (CR 701.24a).New machinery:
PostReplacementDrainOwner(DeliveryTail/CallerEpilogue) ridesDeliveryCtxand the deferred-tail record, so resume-path callers keep their epilogue's ctx-ful continuation drain without duplicating the tail. The counter-pause double-resume semantic is preserved and regression-pinned.Site migrations
handle_play_landExecute arm): raw delivery + hand-applied enter-mods → token delivery. Lands now receive enters-with-counters statics (fail-first test: counter 0 → 1).mark_land_played_from_zonestamps a fresh origin after delivery on both synchronous and counter-pause paths (CR 305.1).pay_top_library_exile_cost): the(object_id, zone)degradation — which discarded post-replacement payloads andappliedsets — replaced by sealing fullProposedEvents. Verified purely structural (the Exile delivery tail early-return predates this change and is preserved exactly).Status
The sole remaining production bucket-A raw delivery is
stack.rs:757, parked on a design decision (played_from_zoneprovenance, PLAN OQ#3). Independent post-rebase census: zero genuinely-unmigrated production sites with live replacement exposure remain; the residual ~36 raw callers are all documented deferral classes (hand/exile no-op-consult, library placements, merged-permanent routing).Verification
cargo check --workspace --all-targetsgreen in the ship worktree against current origin/main.🤖 Generated with Claude Code