ship/cr733 stack pop

crates/engine/src/game/effects/end_phase.rs

@@ -1,4 +1,5 @@
 use crate::game::effects::change_zone::{self, ZoneMoveResult};
+use crate::game::stack::pop_top_stack_entry;
 use crate::types::events::GameEvent;
 use crate::types::game_state::{GameState, StackEntryKind};
 use crate::types::identifiers::ObjectId;
@@ -25,9 +26,8 @@ pub(super) fn exile_nonresolving_stack_objects(
     source_id: ObjectId,
     events: &mut Vec<GameEvent>,
 ) -> bool {
-    while let Some(entry) = state.stack.pop_back() {
-        state.stack_paid_facts.remove(&entry.id);
-        state.stack_trigger_event_batches.remove(&entry.id);
+    while let Some(removed) = pop_top_stack_entry(state) {
+        let entry = removed.entry;
         if matches!(entry.kind, StackEntryKind::Spell { .. }) {
             match change_zone::execute_zone_move(
                 state,

crates/engine/src/game/stack.rs

@@ -16,7 +16,8 @@ use crate::types::identifiers::ObjectId;
 use crate::types::player::PlayerId;
 use crate::types::resolved_commands::{
     ResolvedStackEntryFinalizeCommand, ResolvedStackEntryFinalizeReplayInvariantError,
-    ResolvedStackPushCommand, ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError,
+    ResolvedStackPopCommand, ResolvedStackPopReplayInvariantError, ResolvedStackPushCommand,
+    ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError,
     ResolvedUncommittedTriggerRemovalCommand,
     ResolvedUncommittedTriggerRemovalReplayInvariantError,
 };
@@ -269,6 +270,96 @@ pub fn apply_resolved_stack_entry_finalize(
     Ok(())
 }
 
+/// Everything one stack removal settles: the entry plus the per-entry side-table
+/// rows keyed on it.
+///
+/// The rows are returned rather than discarded because the resolution pop
+/// consumes both — the paid snapshot feeds cost-dependent resolution and the
+/// batch feeds CR 603.7c event context. Callers that only need the entry drop
+/// the rest.
+pub(crate) struct PoppedStackEntry {
+    pub entry: StackEntry,
+    pub paid_facts: Option<StackPaidSnapshot>,
+    pub trigger_event_batch: Option<Vec<GameEvent>>,
+}
+
+/// CR 405.2: removes the topmost object from the stack.
+///
+/// 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.
+///
+/// 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.
+///
+/// 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()?;
+    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
+    // happened at all.
+    let cause = state.current_or_begin_rules_execution_node();
+    state
+        .resolved_rules_journal
+        .record_stack_pop(ResolvedStackPopCommand {
+            entry: Box::new(entry.clone()),
+            resulting_depth: state.stack.len(),
+            cause,
+        })
+        .expect("resolved stack pop must have a live journal cause");
+
+    Some(PoppedStackEntry {
+        entry,
+        paid_facts,
+        trigger_event_batch,
+    })
+}
+
+/// Replays one already-resolved CR 405.2 stack pop.
+///
+/// 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(
+    state: &mut GameState,
+    command: &ResolvedStackPopCommand,
+) -> Result<(), ResolvedStackPopReplayInvariantError> {
+    // 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 {
+            expected: expected_depth,
+            found: 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);
+    }
+
+    let entry = state
+        .stack
+        .pop_back()
+        .expect("the entry was just verified at the top of the stack");
+    state.stack_paid_facts.remove(&entry.id);
+    state.stack_trigger_event_batches.remove(&entry.id);
+    Ok(())
+}
+
 /// CR 603.3d: removes an uncommitted triggered ability from the stack.
 ///
 /// The "push first, choose second" invariant (see
@@ -667,11 +758,14 @@ pub fn resolve_top(state: &mut GameState, events: &mut Vec<GameEvent>) {
     state.announced_source_x = None;
 
     // CR 405.5: When all players pass in succession, the top object on the stack resolves.
-    let entry = match state.stack.pop_back() {
-        Some(e) => e,
-        None => return,
+    let Some(PoppedStackEntry {
+        entry,
+        paid_facts: paid_snapshot,
+        trigger_event_batch,
+    }) = pop_top_stack_entry(state)
+    else {
+        return;
     };
-    let paid_snapshot = state.stack_paid_facts.remove(&entry.id);
 
     // CR 113.3b: Activated keyword abilities (Equip / Crew / Saddle / Station)
     // resolve via their typed payload — they have no ResolvedAbility/targets
@@ -686,8 +780,6 @@ pub fn resolve_top(state: &mut GameState, events: &mut Vec<GameEvent>) {
         return;
     }
 
-    let trigger_event_batch = state.stack_trigger_event_batches.remove(&entry.id);
-
     // CR 603.4: Intervening-if condition rechecked at resolution time.
     if let StackEntryKind::TriggeredAbility {
         condition: Some(ref condition),
@@ -3145,18 +3237,15 @@ fn resolve_batched(
     // CR 400.7j: clear the resolution-scoped self-move re-latch with the entry.
     state.resolution_source_relatch = None;
 
-    // Pop the run's entries (resolution order is back-to-front), cleaning the
-    // per-entry side tables exactly as `resolve_top` does for a single entry.
+    // Pop the run's entries (resolution order is back-to-front) through the same
+    // authority `resolve_top` uses for a single entry, so the per-entry side
+    // tables settle with each removal.
     let mut popped = Vec::with_capacity(consumed as usize);
     for _ in 0..consumed {
-        match state.stack.pop_back() {
-            Some(entry) => {
-                state.stack_paid_facts.remove(&entry.id);
-                state.stack_trigger_event_batches.remove(&entry.id);
-                popped.push(entry);
-            }
-            None => break,
-        }
+        let Some(removed) = pop_top_stack_entry(state) else {
+            break;
+        };
+        popped.push(removed.entry);
     }
 
     // CR 603.7c: Set the trigger event context once from the (identical) top
@@ -3482,13 +3571,11 @@ fn resolve_inert_noop_batch(
     // CR 400.7j: clear the resolution-scoped self-move re-latch with the entry.
     state.resolution_source_relatch = None;
     for _ in 0..consumed {
-        let Some(entry) = state.stack.pop_back() else {
+        let Some(removed) = pop_top_stack_entry(state) else {
             break;
         };
-        state.stack_paid_facts.remove(&entry.id);
-        state.stack_trigger_event_batches.remove(&entry.id);
         events.push(GameEvent::StackResolved {
-            object_id: entry.id,
+            object_id: removed.entry.id,
         });
     }
     consumed

crates/engine/src/types/mod.rs

@@ -92,8 +92,9 @@ pub use resolved_commands::{
     ResolvedPlayerEditCommand, ResolvedPlayerEditReplayInvariantError,
     ResolvedRngReplayInvariantError, ResolvedRulesCommand, ResolvedRulesJournal,
     ResolvedRulesJournalError, ResolvedStackEntryFinalizeCommand,
-    ResolvedStackEntryFinalizeReplayInvariantError, ResolvedStackPushCommand,
-    ResolvedStackPushOrigin, ResolvedStackPushReplayInvariantError, ResolvedTriggerCollection,
+    ResolvedStackEntryFinalizeReplayInvariantError, ResolvedStackPopCommand,
+    ResolvedStackPopReplayInvariantError, ResolvedStackPushCommand, ResolvedStackPushOrigin,
+    ResolvedStackPushReplayInvariantError, ResolvedTriggerCollection,
     ResolvedTriggerCollectionCommand, ResolvedTriggerCollectionReplayInvariantError,
     ResolvedTriggerLedgerEdit, ResolvedUncommittedTriggerRemovalCommand,
     ResolvedUncommittedTriggerRemovalReplayInvariantError, RulesExecutionNodeKind,

crates/engine/src/types/resolved_commands.rs

@@ -1151,6 +1151,47 @@ pub enum ResolvedUncommittedTriggerRemovalReplayInvariantError {
     UnexpectedRemovableEntry(ObjectId),
 }
 
+/// One exact CR 405.2 removal of the topmost 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.
+///
+/// NOT unified with [`ResolvedUncommittedTriggerRemovalCommand`], despite both
+/// removing a stack entry. The 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
+/// 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.
+///
+/// The removed side-table VALUES are deliberately not recorded, for the same
+/// reason the CR 603.3d removal omits them: this command installs nothing. It
+/// drops rows keyed on the recorded entry's own id, so no recorded value would
+/// 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 {
+    /// The removed entry, recorded verbatim so replay verifies the whole object
+    /// rather than trusting the id.
+    pub entry: Box<StackEntry>,
+    /// 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.
+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
+pub enum ResolvedStackPopReplayInvariantError {
+    #[error("stack pop 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,
+}
+
 /// Semantic command payload currently carried by a resolved-rules journal entry.
 ///
 /// Additional command families are intentionally added by their owning P2
@@ -1181,6 +1222,7 @@ pub enum ResolvedRulesCommand {
     StackPush(Box<ResolvedStackPushCommand>),
     StackEntryFinalize(Box<ResolvedStackEntryFinalizeCommand>),
     UncommittedTriggerRemoval(Box<ResolvedUncommittedTriggerRemovalCommand>),
+    StackPop(Box<ResolvedStackPopCommand>),
 }
 
 /// An append-only trigger collection command has no replay-time precondition.
@@ -2258,6 +2300,17 @@ impl ResolvedRulesJournal {
         )
     }
 
+    /// Records one exact CR 405.2 top-of-stack removal under its cause.
+    pub fn record_stack_pop(
+        &mut self,
+        command: ResolvedStackPopCommand,
+    ) -> Result<ResolvedCommandOrdinal, ResolvedRulesJournalError> {
+        self.append_command(
+            command.cause,
+            ResolvedRulesCommand::StackPop(Box::new(command)),
+        )
+    }
+
     fn begin_settlement(
         &mut self,
         identity_for: impl FnOnce(SettlementNodeOrdinal) -> RulesExecutionNodeRef,
@@ -2474,7 +2527,8 @@ impl ResolvedRulesJournal {
                 | ResolvedRulesCommand::TriggerCollection(_)
                 | ResolvedRulesCommand::StackPush(_)
                 | ResolvedRulesCommand::StackEntryFinalize(_)
-                | ResolvedRulesCommand::UncommittedTriggerRemoval(_) => {}
+                | ResolvedRulesCommand::UncommittedTriggerRemoval(_)
+                | ResolvedRulesCommand::StackPop(_) => {}
             }
         }
         for node in &self.nodes {
@@ -2938,6 +2992,19 @@ impl ResolvedRulesJournal {
                     }
                 }
             }
+            ResolvedRulesCommand::StackPop(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
+                // check them against.
+                if entry.node != command.cause {
+                    return Err(ResolvedRulesJournalError::InvalidSerializedAuthority(
+                        "stack pop command has an unrelated cause".to_string(),
+                    ));
+                }
+            }
         }
         Ok(())
     }

crates/engine/tests/integration/cr733_resolved_commands_p2.rs

@@ -144,6 +144,9 @@ 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();
+        }
     }
 }
 
@@ -247,9 +250,8 @@ fn exact_mana_spend_rejects_a_second_removal() {
             | ResolvedRulesCommand::TriggerCollection(_)
             | ResolvedRulesCommand::StackPush(_)
             | ResolvedRulesCommand::StackEntryFinalize(_)
-            | ResolvedRulesCommand::UncommittedTriggerRemoval(_) => {
-                apply_semantic_command(&mut replay, command)
-            }
+            | ResolvedRulesCommand::UncommittedTriggerRemoval(_)
+            | ResolvedRulesCommand::StackPop(_) => apply_semantic_command(&mut replay, command),
         }
     }
     assert!(

crates/engine/tests/integration/cr733_resolved_draw.rs

@@ -109,6 +109,9 @@ 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();
+        }
     }
 }