From 3fb3618e16e7085b25b7fb05d25509cc277fc7d6 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 26 Jul 2026 06:32:22 -0700 Subject: [PATCH 1/2] refactor(engine): extract the CR 405.2 top-of-stack removal authority The resolution pop, the batched-resolution drain, the inert no-op drain and the CR 724.1b stack exile each repeated the same three-line mutation: pop the top entry, then drop the two side-table rows keyed on it. Route all four through one `pop_top_stack_entry` authority that returns the entry together with both removed rows, so the resolution pop still binds the paid snapshot and the CR 603.7c batch it consumes while the drains discard them. The batch row is now dropped at pop time rather than after the KeywordAction early return. That is a no-op, not a behaviour change: batch rows are inserted only in the triggered-ability push path (`triggers.rs`, guarded by `trigger_events.len() > 1`), so a KeywordAction entry can never carry one. Deliberately NOT used by `pop_uncommitted_pending_trigger_entry`, which performs the same mutation but is a distinct CR 603.3d family with its own record; sharing the authority once it journals would record one mutation twice. --- crates/engine/src/game/effects/end_phase.rs | 6 +- crates/engine/src/game/stack.rs | 78 +++++++++++++++------ 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/crates/engine/src/game/effects/end_phase.rs b/crates/engine/src/game/effects/end_phase.rs index 085504f101..7507c6e5be 100644 --- a/crates/engine/src/game/effects/end_phase.rs +++ b/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, ) -> 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, diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index c30e37a9b2..47f70e8d7d 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -269,6 +269,48 @@ 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, + pub trigger_event_batch: Option>, +} + +/// 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 { + 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); + Some(PoppedStackEntry { + entry, + paid_facts, + trigger_event_batch, + }) +} + /// CR 603.3d: removes an uncommitted triggered ability from the stack. /// /// The "push first, choose second" invariant (see @@ -667,11 +709,14 @@ pub fn resolve_top(state: &mut GameState, events: &mut Vec) { 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 +731,6 @@ pub fn resolve_top(state: &mut GameState, events: &mut Vec) { 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 +3188,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 +3522,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 From 6fab47af0a7fb25d5879d0650768e762d4c09b6a Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 26 Jul 2026 07:10:55 -0700 Subject: [PATCH 2/2] feat(engine): journal the CR 405.2 top-of-stack removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pop_top_stack_entry` now records a `ResolvedStackPopCommand` once all three removals settle — the entry and both per-entry side-table rows — so the record describes a stack the entry has already left. A drain of N entries records N commands rather than one bulk removal, which is what lets a replay reproduce the removal ORDER and not merely the final depth. The command carries the entry, the resulting depth and the cause, and NOT the removed side-table values: a removal installs nothing, so no recorded value would pin an invariant, and carrying `Vec` batches would widen every journal entry on the hottest path in the engine. That mirrors the CR 603.3d removal's reasoning. Not unified with `ResolvedUncommittedTriggerRemovalCommand` despite both removing an entry: the axis would span CR 603.3d trigger-construction and CR 405.5 resolution, and the CR 603.3d removal consumes a cursor and may legitimately pop nothing — an outcome with no analogue here. `apply_resolved_stack_pop` checks the CR 405.2 depth and compares the top entry WHOLE (not by id, so a divergent object reusing the identifier is refused) before mutating, so a rejected replay leaves the stack and both side tables untouched. Flips the cross-pop canary the push suite's header anticipated. The existing `StackDepthMismatch` assertion there is about applying a push twice and is left alone; the new test shows, from ONE predecessor, the push alone still failing and pop-then-push succeeding. --- crates/engine/src/game/stack.rs | 51 ++- crates/engine/src/types/mod.rs | 5 +- crates/engine/src/types/resolved_commands.rs | 69 +++- .../integration/cr733_resolved_commands_p2.rs | 8 +- .../tests/integration/cr733_resolved_draw.rs | 3 + .../integration/cr733_resolved_stack_pop.rs | 332 ++++++++++++++++++ .../integration/cr733_resolved_stack_push.rs | 16 +- crates/engine/tests/integration/main.rs | 1 + 8 files changed, 473 insertions(+), 12 deletions(-) create mode 100644 crates/engine/tests/integration/cr733_resolved_stack_pop.rs diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index 47f70e8d7d..8b40f7224a 100644 --- a/crates/engine/src/game/stack.rs +++ b/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, }; @@ -304,6 +305,21 @@ pub(crate) fn pop_top_stack_entry(state: &mut GameState) -> Option Option 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 diff --git a/crates/engine/src/types/mod.rs b/crates/engine/src/types/mod.rs index 8297bbc24f..5708f14c58 100644 --- a/crates/engine/src/types/mod.rs +++ b/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, diff --git a/crates/engine/src/types/resolved_commands.rs b/crates/engine/src/types/resolved_commands.rs index 34017bc59e..4599ef4025 100644 --- a/crates/engine/src/types/resolved_commands.rs +++ b/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` 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, + /// 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), StackEntryFinalize(Box), UncommittedTriggerRemoval(Box), + StackPop(Box), } /// 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 { + 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(()) } diff --git a/crates/engine/tests/integration/cr733_resolved_commands_p2.rs b/crates/engine/tests/integration/cr733_resolved_commands_p2.rs index 8d33dd46a9..72763ae508 100644 --- a/crates/engine/tests/integration/cr733_resolved_commands_p2.rs +++ b/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!( diff --git a/crates/engine/tests/integration/cr733_resolved_draw.rs b/crates/engine/tests/integration/cr733_resolved_draw.rs index 8e8424b684..20a72809fa 100644 --- a/crates/engine/tests/integration/cr733_resolved_draw.rs +++ b/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(); + } } } diff --git a/crates/engine/tests/integration/cr733_resolved_stack_pop.rs b/crates/engine/tests/integration/cr733_resolved_stack_pop.rs new file mode 100644 index 0000000000..efe23366d3 --- /dev/null +++ b/crates/engine/tests/integration/cr733_resolved_stack_pop.rs @@ -0,0 +1,332 @@ +//! CR733 P2 coverage for the CR 405.2 top-of-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. +//! +//! 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. +//! +//! THE CROSS-POP CANARY IS THE ACCEPTANCE TEST FOR THIS FAMILY. Before pops were +//! journaled, `apply_resolved_stack_push` failed `StackDepthMismatch` on any +//! replay whose prefix crossed a removal, and the push suite's module header +//! recorded that every replay there deliberately used a pop-free prefix. +//! `a_recorded_pop_unblocks_a_later_push_replay` flips exactly that: from ONE +//! predecessor it shows the push alone still failing and pop-then-push +//! 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::stack::{apply_resolved_stack_pop, apply_resolved_stack_push}; +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, +}; + +/// A no-target sorcery: nothing to retarget, and no trigger to add stack entries +/// the pop assertions would have to filter around. +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 { + state + .resolved_rules_journal + .entries() + .iter() + .skip(from) + .filter_map(|entry| entry.command.clone()) + .filter_map(|command| match command { + ResolvedRulesCommand::StackPop(command) => Some(*command), + _ => None, + }) + .collect() +} + +/// Every stack push journaled after `from`, in journal order. +fn stack_pushes(state: &GameState, from: usize) -> Vec { + state + .resolved_rules_journal + .entries() + .iter() + .skip(from) + .filter_map(|entry| entry.command.clone()) + .filter_map(|command| match command { + ResolvedRulesCommand::StackPush(command) => Some(*command), + _ => None, + }) + .collect() +} + +fn cast(runner: &mut GameRunner, spell: ObjectId) { + let card_id = runner.state().objects[&spell].card_id; + runner + .act(GameAction::CastSpell { + object_id: spell, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("the real cast must put the spell on the stack"); +} + +fn scenario_with_spells(names: &[&str]) -> (GameRunner, Vec) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let ids = names + .iter() + .map(|name| { + scenario + .add_spell_to_hand_from_oracle(P0, name, true, ELVISH_TOKEN_SPELL) + .with_mana_cost(ManaCost::zero()) + .id() + }) + .collect(); + (scenario.build(), ids) +} + +#[test] +fn resolving_a_spell_journals_an_exact_pop() { + let (mut runner, ids) = scenario_with_spells(&["Journal Pop One"]); + let spell = ids[0]; + cast(&mut runner, spell); + + // Reach guard: the pop assertions below are meaningless unless the spell is + // genuinely on the stack first. + let before_pop = runner.state().clone(); + assert_eq!( + before_pop.stack.len(), + 1, + "CR 405.1: the cast spell is the only object on the stack" + ); + let journal_start = before_pop.resolved_rules_journal.entries().len(); + + runner.resolve_top(); + + // The discriminating assertion: the removal is journaled. A raw + // `stack.pop_back()` records nothing here. + let pops = stack_pops(runner.state(), journal_start); + let recorded: Vec<_> = pops.iter().filter(|pop| pop.entry.id == spell).collect(); + assert_eq!( + recorded.len(), + 1, + "CR 405.5: resolving the spell must journal exactly one pop for it" + ); + let pop = recorded[0]; + assert_eq!( + pop.resulting_depth, 0, + "CR 405.2: the recorded depth is the depth AFTER the removal" + ); + assert_eq!( + *pop.entry, before_pop.stack[0], + "the recorded entry is the entry that was on the stack, verbatim" + ); + + // 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) + .expect("the recorded pop must replay against its captured predecessor"); + assert!(replay.stack.is_empty(), "replay removes the recorded entry"); + assert!( + !replay.stack_paid_facts.contains_key(&spell), + "replay drops the paid-facts row keyed on the removed entry" + ); + assert!( + !replay.stack_trigger_event_batches.contains_key(&spell), + "replay drops the trigger-batch row keyed on the removed entry" + ); + + // Re-applying is not idempotent: the stack is now shallower than recorded, + // so it fails closed rather than popping an unrelated object. + assert!( + matches!( + apply_resolved_stack_pop(&mut replay, pop), + Err(ResolvedStackPopReplayInvariantError::DepthMismatch { + expected: 1, + found: 0 + }) + ), + "a stack pop is not idempotent: a second application must fail closed" + ); +} + +/// Each probe diverges exactly one axis, so a rejection can only come from the +/// precondition being probed. +#[test] +fn pop_rejects_a_divergent_predecessor() { + let (mut runner, ids) = scenario_with_spells(&["Journal Pop Two"]); + let spell = ids[0]; + cast(&mut runner, spell); + 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 pop = pops + .iter() + .find(|pop| pop.entry.id == spell) + .expect("the resolution journaled a pop"); + + // Right entry, wrong depth: a deeper stack means the replay is not at the + // point the record describes. + let mut too_deep = before_pop.clone(); + let mut duplicate = before_pop.stack[0].clone(); + duplicate.id = ObjectId(9999); + too_deep.stack.push_front(duplicate); + assert!( + matches!( + apply_resolved_stack_pop(&mut too_deep, pop), + Err(ResolvedStackPopReplayInvariantError::DepthMismatch { + expected: 1, + found: 2 + }) + ), + "a pop must refuse a predecessor at the wrong depth" + ); + assert_eq!( + too_deep.stack.len(), + 2, + "the rejected replay must not have mutated the stack" + ); + + // Right depth, wrong entry on top. Comparing the entry WHOLE rather than by + // id is what catches this — an applier matching on `id` alone would happily + // discard a divergent object that reused the identifier. + let mut wrong_top = before_pop.clone(); + wrong_top + .stack + .back_mut() + .expect("the predecessor has the entry on top") + .source_id = ObjectId(4242); + assert!( + matches!( + apply_resolved_stack_pop(&mut wrong_top, pop), + Err(ResolvedStackPopReplayInvariantError::PoppedEntryMismatch) + ), + "a pop must refuse a predecessor whose top entry diverges from the record" + ); + assert_eq!( + wrong_top.stack.len(), + 1, + "the rejected replay must not have mutated the stack" + ); +} + +/// A drain records one command per entry, in removal order — not one bulk +/// removal that only pins the final depth. +#[test] +fn resolving_two_spells_journals_two_pops_in_lifo_order() { + let (mut runner, ids) = scenario_with_spells(&["Journal Pop Lower", "Journal Pop Upper"]); + let (lower, upper) = (ids[0], ids[1]); + cast(&mut runner, lower); + cast(&mut runner, upper); + + // Reach guard: both entries are live, and `upper` is the one CR 405.5 will + // remove first. + let before = runner.state().clone(); + assert_eq!(before.stack.len(), 2, "both spells are on the stack"); + assert_eq!( + before.stack.back().map(|entry| entry.id), + Some(upper), + "CR 405.2: the last-added spell is on top" + ); + let journal_start = before.resolved_rules_journal.entries().len(); + + runner.advance_until_stack_empty(); + + let pops: Vec<_> = stack_pops(runner.state(), journal_start) + .into_iter() + .filter(|pop| pop.entry.id == lower || pop.entry.id == upper) + .collect(); + assert_eq!(pops.len(), 2, "each removal is journaled separately"); + assert_eq!( + (pops[0].entry.id, pops[1].entry.id), + (upper, lower), + "CR 405.5: the last-added object resolves first, and the records carry \ + that order" + ); + assert_eq!( + (pops[0].resulting_depth, pops[1].resulting_depth), + (1, 0), + "the recorded depths descend one per removal, which a single bulk record \ + could not express" + ); +} + +/// THE CANARY FLIP. See the module header: before this family, a replay whose +/// prefix crossed a pop failed `StackDepthMismatch` in the push applier. +#[test] +fn a_recorded_pop_unblocks_a_later_push_replay() { + let (mut runner, ids) = scenario_with_spells(&["Journal Pop First", "Journal Push Second"]); + let (first, second) = (ids[0], ids[1]); + + cast(&mut runner, first); + let predecessor = runner.state().clone(); + assert_eq!( + predecessor.stack.len(), + 1, + "reach guard: the replay predecessor has the first spell on the stack" + ); + let journal_start = predecessor.resolved_rules_journal.entries().len(); + + // Resolve the first spell (journals a pop), then cast the second (journals a + // push at the depth the pop left behind). + runner.resolve_top(); + cast(&mut runner, second); + + let pop = stack_pops(runner.state(), journal_start) + .into_iter() + .find(|pop| pop.entry.id == first) + .expect("resolving the first spell journaled its pop"); + let push = stack_pushes(runner.state(), journal_start) + .into_iter() + .find(|push| push.entry.id == second) + .expect("casting the second spell journaled its push"); + assert_eq!( + push.resulting_position, 0, + "CR 405.2: the second spell lands at index 0 precisely because the pop \ + emptied the stack first" + ); + + // BEFORE-half: the push alone cannot replay against this predecessor, + // because the un-removed first spell leaves the stack one deeper than the + // push recorded. This is the exact failure the push suite's header records. + let mut push_only = predecessor.clone(); + assert!( + matches!( + apply_resolved_stack_push(&mut push_only, &push), + Err(ResolvedStackPushReplayInvariantError::StackDepthMismatch { + expected: 0, + found: 1 + }) + ), + "the push record must still fail against a predecessor that has not had \ + the pop applied — otherwise this test proves nothing about the pop" + ); + + // AFTER-half: same predecessor, pop applied first, and the push now lands. + let mut sequenced = predecessor.clone(); + apply_resolved_stack_pop(&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"); + assert_eq!( + sequenced.stack.len(), + 1, + "the sequenced replay leaves exactly the second spell on the stack" + ); + assert_eq!( + sequenced.stack[push.resulting_position], *push.entry, + "CR 405.2: the replay installs the recorded entry at the recorded index" + ); +} diff --git a/crates/engine/tests/integration/cr733_resolved_stack_push.rs b/crates/engine/tests/integration/cr733_resolved_stack_push.rs index 42128a4405..4a2337fd5e 100644 --- a/crates/engine/tests/integration/cr733_resolved_stack_push.rs +++ b/crates/engine/tests/integration/cr733_resolved_stack_push.rs @@ -17,11 +17,17 @@ //! point is unobservable. Do not replace it with a simpler card. //! //! Every replay below applies a recorded push to a prefix containing no stack -//! POP, because pops are not journaled yet (CR 608.1 / CR 603.3c/d / CR 701.6a -//! / CR 601.2a). Past the first un-journaled removal the recorded -//! `resulting_position` no longer matches the replayed depth and the applier -//! fails closed by design — so a future test that replays across a pop should -//! assert `StackDepthMismatch`, not relax the precondition. +//! POP, and the `StackDepthMismatch` assertions here are about a push applied +//! TWICE — a push is not idempotent — not about crossing a removal. +//! +//! The CR 405.2 top-of-stack pop IS journaled now +//! (`cr733_resolved_stack_pop.rs`), so a replay may cross one by applying the +//! recorded pop first; `a_recorded_pop_unblocks_a_later_push_replay` in that +//! file demonstrates exactly that against a predecessor where the push alone +//! still fails. Removals that remain un-journaled (remove-at-index and +//! retain-by-predicate, CR 701.6a and the CR 800.4a elimination sweep) still +//! make a crossing replay fail closed by design — the fix there is to journal +//! them, never to relax this applier's precondition. use engine::game::scenario::{GameRunner, GameScenario, P0}; use engine::game::stack::apply_resolved_stack_push; diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index a84a1fc1a9..83f6ee17cb 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -105,6 +105,7 @@ 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_token_creation; mod cr733_resolved_transform;