ship/t157 l1 library#6048
Conversation
matthewevans
commented
Jul 16, 2026
- refactor(engine): route effect-owned library placement through the zone pipeline (L1)
- chore: tighten zone-authority baseline after L1 library-placement migration
…ne pipeline (L1) Migrate the three simple effect-owned library placement paths onto ZoneMoveRequest::effect(..).at_library_position(..) with the existing batch carrier owning plural placement across replacement pauses: - cascade::shuffle_to_bottom (CR 702.85a + CR 401.4): randomized bottom batch via move_objects_simultaneously_then; the no-hit CascadeMissed + EffectResolved tail rides a new BatchCompletion::CascadeBottomComplete so it fires exactly once after every placement (including redirected ones) settles. The helper now takes the cascading source explicitly; ResolutionCastCleanup and the Cascade/Discover/Ripple cast offers carry a typed source_id instead of self-attributing misses. - discover::shuffle_to_bottom (CR 701.57a + CR 401.4): same batch shape; no-hit, declined (DiscoverDeclined keeps the raw hit-to-hand instruction after the paused miss batch), and rejected-cast tails each complete exactly once behind their placement batch. - put_on_top::resolve (CR 701.24a + CR 401.4): both ordering branches build placement-ordered batch requests (Top reversed at construction); BeneathTop depth is frozen to a concrete value before parking so a CR 616.1 pause carries a resolved placement; exile-link cleanup and the resolution event moved into BatchCompletion::PutOnTopComplete. Effect tranche only: no PendingCostMoveResume or PostReplacementContinuation changes; the rejection path's hit-to-hand move stays raw for the R1 unit. Witnesses: library-redirect pause (CR 616.1 dual synthetic Moved defs), prevented/declined tails, order retention, and synchronous regression coverage for all three sites in cost_zone_pipeline.
There was a problem hiding this comment.
Code Review
This pull request refactors library-placement effects (Cascade, Discover, Ripple, and PutAtLibraryPosition) to route through the replacement-aware library-placement pipeline, ensuring that resolution and completion events wait for randomized placements to settle under CR 616.1 and CR 401.4. It introduces several new BatchCompletion variants to handle these deferred completions. The review feedback correctly identifies a violation of Rule R6, pointing out that the newly added batch completion handlers in run_batch_completion lack mandatory CR annotations.
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.
| BatchCompletion::CascadeBottomComplete { | ||
| controller, | ||
| source_id, | ||
| exiled_count, | ||
| } => { | ||
| events.push(GameEvent::CascadeMissed { | ||
| controller, | ||
| source_id, | ||
| exiled_count, | ||
| }); | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::Cascade, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| } | ||
| BatchCompletion::DiscoverBottomComplete { source_id } => { | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::Discover, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| } | ||
| BatchCompletion::DiscoverPlacementComplete { source_id } => { | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::Discover, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| } | ||
| BatchCompletion::DiscoverDeclined { | ||
| player, | ||
| hit_card, | ||
| source_id, | ||
| } => { | ||
| zones::move_to_zone(state, hit_card, Zone::Hand, events); | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::Discover, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| finish_with_continuation(state, player, events); | ||
| } | ||
| BatchCompletion::ResolutionCastRejectedToHand { | ||
| player, | ||
| hit_card, | ||
| source_id, | ||
| } => { | ||
| zones::move_to_zone(state, hit_card, Zone::Hand, events); | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::Discover, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| crate::game::priority::clear_priority_passes(state); | ||
| state.waiting_for = WaitingFor::Priority { player }; | ||
| } | ||
| BatchCompletion::PutOnTopComplete { | ||
| source_id, | ||
| removed_exile_links, | ||
| } => { | ||
| state.exile_links.retain(|link| { | ||
| link.source_id != source_id || !removed_exile_links.contains(&link.exiled_id) | ||
| }); | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::PutAtLibraryPosition, | ||
| source_id, | ||
| subject: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[HIGH] Missing mandatory CR annotations for rules-touching batch completion handlers. Evidence: crates/engine/src/game/engine_resolution_choices.rs:5394-5463.
Why it matters: Under Rule R6 of the phase.rs style guide, every rules-touching line of engine code must carry a comment of the form CR <number>: <description> to ensure strict fidelity to the Magic Comprehensive Rules. When moving the cascade, discover, and top-placement completion logic to run_batch_completion, the original CR annotations (such as CR 406.6 for dropping exile links) were omitted.
Suggested fix: Add the verified CR annotations to each match arm in run_batch_completion.
BatchCompletion::CascadeBottomComplete {
controller,
source_id,
exiled_count,
} => {
// CR 702.85a: A no-hit Cascade's randomized bottom batch has settled, so its completion events may fire.
events.push(GameEvent::CascadeMissed {
controller,
source_id,
exiled_count,
});
events.push(GameEvent::EffectResolved {
kind: EffectKind::Cascade,
source_id,
subject: None,
});
}
BatchCompletion::DiscoverBottomComplete { source_id } => {
// CR 701.57a: A no-hit Discover's randomized bottom batch has settled, so the discover resolution completes.
events.push(GameEvent::EffectResolved {
kind: EffectKind::Discover,
source_id,
subject: None,
});
}
BatchCompletion::DiscoverPlacementComplete { source_id } => {
// CR 701.57a: A Discover that found a card has finished placing every miss after its cast/hand decision.
events.push(GameEvent::EffectResolved {
kind: EffectKind::Discover,
source_id,
subject: None,
});
}
BatchCompletion::DiscoverDeclined {
player,
hit_card,
source_id,
} => {
// CR 701.57a: A declined Discover's miss batch settled; move the hit card to hand.
zones::move_to_zone(state, hit_card, Zone::Hand, events);
events.push(GameEvent::EffectResolved {
kind: EffectKind::Discover,
source_id,
subject: None,
});
finish_with_continuation(state, player, events);
}
BatchCompletion::ResolutionCastRejectedToHand {
player,
hit_card,
source_id,
} => {
// CR 608.2g / CR 701.57a: A Discover cast rejected at finalization waits for the miss batch before its hit-to-hand instruction.
zones::move_to_zone(state, hit_card, Zone::Hand, events);
events.push(GameEvent::EffectResolved {
kind: EffectKind::Discover,
source_id,
subject: None,
});
crate::game::priority::clear_priority_passes(state);
state.waiting_for = WaitingFor::Priority { player };
}
BatchCompletion::PutOnTopComplete {
source_id,
removed_exile_links,
} => {
// CR 406.6: The exiled cards left the exile zone — drop their source links.
state.exile_links.retain(|link| {
link.source_id != source_id || !removed_exile_links.contains(&link.exiled_id)
});
events.push(GameEvent::EffectResolved {
kind: EffectKind::PutAtLibraryPosition,
source_id,
subject: None,
});
}References
- Every rules-touching line of engine code must carry a comment of the form CR : (regex CR \d{3}(.\d+[a-z]?)?). (link)
- Avoid omitting mandatory CR annotations on rules-touching code blocks to maintain strict fidelity to the Magic Comprehensive Rules.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…ast_success, if/else for removed_exile_links PR #6048 queue CI failed on two -D warnings lints introduced by the L1 library-placement migration: handle_resolution_cast_success grew to 9 args (allow attribute per existing repo precedent) and an obfuscated_if_else in put_on_top.rs (rewritten as a plain if/else).