From 54a65c2d30f4a05dc7eeb16b16a66b815acd18f6 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 26 Jul 2026 08:31:16 -0700 Subject: [PATCH] feat(engine): parameterize the CR 405.2 stack removal by index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widens `ResolvedStackPopCommand` into `ResolvedStackRemovalCommand` with a recorded `index` and routes the four remove-at-index sites through the same authority: the CR 701.6a counter (x2) and the CR 601.2a / CR 601.2i cast rollbacks. Parameterized rather than given a `StackRemoveAt` sibling. A top-of-stack pop is exactly the removal at `index == resulting_depth`, so a separate pop command would carry a field its caller could derive — and a third removal-ish variant next to StackPush / StackEntryFinalize / UncommittedTriggerRemoval is the sibling cluster the architecture rules name as compounding debt. `pop_top_stack_entry` survives as a thin wrapper because the resolution and drain callers have no index to pass. The index is recorded rather than re-found because the production sites locate the entry with predicates that can match a DIFFERENT entry on a diverged stack — `counter.rs` scans on `id OR source_id`, which matches every ability sharing a source permanent. Routing the four sites also closes stranded-row leaks: all four previously left `stack_trigger_event_batches` behind, and the CR 601.2a reject dropped neither side table. A countered triggered ability can genuinely carry a batch row, so this is a live leak rather than a theoretical one. `im::Vector::remove` panics out of range instead of returning `Option`, so both the authority and the applier bounds-check explicitly. The buried-counter test is what makes the index non-vacuous: it asserts `index != resulting_depth`, which no top pop can produce. It needs three stack entries because a counterspell removes itself before its effect runs, so with two the victim would be on top and the index indistinguishable from a pop. --- crates/engine/src/game/casting.rs | 4 +- crates/engine/src/game/casting_costs.rs | 3 +- crates/engine/src/game/effects/counter.rs | 28 +-- crates/engine/src/game/stack.rs | 99 +++++++---- crates/engine/src/types/mod.rs | 6 +- crates/engine/src/types/resolved_commands.rs | 59 ++++--- .../integration/cr733_resolved_commands_p2.rs | 6 +- .../tests/integration/cr733_resolved_draw.rs | 4 +- ...pop.rs => cr733_resolved_stack_removal.rs} | 163 +++++++++++++++--- crates/engine/tests/integration/main.rs | 2 +- 10 files changed, 269 insertions(+), 105 deletions(-) rename crates/engine/tests/integration/{cr733_resolved_stack_pop.rs => cr733_resolved_stack_removal.rs} (62%) diff --git a/crates/engine/src/game/casting.rs b/crates/engine/src/game/casting.rs index 4d37463624..a98ee3b7d9 100644 --- a/crates/engine/src/game/casting.rs +++ b/crates/engine/src/game/casting.rs @@ -17851,8 +17851,8 @@ pub fn handle_cancel_cast( .iter() .rposition(|entry| entry.id == pending.object_id) { - state.stack.remove(pos); - state.stack_paid_facts.remove(&pending.object_id); + super::stack::remove_stack_entry_at(state, pos) + .expect("rposition yielded a live stack index"); } } diff --git a/crates/engine/src/game/casting_costs.rs b/crates/engine/src/game/casting_costs.rs index a57553e17a..9f258511ad 100644 --- a/crates/engine/src/game/casting_costs.rs +++ b/crates/engine/src/game/casting_costs.rs @@ -9399,7 +9399,8 @@ fn handle_resolution_cast_rejection( // finishes entering the stack because we abort before the Hand→Stack // zone move in `finalize_cast_with_phyrexian_choices`. if let Some(pos) = state.stack.iter().rposition(|entry| entry.id == object_id) { - state.stack.remove(pos); + super::stack::remove_stack_entry_at(state, pos) + .expect("rposition yielded a live stack index"); } let needs_choice = match reject_action { diff --git a/crates/engine/src/game/effects/counter.rs b/crates/engine/src/game/effects/counter.rs index c8893c5b76..5061db6a49 100644 --- a/crates/engine/src/game/effects/counter.rs +++ b/crates/engine/src/game/effects/counter.rs @@ -124,22 +124,25 @@ pub fn resolve( .iter() .rposition(|e| e.id == obj_id || e.source_id == obj_id); if let Some(idx) = stack_idx { - let is_spell = matches!(state.stack[idx].kind, StackEntryKind::Spell { .. }); + // CR 701.6a: the removal IS the counter, so it goes through the + // single CR 405.2 removal authority, which journals it and drops + // both per-entry side tables. + let removed = crate::game::stack::remove_stack_entry_at(state, idx) + .expect("rposition yielded a live stack index") + .entry; + let is_spell = matches!(removed.kind, StackEntryKind::Spell { .. }); // CR 702.34a / CR 702.127a / CR 702.180a: Flashback, // Aftermath, and Harmonize exile when leaving the stack for // any reason, including when countered. Escape (CR 702.138) // has no such clause — countered escape spells go to graveyard. - let casting_variant = match &state.stack[idx].kind { + let casting_variant = match &removed.kind { StackEntryKind::Spell { casting_variant, .. } => *casting_variant, _ => CastingVariant::Normal, }; let exiles_on_counter = casting_variant.replaces_stack_to_graveyard_with_exile(); - let source_permanent_id = state.stack[idx].source_id; - let removed_entry_id = state.stack[idx].id; - state.stack.remove(idx); - state.stack_paid_facts.remove(&removed_entry_id); + let source_permanent_id = removed.source_id; // CR 701.6a: removal from the stack IS the counter; emit the // event now (before the consequent zone move) so a pause on a @@ -358,20 +361,23 @@ pub fn resolve_all( let stack_idx = state.stack.iter().position(|e| e.id == obj_id); let Some(idx) = stack_idx else { continue }; - let is_spell = matches!(state.stack[idx].kind, StackEntryKind::Spell { .. }); + // CR 701.6a: the removal IS the counter, so it goes through the single + // CR 405.2 removal authority, which journals it and drops both + // per-entry side tables. + let removed = crate::game::stack::remove_stack_entry_at(state, idx) + .expect("position yielded a live stack index") + .entry; + let is_spell = matches!(removed.kind, StackEntryKind::Spell { .. }); // CR 702.34a / CR 702.127a / CR 702.180a: Flashback / Aftermath / // Harmonize exile on leaving the stack for any reason, including // counter. Escape (CR 702.138) has no such clause. - let casting_variant = match &state.stack[idx].kind { + let casting_variant = match &removed.kind { StackEntryKind::Spell { casting_variant, .. } => *casting_variant, _ => CastingVariant::Normal, }; let exiles_on_counter = casting_variant.replaces_stack_to_graveyard_with_exile(); - let removed_entry_id = state.stack[idx].id; - state.stack.remove(idx); - state.stack_paid_facts.remove(&removed_entry_id); // CR 701.6a: removal from the stack IS the counter; emit the event // before any consequent zone move. diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index 8b40f7224a..46d9f662f1 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -16,8 +16,8 @@ use crate::types::identifiers::ObjectId; use crate::types::player::PlayerId; use crate::types::resolved_commands::{ ResolvedStackEntryFinalizeCommand, ResolvedStackEntryFinalizeReplayInvariantError, - ResolvedStackPopCommand, ResolvedStackPopReplayInvariantError, ResolvedStackPushCommand, - ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError, + ResolvedStackPushCommand, ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError, + ResolvedStackRemovalCommand, ResolvedStackRemovalReplayInvariantError, ResolvedUncommittedTriggerRemovalCommand, ResolvedUncommittedTriggerRemovalReplayInvariantError, }; @@ -283,42 +283,53 @@ pub(crate) struct PoppedStackEntry { pub trigger_event_batch: Option>, } -/// CR 405.2: removes the topmost object from the stack. +/// CR 405.2: removes one object from the stack at a known index. /// -/// The single authority for the ordinary top-of-stack removal — the CR 405.5 -/// resolution pop and the drain loops that clear several entries in one pass -/// (batched resolution, inert no-op batches, CR 724.1b stack exile). Each call -/// removes exactly one object, so a drain of N entries is N removals rather -/// than one bulk mutation. +/// The single authority for every one-entry stack removal — the CR 405.5 +/// resolution pop and the drain loops (batched resolution, inert no-op batches, +/// CR 724.1b stack exile) via [`pop_top_stack_entry`], the CR 701.6a counter, +/// and the CR 601.2a / CR 601.2i cast rollbacks. Each call removes exactly one +/// object, so a drain of N entries is N removals rather than one bulk mutation. /// /// Both side tables are dropped here rather than by the callers because they are -/// keyed on the entry and settle WITH the pop: a removal that dropped the entry -/// but left `stack_paid_facts` or `stack_trigger_event_batches` behind would -/// strand rows against an id no longer on the stack. +/// keyed on the entry and settle WITH the removal: dropping the entry but +/// leaving `stack_paid_facts` or `stack_trigger_event_batches` behind would +/// strand rows against an id no longer on the stack. The counter and rollback +/// sites previously dropped only `stack_paid_facts` (and the CR 601.2a reject +/// dropped neither), so routing them here also closes those leaks. /// /// NOT used by [`pop_uncommitted_pending_trigger_entry`], which performs the -/// same three-line mutation. That is deliberate: the CR 603.3d removal is a -/// distinct family with its own record, and routing it through this authority -/// would journal one mutation twice, so a replay would pop two entries where -/// execution popped one. -pub(crate) fn pop_top_stack_entry(state: &mut GameState) -> Option { - let entry = state.stack.pop_back()?; +/// same mutation. That is deliberate: the CR 603.3d removal is a distinct family +/// with its own record, and routing it through this authority would journal one +/// mutation twice, so a replay would remove two entries where execution removed +/// one. +pub(crate) fn remove_stack_entry_at( + state: &mut GameState, + index: usize, +) -> Option { + // `im::Vector::remove` panics out of range rather than returning `Option`, + // so the bound is checked here rather than leaned on. + if index >= state.stack.len() { + return None; + } + let entry = state.stack.remove(index); let paid_facts = state.stack_paid_facts.remove(&entry.id); let trigger_event_batch = state.stack_trigger_event_batches.remove(&entry.id); // CR 733: journal once ALL THREE removals have settled, so the record - // describes a stack the entry has already left. An empty stack is the one - // case that journals nothing — `?` returns above, because no mutation + // describes a stack the entry has already left. An out-of-range index is the + // one case that journals nothing — `?` returns above, because no mutation // happened at all. let cause = state.current_or_begin_rules_execution_node(); state .resolved_rules_journal - .record_stack_pop(ResolvedStackPopCommand { + .record_stack_removal(ResolvedStackRemovalCommand { entry: Box::new(entry.clone()), + index, resulting_depth: state.stack.len(), cause, }) - .expect("resolved stack pop must have a live journal cause"); + .expect("resolved stack removal must have a live journal cause"); Some(PoppedStackEntry { entry, @@ -327,34 +338,50 @@ pub(crate) fn pop_top_stack_entry(state: &mut GameState) -> Option Option { + remove_stack_entry_at(state, state.stack.len().checked_sub(1)?) +} + +/// Replays one already-resolved CR 405.2 stack removal. +/// +/// Installs the recorded removal with nothing re-derived: the entry is verified +/// at the RECORDED index rather than located by a fresh scan, which matters +/// because the production sites find it with predicates that can match a +/// different entry on a diverged stack. All preconditions are checked BEFORE any +/// mutation, so a rejected replay leaves the stack and both side tables +/// untouched. +pub fn apply_resolved_stack_removal( state: &mut GameState, - command: &ResolvedStackPopCommand, -) -> Result<(), ResolvedStackPopReplayInvariantError> { + command: &ResolvedStackRemovalCommand, +) -> Result<(), ResolvedStackRemovalReplayInvariantError> { // CR 405.2: the predecessor must be exactly one deeper than the record. let expected_depth = command.resulting_depth + 1; if state.stack.len() != expected_depth { - return Err(ResolvedStackPopReplayInvariantError::DepthMismatch { + return Err(ResolvedStackRemovalReplayInvariantError::DepthMismatch { expected: expected_depth, found: state.stack.len(), }); } + let Some(found) = state.stack.get(command.index) else { + return Err(ResolvedStackRemovalReplayInvariantError::IndexOutOfRange { + index: command.index, + depth: state.stack.len(), + }); + }; // Compared WHOLE rather than by id: an applier that matched on `id` alone // would discard a divergent object that merely reused the identifier. - if state.stack.back() != Some(command.entry.as_ref()) { - return Err(ResolvedStackPopReplayInvariantError::PoppedEntryMismatch); + if found != command.entry.as_ref() { + return Err(ResolvedStackRemovalReplayInvariantError::RemovedEntryMismatch); } - let entry = state - .stack - .pop_back() - .expect("the entry was just verified at the top of the stack"); + // In range: the `get` above returned `Some`, so this cannot panic. + let entry = state.stack.remove(command.index); state.stack_paid_facts.remove(&entry.id); state.stack_trigger_event_batches.remove(&entry.id); Ok(()) diff --git a/crates/engine/src/types/mod.rs b/crates/engine/src/types/mod.rs index 5708f14c58..57405e168b 100644 --- a/crates/engine/src/types/mod.rs +++ b/crates/engine/src/types/mod.rs @@ -92,9 +92,9 @@ pub use resolved_commands::{ ResolvedPlayerEditCommand, ResolvedPlayerEditReplayInvariantError, ResolvedRngReplayInvariantError, ResolvedRulesCommand, ResolvedRulesJournal, ResolvedRulesJournalError, ResolvedStackEntryFinalizeCommand, - ResolvedStackEntryFinalizeReplayInvariantError, ResolvedStackPopCommand, - ResolvedStackPopReplayInvariantError, ResolvedStackPushCommand, ResolvedStackPushOrigin, - ResolvedStackPushReplayInvariantError, ResolvedTriggerCollection, + ResolvedStackEntryFinalizeReplayInvariantError, ResolvedStackPushCommand, + ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError, ResolvedStackRemovalCommand, + ResolvedStackRemovalReplayInvariantError, ResolvedTriggerCollection, ResolvedTriggerCollectionCommand, ResolvedTriggerCollectionReplayInvariantError, ResolvedTriggerLedgerEdit, ResolvedUncommittedTriggerRemovalCommand, ResolvedUncommittedTriggerRemovalReplayInvariantError, RulesExecutionNodeKind, diff --git a/crates/engine/src/types/resolved_commands.rs b/crates/engine/src/types/resolved_commands.rs index 4599ef4025..06f21370e4 100644 --- a/crates/engine/src/types/resolved_commands.rs +++ b/crates/engine/src/types/resolved_commands.rs @@ -1151,19 +1151,28 @@ pub enum ResolvedUncommittedTriggerRemovalReplayInvariantError { UnexpectedRemovableEntry(ObjectId), } -/// One exact CR 405.2 removal of the topmost object from the stack. +/// One exact CR 405.2 removal of a single object from the stack. /// -/// Recorded by `stack::pop_top_stack_entry`, the single authority behind the -/// CR 405.5 resolution pop and the drain loops (batched resolution, inert no-op -/// batches, CR 724.1b stack exile). A drain of N entries records N of these -/// rather than one bulk removal, so a replay reproduces the removal ORDER and -/// not merely the final depth. +/// Recorded by `stack::remove_stack_entry_at`, the single authority behind every +/// one-entry stack removal: the CR 405.5 resolution pop, the drain loops +/// (batched resolution, inert no-op batches, CR 724.1b stack exile), the +/// CR 701.6a counter, and the CR 601.2a/601.2i cast rollbacks. +/// +/// PARAMETERIZED BY `index` RATHER THAN SPLIT INTO POP/REMOVE-AT SIBLINGS. A +/// top-of-stack pop is exactly the removal at `index == resulting_depth`, so a +/// separate pop command would be this one with a field the caller could derive. +/// Adding that sibling is what the enum's existing `StackPush` / +/// `StackEntryFinalize` / `UncommittedTriggerRemoval` cluster makes tempting and +/// is precisely the debt to avoid. +/// +/// A drain of N entries records N of these rather than one bulk removal, so a +/// replay reproduces the removal ORDER and not merely the final depth. /// /// NOT unified with [`ResolvedUncommittedTriggerRemovalCommand`], despite both -/// removing a stack entry. The axis would have to span CR 603.3d +/// removing a stack entry. That axis would have to span CR 603.3d /// trigger-construction and CR 405.5 resolution — different rule sections the /// engine resolves separately — and the operand sets genuinely differ: the -/// CR 603.3d removal consumes a cursor and may legitimately pop NOTHING, an +/// CR 603.3d removal consumes a cursor and may legitimately remove NOTHING, an /// outcome that has no analogue here. Unifying them would buy one enum variant /// at the cost of an applier that checks preconditions belonging to whichever /// family it was not handed. @@ -1174,22 +1183,30 @@ pub enum ResolvedUncommittedTriggerRemovalReplayInvariantError { /// pin an invariant, and carrying `Vec` batches would widen every /// journal entry on the hottest path in the engine for nothing. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ResolvedStackPopCommand { +pub struct ResolvedStackRemovalCommand { /// The removed entry, recorded verbatim so replay verifies the whole object /// rather than trusting the id. pub entry: Box, + /// CR 405.2: the index the entry occupied. Recorded rather than re-found, + /// because the production sites locate it by a `position`/`rposition` scan + /// whose predicate can match a DIFFERENT entry on a stack that has since + /// diverged — `counter.rs` in particular scans on `id OR source_id`, which + /// matches every ability sharing a source permanent. + pub index: usize, /// Stack depth AFTER the removal (CR 405.2). pub resulting_depth: usize, pub cause: RulesExecutionNodeRef, } -/// Typed failure while applying one already-resolved CR 405.2 stack pop. +/// Typed failure while applying one already-resolved CR 405.2 stack removal. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] -pub enum ResolvedStackPopReplayInvariantError { - #[error("stack pop expected depth {expected} before removal, found {found}")] +pub enum ResolvedStackRemovalReplayInvariantError { + #[error("stack removal expected depth {expected} before removal, found {found}")] DepthMismatch { expected: usize, found: usize }, - #[error("stack pop expected a different entry on top of the stack")] - PoppedEntryMismatch, + #[error("stack removal targets index {index}, but the stack holds only {depth} entries")] + IndexOutOfRange { index: usize, depth: usize }, + #[error("stack removal expected a different entry at the recorded index")] + RemovedEntryMismatch, } /// Semantic command payload currently carried by a resolved-rules journal entry. @@ -1222,7 +1239,7 @@ pub enum ResolvedRulesCommand { StackPush(Box), StackEntryFinalize(Box), UncommittedTriggerRemoval(Box), - StackPop(Box), + StackRemoval(Box), } /// An append-only trigger collection command has no replay-time precondition. @@ -2301,13 +2318,13 @@ impl ResolvedRulesJournal { } /// Records one exact CR 405.2 top-of-stack removal under its cause. - pub fn record_stack_pop( + pub fn record_stack_removal( &mut self, - command: ResolvedStackPopCommand, + command: ResolvedStackRemovalCommand, ) -> Result { self.append_command( command.cause, - ResolvedRulesCommand::StackPop(Box::new(command)), + ResolvedRulesCommand::StackRemoval(Box::new(command)), ) } @@ -2528,7 +2545,7 @@ impl ResolvedRulesJournal { | ResolvedRulesCommand::StackPush(_) | ResolvedRulesCommand::StackEntryFinalize(_) | ResolvedRulesCommand::UncommittedTriggerRemoval(_) - | ResolvedRulesCommand::StackPop(_) => {} + | ResolvedRulesCommand::StackRemoval(_) => {} } } for node in &self.nodes { @@ -2992,12 +3009,12 @@ impl ResolvedRulesJournal { } } } - ResolvedRulesCommand::StackPop(command) => { + ResolvedRulesCommand::StackRemoval(command) => { // Cause-only. A pop draws no id and no timestamp, so there is no // allocator receipt to cross-check. Both of its preconditions // (the CR 405.2 depth and the exact entry on top) are // state-dependent and are enforced by - // `stack::apply_resolved_stack_pop`, where the state exists to + // `stack::apply_resolved_stack_removal`, where the state exists to // check them against. if entry.node != command.cause { return Err(ResolvedRulesJournalError::InvalidSerializedAuthority( diff --git a/crates/engine/tests/integration/cr733_resolved_commands_p2.rs b/crates/engine/tests/integration/cr733_resolved_commands_p2.rs index 72763ae508..b2a2969cd4 100644 --- a/crates/engine/tests/integration/cr733_resolved_commands_p2.rs +++ b/crates/engine/tests/integration/cr733_resolved_commands_p2.rs @@ -144,8 +144,8 @@ fn apply_semantic_command(state: &mut GameState, command: &ResolvedRulesCommand) ) .unwrap(); } - ResolvedRulesCommand::StackPop(command) => { - engine::game::stack::apply_resolved_stack_pop(state, command.as_ref()).unwrap(); + ResolvedRulesCommand::StackRemoval(command) => { + engine::game::stack::apply_resolved_stack_removal(state, command.as_ref()).unwrap(); } } } @@ -251,7 +251,7 @@ fn exact_mana_spend_rejects_a_second_removal() { | ResolvedRulesCommand::StackPush(_) | ResolvedRulesCommand::StackEntryFinalize(_) | ResolvedRulesCommand::UncommittedTriggerRemoval(_) - | ResolvedRulesCommand::StackPop(_) => apply_semantic_command(&mut replay, command), + | ResolvedRulesCommand::StackRemoval(_) => apply_semantic_command(&mut replay, command), } } assert!( diff --git a/crates/engine/tests/integration/cr733_resolved_draw.rs b/crates/engine/tests/integration/cr733_resolved_draw.rs index 20a72809fa..a9245cd99f 100644 --- a/crates/engine/tests/integration/cr733_resolved_draw.rs +++ b/crates/engine/tests/integration/cr733_resolved_draw.rs @@ -109,8 +109,8 @@ fn apply_semantic_command(state: &mut GameState, command: &ResolvedRulesCommand) ) .unwrap(); } - ResolvedRulesCommand::StackPop(command) => { - engine::game::stack::apply_resolved_stack_pop(state, command.as_ref()).unwrap(); + ResolvedRulesCommand::StackRemoval(command) => { + engine::game::stack::apply_resolved_stack_removal(state, command.as_ref()).unwrap(); } } } diff --git a/crates/engine/tests/integration/cr733_resolved_stack_pop.rs b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs similarity index 62% rename from crates/engine/tests/integration/cr733_resolved_stack_pop.rs rename to crates/engine/tests/integration/cr733_resolved_stack_removal.rs index efe23366d3..f383ee7dcb 100644 --- a/crates/engine/tests/integration/cr733_resolved_stack_pop.rs +++ b/crates/engine/tests/integration/cr733_resolved_stack_removal.rs @@ -1,17 +1,23 @@ -//! CR733 P2 coverage for the CR 405.2 top-of-stack removal. +//! CR733 P2 coverage for the CR 405.2 single-entry stack removal. //! -//! CR 405.5: "When all players pass in succession, the top (last-added) object -//! on the stack resolves." That removal, plus the drain loops that clear several -//! entries in one pass (batched resolution, inert no-op batches, and the -//! CR 724.1b end-phase stack exile), all funnel through the single authority -//! `stack::pop_top_stack_entry`, which drops the entry together with the two -//! per-entry side tables keyed on it. +//! Every one-entry removal funnels through `stack::remove_stack_entry_at`, which +//! drops the entry together with the two per-entry side tables keyed on it: the +//! CR 405.5 resolution pop and the drain loops (batched resolution, inert no-op +//! batches, CR 724.1b end-phase exile) via the `pop_top_stack_entry` wrapper, +//! the CR 701.6a counter, and the CR 601.2a / CR 601.2i cast rollbacks. +//! +//! ONE COMMAND PARAMETERIZED BY `index`, not a pop/remove-at pair. A top pop is +//! the removal at `index == resulting_depth`, so a separate pop command would +//! carry a field its caller could derive. +//! `countering_a_buried_spell_journals_a_removal_at_the_recorded_index` is what +//! makes the field non-vacuous — it asserts `index != resulting_depth`, which is +//! unreachable through any pop. //! //! A drain of N entries journals N separate commands rather than one bulk //! removal, so a replay reproduces the removal ORDER and not merely the final //! depth. `resolving_two_spells_journals_two_pops_in_lifo_order` is what pins //! that: it would still pass on a bulk record if it only checked the end state, -//! so it asserts the per-command depths descend. +//! so it asserts the per-command depths AND indices descend. //! //! THE CROSS-POP CANARY IS THE ACCEPTANCE TEST FOR THIS FAMILY. Before pops were //! journaled, `apply_resolved_stack_push` failed `StackDepthMismatch` on any @@ -23,15 +29,15 @@ //! is what proves the pop record is doing the work. use engine::game::scenario::{GameRunner, GameScenario, P0}; -use engine::game::stack::{apply_resolved_stack_pop, apply_resolved_stack_push}; +use engine::game::stack::{apply_resolved_stack_push, apply_resolved_stack_removal}; use engine::types::actions::GameAction; use engine::types::game_state::{CastPaymentMode, GameState}; use engine::types::identifiers::ObjectId; use engine::types::mana::ManaCost; use engine::types::phase::Phase; use engine::types::resolved_commands::{ - ResolvedRulesCommand, ResolvedStackPopCommand, ResolvedStackPopReplayInvariantError, - ResolvedStackPushCommand, ResolvedStackPushReplayInvariantError, + ResolvedRulesCommand, ResolvedStackPushCommand, ResolvedStackPushReplayInvariantError, + ResolvedStackRemovalCommand, ResolvedStackRemovalReplayInvariantError, }; /// A no-target sorcery: nothing to retarget, and no trigger to add stack entries @@ -39,7 +45,7 @@ use engine::types::resolved_commands::{ const ELVISH_TOKEN_SPELL: &str = "Create a 1/1 green Elf Warrior creature token."; /// Every stack pop journaled after `from`, in journal order. -fn stack_pops(state: &GameState, from: usize) -> Vec { +fn stack_removals(state: &GameState, from: usize) -> Vec { state .resolved_rules_journal .entries() @@ -47,7 +53,7 @@ fn stack_pops(state: &GameState, from: usize) -> Vec { .skip(from) .filter_map(|entry| entry.command.clone()) .filter_map(|command| match command { - ResolvedRulesCommand::StackPop(command) => Some(*command), + ResolvedRulesCommand::StackRemoval(command) => Some(*command), _ => None, }) .collect() @@ -115,7 +121,7 @@ fn resolving_a_spell_journals_an_exact_pop() { // The discriminating assertion: the removal is journaled. A raw // `stack.pop_back()` records nothing here. - let pops = stack_pops(runner.state(), journal_start); + let pops = stack_removals(runner.state(), journal_start); let recorded: Vec<_> = pops.iter().filter(|pop| pop.entry.id == spell).collect(); assert_eq!( recorded.len(), @@ -127,6 +133,11 @@ fn resolving_a_spell_journals_an_exact_pop() { pop.resulting_depth, 0, "CR 405.2: the recorded depth is the depth AFTER the removal" ); + assert_eq!( + pop.index, 0, + "CR 405.2: a top-of-stack pop is the removal at index == resulting_depth, \ + which is what makes a separate pop command unnecessary" + ); assert_eq!( *pop.entry, before_pop.stack[0], "the recorded entry is the entry that was on the stack, verbatim" @@ -135,7 +146,7 @@ fn resolving_a_spell_journals_an_exact_pop() { // Replay-exactness: from the captured predecessor, applying the record // reproduces the removal with nothing re-derived. let mut replay = before_pop.clone(); - apply_resolved_stack_pop(&mut replay, pop) + apply_resolved_stack_removal(&mut replay, pop) .expect("the recorded pop must replay against its captured predecessor"); assert!(replay.stack.is_empty(), "replay removes the recorded entry"); assert!( @@ -151,8 +162,8 @@ fn resolving_a_spell_journals_an_exact_pop() { // so it fails closed rather than popping an unrelated object. assert!( matches!( - apply_resolved_stack_pop(&mut replay, pop), - Err(ResolvedStackPopReplayInvariantError::DepthMismatch { + apply_resolved_stack_removal(&mut replay, pop), + Err(ResolvedStackRemovalReplayInvariantError::DepthMismatch { expected: 1, found: 0 }) @@ -171,7 +182,7 @@ fn pop_rejects_a_divergent_predecessor() { let before_pop = runner.state().clone(); let journal_start = before_pop.resolved_rules_journal.entries().len(); runner.resolve_top(); - let pops = stack_pops(runner.state(), journal_start); + let pops = stack_removals(runner.state(), journal_start); let pop = pops .iter() .find(|pop| pop.entry.id == spell) @@ -185,8 +196,8 @@ fn pop_rejects_a_divergent_predecessor() { too_deep.stack.push_front(duplicate); assert!( matches!( - apply_resolved_stack_pop(&mut too_deep, pop), - Err(ResolvedStackPopReplayInvariantError::DepthMismatch { + apply_resolved_stack_removal(&mut too_deep, pop), + Err(ResolvedStackRemovalReplayInvariantError::DepthMismatch { expected: 1, found: 2 }) @@ -210,8 +221,8 @@ fn pop_rejects_a_divergent_predecessor() { .source_id = ObjectId(4242); assert!( matches!( - apply_resolved_stack_pop(&mut wrong_top, pop), - Err(ResolvedStackPopReplayInvariantError::PoppedEntryMismatch) + apply_resolved_stack_removal(&mut wrong_top, pop), + Err(ResolvedStackRemovalReplayInvariantError::RemovedEntryMismatch) ), "a pop must refuse a predecessor whose top entry diverges from the record" ); @@ -244,7 +255,7 @@ fn resolving_two_spells_journals_two_pops_in_lifo_order() { runner.advance_until_stack_empty(); - let pops: Vec<_> = stack_pops(runner.state(), journal_start) + let pops: Vec<_> = stack_removals(runner.state(), journal_start) .into_iter() .filter(|pop| pop.entry.id == lower || pop.entry.id == upper) .collect(); @@ -261,6 +272,108 @@ fn resolving_two_spells_journals_two_pops_in_lifo_order() { "the recorded depths descend one per removal, which a single bulk record \ could not express" ); + assert_eq!( + (pops[0].index, pops[1].index), + (1, 0), + "CR 405.2: each record names the index its entry occupied at the moment \ + it was removed, not its index in the original stack" + ); +} + +/// CR 701.6a: a counter removes an entry that is NOT on top, which is the case +/// the recorded index exists for. A pop-shaped command could not express it. +#[test] +fn countering_a_buried_spell_journals_a_removal_at_the_recorded_index() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let victim = scenario + .add_spell_to_hand_from_oracle(P0, "Journal Counter Victim", true, ELVISH_TOKEN_SPELL) + .with_mana_cost(ManaCost::zero()) + .id(); + let counterspell = scenario + // Instant speed: a counterspell must be castable while its target is on + // the stack, which is the whole point of the buried-index fixture. + .add_spell_to_hand_from_oracle(P0, "Journal Counterspell", true, "Counter target spell.") + .with_mana_cost(ManaCost::zero()) + .id(); + // A third entry is REQUIRED for this fixture to be non-vacuous. The + // counterspell removes itself from the stack before its effect executes + // (CR 608.2 resolution pops first), so with only two entries the victim + // would be on TOP when countered and the index would be indistinguishable + // from a pop. The filler sits above the victim and stays there. + let filler = scenario + .add_spell_to_hand_from_oracle(P0, "Journal Counter Filler", true, ELVISH_TOKEN_SPELL) + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + + cast(&mut runner, victim); + cast(&mut runner, filler); + let journal_start = runner.state().resolved_rules_journal.entries().len(); + let predecessor = runner.state().clone(); + assert_eq!( + predecessor.stack.len(), + 2, + "reach guard: the victim is BURIED under the filler before the counter is cast" + ); + + // The `targets` field on `CastSpell` does NOT pre-fill target selection — + // the engine parks in `WaitingFor::TargetSelection` — so the choice goes + // through the runner's builder, which submits it. + runner + .cast(counterspell) + .target_object(victim) + .commit() + .resolve(); + runner.advance_until_stack_empty(); + + let removals = stack_removals(runner.state(), journal_start); + let victim_removal = removals + .iter() + .find(|removal| removal.entry.id == victim) + .expect("CR 701.6a: countering the victim journals its removal"); + + // The discriminating assertion for the index field: the victim sat UNDER the + // counterspell, so it was removed from index 0 while the stack still held + // the counterspell above it. A pop-shaped record would have described the + // wrong entry. + assert_eq!( + victim_removal.index, 0, + "CR 701.6a: the countered spell is removed from the index it occupied, \ + underneath the counterspell" + ); + assert!( + victim_removal.resulting_depth >= 1, + "the counterspell is still on the stack when its target is removed, so \ + the removal is genuinely not a top-of-stack pop (depth {})", + victim_removal.resulting_depth + ); + assert_ne!( + victim_removal.index, victim_removal.resulting_depth, + "index != resulting_depth is exactly the case a pop-only command could \ + not express" + ); + + // Replay-exactness against a reconstructed predecessor: the recorded index + // is used verbatim rather than re-scanned. + let mut replay = predecessor.clone(); + // Rebuild the pre-removal stack: victim at index 0, counterspell above it. + while replay.stack.len() < victim_removal.resulting_depth + 1 { + let mut filler = replay.stack[0].clone(); + filler.id = ObjectId(9100 + replay.stack.len() as u64); + replay.stack.push_back(filler); + } + apply_resolved_stack_removal(&mut replay, victim_removal) + .expect("the recorded counter removal replays at its recorded index"); + assert!( + !replay.stack.iter().any(|entry| entry.id == victim), + "replay removes the countered entry" + ); + assert_eq!( + replay.stack.len(), + victim_removal.resulting_depth, + "replay lands on the recorded depth" + ); } /// THE CANARY FLIP. See the module header: before this family, a replay whose @@ -284,7 +397,7 @@ fn a_recorded_pop_unblocks_a_later_push_replay() { runner.resolve_top(); cast(&mut runner, second); - let pop = stack_pops(runner.state(), journal_start) + let pop = stack_removals(runner.state(), journal_start) .into_iter() .find(|pop| pop.entry.id == first) .expect("resolving the first spell journaled its pop"); @@ -316,7 +429,7 @@ fn a_recorded_pop_unblocks_a_later_push_replay() { // AFTER-half: same predecessor, pop applied first, and the push now lands. let mut sequenced = predecessor.clone(); - apply_resolved_stack_pop(&mut sequenced, &pop) + apply_resolved_stack_removal(&mut sequenced, &pop) .expect("the recorded pop replays against the predecessor"); apply_resolved_stack_push(&mut sequenced, &push) .expect("with the pop applied, the push replays across it"); diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index d883e44f4c..65cffd9e31 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -105,8 +105,8 @@ mod cr733_resolved_modifier_install; mod cr733_resolved_object_cease; mod cr733_resolved_player_leave; mod cr733_resolved_stack_entry_finalize; -mod cr733_resolved_stack_pop; mod cr733_resolved_stack_push; +mod cr733_resolved_stack_removal; mod cr733_resolved_token_creation; mod cr733_resolved_transform; mod cr733_resolved_trigger_collection;