From f4b637dafcce569a078dbc05592a61dc8340252e Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 26 Jul 2026 10:14:15 -0700 Subject: [PATCH 1/2] refactor(engine): journal the CR 800.4a retain-shaped stack removals The three `state.stack.retain(..)` sites removed entries without recording them, so a replay of the resolved-command journal reproduced neither the count nor the surviving order of the stack. Route all three through the existing `remove_stack_entry_at` authority (CR 405.2), which journals one `ResolvedStackRemovalCommand` per removed entry. No new command shape is needed: a plural removal flattens losslessly into N indexed removals because each record names the LIVE index at its own removal, so a sequential replay reproduces both count and surviving order. Two of the three predicates filter on a unique id and so remove at most one entry; only `elimination.rs` is genuinely plural (every entry controlled by the leaving player, CR 800.4a). Routing these also closes stranded-row leaks: `retain` dropped entries without clearing their `stack_paid_facts` / `stack_trigger_event_batches` side-table rows, which the authority removes. --- crates/engine/src/game/effects/prepare.rs | 10 +- crates/engine/src/game/elimination.rs | 16 ++- crates/engine/src/game/zones.rs | 11 +- .../cr733_resolved_stack_removal.rs | 104 +++++++++++++++++- 4 files changed, 134 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/game/effects/prepare.rs b/crates/engine/src/game/effects/prepare.rs index 9cd9285a3f..6c9f55e7fa 100644 --- a/crates/engine/src/game/effects/prepare.rs +++ b/crates/engine/src/game/effects/prepare.rs @@ -213,8 +213,14 @@ pub(crate) fn open_copy_target_selection( fn cleanup_failed_prepared_copy_cast(state: &mut GameState, copy_id: ObjectId) { // Defensive cleanup for any failed cast attempt after synthesizing the - // ephemeral copy object. - state.stack.retain(|entry| entry.id != copy_id); + // ephemeral copy object. The predicate filters on a unique id, so this + // removes at most ONE entry — routed through the CR 405.2 removal authority + // rather than expressed as a `retain`, which would leave both per-entry side + // tables stranded and the removal unjournaled. + if let Some(idx) = state.stack.iter().position(|entry| entry.id == copy_id) { + crate::game::stack::remove_stack_entry_at(state, idx) + .expect("position yielded a live stack index"); + } state.objects.remove(©_id); } diff --git a/crates/engine/src/game/elimination.rs b/crates/engine/src/game/elimination.rs index 5a7e167249..e1631f1253 100644 --- a/crates/engine/src/game/elimination.rs +++ b/crates/engine/src/game/elimination.rs @@ -605,8 +605,20 @@ fn do_eliminate( crate::game::planechase::preserve_phenomenon_stack_abilities_for_handoff(state, planar_handoff); - // CR 800.4a: Remove spells they control from the stack - state.stack.retain(|entry| entry.controller != player); + // CR 800.4a: Remove spells they control from the stack, one at a time + // through the single CR 405.2 removal authority — the same shape as the + // scheduled-control release below. A `retain` would drop several entries in + // one unjournalable mutation; removing by position instead records each + // entry with the index it occupied at the moment IT was removed, so a replay + // reproduces both the count and the surviving entries' relative order. + while let Some(idx) = state + .stack + .iter() + .position(|entry| entry.controller == player) + { + super::stack::remove_stack_entry_at(state, idx) + .expect("position yielded a live stack index"); + } // CR 800.4a + CR 800.4b: A control-another-player effect (CR 723, e.g. // Mindslaver / Secret of Bloodbending) ends when EITHER party leaves the diff --git a/crates/engine/src/game/zones.rs b/crates/engine/src/game/zones.rs index 1fe3fd0721..60156f2283 100644 --- a/crates/engine/src/game/zones.rs +++ b/crates/engine/src/game/zones.rs @@ -1703,8 +1703,15 @@ pub fn remove_from_zone(state: &mut GameState, object_id: ObjectId, zone: Zone, } Zone::Battlefield => state.battlefield.retain(|id| *id != object_id), Zone::Stack => { - state.stack.retain(|e| e.id != object_id); - state.stack_paid_facts.remove(&object_id); + // A unique id, so at most ONE entry matches. Routed through the + // CR 405.2 removal authority, which journals it and drops BOTH + // per-entry side tables (this arm previously dropped only + // `stack_paid_facts`). A miss is normal: the resolution pop already + // removed the entry before the card is routed to its next zone. + if let Some(idx) = state.stack.iter().position(|e| e.id == object_id) { + crate::game::stack::remove_stack_entry_at(state, idx) + .expect("position yielded a live stack index"); + } } Zone::Exile => state.exile.retain(|id| *id != object_id), Zone::Command => { diff --git a/crates/engine/tests/integration/cr733_resolved_stack_removal.rs b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs index f383ee7dcb..e24e1e0c77 100644 --- a/crates/engine/tests/integration/cr733_resolved_stack_removal.rs +++ b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs @@ -28,7 +28,7 @@ //! succeeding. Do not weaken it to a bare success assertion — the failing half //! is what proves the pop record is doing the work. -use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; use engine::game::stack::{apply_resolved_stack_push, apply_resolved_stack_removal}; use engine::types::actions::GameAction; use engine::types::game_state::{CastPaymentMode, GameState}; @@ -443,3 +443,105 @@ fn a_recorded_pop_unblocks_a_later_push_replay() { "CR 405.2: the replay installs the recorded entry at the recorded index" ); } + +/// CR 800.4a: a leaving player's spells are removed from the stack. This is the +/// PLURAL case — one `retain` pass used to drop several entries at once — and it +/// is journaled as one command per entry rather than one bulk record. +/// +/// The load-bearing assertion is that BOTH removals record index 0. An +/// implementation that captured each entry's ORIGINAL position would record +/// (0, 1); recording (0, 0) is what proves each index is the LIVE index at the +/// moment that entry was removed, which is the only form a sequential replay can +/// reproduce. +/// +/// The leaving player is the one casting, so no priority juggling is needed: a +/// self-targeted burn spell takes its own controller to 0 and the CR 704.5a +/// state-based action runs the sweep through the real pipeline. +#[test] +fn eliminating_a_player_journals_one_removal_per_controlled_stack_entry() { + // Three players so the elimination does not end the game outright. + let mut scenario = GameScenario::new_n_player(3, 7); + scenario.at_phase(Phase::PreCombatMain); + let bolt = scenario.add_bolt_to_hand(P0); + let victim_lower = scenario + .add_spell_to_hand_from_oracle(P0, "Journal Victim Lower", true, ELVISH_TOKEN_SPELL) + .with_mana_cost(ManaCost::zero()) + .id(); + let victim_upper = scenario + .add_spell_to_hand_from_oracle(P0, "Journal Victim Upper", true, ELVISH_TOKEN_SPELL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + runner + .state_mut() + .players + .iter_mut() + .find(|player| player.id == P0) + .expect("the caster is in the game") + .life = 3; + + cast(&mut runner, victim_lower); + cast(&mut runner, victim_upper); + assert_eq!( + runner + .state() + .stack + .iter() + .map(|entry| entry.id) + .collect::>(), + vec![victim_lower, victim_upper], + "reach guard: both of the leaving player's spells are on the stack" + ); + let journal_start = runner.state().resolved_rules_journal.entries().len(); + + // CR 704.5a through the real pipeline: self-targeted burn takes the caster + // to 0 and the state-based action eliminates them, running the CR 800.4a + // sweep over the two spells still on the stack beneath it. + runner.cast(bolt).target_player(P0).resolve(); + + assert!( + runner + .state() + .players + .iter() + .find(|player| player.id == P0) + .expect("the player record survives elimination") + .is_eliminated, + "CR 704.5a reach guard: the caster actually left the game" + ); + + let victim_removals: Vec<_> = stack_removals(runner.state(), journal_start) + .into_iter() + .filter(|removal| removal.entry.id == victim_lower || removal.entry.id == victim_upper) + .collect(); + assert_eq!( + victim_removals.len(), + 2, + "CR 800.4a: each removed entry is journaled separately, not as one bulk record" + ); + assert_eq!( + (victim_removals[0].entry.id, victim_removals[1].entry.id), + (victim_lower, victim_upper), + "the sweep removes by ascending position, and the records carry that order" + ); + assert_eq!( + (victim_removals[0].index, victim_removals[1].index), + (0, 0), + "each index is the LIVE index at its own removal: once the lower spell \ + leaves index 0 the upper one slides down into it. Original-position \ + capture would record (0, 1) and a replay would remove the wrong entry" + ); + assert_eq!( + ( + victim_removals[0].resulting_depth, + victim_removals[1].resulting_depth + ), + (1, 0), + "the depths descend one per removal" + ); + assert!( + runner.state().stack.is_empty(), + "CR 800.4a: every spell the leaving player controlled is gone" + ); +} From 77b3d1f46b27a17a22d945c12af12a2e21daf2b8 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 26 Jul 2026 11:16:21 -0700 Subject: [PATCH 2/2] fix(engine): drop the unused P1 import and the misattributed CR 405.2 citation The unused `P1` import failed CI: the workspace builds with `-D warnings`, so it is a hard error that broke the integration test binary and with it all four Rust jobs (lint, both test shards, coverage gate). It went unused when the elimination fixture was reshaped to make the LEAVING player the caster. Also drop the "CR 405.2 removal authority" phrasing at the three call sites (CodeRabbit). CR 405.2 reads: "The stack keeps track of the order that spells and/or abilities were added to it. Each time an object is put on the stack, it's put on top of all objects already there." That describes ADDING and ordering, not removal, so it does not support naming `remove_stack_entry_at` after it. The removal mechanism is internal and its callers carry their own CR (CR 800.4a here); describe it as the shared stack-removal authority. CR 800.4a is verified and retained: "if that player controlled any objects on the stack not represented by cards, those objects cease to exist." --- crates/engine/src/game/effects/prepare.rs | 6 +++--- crates/engine/src/game/elimination.rs | 2 +- crates/engine/src/game/zones.rs | 2 +- .../tests/integration/cr733_resolved_stack_removal.rs | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/game/effects/prepare.rs b/crates/engine/src/game/effects/prepare.rs index 6c9f55e7fa..66d09cccea 100644 --- a/crates/engine/src/game/effects/prepare.rs +++ b/crates/engine/src/game/effects/prepare.rs @@ -214,9 +214,9 @@ pub(crate) fn open_copy_target_selection( fn cleanup_failed_prepared_copy_cast(state: &mut GameState, copy_id: ObjectId) { // Defensive cleanup for any failed cast attempt after synthesizing the // ephemeral copy object. The predicate filters on a unique id, so this - // removes at most ONE entry — routed through the CR 405.2 removal authority - // rather than expressed as a `retain`, which would leave both per-entry side - // tables stranded and the removal unjournaled. + // removes at most ONE entry — routed through the shared stack-removal + // authority rather than expressed as a `retain`, which would leave both + // per-entry side tables stranded and the removal unjournaled. if let Some(idx) = state.stack.iter().position(|entry| entry.id == copy_id) { crate::game::stack::remove_stack_entry_at(state, idx) .expect("position yielded a live stack index"); diff --git a/crates/engine/src/game/elimination.rs b/crates/engine/src/game/elimination.rs index e1631f1253..bae1a9f13e 100644 --- a/crates/engine/src/game/elimination.rs +++ b/crates/engine/src/game/elimination.rs @@ -606,7 +606,7 @@ fn do_eliminate( crate::game::planechase::preserve_phenomenon_stack_abilities_for_handoff(state, planar_handoff); // CR 800.4a: Remove spells they control from the stack, one at a time - // through the single CR 405.2 removal authority — the same shape as the + // through the shared stack-removal authority — the same shape as the // scheduled-control release below. A `retain` would drop several entries in // one unjournalable mutation; removing by position instead records each // entry with the index it occupied at the moment IT was removed, so a replay diff --git a/crates/engine/src/game/zones.rs b/crates/engine/src/game/zones.rs index 60156f2283..1ea1e3fa11 100644 --- a/crates/engine/src/game/zones.rs +++ b/crates/engine/src/game/zones.rs @@ -1704,7 +1704,7 @@ pub fn remove_from_zone(state: &mut GameState, object_id: ObjectId, zone: Zone, Zone::Battlefield => state.battlefield.retain(|id| *id != object_id), Zone::Stack => { // A unique id, so at most ONE entry matches. Routed through the - // CR 405.2 removal authority, which journals it and drops BOTH + // shared stack-removal authority, which journals it and drops BOTH // per-entry side tables (this arm previously dropped only // `stack_paid_facts`). A miss is normal: the resolution pop already // removed the entry before the card is routed to its next zone. diff --git a/crates/engine/tests/integration/cr733_resolved_stack_removal.rs b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs index e24e1e0c77..03a1845966 100644 --- a/crates/engine/tests/integration/cr733_resolved_stack_removal.rs +++ b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs @@ -28,7 +28,7 @@ //! succeeding. Do not weaken it to a bare success assertion — the failing half //! is what proves the pop record is doing the work. -use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::game::scenario::{GameRunner, GameScenario, P0}; use engine::game::stack::{apply_resolved_stack_push, apply_resolved_stack_removal}; use engine::types::actions::GameAction; use engine::types::game_state::{CastPaymentMode, GameState};