From c7f7e3e44c095e4cf44f527f4532676f04d6786f Mon Sep 17 00:00:00 2001 From: matthewevans Date: Wed, 5 Aug 2026 17:51:31 -0700 Subject: [PATCH 1/2] fix(engine): release trigger carrier on drop and unstick empty attackers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent stalls reported from one game state, both fixed here. They share a commit because the CR 603.5 prompt-census pin below is only correct once BOTH are applied. CR 603.3d — leaked trigger event batch: `drop_mid_construction_pending_trigger` released `pending_trigger` and `pending_trigger_firing` but left `pending_trigger_event_batch` populated. Because `begin_pending_trigger_target_selection` re-reads that batch as the event context for whichever trigger is being constructed, and the pause paths write it straight back, a single no-legal-target drop latched a dead event into the game state permanently. `resolve_proven_inert_trigger_batch` gates on `inert_trigger_batch_state_is_settled`, which requires an empty batch, so batch auto-resolve ("resolve all") stayed disabled for the rest of the game. The function is now the single authority for releasing all four in-flight construction cursors, and the four duplicated inline drop blocks collapse into calls to it. CR 508.1a — forced empty attack declaration parked the game: `run_auto_pass_loop`'s `DeclareAttackers` arm only auto-submitted under `AutoPassMode::UntilTurnBoundary`. With no auto-pass configured, a player controlling zero creatures was parked on a Declare Attackers prompt whose entire legal-action set was a single no-op submission, and the loop fell through to `_ => break`. The arm now also fires when `valid_attacker_ids` is empty, mirroring the `DeclareBlockers` arm, which already auto-submits when there is nothing to choose. A phase stop still overrides both. Also updates the CR 603.5 prompt-census pin `game/engine.rs:11697` -> `:11712` with a drift-log entry. Pure line movement: the producer is sha256-identical (`8a544e878d3e77fb...`), still the only line in the file matching the producer shape outside `#[cfg(test)]`, still inside `begin_pending_trigger_target_selection`, and the set is unchanged at five producers (total 37, partition 5/7/25). Both fixes were verified with negative controls in isolated target dirs: reverted -> red on the intended assertion -> restored -> green. --- crates/engine/src/game/engine.rs | 96 ++++++++++------ .../game/engine_auto_pass_decision_tests.rs | 53 +++++++++ .../src/game/engine_trigger_target_tests.rs | 104 ++++++++++++++++++ 3 files changed, 222 insertions(+), 31 deletions(-) diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index f4b36b7cea..81a0e1dea5 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -6454,10 +6454,25 @@ fn run_auto_pass_loop(state: &mut GameState, result: &mut ActionResult) -> bool } } - // UntilTurnBoundary: auto-submit empty attackers unless the user - // flagged this phase as a stop. - WaitingFor::DeclareAttackers { player, .. } - if end_of_turn_active(state, *player) && !state.phase_stop_hit(*player) => + // CR 508.1a: "The active player chooses which creatures that they + // control, IF ANY, will attack." When no creature can legally attack, + // the empty declaration is the ONLY legal declaration — there is no + // choice to make. Parking on the prompt strands the game on a decision + // whose entire legal-action set is a single no-op submission, which the + // player must click through every combat (and which an automated seat + // may never submit at all). Mirrors the `DeclareBlockers` arm below, + // which already auto-submits whenever there is nothing to choose. + // + // `UntilTurnBoundary` additionally auto-submits when candidates DO + // exist — that is the player's standing pre-commitment to attack with + // nothing. A phase stop overrides both: an explicit request to pause + // here is honored even when the declaration is forced. + WaitingFor::DeclareAttackers { + player, + valid_attacker_ids, + .. + } if !state.phase_stop_hit(*player) + && (valid_attacker_ids.is_empty() || end_of_turn_active(state, *player)) => { let mut events = Vec::new(); match engine_combat::handle_empty_attackers(state, &mut events) { @@ -11508,8 +11523,22 @@ fn apply_retarget( Ok(state.waiting_for.clone()) } -/// CR 603.3c + CR 608.2c: Drop a mid-construction optional triggered modal that -/// was declined before mode choice. +/// CR 603.3c + CR 603.3d + CR 608.2c: Single authority for dropping a +/// mid-construction triggered ability — an optional modal declined before mode +/// choice, or CR 603.3d's "if a choice is required when the triggered ability +/// goes on the stack but no legal choices can be made for it ... the ability is +/// simply removed from the stack." +/// +/// Every in-flight construction cursor must be released together. The event +/// batch is the paused trigger's carrier: `begin_pending_trigger_target_selection` +/// re-reads `pending_trigger_event_batch` as the event context for whichever +/// trigger is being constructed, and the pause paths write it straight back. A +/// drop that clears the trigger but leaks the batch therefore leaves a dead +/// event latched in state, where it (a) poisons the event context of every +/// later trigger that pauses for a choice, and (b) permanently fails the +/// `inert_trigger_batch_state_is_settled` gate that lets contiguous inert +/// trigger runs skip priority. Mirrors `triggers::abandon_ceased_pending_trigger`, +/// which already releases all four cursors on the error-recovery path. pub(super) fn drop_mid_construction_pending_trigger(state: &mut GameState) { super::stack::pop_uncommitted_pending_trigger_entry( state, @@ -11517,6 +11546,7 @@ pub(super) fn drop_mid_construction_pending_trigger(state: &mut GameState) { ); state.pending_trigger = None; state.pending_trigger_firing = None; + state.pending_trigger_event_batch.clear(); } /// Clear optionality after the controller accepts a "you may choose N" gate so @@ -11610,12 +11640,7 @@ pub(super) fn begin_pending_trigger_target_selection( &mode_abilities, &unavailable_modes, ) else { - super::stack::pop_uncommitted_pending_trigger_entry( - state, - super::lifecycle::DelayedTerminalDisposition::NoLegalChoice, - ); - state.pending_trigger = None; - state.pending_trigger_firing = None; + drop_mid_construction_pending_trigger(state); return Ok(None); }; @@ -11633,12 +11658,7 @@ pub(super) fn begin_pending_trigger_target_selection( dispatch_pending_trigger_context must resolve it inline", ); if modal.selection.is_random() { - super::stack::pop_uncommitted_pending_trigger_entry( - state, - super::lifecycle::DelayedTerminalDisposition::NoLegalChoice, - ); - state.pending_trigger = None; - state.pending_trigger_firing = None; + drop_mid_construction_pending_trigger(state); return Ok(None); } @@ -11651,12 +11671,7 @@ pub(super) fn begin_pending_trigger_target_selection( // dead branch — kept as a defensive cleanup for any // delayed-revalidation paths. if unavailable_modes.len() >= modal.mode_count { - super::stack::pop_uncommitted_pending_trigger_entry( - state, - super::lifecycle::DelayedTerminalDisposition::NoLegalChoice, - ); - state.pending_trigger = None; - state.pending_trigger_firing = None; + drop_mid_construction_pending_trigger(state); return Ok(None); } @@ -11770,12 +11785,7 @@ pub(super) fn begin_pending_trigger_target_selection( // branch above: if the "push first" dispatcher already pushed an // in-construction entry for this trigger, pop it before clearing the // cursor. - super::stack::pop_uncommitted_pending_trigger_entry( - state, - super::lifecycle::DelayedTerminalDisposition::NoLegalChoice, - ); - state.pending_trigger = None; - state.pending_trigger_firing = None; + drop_mid_construction_pending_trigger(state); return Ok(None); }; Ok(Some(WaitingFor::TriggerTargetSelection { @@ -15654,7 +15664,31 @@ mod stage2_injector_tests { // PR #7041's typed trigger-provenance initializers sit above the // first three effects producers and this engine producer. CI // re-derived the same five writes at these coordinates. - "game/engine.rs:11697".to_string(), + // + // CR 603.3d CARRIER-RELEASE FIX: `:11697 ⇒ :11712`. Pure line movement from + // two edits in this file, neither of which mints a prompt: the empty-attackers + // auto-submit guard in `run_auto_pass_loop` (`:6457`, +15) and the collapse of + // four duplicated mid-construction drop blocks into calls to + // `drop_mid_construction_pending_trigger` (`:11511` +14 for its doc comment, + // `:11519` +1 for the `pending_trigger_event_batch.clear()` line, then -5 at + // each of `:11613`/`:11636`/`:11654`). Those six hunks sum to +15 and + // 11697 + 15 = 11712 exactly. The fourth collapsed block (`:11773`, also -5) + // sits BELOW this producer and therefore cannot move it, which is why the sum + // is +15 rather than the whole-file +10. + // + // LOCATED BY CONTENT, as this log requires: `:11712` hashes to sha256 + // `8a544e878d3e77fb…`, the same prefix carried for this producer since + // `a6d1a0e62`, and it is the ONLY line in the file matching the producer shape + // outside `#[cfg(test)]`. Still inside `begin_pending_trigger_target_selection`, + // which moved `:11548 ⇒ :11578` by the +30 of the three hunks above the function + // itself — the same arithmetic re-derived against a different anchor. + // + // SET PRESERVATION: the other four entries are byte-identical AND in place + // (`effects/` and `scoped_library_search.rs` are untouched by this change), and + // the two tests this change adds contain no line matching the needle, so the + // total stays 37 and the partition stays 5/7/25. A drop path releasing a + // construction cursor cannot mint a CR 603.5 prompt. + "game/engine.rs:11712".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_auto_pass_decision_tests.rs b/crates/engine/src/game/engine_auto_pass_decision_tests.rs index 7968a50261..46bdfb9cb9 100644 --- a/crates/engine/src/game/engine_auto_pass_decision_tests.rs +++ b/crates/engine/src/game/engine_auto_pass_decision_tests.rs @@ -1029,3 +1029,56 @@ fn loop_gate_probes_all_living_players_not_just_current_holder() { wrongly clear, proving the all-players probe is load-bearing" ); } + +/// CR 508.1a: "The active player chooses which creatures that they control, IF +/// ANY, will attack." When the candidate set is empty there is no choice to +/// make — the empty declaration is the only legal one — so the engine must +/// submit it rather than park on the prompt. +/// +/// Regression: this arm previously auto-submitted ONLY when the player was in +/// `AutoPassMode::UntilTurnBoundary`. A player with no auto-pass configured and +/// no creatures therefore sat on a Declare Attackers prompt whose entire legal +/// action set was a single no-op `DeclareAttackers { attacks: [], bands: [] }`, +/// which had to be clicked through every combat. The `DeclareBlockers` arm has +/// always carried the equivalent "nothing to choose" escape; this pins the +/// attacker side to the same rule. +#[test] +fn empty_attacker_set_auto_submits_without_any_auto_pass_mode() { + let waiting_for = WaitingFor::DeclareAttackers { + player: PlayerId(0), + valid_attacker_ids: Vec::new(), + valid_attack_targets: Vec::new(), + valid_attack_targets_by_attacker: Some(Default::default()), + attacker_constraints: Default::default(), + }; + let mut state = GameState::new_two_player(42); + state.phase = Phase::DeclareAttackers; + state.active_player = PlayerId(0); + state.priority_player = PlayerId(0); + // Production sets combat before advancing into the declare step. + state.combat = Some(crate::game::combat::CombatState::default()); + state.waiting_for = waiting_for.clone(); + + // The stalling configuration: no auto-pass mode for the declaring player. + assert!( + state.auto_pass.is_empty(), + "fixture must exercise the no-auto-pass case that stalled" + ); + + let mut result = ActionResult { + events: Vec::new(), + waiting_for, + log_entries: Vec::new(), + }; + let advanced = run_auto_pass_loop(&mut state, &mut result); + + assert!( + advanced, + "CR 508.1a: a forced empty attack declaration must not park the game" + ); + assert!( + !matches!(result.waiting_for, WaitingFor::DeclareAttackers { .. }), + "CR 508.1a: the forced empty declaration must be submitted, not re-offered; got {:?}", + result.waiting_for + ); +} diff --git a/crates/engine/src/game/engine_trigger_target_tests.rs b/crates/engine/src/game/engine_trigger_target_tests.rs index dd97a45350..6f07056a54 100644 --- a/crates/engine/src/game/engine_trigger_target_tests.rs +++ b/crates/engine/src/game/engine_trigger_target_tests.rs @@ -1265,3 +1265,107 @@ fn modal_mode_tracking_resets_on_new_turn() { // Game-scoped should persist. assert!(state.modal_modes_chosen_this_game.contains(&(source_id, 0))); } + +/// CR 603.3d: "If a choice is required when the triggered ability goes on the +/// stack but no legal choices can be made for it ... the ability is simply +/// removed from the stack." Removing it must release EVERY in-flight +/// construction cursor, including `pending_trigger_event_batch`. +/// +/// Regression: Nimble Obstructionist ("When you cycle this card, counter target +/// activated or triggered ability you don't control") cycled with nothing legal +/// to counter took this drop path. The drop cleared `pending_trigger` and +/// `pending_trigger_firing` but leaked the batch, latching a dead `Cycled` +/// event into the game state permanently — it then poisoned the trigger event +/// context of every later trigger that paused for a choice (firing "whenever a +/// player cycles" / "whenever you draw" observers for a cycle that never +/// happened) and permanently failed the settled-state gate that lets contiguous +/// inert trigger runs skip priority. +#[test] +fn no_legal_target_trigger_drop_releases_pending_trigger_event_batch() { + let mut state = GameState::new_two_player(42); + state.turn_number = 2; + state.phase = Phase::PreCombatMain; + state.active_player = PlayerId(0); + state.priority_player = PlayerId(0); + + let source_id = create_object( + &mut state, + CardId(20), + PlayerId(0), + "Cycled Trigger Source".to_string(), + Zone::Graveyard, + ); + + // The battlefield is deliberately empty, so a creature-targeting trigger has + // no legal target at choose-time — the CR 603.3d removal branch. + let ability = ResolvedAbility::new( + Effect::DealDamage { + amount: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Typed(TypedFilter::creature()), + damage_source: None, + excess: None, + }, + Vec::new(), + source_id, + PlayerId(0), + ); + + let cycled_event = GameEvent::Cycled { + player_id: PlayerId(0), + object_id: source_id, + }; + let pending = crate::game::triggers::PendingTrigger { + source_id, + controller: PlayerId(0), + condition: None, + ability: Box::new(ability), + timestamp: 1, + target_constraints: Vec::new(), + distribute: None, + trigger_event: Some(cycled_event.clone()), + modal: None, + mode_abilities: vec![], + description: Some("When you cycle this card, counter target ability".to_string()), + may_trigger_origin: None, + subject_match_count: None, + die_result: None, + provenance: None, + }; + let pending_for_state = pending.clone(); + let mut setup_events = Vec::new(); + let entry_id = crate::game::triggers::push_pending_trigger_to_stack( + &mut state, + pending, + &mut setup_events, + ); + state.pending_trigger = Some(Box::new(pending_for_state)); + mark_ordinary_pending_trigger_construction(&mut state, entry_id); + // Production installs the carrier AFTER the push (the push drains it), which + // is exactly the state a paused construction is re-entered in. + state.pending_trigger_event_batch = vec![cycled_event]; + + // Non-vacuity guard: the assertion below is only meaningful if the carrier is + // actually populated going in. `push_pending_trigger_to_stack` DRAINS the + // batch, so a future reordering of this fixture would silently turn the + // regression assert into a tautology that passes with the fix reverted. + assert!( + !state.pending_trigger_event_batch.is_empty(), + "fixture must enter the drop path with a populated carrier" + ); + + let waiting = crate::game::engine::begin_pending_trigger_target_selection(&mut state) + .expect("no-legal-target drop is not an engine error"); + + assert!( + waiting.is_none(), + "CR 603.3d: a trigger with no legal target must not surface a prompt" + ); + assert!( + state.pending_trigger_event_batch.is_empty(), + "CR 603.3d: removing the ability must release its event-batch carrier, \ + not latch a dead event into the game state" + ); + assert!(state.pending_trigger.is_none()); + assert!(state.pending_trigger_entry.is_none()); + assert!(state.pending_trigger_firing.is_none()); +} From 1c45081a17a0ac4d3f91f17bc7ad5c7f39ae1191 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Wed, 5 Aug 2026 18:38:34 -0700 Subject: [PATCH 2/2] test(engine): keep the attacker-refresh prompt observable `debug_set_summoning_sickness_removes_from_valid_attackers` drove its only eligible attacker sick, emptying `valid_attacker_ids` while parked at the prompt. The CR 508.1a empty-attackers guard then auto-submitted the sole legal (empty) declaration and advanced to priority, so the refreshed `DeclareAttackers` the test exists to inspect no longer existed. Add a second eligible attacker so the refreshed snapshot stays non-empty and the declaration is not forced. In real play this transition is unreachable: declaring attackers is a turn-based action (CR 508.1) with no priority window mid-declaration, so `valid_attacker_ids` cannot go non-empty -> empty while a player is parked at the prompt; emptiness is decided when the prompt is built. Only the debug sandbox can produce it. The paired assertion is also strictly sharper: it now proves the refresh dropped *that* creature while retaining the untouched one, which an emptied set cannot distinguish from a snapshot that simply cleared everything. --- crates/engine/src/game/combat.rs | 39 +++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/game/combat.rs b/crates/engine/src/game/combat.rs index 726ac1491a..38f6e633e9 100644 --- a/crates/engine/src/game/combat.rs +++ b/crates/engine/src/game/combat.rs @@ -7367,6 +7367,15 @@ mod tests { // Non-sick, eligible creature (create_creature leaves summoning_sick false). let id = create_creature(&mut state, PlayerId(0), "Bear", 2, 2); + // CR 508.1a: a SECOND eligible attacker keeps the refreshed snapshot + // non-empty after `id` goes sick. Without it the declaration becomes + // forced, and `run_auto_pass_loop` auto-submits the only legal (empty) + // declaration — the prompt is gone and this row can no longer observe + // the refresh at all. Keeping the set non-empty also makes the + // assertion strictly sharper: it proves the refresh dropped THAT + // creature while retaining the other, which an emptied set cannot + // distinguish from a snapshot that simply cleared everything. + let companion = create_creature(&mut state, PlayerId(0), "Ox", 3, 3); state.waiting_for = WaitingFor::DeclareAttackers { player: PlayerId(0), @@ -7378,10 +7387,17 @@ mod tests { match &state.waiting_for { WaitingFor::DeclareAttackers { valid_attacker_ids, .. - } => assert!( - valid_attacker_ids.contains(&id), - "precondition: eligible creature must be a valid attacker" - ), + } => { + assert!( + valid_attacker_ids.contains(&id), + "precondition: eligible creature must be a valid attacker" + ); + assert!( + valid_attacker_ids.contains(&companion), + "precondition: the companion must also be a valid attacker, \ + or the post-refresh set would be empty for the wrong reason" + ); + } other => panic!("expected DeclareAttackers, got {other:?}"), } @@ -7398,10 +7414,17 @@ mod tests { match &result.waiting_for { WaitingFor::DeclareAttackers { valid_attacker_ids, .. - } => assert!( - !valid_attacker_ids.contains(&id), - "refreshed snapshot must drop the now-sick creature" - ), + } => { + assert!( + !valid_attacker_ids.contains(&id), + "refreshed snapshot must drop the now-sick creature" + ); + assert!( + valid_attacker_ids.contains(&companion), + "the refresh must be selective, not a wholesale clear: the \ + untouched companion is still an eligible attacker" + ); + } other => panic!("expected refreshed DeclareAttackers, got {other:?}"), } }