diff --git a/crates/engine/src/game/effects/counters.rs b/crates/engine/src/game/effects/counters.rs index 7310b30eb2..4103db1b11 100644 --- a/crates/engine/src/game/effects/counters.rs +++ b/crates/engine/src/game/effects/counters.rs @@ -20,6 +20,7 @@ use crate::types::game_state::{ use crate::types::identifiers::{ObjectId, ObjectIncarnationRef}; use crate::types::player::PlayerId; use crate::types::proposed_event::{CounterMoveStage, CounterPlacement, ProposedEvent}; +use crate::types::resolution::FrameGate; use crate::types::resolved_commands::{ ResolvedObjectCounterCommand, ResolvedObjectCounterEdit, ResolvedObjectCounterReplayInvariantError, @@ -336,7 +337,7 @@ fn merge_pending_counter_completion_after_nested_pause( completion: PendingEffectResolved, ) { let Some(queue) = state.active_counter_additions_mut() else { - stash_pending_counter_additions(state, Vec::new(), completion); + park_counter_completion_outside_active_direct_choice(state, completion); return; }; @@ -369,6 +370,70 @@ fn merge_pending_counter_completion_after_nested_pause( } } +/// CR 608.2c + CR 616.1: Park a completion that outlived a paused post-action +/// when no counter-additions queue is active to absorb it. +/// +/// The default is unchanged — push the completion as the active inner frame. +/// The one exception is a pause that installed a direct-choice owner (a fresh +/// `ProliferateChoice`, say). That owner must stay at the stack top until its +/// action handler consumes it — `ResolutionStack::validate` rejects a buried +/// direct-choice owner — so pushing on top of it would corrupt the stack exactly +/// the way issue #7384 did. There the completion becomes the owner's PARENT +/// instead and runs once the owner is consumed, preserving the instruction +/// order; and when it owes nothing at all it is dropped rather than parked, so +/// no empty frame is installed above a live prompt. +/// +/// Note that a completion parked as a parent is no longer the ACTIVE queue, so a +/// later `append_pending_counter_post_actions` would not find it. No such +/// appender is reachable while a direct-choice prompt is live, and the ordering +/// caveat on `ContinueProliferateActions` records the condition that would +/// change that. +fn park_counter_completion_outside_active_direct_choice( + state: &mut GameState, + completion: PendingEffectResolved, +) { + let active_owns_prompt = state + .resolution_stack + .last() + .is_some_and(|frame| matches!(frame.gate(), FrameGate::DirectChoice(_))); + if !active_owns_prompt { + // Every non-direct-choice pause keeps its historical shape, including + // the empty placeholder frame that a later + // `append_pending_counter_post_actions` may still land work on. + stash_pending_counter_additions(state, Vec::new(), completion); + return; + } + if completion.is_noop() { + return; + } + let queue = PendingCounterAdditionQueue { + remaining: Vec::new(), + completion: Some(completion), + }; + if state + .insert_counter_additions_parent_of_active(queue) + .is_err() + { + // Unreachable from a valid stack: the guard above proves an active child + // exists, and inserting BELOW the top leaves the top — and so the prompt + // gate — untouched. The insert validates a CLONE and assigns only on + // success, so a failure leaves both stack and journal untouched. + // + // A failure therefore means the stack was ALREADY invalid, and the two + // recoveries are not symmetric. Pushing the queue instead would stack an + // owner above a live prompt, adding a SECOND validate violation to a + // stack that already has one; dropping the completion forfeits its + // terminal event but leaves the stack no worse than it was found. The + // drop is the deliberate choice: compounding stack corruption is what + // makes this class unrecoverable, and panicking is the very failure mode + // #7384 reported. + debug_assert!( + false, + "inserting a counter-additions parent below a direct-choice owner must validate" + ); + } +} + pub(crate) fn drain_pending_counter_additions(state: &mut GameState, events: &mut Vec) { while let Some(mut queue) = state.active_counter_additions().cloned() { let Some(next) = queue.remaining.first().cloned() else { @@ -484,6 +549,14 @@ fn apply_pending_counter_post_action( }); true } + // CR 701.34a: The interrupted proliferate action is now complete — + // publish it and drive whatever actions the effect still owes. Returns + // `false` when another `ProliferateChoice` is open; the completion this + // ran from is empty by construction, so nothing is re-parked and the + // fresh direct-choice frame is left owning the stack top. + PendingCounterPostAction::ContinueProliferateActions { pending } => { + super::proliferate::continue_proliferate_actions(state, pending, events) + } PendingCounterPostAction::AddSubtype { object_id, subtype } => { if let Some(obj) = state.objects.get_mut(&object_id) { if !obj @@ -6850,4 +6923,108 @@ mod tests { ); } } + + /// Building-block rows for `park_counter_completion_outside_active_direct_choice` + /// (issue #7384). A post-action may pause having installed a direct-choice + /// owner; `ResolutionStack::validate` rejects a buried direct-choice owner, + /// so the completion that outlives the pause may not simply be pushed on top + /// of it. + mod parking_a_completion_after_a_paused_post_action { + use super::*; + use crate::types::game_state::{PendingEffectResolutionEvent, PendingEffectResolved}; + use crate::types::resolution::{FrameKind, PendingProliferateActions}; + + fn live_proliferate_prompt() -> GameState { + let mut state = GameState::new_two_player(42); + state + .install_direct_choice_frame( + ResolutionFrame::Proliferate(PendingProliferateActions { + actor: PlayerId(0), + source_id: ObjectId(77), + remaining: 1, + }), + WaitingFor::ProliferateChoice { + player: PlayerId(0), + eligible: vec![TargetRef::Player(PlayerId(0))], + }, + ) + .expect("a proliferate owner installs with its own prompt"); + state + } + + fn owed_completion() -> PendingEffectResolved { + PendingEffectResolved::new(EffectKind::Proliferate, ObjectId(77)) + } + + /// THE row this branch exists for: the completion goes BELOW the live + /// prompt, and the resulting stack still validates. Pushing it on top + /// instead is exactly the corruption #7384 reported. + #[test] + fn a_completion_owed_behind_a_live_prompt_becomes_its_parent() { + let mut state = live_proliferate_prompt(); + + merge_pending_counter_completion_after_nested_pause(&mut state, owed_completion()); + + let frames: Vec<_> = state.resolution_stack.iter().map(|f| f.kind()).collect(); + assert_eq!( + frames, + vec![FrameKind::CounterAdditions, FrameKind::Proliferate], + "the owed completion parks BELOW the direct-choice owner, which keeps \ + the stack top — and so the prompt gate — untouched" + ); + state + .resolution_stack + .validate(&state.waiting_for) + .expect("a direct-choice owner with a parent completion is a valid stack"); + } + + /// A pause that owes nothing installs no frame at all — an empty owner + /// above a live prompt would bury it for no benefit. + #[test] + fn a_completion_owing_nothing_behind_a_live_prompt_is_dropped() { + let mut state = live_proliferate_prompt(); + let spent = PendingEffectResolved { + resolution_event: PendingEffectResolutionEvent::Suppress, + post_actions: Vec::new(), + player_action: None, + ..owed_completion() + }; + assert!(spent.is_noop(), "the row's premise: nothing is owed"); + + merge_pending_counter_completion_after_nested_pause(&mut state, spent); + + let frames: Vec<_> = state.resolution_stack.iter().map(|f| f.kind()).collect(); + assert_eq!( + frames, + vec![FrameKind::Proliferate], + "nothing owed means nothing parked" + ); + } + + /// The historical shape is preserved for every pause that did NOT + /// install a direct-choice owner — including an empty completion, whose + /// placeholder frame a later `append_pending_counter_post_actions` may + /// still land work on. + #[test] + fn a_pause_without_a_live_prompt_still_pushes_its_placeholder_frame() { + let mut state = GameState::new_two_player(42); + let spent = PendingEffectResolved { + resolution_event: PendingEffectResolutionEvent::Suppress, + post_actions: Vec::new(), + player_action: None, + ..owed_completion() + }; + assert!(spent.is_noop()); + + merge_pending_counter_completion_after_nested_pause(&mut state, spent); + + let frames: Vec<_> = state.resolution_stack.iter().map(|f| f.kind()).collect(); + assert_eq!( + frames, + vec![FrameKind::CounterAdditions], + "a non-direct-choice pause keeps the placeholder frame it has always \ + pushed, so a later append still has a queue to land on" + ); + } + } } diff --git a/crates/engine/src/game/effects/proliferate.rs b/crates/engine/src/game/effects/proliferate.rs index 940952b012..d65a026db7 100644 --- a/crates/engine/src/game/effects/proliferate.rs +++ b/crates/engine/src/game/effects/proliferate.rs @@ -101,19 +101,11 @@ fn drive_single_proliferate_action( return true; } - if remaining_after_this > 0 { - state.push_proliferate_frame(PendingProliferateActions { - actor, - source_id, - remaining: remaining_after_this, - }); - } else { - state.push_proliferate_frame(PendingProliferateActions { - actor, - source_id, - remaining: 0, - }); - } + state.push_proliferate_frame(PendingProliferateActions { + actor, + source_id, + remaining: remaining_after_this, + }); state.waiting_for = WaitingFor::ProliferateChoice { player: actor, @@ -180,6 +172,48 @@ pub fn resume_proliferate_actions( ) } +/// CR 701.34a + CR 608.2c: The single authority for finishing one proliferate +/// action and continuing to the next. +/// +/// Publishes the player-action event for the action that just completed — so +/// "whenever you proliferate" triggers observe each action in the order it +/// happened — then drives the actions still owed, emitting `EffectResolved` +/// only once every one of them has finished. +/// +/// Returns `false` when a further `ProliferateChoice` is now open, in which case +/// the fresh frame owns finishing the resolution on its own handler cycle. +/// +/// Both the direct `ProliferateChoice` handler and the +/// `ContinueProliferateActions` post-action route through here, so a PROMPTED +/// action's ordering is stated exactly once (issue #7384). CR 701.34a is that +/// every action emits exactly one player-action event, from one of two sites: +/// an action with no eligible permanents or players never prompts and is +/// published by `emit_empty_proliferate_action` instead — do not delete that +/// emit on the strength of this one. +pub(crate) fn continue_proliferate_actions( + state: &mut GameState, + pending: PendingProliferateActions, + events: &mut Vec, +) -> bool { + let source_id = pending.source_id; + events.push(GameEvent::PlayerPerformedAction { + player_id: pending.actor, + action: PlayerActionKind::Proliferate, + look_count: None, + scry_bottom_count: None, + scry_top_count: None, + }); + if !resume_proliferate_actions(state, pending, events) { + return false; + } + events.push(GameEvent::EffectResolved { + kind: EffectKind::Proliferate, + source_id, + subject: None, + }); + true +} + /// CR 614.6 + CR 614.11: Single authority for propose → replace → apply. pub(crate) fn proliferate_through_replacement( state: &mut GameState, @@ -234,9 +268,10 @@ pub fn resolve( Ok(()) } -/// CR 701.34a (operation) + CR 122.1: Resolve `Effect::ProliferateTarget` — the -/// forced single-target form ("for each kind of counter on target permanent or -/// player, give that permanent or player another counter of that kind"). +/// CR 608.2c + CR 122.1: Resolve `Effect::ProliferateTarget` as its direct +/// counter instruction ("for each kind of counter on target permanent or player, +/// give that permanent or player another counter of that kind"). This is not the +/// CR 701.34a proliferate keyword action. /// /// Unlike `resolve` (the chooser-driven `Proliferate`), the target is already /// fixed in `ability.targets`, so there is no `ProliferateChoice` prompt: it @@ -249,9 +284,23 @@ pub fn resolve_target( ability: &ResolvedAbility, events: &mut Vec, ) -> Result<(), EffectError> { - apply_proliferate(state, ability.controller, &ability.targets, events); + let kind = EffectKind::from(&ability.effect); + // CR 614.1a: a counter-placement replacement may pause mid-application. The + // completion carries this effect's own identity so the paused path emits the + // same single `EffectResolved` the synchronous path does — and no + // `PlayerActionKind::Proliferate`, which this forced-target form must never + // publish. + if !apply_proliferate( + state, + ability.controller, + &ability.targets, + PendingEffectResolved::new(kind, ability.source_id), + events, + ) { + return Ok(()); + } events.push(GameEvent::EffectResolved { - kind: EffectKind::from(&ability.effect), + kind, source_id: ability.source_id, subject: None, }); @@ -260,10 +309,22 @@ pub fn resolve_target( /// Apply proliferate to the selected targets — adds one counter of each kind /// already present. Called from the engine handler after player makes their choice. +/// +/// `completion` is the work owed once every counter has landed. It is supplied +/// by the caller rather than assumed here because the two callers owe different +/// things: the chooser-driven `Proliferate` continues its remaining actions and +/// publishes `PlayerActionKind::Proliferate`, while `Effect::ProliferateTarget` +/// must do neither (CR 122.1 — the card spells out the counter-add instead of +/// using the keyword action, so it must not fire "whenever you proliferate"). +/// +/// Returns `false` when a counter-placement replacement opened a choice; the +/// remaining additions and `completion` are parked on a `CounterAdditions` frame +/// and resume through `drain_pending_counter_additions`. pub fn apply_proliferate( state: &mut GameState, actor: PlayerId, selected: &[TargetRef], + completion: PendingEffectResolved, events: &mut Vec, ) -> bool { for target in selected { @@ -275,12 +336,6 @@ pub fn apply_proliferate( } let additions = proliferate_addition_plan(state, actor, selected); - let completion = PendingEffectResolved::with_player_action( - EffectKind::Proliferate, - crate::types::identifiers::ObjectId(0), - actor, - PlayerActionKind::Proliferate, - ); for (index, addition) in additions.iter().cloned().enumerate() { if !apply_counter_addition_plan_item(state, addition, events) { @@ -402,6 +457,7 @@ mod tests { Effect, QuantityModification, ReplacementDefinition, ReplacementPlayerScope, TargetFilter, TypedFilter, }; + use crate::types::game_state::{PendingCounterPostAction, PendingEffectResolutionEvent}; use crate::types::identifiers::{CardId, ObjectId}; use crate::types::player::PlayerId; use crate::types::replacements::ReplacementEvent; @@ -411,6 +467,13 @@ mod tests { ResolvedAbility::new(Effect::Proliferate, vec![], ObjectId(100), PlayerId(0)) } + /// A completion for rows that exercise counter placement only and never + /// reach the paused path. Rows that assert the parked completion's shape + /// build the real chooser-driven one instead. + fn proliferate_test_completion() -> PendingEffectResolved { + PendingEffectResolved::new(EffectKind::Proliferate, ObjectId(100)) + } + /// CR 701.34a + CR 614.1a: Tekuthal-style proliferate replacement doubles /// the action count via `repeat_for` on the execute ability. #[test] @@ -533,6 +596,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj1)], + proliferate_test_completion(), &mut events, ); @@ -567,6 +631,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj)], + proliferate_test_completion(), &mut events, ); @@ -599,6 +664,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj)], + proliferate_test_completion(), &mut events, ); @@ -756,6 +822,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Player(PlayerId(1))], + proliferate_test_completion(), &mut events, ); @@ -839,11 +906,26 @@ mod tests { object.counters.insert(CounterType::Plus1Plus1, 1); object.counters.insert(CounterType::Stun, 1); + // The completion the `ProliferateChoice` handler actually supplies: the + // player-action event and the remaining actions both ride + // `ContinueProliferateActions`, so the completion itself owes neither. + let pending_frame = PendingProliferateActions { + actor: PlayerId(0), + source_id: ObjectId(77), + remaining: 1, + }; let mut events = Vec::new(); assert!(!apply_proliferate( &mut state, PlayerId(0), &[TargetRef::Object(obj)], + PendingEffectResolved::with_post_actions_without_effect( + EffectKind::Proliferate, + pending_frame.source_id, + vec![PendingCounterPostAction::ContinueProliferateActions { + pending: pending_frame.clone(), + }], + ), &mut events, )); @@ -851,19 +933,36 @@ mod tests { state.waiting_for, WaitingFor::ReplacementChoice { .. } )); - let pending = state + let queued = state .active_counter_additions() .expect("remaining proliferate additions should be queued"); - assert_eq!(pending.remaining.len(), 1); + assert_eq!(queued.remaining.len(), 1); + let completion = queued + .completion + .as_ref() + .expect("the paused proliferate must park its completion"); + assert_eq!(completion.kind, EffectKind::Proliferate); + assert_eq!( + completion.source_id, pending_frame.source_id, + "the completion must carry the real source, not ObjectId(0)" + ); + assert!( + completion.player_action.is_none(), + "the proliferate player action is published by ContinueProliferateActions, \ + so parking it here too would double-fire proliferate triggers" + ); assert!(matches!( - pending.completion, - Some(PendingEffectResolved { - kind: EffectKind::Proliferate, - source_id: ObjectId(0), - player_action: Some(_), - .. - }) + completion.resolution_event, + PendingEffectResolutionEvent::Suppress )); + assert_eq!( + completion.post_actions, + vec![PendingCounterPostAction::ContinueProliferateActions { + pending: pending_frame + }], + "the parked completion must carry the resume that keeps the remaining \ + proliferate actions alive across the replacement choice" + ); } #[test] @@ -904,6 +1003,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj)], + proliferate_test_completion(), &mut events, ); @@ -941,6 +1041,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Player(PlayerId(1))], + proliferate_test_completion(), &mut events, ); @@ -978,6 +1079,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Player(PlayerId(1))], + proliferate_test_completion(), &mut events, ); @@ -1071,6 +1173,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj)], + proliferate_test_completion(), &mut events, ); @@ -1130,6 +1233,7 @@ mod tests { &mut state, PlayerId(0), &[TargetRef::Object(obj)], + proliferate_test_completion(), &mut events, ); diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 6c1e9575f9..d950494396 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -8,12 +8,13 @@ use crate::types::ability::{EffectScope, TapStateChange}; use crate::types::actions::{ DebugAction, GameAction, MayTriggerAutoChoiceOp, PriorityYieldOp, TriggerOrderTemplateOp, }; -use crate::types::events::{BendingType, ContestRound, GameEvent, ManaTapState, PlayerActionKind}; +use crate::types::events::{BendingType, ContestRound, GameEvent, ManaTapState}; use crate::types::game_state::{ ActionResult, AssistState, AutoMayChoice, AutoPassMode, AutoPassRequest, CastOfferKind, CastingVariant, ConvokeMode, CostResume, GameState, LandPlayRecord, LoopDetectionMode, - ManaAbilityResume, MayTriggerAutoChoiceKey, PayCostKind, PendingCostMoveResume, RetargetScope, - StackEntry, StackEntryKind, WaitingFor, + ManaAbilityResume, MayTriggerAutoChoiceKey, PayCostKind, PendingCostMoveResume, + PendingCounterPostAction, PendingEffectResolved, RetargetScope, StackEntry, StackEntryKind, + WaitingFor, }; use crate::types::identifiers::{CardId, DelayedTriggerOrigin, ObjectId, ObjectIncarnationRef}; use crate::types::match_config::MatchType; @@ -11597,21 +11598,14 @@ fn apply_action( )); } } - if !effects::proliferate::apply_proliferate(state, p, &targets, &mut events) { - return Ok(ActionResult { - events, - waiting_for: state.waiting_for.clone(), - log_entries: vec![], - }); - } - // CR 701.34a: Emit player-action event so proliferate triggers fire. - events.push(GameEvent::PlayerPerformedAction { - player_id: p, - action: PlayerActionKind::Proliferate, - look_count: None, - scry_bottom_count: None, - scry_top_count: None, - }); + // CR 701.34a + issue #7384: take the frame BEFORE applying counters. + // A counter-placement replacement can pause mid-application, and any + // path that returns while this direct-choice frame is still resident + // strands it on the resolution stack — every later frame transition + // then fails `ResolutionStack::validate` against a prompt that has + // long since moved on. A wrong stack top degrades to a rejected + // action here rather than to silent corruption, because + // `take_active_proliferate_frame` reports `UnexpectedTop`. let pending = state .take_active_proliferate_frame() .map_err(|error| EngineError::InvalidAction(error.to_string()))? @@ -11624,6 +11618,15 @@ fn apply_action( // (Pentad's charge) — never "all eligible", which could grow an opponent's // counters/poison and introduce a loss axis. Slot source = the trigger source (Kilo); // `index: 0` (distinct source from the Relic tap-cost/color pins). + // + // Recorded BEFORE the counters are applied: a counter-placement + // replacement can pause `apply_proliferate`, and that path returns + // early. Leaving the pin below it would silently drop the pin on + // exactly the proliferate this fix made complete, falling back to + // the "all eligible" replay this comment rules out. Everything read + // here — `state`, `targets`, `p`, `completion_source` — is already + // settled, and `object_decision_source` resolves card identity, + // which the pending counters do not affect. if let Some(source) = object_decision_source(state, completion_source) { let target_pins: Vec = targets .iter() @@ -11651,17 +11654,37 @@ fn apply_action( ); } } - if !effects::proliferate::resume_proliferate_actions(state, pending, &mut events) { + // The player-action event and any remaining actions are owed once + // the counters land, so they ride the completion rather than being + // emitted here — `continue_proliferate_actions` is the single + // authority for both, on the synchronous and paused paths alike. + let completion = PendingEffectResolved::with_post_actions_without_effect( + crate::types::ability::EffectKind::Proliferate, + completion_source, + vec![PendingCounterPostAction::ContinueProliferateActions { + pending: pending.clone(), + }], + ); + if !effects::proliferate::apply_proliferate( + state, + p, + &targets, + completion, + &mut events, + ) { + return Ok(ActionResult { + events, + waiting_for: state.waiting_for.clone(), + log_entries: vec![], + }); + } + if !effects::proliferate::continue_proliferate_actions(state, pending, &mut events) { return Ok(ActionResult { events, waiting_for: state.waiting_for.clone(), log_entries: vec![], }); } - events.push(GameEvent::EffectResolved { - kind: crate::types::ability::EffectKind::Proliferate, - source_id: completion_source, - subject: None,}); state.waiting_for = WaitingFor::Priority { player: p }; state.priority_player = p; resume_pending_continuation_if_priority(state, &mut events)?; @@ -18938,7 +18961,36 @@ mod stage2_injector_tests { // #7320's random-discard continuation adds ten lines above this producer in the // merged tree. Re-derived by the exact producer text at `:12773`, not by carrying // the prior coordinate. - "game/engine.rs:12773".to_string(), + // Proliferate frame-orphan fix (#7384): `:12773 ⇒ :12796`, +23, and ONLY this + // engine.rs entry moved — the four `effects/mod.rs` + + // `scoped_library_search` entries were re-read byte-identical AND in + // place, which is the set-preservation evidence. `git diff -U0` on this + // file has exactly seven hunks; six sit at `:11`–`:11687`, entirely ABOVE + // this producer: net `0` (a dropped `PlayerActionKind` import), `+1` (the + // `game_state` import list gaining a line), `-7` and `+9` (the + // `ProliferateChoice` handler taking its frame BEFORE applying counters), + // `+24` (the loop-pin block moved above `apply_proliferate`, plus the + // completion construction) and `-4` (the terminal `EffectResolved` push + // moving into `continue_proliferate_actions`). `0+1-7+9+24-4 = +23`, and + // predicted `12773+23` equals the observed coordinate exactly. The seventh + // and only remaining hunk is THIS drift note, which sits at `:18964` — + // below the producer — so nothing that moved it is unaccounted for. + // Deliberately stated WITHOUT pinning that hunk's own line count or the + // whole-file delta: this note is self-referential, its length feeds any + // such total, and the previous revision of this row asserted a + // whole-file figure that its own next wording edit falsified by exactly + // the size of that edit. The six above-producer hunks are the whole + // load-bearing claim; the seventh is identified by position, which no + // rewording can invalidate. + // None of it mints a prompt: the handler consumes an ALREADY-minted + // `ProliferateChoice`, and the completion defers a keyword action rather + // than creating a recipient, so the census set is still exactly 5. + // Identity re-established, not assumed, on BOTH controls this row uses: + // the line at `:12796` is sha256-identical (`8a544e87…5cc7d63`) to + // `origin/main:crates/engine/src/game/engine.rs:12773`, and its offset from + // `begin_pending_trigger_target_selection` (`:12662`) is STILL 134 — the + // control that caught this row's one historical SILENT drift. + "game/engine.rs:12796".to_string(), ], "the five production producers, NAMED: the CR 603.5 gate in `resolve_chain_body` \ plus the two repeated-optional-payment drivers, the per-player acceptance cursor \ diff --git a/crates/engine/src/game/engine_tests.rs b/crates/engine/src/game/engine_tests.rs index db8b8ac37f..cde84f7eb6 100644 --- a/crates/engine/src/game/engine_tests.rs +++ b/crates/engine/src/game/engine_tests.rs @@ -14,6 +14,7 @@ use crate::types::ability::{ use crate::types::card_type::CardType; use crate::types::card_type::{CoreType, Supertype}; use crate::types::counter::CounterType; +use crate::types::events::PlayerActionKind; use crate::types::format::FormatConfig; use crate::types::game_state::{ CastPaymentMode, CastingVariant, PendingCast, ProductionOverride, TargetSelectionProgress, diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index a2806b02e1..2212d7691a 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -5328,6 +5328,20 @@ impl PendingEffectResolved { player_action: Some(PendingPlayerAction { player_id, action }), } } + + /// Whether this completion has no work left to perform. + /// + /// A post-action that pauses hands back whatever of its completion has not + /// run yet. When that residue is empty there is nothing to re-park, and + /// parking it anyway would install a frame that only exists to do nothing. + pub fn is_noop(&self) -> bool { + self.post_actions.is_empty() + && matches!( + self.resolution_event, + PendingEffectResolutionEvent::Suppress + ) + && self.player_action.is_none() + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] @@ -5529,6 +5543,48 @@ pub enum PendingCounterPostAction { MarkRenowned { object_id: ObjectId, }, + /// CR 701.34a + CR 614.1a: The proliferate action whose counter placement + /// paused on a replacement choice has finished; continue with the actions + /// the originating effect still owes. + /// + /// The `Proliferate` direct-choice frame is deliberately NOT resident while + /// this is parked. Its handler takes the frame before applying counters, so + /// a counter-addition pause can never strand it on the stack (issue #7384); + /// continuing pushes a fresh frame only if another target choice is needed. + /// + /// CR 608.2c ordering: this is installed at index 0 of the completion's + /// `post_actions`, so anything a replacement's execute ability later appends + /// through `append_pending_counter_post_actions` runs AFTER the next + /// proliferate action. That is correct for the appenders that exist — they + /// are entry/delivery tails of the counter placement that already finished, + /// and CR 701.34a makes each proliferate action a separate event — but a + /// future appender that belongs to the SAME counter-placement event would + /// need to be ordered ahead of this instead. + /// + /// `add-engine-variant` gate, recorded because the verdict is binding: + /// * Stage 1 DOES_NOT_EXIST — no post-action resumes a pausable proliferate, + /// and the declared alternative (`replace_active_proliferate_frame`) is + /// structurally unusable here, as its own doc note records. + /// * Stage 2 EXTEND_OK — the `Continue*` members share a name root but carry + /// structurally disjoint payloads for distinct suspended operations, with + /// no `(op, scope, target)` axis to parameterize over. This enum is a + /// tagged union of suspended-work families — the `ResolutionFrame` shape — + /// not a parameterization gap. + /// * Stage 3 WITHIN_SECTION — the payload lies wholly inside CR 701.34, and + /// this is a resumption layer, not a leaf-reference layer. + /// + /// Serialized surface: this enum reaches persisted state through + /// `PendingEffectResolved::post_actions`, carried on the + /// `RESOLUTION_STATE_WIRE_VERSION` = 2 frame wire. A new externally-tagged + /// variant is backward compatible — no save written before it can contain + /// one, so existing saves still decode — but NOT forward compatible: a build + /// predating this variant cannot decode a save taken mid-paused-proliferate. + /// That is the same one-way contract `MarkMonstrous`, `MarkRenowned` and + /// `EmitCommittedCopyTokenEntry` shipped under, so the wire version is + /// deliberately not bumped. + ContinueProliferateActions { + pending: PendingProliferateActions, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] @@ -18444,6 +18500,27 @@ impl GameState { self.resolution_stack.push_counter_additions(pending); } + /// Insert a CounterAdditions queue immediately below the active child. + /// + /// A paused post-action may have installed a direct-choice owner (a fresh + /// `ProliferateChoice`, say). That owner must remain the stack top until + /// its action handler consumes it — `ResolutionStack::validate` rejects a + /// direct-choice owner buried below another frame — so a completion that + /// outlives the pause becomes the owner's PARENT instead of being pushed + /// on top of it. + pub fn insert_counter_additions_parent_of_active( + &mut self, + pending: PendingCounterAdditionQueue, + ) -> Result<(), ResolutionStackError> { + self.resolve_and_apply_frame_transition(ResolvedFrameTransition::InsertParentOfActive { + frame: super::resolution::ResolutionFrame::CounterAdditions(pending), + }) + .map(|_| ()) + .map_err(|error| match error { + ResolvedFrameTransitionReplayInvariantError::Stack(error) => error, + }) + } + /// Re-park the active CounterAdditions queue after it advances or pauses /// again. pub fn replace_active_counter_additions( @@ -18871,8 +18948,15 @@ impl GameState { self.resolution_stack.push_proliferate(pending); } - /// Re-parks the active proliferate owner after a replacement-produced - /// subsequent target choice. + /// Re-parks the active proliferate owner in place. + /// + /// NOTE: this is deliberately NOT the path for surviving a counter-addition + /// replacement choice. A proliferate that pauses that way has a + /// `CounterAdditions` frame at the stack top, so re-parking the proliferate + /// owner beneath it would bury a direct-choice owner — which + /// `ResolutionStack::validate` rejects. That case rides + /// `PendingCounterPostAction::ContinueProliferateActions` on the + /// counter-additions completion instead (issue #7384). pub fn replace_active_proliferate_frame( &mut self, pending: PendingProliferateActions, diff --git a/crates/engine/tests/integration/issue_7384_proliferate_counter_replacement_frame.rs b/crates/engine/tests/integration/issue_7384_proliferate_counter_replacement_frame.rs new file mode 100644 index 0000000000..509af06163 --- /dev/null +++ b/crates/engine/tests/integration/issue_7384_proliferate_counter_replacement_frame.rs @@ -0,0 +1,423 @@ +//! Regression tests for GitHub issue #7384 — an orphaned `Proliferate` frame +//! poisons the resolution stack for the rest of the game. +//! +//! `WaitingFor::ProliferateChoice`'s handler used to call `apply_proliferate` +//! BEFORE taking the proliferate frame off the resolution stack, and to return +//! early when that call paused. `apply_proliferate` pauses whenever a +//! counter-placement replacement needs a CR 616.1 ordering choice (two +//! simultaneously-applicable `AddCounter` replacements — Hardened Scales plus +//! Doubling Season is the common pairing). The counter-additions drain that +//! resumes after the choice never popped the proliferate frame and never +//! resumed the proliferate, so: +//! +//! * the `Proliferate` direct-choice frame stayed on the stack forever, and +//! `ResolutionStack::validate` failed every LATER frame transition against +//! it — the reported panic was a tutor (`SearchLibrary` + trailing +//! `Shuffle`) parking its tail through `prepend_to_pending_continuation`, +//! reporting `PromptMismatch { frame: Proliferate, waiting_for: +//! "SearchChoice" }`; +//! * every proliferate action after the first was silently skipped, so +//! "proliferate twice" (Tekuthal, Inquiry Dominus) performed once +//! (CR 701.34a). +//! +//! The frame is now taken before any counter is applied, and the remaining +//! actions ride `PendingCounterPostAction::ContinueProliferateActions` on the +//! counter-additions completion. + +use engine::types::ability::{ + AbilityDefinition, AbilityKind, Effect, QuantityExpr, QuantityModification, + ReplacementDefinition, ReplacementPlayerScope, ResolvedAbility, SearchSelectionConstraint, + TargetFilter, TypedFilter, +}; +use engine::types::actions::GameAction; +use engine::types::counter::CounterType; +use engine::types::events::{GameEvent, PlayerActionKind}; +use engine::types::game_state::WaitingFor; +use engine::types::replacements::ReplacementEvent; +use engine::types::resolution::ResolutionFrame; + +use engine::game::scenario::{GameScenario, P0}; +use engine::types::ability::EffectKind; + +/// CR 614.1a: a counter-placement replacement in the Hardened Scales / Doubling +/// Season class. Two of these are simultaneously applicable to the same +/// `AddCounter`, which is what makes the engine raise a CR 616.1 ordering +/// prompt mid-proliferate. +fn counter_modifier(modification: QuantityModification) -> ReplacementDefinition { + ReplacementDefinition::new(ReplacementEvent::AddCounter) + .valid_card(TargetFilter::Typed(TypedFilter::creature())) + .quantity_modification(modification) +} + +/// CR 701.34a + CR 614.1a: Tekuthal, Inquiry Dominus — "If you would +/// proliferate, proliferate twice instead." +fn proliferate_doubler() -> ReplacementDefinition { + let mut execute = AbilityDefinition::new(AbilityKind::Spell, Effect::Proliferate); + execute.repeat_for = Some(QuantityExpr::Fixed { value: 2 }); + let mut replacement = + ReplacementDefinition::new(ReplacementEvent::Proliferate).execute(execute); + replacement.valid_player = Some(ReplacementPlayerScope::You); + replacement +} + +fn proliferate_frames(state: &engine::types::game_state::GameState) -> usize { + state + .resolution_stack + .iter() + .filter(|frame| matches!(frame, ResolutionFrame::Proliferate(_))) + .count() +} + +fn count_events(events: &[GameEvent], action: PlayerActionKind) -> usize { + events + .iter() + .filter(|event| { + matches!( + event, + GameEvent::PlayerPerformedAction { action: got, .. } if *got == action + ) + }) + .count() +} + +/// The discriminating row. A doubled proliferate whose counter placement pauses +/// on a CR 616.1 ordering choice must (a) leave no `Proliferate` frame stranded +/// on the resolution stack and (b) still perform BOTH actions. +/// +/// Pre-fix this stranded a `Proliferate` frame after the very first replacement +/// choice and performed one action instead of two. +#[test] +fn issue_7384_proliferate_paused_by_counter_replacement_keeps_the_stack_clean() { + let mut scenario = GameScenario::new(); + let tekuthal = scenario + .add_creature(P0, "Tekuthal, Inquiry Dominus", 3, 3) + .with_replacement_definition(proliferate_doubler()) + .id(); + scenario + .add_creature(P0, "Doubling Season", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::DOUBLE)); + scenario + .add_creature(P0, "Hardened Scales", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::Plus { value: 1 })); + let grown = scenario + .add_creature(P0, "Counter Carrier", 1, 1) + .with_plus_counters(1) + .id(); + + let mut runner = scenario.build(); + let starting_counters = runner.state().objects[&grown].counters[&CounterType::Plus1Plus1]; + + // Tekuthal's proliferate replacement makes two actions. CR 701.34a defines + // each action; the first opens a choice with `remaining: 1` parked behind it. + let ability = ResolvedAbility::new(Effect::Proliferate, vec![], tekuthal, P0); + let mut events = Vec::new(); + engine::game::effects::proliferate::resolve(runner.state_mut(), &ability, &mut events).unwrap(); + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::ProliferateChoice { .. } + ), + "the doubled proliferate must open its first target choice" + ); + assert_eq!( + proliferate_frames(runner.state()), + 1, + "the open choice is owned by exactly one proliferate frame" + ); + + // Drive both proliferate actions, answering each CR 616.1 ordering prompt + // the counter placement raises. Bounded so a regression that wedges or + // re-prompts forever fails loudly instead of hanging. + let mut proliferate_choices = 0; + let mut replacement_choices = 0; + let mut all_events: Vec = events; + for _ in 0..12 { + let result = match &runner.state().waiting_for { + WaitingFor::ProliferateChoice { .. } => { + proliferate_choices += 1; + runner.act(GameAction::SelectTargets { + targets: vec![engine::types::ability::TargetRef::Object(grown)], + }) + } + WaitingFor::ReplacementChoice { .. } => { + replacement_choices += 1; + // THE key intermediate assertion: while the counter-placement + // choice is open the proliferate frame must NOT be resident. + // Pre-fix it was, buried under the `CounterAdditions` frame. + assert_eq!( + proliferate_frames(runner.state()), + 0, + "no proliferate frame may survive across a counter-placement choice" + ); + runner.act(GameAction::ChooseReplacement { index: 0 }) + } + _ => break, + } + .expect("every prompt raised by a paused proliferate must be answerable"); + all_events.extend(result.events); + } + + let state = runner.state(); + assert_eq!( + replacement_choices, 2, + "each of the two proliferate actions places a counter and so raises its own \ + CR 616.1 ordering prompt — fewer means an action was skipped" + ); + assert_eq!( + proliferate_choices, 2, + "Tekuthal's replacement makes two proliferate actions; CR 701.34a defines \ + each action's target choice (pre-fix the second never opened)" + ); + + // The panic's precondition, stated directly: nothing proliferate-shaped may + // outlive the resolution anywhere in the stack (not merely at its top). + assert_eq!( + proliferate_frames(state), + 0, + "a completed proliferate must leave no frame stranded on the resolution stack" + ); + assert!( + state.active_counter_additions().is_none(), + "the parked counter additions must be fully drained" + ); + + // Reach-guard: the negatives above must not pass on a fixture where + // proliferate never actually did anything. + // Exact, not merely "grew": both `ChooseReplacement { index: 0 }` answers are + // deterministic, so each action's single counter becomes +1 (Hardened Scales) + // then doubled (Doubling Season) = 4. Starting at 1, two actions land 1+4+4. + // An exact total is what discriminates a dropped or duplicated addition out + // of the parked `remaining` queue, which an inequality cannot. + assert_eq!( + state.objects[&grown].counters[&CounterType::Plus1Plus1], + 9, + "two proliferate actions, each placing one replacement-modified counter" + ); + assert_eq!( + starting_counters, 1, + "the fixture's starting point, pinned so the total above stays derivable" + ); + assert_eq!( + count_events(&all_events, PlayerActionKind::Proliferate), + 2, + "CR 701.34a: exactly one player-action event per proliferate action" + ); + assert_eq!( + all_events + .iter() + .filter(|event| matches!( + event, + GameEvent::EffectResolved { + kind: EffectKind::Proliferate, + source_id, + .. + } if *source_id == tekuthal + )) + .count(), + 1, + "the whole doubled proliferate resolves exactly once with Tekuthal as its source, after its last action" + ); +} + +/// The reported crash itself: after a proliferate that paused on a counter +/// replacement, a later chain that parks a continuation must not blow up. The +/// reporter's trigger was a tutor — `SearchLibrary` with a trailing `Shuffle` — +/// whose tail is parked through `prepend_to_pending_continuation`, the +/// `.expect` that panicked. +#[test] +fn issue_7384_tutor_after_paused_proliferate_does_not_panic() { + let mut scenario = GameScenario::new(); + scenario + .add_creature(P0, "Doubling Season", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::DOUBLE)); + scenario + .add_creature(P0, "Hardened Scales", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::Plus { value: 1 })); + let grown = scenario + .add_creature(P0, "Counter Carrier", 1, 1) + .with_plus_counters(1) + .id(); + scenario.add_card_to_library_top(P0, "Tutor Target"); + + let mut runner = scenario.build(); + let source = grown; + + let ability = ResolvedAbility::new(Effect::Proliferate, vec![], source, P0); + engine::game::effects::proliferate::resolve(runner.state_mut(), &ability, &mut Vec::new()) + .unwrap(); + runner + .act(GameAction::SelectTargets { + targets: vec![engine::types::ability::TargetRef::Object(grown)], + }) + .expect("submit the proliferate targets"); + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::ReplacementChoice { .. } + ), + "two applicable counter replacements must raise a CR 616.1 ordering prompt" + ); + runner + .act(GameAction::ChooseReplacement { index: 0 }) + .expect("answer the ordering prompt"); + assert_eq!( + proliferate_frames(runner.state()), + 0, + "the proliferate frame must not outlive the counter-placement choice" + ); + + // CR 701.23a: the tutor whose trailing `Shuffle` is parked as a + // continuation. Pre-fix this panicked in `prepend_to_pending_continuation` + // with PromptMismatch { frame: Proliferate, waiting_for: "SearchChoice" }. + let mut search = ResolvedAbility::new( + Effect::SearchLibrary { + source_zones: vec![engine::types::zones::Zone::Library], + filter: TargetFilter::Any, + count: QuantityExpr::Fixed { value: 1 }, + reveal: false, + target_player: None, + selection_constraint: SearchSelectionConstraint::None, + split: None, + }, + vec![], + source, + P0, + ); + search.sub_ability = Some(Box::new(ResolvedAbility::new( + Effect::Shuffle { + target: TargetFilter::Any, + }, + vec![], + source, + P0, + ))); + + let mut events = Vec::new(); + engine::game::effects::resolve_ability_chain(runner.state_mut(), &search, &mut events, 0) + .expect("the tutor chain must resolve, not panic on a stranded proliferate frame"); + + assert!( + matches!(runner.state().waiting_for, WaitingFor::SearchChoice { .. }), + "the tutor opens its search choice" + ); +} + +/// `Effect::ProliferateTarget` (Skyship Plunderer's forced single-target form) +/// directly instructs the engine to add another counter of each kind; it does +/// not use the proliferate keyword action defined by CR 701.34a. It must NEVER +/// publish `PlayerActionKind::Proliferate`, which would fire "whenever you +/// proliferate" triggers off a card that does not proliferate. +/// +/// It shares `apply_proliferate` with the chooser-driven form, and before this +/// fix that function hardcoded a completion of +/// `with_player_action(EffectKind::Proliferate, ObjectId(0), .., Proliferate)`. +/// So on the paused path it emitted the forbidden player action AND a second +/// `EffectResolved` — the effect had already pushed its own, eagerly, before the +/// counters landed. +#[test] +fn issue_7384_proliferate_target_paused_by_counter_replacement_emits_no_keyword_action() { + let mut scenario = GameScenario::new(); + scenario + .add_creature(P0, "Doubling Season", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::DOUBLE)); + scenario + .add_creature(P0, "Hardened Scales", 0, 0) + .as_enchantment() + .with_replacement_definition(counter_modifier(QuantityModification::Plus { value: 1 })); + let plunderer = scenario.add_creature(P0, "Skyship Plunderer", 2, 1).id(); + let grown = scenario + .add_creature(P0, "Counter Carrier", 1, 1) + .with_plus_counters(1) + .id(); + + let mut runner = scenario.build(); + let ability = ResolvedAbility::new( + Effect::ProliferateTarget { + target: TargetFilter::Any, + }, + vec![engine::types::ability::TargetRef::Object(grown)], + plunderer, + P0, + ); + + let mut all_events = Vec::new(); + engine::game::effects::proliferate::resolve_target( + runner.state_mut(), + &ability, + &mut all_events, + ) + .unwrap(); + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::ReplacementChoice { .. } + ), + "two applicable counter replacements must pause the targeted form too — \ + otherwise this row never reaches the completion it is about" + ); + // The eager emit must NOT have fired yet: the effect has not finished. + assert_eq!( + all_events + .iter() + .filter(|event| matches!(event, GameEvent::EffectResolved { .. })) + .count(), + 0, + "a paused ProliferateTarget must not announce itself resolved before its \ + counters land" + ); + + let result = runner + .act(GameAction::ChooseReplacement { index: 0 }) + .expect("answer the CR 616.1 ordering prompt"); + all_events.extend(result.events); + + assert_eq!( + count_events(&all_events, PlayerActionKind::Proliferate), + 0, + "CR 701.34a: the forced-target form does not use the proliferate \ + keyword action, or it fires 'whenever you proliferate' triggers" + ); + assert_eq!( + all_events + .iter() + .filter(|event| matches!( + event, + GameEvent::EffectResolved { + kind: EffectKind::ProliferateTarget, + source_id, + .. + } if *source_id == plunderer + )) + .count(), + 1, + "exactly one EffectResolved, carrying Skyship Plunderer and this effect's own kind" + ); + assert!( + !all_events.iter().any(|event| matches!( + event, + GameEvent::EffectResolved { + kind: EffectKind::Proliferate, + .. + } + )), + "the shared completion must not mis-attribute the targeted form as the \ + chooser-driven Proliferate" + ); + // Exact, for the same reason as the row above. `ChooseReplacement { index: 0 }` + // is deterministic but the applicable set is ordered per fixture, and this + // one resolves Doubling Season first: one counter doubled to 2, then +1 from + // Hardened Scales = 3, onto a starting 1. (The doubled-proliferate row above + // has a different permanent set and so resolves them the other way round — + // which is why each row pins its own measured total rather than sharing a + // constant.) + assert_eq!( + runner.state().objects[&grown].counters[&CounterType::Plus1Plus1], + 4, + "reach-guard: the replacement-modified counter must actually have landed" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index e59c831416..cffaf4d06f 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -733,6 +733,7 @@ mod issue_7234_cumulative_upkeep_effect_cost; mod issue_735_amalia_power_threshold; mod issue_735_cost_paid_object_non_regression; mod issue_735_lily_bowen_power_double; +mod issue_7384_proliferate_counter_replacement_frame; mod issue_787_once_upon_a_time; mod issue_788_unexpectedly_absent; mod issue_822_erode_path_to_exile_search_controller; diff --git a/crates/engine/tests/integration/proliferate_zero_counter.rs b/crates/engine/tests/integration/proliferate_zero_counter.rs index f6baa49d11..7f1ef9361e 100644 --- a/crates/engine/tests/integration/proliferate_zero_counter.rs +++ b/crates/engine/tests/integration/proliferate_zero_counter.rs @@ -6,10 +6,12 @@ use engine::game::effects::counters::resolve_remove; use engine::game::effects::proliferate::{apply_proliferate, resolve}; use engine::game::zones::create_object; -use engine::types::ability::{Effect, QuantityExpr, ResolvedAbility, TargetFilter, TargetRef}; +use engine::types::ability::{ + Effect, EffectKind, QuantityExpr, ResolvedAbility, TargetFilter, TargetRef, +}; use engine::types::counter::CounterType; use engine::types::events::GameEvent; -use engine::types::game_state::{GameState, WaitingFor}; +use engine::types::game_state::{GameState, PendingEffectResolved, WaitingFor}; use engine::types::identifiers::{CardId, ObjectId}; use engine::types::player::PlayerId; use engine::types::zones::Zone; @@ -101,6 +103,7 @@ fn issue_1995_stale_zero_map_entry_does_not_reopen_proliferate() { &mut state, PlayerId(0), &[TargetRef::Object(creature)], + PendingEffectResolved::new(EffectKind::Proliferate, ObjectId(999)), &mut apply_events, ); @@ -140,6 +143,7 @@ fn issue_1995_mixed_zero_and_positive_only_proliferates_present_kinds() { &mut state, PlayerId(0), &[TargetRef::Object(artifact)], + PendingEffectResolved::new(EffectKind::Proliferate, ObjectId(999)), &mut events, );