feat(engine): parameterize the CR 405.2 stack removal by index

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");
         }
     }
 

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 {

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.

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<Vec<GameEvent>>,
 }
 
-/// 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<PoppedStackEntry> {
-    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<PoppedStackEntry> {
+    // `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<PoppedStackEn
     })
 }
 
-/// Replays one already-resolved CR 405.2 stack pop.
+/// CR 405.2: removes the topmost object from the stack.
 ///
-/// Installs the recorded removal with nothing re-derived: the entry to remove is
-/// verified against the record rather than located by a fresh scan. Both
-/// preconditions are checked BEFORE any mutation, so a rejected replay leaves
-/// the stack and both side tables untouched.
-pub fn apply_resolved_stack_pop(
+/// A thin wrapper over [`remove_stack_entry_at`] — the top of an N-deep stack is
+/// index N-1 — kept because the resolution and drain callers have no index to
+/// pass and reading `remove_stack_entry_at(state, state.stack.len() - 1)` at
+/// each of them would obscure that they are simply resolving the top object.
+pub(crate) fn pop_top_stack_entry(state: &mut GameState) -> Option<PoppedStackEntry> {
+    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(())

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,

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<GameEvent>` 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<StackEntry>,
+    /// 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<ResolvedStackPushCommand>),
     StackEntryFinalize(Box<ResolvedStackEntryFinalizeCommand>),
     UncommittedTriggerRemoval(Box<ResolvedUncommittedTriggerRemovalCommand>),
-    StackPop(Box<ResolvedStackPopCommand>),
+    StackRemoval(Box<ResolvedStackRemovalCommand>),
 }
 
 /// 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<ResolvedCommandOrdinal, ResolvedRulesJournalError> {
         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(