ship/t164 put at library position split#6085
Conversation
matthewevans
commented
Jul 17, 2026
matthewevans
commented
- fix(engine): route hand-origin PutAtLibraryPosition deliveries through the zone pipeline
- chore(engine): refresh zone-authority baseline after PutAtLibraryPosition split (43 hits / 32 rows; reviewed exempt reclassifications)
…h the zone pipeline The mixed-source EffectZoneChoice arm for PutAtLibraryPosition delivered chosen cards with raw library movers, bypassing replacement effects entirely: a "if a card would be put into your library" replacement never applied to Brainstorm-class put-backs, and the arm's tail (tracked set, EffectResolved, continuation drain) could never pause. Partition the chosen cards by their current zone at delivery time: - Cards outside the library are real zone changes (CR 401.4) and now route through move_objects_simultaneously_then with .at_library_position(...) requests and a typed BatchCompletion::EffectZonePutAtLibraryPositionComplete tail, so CR 614.1/616.1 replacement choices pause the arm and the tail runs exactly once after delivery settles. - Cards already in the library are in-zone repositions, not zone-change events — they stay raw, annotated `allow-raw-zone`, and are re-placed after batch delivery by replaying the original placement order (CR 608.2c) so mixed hand/library selections produce the identical final library order as the old synchronous path (exact-order witness for Top, Bottom, and NthFromTop). Also applies the three reviewed exemption annotations for in-library reorders that must NOT migrate (they never leave the library, so no zone event exists to replace): scry reorder (CR 701.22a), clash placement (CR 701.30a + CR 701.20b), and dig looked-at container mutations (CR 701.20b/e). The zone-authority census keeps all 43 hits; the annotated sites reclassify into the ratcheted `exempt` family and the baseline is tightened accordingly. Witnesses: hand-origin redirect pauses before the tail (competing CR 616.1 replacements, chained life-gain rider stays parked, terminal event exactly once) plus the mixed-source exact-order regression, in cost_zone_pipeline.
…tion split (43 hits / 32 rows; reviewed exempt reclassifications)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a robust handling mechanism for EffectKind::PutAtLibraryPosition in the game engine, ensuring that mixed hand and library sources are correctly ordered and that replacement effects are properly applied before terminal effects resolve. It introduces several helper functions for managing library placements, updates the BatchCompletion state, and adds comprehensive integration tests. The review feedback highlights critical safety concerns regarding direct map indexing with state.objects[&card_id] in the new helper functions, which could lead to engine panics, and notes several missing mandatory CR annotations required by the repository's style guide.
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.
| let mut owners = Vec::new(); | ||
| for &card_id in chosen { | ||
| let owner = state.objects[&card_id].owner; | ||
| if !owners.contains(&owner) { | ||
| owners.push(owner); | ||
| } | ||
| } |
There was a problem hiding this comment.
[HIGH] Unsafe map indexing in
effect_zone_non_library_delivery_order. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5674.
Why it matters: Direct indexing withstate.objects[&card_id]will panic if the card is not present in the game state, which can crash the engine.
Suggested fix: Use.get(&card_id)defensively to safely retrieve the owner.
| let mut owners = Vec::new(); | |
| for &card_id in chosen { | |
| let owner = state.objects[&card_id].owner; | |
| if !owners.contains(&owner) { | |
| owners.push(owner); | |
| } | |
| } | |
| let mut owners = Vec::new(); | |
| for &card_id in chosen { | |
| if let Some(owner) = state.objects.get(&card_id).map(|obj| obj.owner) { | |
| if !owners.contains(&owner) { | |
| owners.push(owner); | |
| } | |
| } | |
| } |
References
- Specifically enforce defensive programming: for languages with strict null safety or option types (e.g., Rust), ensure idiomatic handling (e.g., pattern matching, optional chaining) is used. Ensure that invalid inputs or states are safely handled in all cases.
| for card_id in effect_zone_library_placement_order(chosen, library_position) { | ||
| if state.objects[&card_id].owner != owner || !placed.contains(&card_id) { | ||
| continue; | ||
| } | ||
| library.retain(|id| *id != card_id); |
There was a problem hiding this comment.
[HIGH] Unsafe map indexing in
replay_effect_zone_library_placement. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5726.
Why it matters: Direct indexing withstate.objects[&card_id]will panic if the card is not present in the game state, which can crash the engine.
Suggested fix: Use.get(&card_id)defensively to safely retrieve the owner.
for card_id in effect_zone_library_placement_order(chosen, library_position) {
let Some(obj) = state.objects.get(&card_id) else {
continue;
};
if obj.owner != owner || !placed.contains(&card_id) {
continue;
}
library.retain(|id| *id != card_id);References
- Specifically enforce defensive programming: for languages with strict null safety or option types (e.g., Rust), ensure idiomatic handling (e.g., pattern matching, optional chaining) is used. Ensure that invalid inputs or states are safely handled in all cases.
| for &card_id in library_origin { | ||
| let owner = state.objects[&card_id].owner; | ||
| if !owners.contains(&owner) { | ||
| owners.push(owner); | ||
| } | ||
| } |
There was a problem hiding this comment.
[HIGH] Unsafe map indexing in
reposition_library_origins_after_batch_delivery. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5800.
Why it matters: Direct indexing withstate.objects[&card_id]will panic if the card is not present in the game state, which can crash the engine.
Suggested fix: Use.get(&card_id)defensively to safely retrieve the owner.
let mut owners = Vec::new();
for &card_id in library_origin {
if let Some(owner) = state.objects.get(&card_id).map(|obj| obj.owner) {
if !owners.contains(&owner) {
owners.push(owner);
}
}
}References
- Specifically enforce defensive programming: for languages with strict null safety or option types (e.g., Rust), ensure idiomatic handling (e.g., pattern matching, optional chaining) is used. Ensure that invalid inputs or states are safely handled in all cases.
| EffectKind::PutAtLibraryPosition => { | ||
| let library_position = match library_position { | ||
| Some(LibraryPosition::Bottom) => LibraryPosition::Bottom, | ||
| Some(LibraryPosition::NthFromTop { n }) => { | ||
| LibraryPosition::NthFromTop { n } | ||
| } | ||
| } | ||
| Some(LibraryPosition::NthFromTop { n }) => { | ||
| let index = Some(n.saturating_sub(1) as usize); | ||
| for &card_id in &chosen { | ||
| super::zones::move_to_library_at_index(state, card_id, index, events); | ||
| } | ||
| } | ||
| _ => { | ||
| for &card_id in chosen.iter().rev() { | ||
| super::zones::move_to_library_at_index(state, card_id, Some(0), events); | ||
| _ => LibraryPosition::Top, | ||
| }; |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation on
EffectKind::PutAtLibraryPositionmatch arm. Evidence:crates/engine/src/game/engine_resolution_choices.rs:4116.
Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add aCR 401.4annotation to the match arm.
// CR 401.4: If an effect puts two or more cards on the top or bottom of a library at the same time, the owner of those cards may arrange them in any order.
EffectKind::PutAtLibraryPosition => {
let library_position = match library_position {
Some(LibraryPosition::Bottom) => LibraryPosition::Bottom,
Some(LibraryPosition::NthFromTop { n }) => {
LibraryPosition::NthFromTop { n }
}
_ => LibraryPosition::Top,
};References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| fn effect_zone_library_placement_order( | ||
| chosen: &[ObjectId], | ||
| library_position: &LibraryPosition, | ||
| ) -> Vec<ObjectId> { |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation on
effect_zone_library_placement_order. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5652.
Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add aCR 401.4annotation to the function.
/// CR 401.4: If an effect puts two or more cards on the top or bottom of a library at the same time, the owner of those cards may arrange them in any order.
fn effect_zone_library_placement_order(
chosen: &[ObjectId],
library_position: &LibraryPosition,
) -> Vec<ObjectId> {References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| fn replay_effect_zone_library_placement( | ||
| state: &GameState, | ||
| owner: crate::types::player::PlayerId, | ||
| mut library: Vec<ObjectId>, | ||
| chosen: &[ObjectId], | ||
| placed: &[ObjectId], | ||
| library_position: &LibraryPosition, | ||
| ) -> Vec<ObjectId> { |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation on
replay_effect_zone_library_placement. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5718.
Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add aCR 401.4annotation to the function.
/// CR 401.4: If an effect puts two or more cards on the top or bottom of a library at the same time, the owner of those cards may arrange them in any order.
fn replay_effect_zone_library_placement(
state: &GameState,
owner: crate::types::player::PlayerId,
mut library: Vec<ObjectId>,
chosen: &[ObjectId],
placed: &[ObjectId],
library_position: &LibraryPosition,
) -> Vec<ObjectId> {References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| fn finish_effect_zone_put_at_library_position( | ||
| state: &mut GameState, | ||
| player: crate::types::player::PlayerId, | ||
| source_id: ObjectId, | ||
| chosen: Vec<ObjectId>, | ||
| library_origin: Vec<ObjectId>, | ||
| library_position: LibraryPosition, | ||
| events: &mut Vec<GameEvent>, | ||
| ) { |
There was a problem hiding this comment.
[MEDIUM] Missing CR annotation on
finish_effect_zone_put_at_library_position. Evidence:crates/engine/src/game/engine_resolution_choices.rs:5835.
Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add aCR 401.4andCR 608.2cannotation to the function.
/// CR 401.4 + CR 608.2c: Complete the resolution of putting cards at library positions after batch delivery.
fn finish_effect_zone_put_at_library_position(
state: &mut GameState,
player: crate::types::player::PlayerId,
source_id: ObjectId,
chosen: Vec<ObjectId>,
library_origin: Vec<ObjectId>,
library_position: LibraryPosition,
events: &mut Vec<GameEvent>,
) {References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
