From 0ec55a6119d597559c3fca4e246f347459307d2c Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 19 Jul 2026 02:15:35 -0700 Subject: [PATCH 1/5] fix(ai-commander): surface driver stalls instead of exiting as normal game end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #6080 diagnosis: an empty run_ai_actions batch while waiting_for != GameOver was silently treated as a normal game end, hiding the whole p0-softlock issue family (#5250, #4345, #5958, #6172, #3886, #3919, #3233). The only prior signal at any of run_ai_actions' three break doors (no AI actor, choose_action None, apply() Err) was a tracing::error/warn that no harness subscriber captures. run_ai_actions now returns AiActionsRun (Deref> for source compatibility with existing callers) carrying an Option naming which door fired. ai_commander prints a machine-readable STALL line (waiting_for variant, turn, active/priority player, pending-cast summary) plus the break reason, and exits with a distinct code (3) instead of falling through to a clean exit — distinguishing a stall from both a completed game (0) and an action-cap abort (2). The existing "Game did NOT reach GameOver" substring pod-lab's runner.py matches on is preserved verbatim. Co-Authored-By: Claude Fable 5 --- crates/phase-ai/src/auto_play.rs | 96 ++++++++++++++++++++++++- crates/phase-ai/src/bin/ai_commander.rs | 45 ++++++++++++ 2 files changed, 138 insertions(+), 3 deletions(-) diff --git a/crates/phase-ai/src/auto_play.rs b/crates/phase-ai/src/auto_play.rs index 6bc1298976..3639e6c707 100644 --- a/crates/phase-ai/src/auto_play.rs +++ b/crates/phase-ai/src/auto_play.rs @@ -1,6 +1,6 @@ use std::collections::{HashMap, HashSet}; -use engine::game::engine::apply; +use engine::game::engine::{apply, EngineError}; use engine::game::turn_control; use engine::types::actions::GameAction; use engine::types::events::GameEvent; @@ -26,6 +26,80 @@ pub struct AiActionResult { pub log_entries: Vec, } +/// Which of `run_ai_actions`'s three break doors (see its doc comment) ended +/// the batch before the safety cap. `None` means the loop ran out AI actions +/// to take for a benign reason the caller already distinguishes elsewhere +/// (hit `MAX_AI_ACTIONS_PER_SEQUENCE`, or the very first iteration found no +/// actor at all — that case is folded into `NoActor` below instead, so `None` +/// in practice only means "hit the safety cap"). +/// +/// Diagnostic surface for phase#6080 (the driver-stall family): today the only +/// signal at these break points is a `tracing::error`/`tracing::warn` that no +/// harness subscriber captures. Exposing the reason as typed data lets a +/// caller like `ai_commander` print it instead of installing a subscriber. +#[derive(Debug, Clone)] +pub enum AiActionsBreakReason { + /// No acting player was found for the current `waiting_for`, or none of + /// the acting players maps (via `turn_control::authorized_submitter_for_player`) + /// to an AI-controlled seat. + NoActor, + /// `choose_action_with_session` returned `None` for `player` — the AI + /// policy stack produced no legal action for a decision it was asked to + /// make. + ChooseActionNone { player: PlayerId }, + /// `apply()` rejected `player`'s chosen `action`. Boxed: `GameAction` and + /// `EngineError` are large relative to the other variants (clippy + /// `large_enum_variant`), and this variant is a diagnostic path taken + /// only when a batch breaks early, not a hot allocation. + ApplyFailed { + player: PlayerId, + action: Box, + error: EngineError, + }, +} + +/// Outcome of a `run_ai_actions` batch. +/// +/// `Deref`s to `Vec` so the many existing callers that only +/// care about the actions taken (`.is_empty()`, `.len()`, indexing, iterating +/// by reference) are source-compatible; only diagnostic consumers need +/// `break_reason`. +pub struct AiActionsRun { + pub results: Vec, + pub break_reason: Option, +} + +impl std::ops::Deref for AiActionsRun { + type Target = Vec; + fn deref(&self) -> &Vec { + &self.results + } +} + +impl IntoIterator for AiActionsRun { + type Item = AiActionResult; + type IntoIter = std::vec::IntoIter; + fn into_iter(self) -> Self::IntoIter { + self.results.into_iter() + } +} + +impl<'a> IntoIterator for &'a AiActionsRun { + type Item = &'a AiActionResult; + type IntoIter = std::slice::Iter<'a, AiActionResult>; + fn into_iter(self) -> Self::IntoIter { + self.results.iter() + } +} + +impl<'a> IntoIterator for &'a mut AiActionsRun { + type Item = &'a mut AiActionResult; + type IntoIter = std::slice::IterMut<'a, AiActionResult>; + fn into_iter(self) -> Self::IntoIter { + self.results.iter_mut() + } +} + /// Run AI actions on the game state until the next actor is human or the game is over. /// /// Returns one `AiActionResult` per AI action taken, preserving granularity for @@ -44,8 +118,9 @@ pub fn run_ai_actions( ai_configs: &HashMap, rng: &mut impl Rng, session: &Arc, -) -> Vec { +) -> AiActionsRun { let mut results = Vec::new(); + let mut break_reason = None; for _ in 0..MAX_AI_ACTIONS_PER_SEQUENCE { // CR 723.5: Under turn control (Mindslaver, Emrakul), the authorized @@ -60,6 +135,7 @@ pub fn run_ai_actions( .find(|player| ai_players.contains(player)); let Some(actor) = actor else { + break_reason = Some(AiActionsBreakReason::NoActor); break; }; @@ -67,6 +143,11 @@ pub fn run_ai_actions( Some(c) => c, None => { tracing::warn!(player = ?actor, "AI seat has no config — stopping AI loop"); + // No dedicated break-reason variant: a missing config is a + // caller wiring bug (every AI seat must be registered), not + // one of the three doors phase#6080 instruments. Falls back + // to the closest existing category. + break_reason = Some(AiActionsBreakReason::NoActor); break; } }; @@ -75,6 +156,7 @@ pub fn run_ai_actions( Some(a) => a, None => { tracing::warn!(player = ?actor, "choose_action returned None — stopping AI loop"); + break_reason = Some(AiActionsBreakReason::ChooseActionNone { player: actor }); break; } }; @@ -93,6 +175,11 @@ pub fn run_ai_actions( } Err(e) => { tracing::error!(player = ?actor, error = %e, "AI action apply failed — stopping"); + break_reason = Some(AiActionsBreakReason::ApplyFailed { + player: actor, + action: Box::new(action), + error: e, + }); break; } } @@ -105,5 +192,8 @@ pub fn run_ai_actions( ); } - results + AiActionsRun { + results, + break_reason, + } } diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index 5ca8cc5f84..7b9f08f34e 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -180,6 +180,10 @@ fn main() { let mut aborted = false; let mut ai_rng = StdRng::seed_from_u64(seed); let ai_session = phase_ai::session::AiSession::arc_from_game(&state); + // phase#6080: the reason the most recent `run_ai_actions` batch broke + // early (one of its three break doors), so a stall can be diagnosed from + // the game output alone instead of a `tracing::error` no harness captures. + let mut last_break_reason = None; loop { let mut results = run_ai_actions( @@ -190,6 +194,7 @@ fn main() { &ai_session, ); if results.is_empty() { + last_break_reason = results.break_reason; break; } if dump_log_path.is_some() { @@ -238,6 +243,7 @@ fn main() { println!("Turns played: {}", state.turn_number); println!(); + let mut stalled = false; match &state.waiting_for { WaitingFor::GameOver { winner } => { println!( @@ -246,6 +252,39 @@ fn main() { ); } other => { + stalled = true; + // phase#6080: an empty AI-action batch while parked on anything + // but GameOver is a driver stall, not a normal game end (the + // family of p0-softlock issues #5250/#4345/#5958/#6172/#3886/ + // #3919/#3233). Print a machine-readable line with enough context + // to reproduce (waiting_for variant, turn, active/priority + // player, pending-cast summary) plus which break door fired, then + // exit with a distinct code — the caller must not silently treat + // this as a completed game. + println!( + "STALL: waiting_for={} turn={} active=P{} priority=P{} pending_cast={}", + other.variant_name(), + state.turn_number, + state.active_player.0, + state.priority_player.0, + state + .pending_cast + .as_ref() + .map(|pc| format!( + "object={:?} card={:?} variant={:?}", + pc.object_id, pc.card_id, pc.casting_variant + )) + .unwrap_or_else(|| "none".to_string()), + ); + match &last_break_reason { + Some(reason) => println!("STALL: break_reason={reason:?}"), + None => println!( + "STALL: break_reason=unknown (run_ai_actions batch was non-empty; \ + stall detected on a later empty batch this process did not observe)" + ), + } + // Preserved verbatim: pod-lab's runner.py classifies outcomes by + // matching this exact substring in stdout — do not reword it. println!("Game did NOT reach GameOver. waiting_for = {other:?}"); } } @@ -286,6 +325,12 @@ fn main() { if aborted { std::process::exit(2); } + // Distinct from a clean exit (0) and an action-cap abort (2): a stall + // must never be mistaken for either by a caller keying off exit status + // alone (phase#6080). + if stalled { + std::process::exit(3); + } } /// Reads an opt-in dump-destination env var once at startup. Absence is a From ebe4f7bb56f2cb12f7913c84064df9bbf9e1eae2 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 19 Jul 2026 02:16:09 -0700 Subject: [PATCH 2/5] fix(ai-commander): hard-fail unknown --difficulty labels instead of silently downgrading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parse_difficulty previously matched only five exact-case strings and fell back to Easy for anything else, including typos and CEDH (which has no arm here yet) — silently poisoning an entire batch of games with the wrong skill tier and no indication anything went wrong. Routes the actual label -> AiDifficulty mapping through the crate's existing single-authority AiDifficulty::from_label (so this binary's understanding of a label can never drift from every other transport that parses one), but layers a hard startup error on top for anything from_label would otherwise silently downgrade to Medium. Accepted values are listed explicitly (VeryEasy, Easy, Medium, Hard, VeryHard, CEDH) so a wrong flag names every valid spelling instead of just failing quietly; adding CEDH here is what unlocks the CEDH tier for pod-lab's gauntlet plan (§3.8). Co-Authored-By: Claude Fable 5 --- crates/phase-ai/src/bin/ai_commander.rs | 36 ++++++++++++++++++++----- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index 7b9f08f34e..3878a0dceb 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -346,13 +346,35 @@ fn read_dump_env(key: &str) -> Option { .expect("invalid Unicode in dump-destination env var") } +/// Every label `AiDifficulty::from_label` (the crate's single-authority +/// label parser) maps to a real difficulty rather than falling back to its +/// own unknown-label default (`Medium`). Kept as an explicit list rather than +/// derived from the enum so a hard-error message can name every accepted +/// spelling; a new arm added to `from_label` must be mirrored here to be +/// reachable from this binary's `--difficulty` flag. +const ACCEPTED_DIFFICULTY_LABELS: &[&str] = + &["VeryEasy", "Easy", "Medium", "Hard", "VeryHard", "CEDH"]; + +/// Parses a `--difficulty` value. Delegates the actual label→enum mapping to +/// `AiDifficulty::from_label` (the crate's single authority — see its doc +/// comment) so this binary's understanding of each label can never drift from +/// every other transport that parses one. Unlike `from_label` itself, which +/// silently downgrades an unrecognized label to `Medium`, an unrecognized +/// label here is a hard startup error: silently running a garbled +/// `--difficulty` value would poison an entire batch of games with a +/// mislabeled skill tier with no indication anything went wrong (phase#6080 +/// diagnosis; pod-lab gauntlet plan §3.8/§4.5, whose local tier-echo guard +/// exists precisely because this class of silent downgrade was possible). fn parse_difficulty(s: &str) -> AiDifficulty { - match s { - "VeryEasy" => AiDifficulty::VeryEasy, - "Easy" => AiDifficulty::Easy, - "Medium" => AiDifficulty::Medium, - "Hard" => AiDifficulty::Hard, - "VeryHard" => AiDifficulty::VeryHard, - _ => AiDifficulty::Easy, + if !ACCEPTED_DIFFICULTY_LABELS + .iter() + .any(|label| label.eq_ignore_ascii_case(s.trim())) + { + eprintln!( + "error: unrecognized --difficulty {s:?}; accepted values: {}", + ACCEPTED_DIFFICULTY_LABELS.join(", ") + ); + std::process::exit(1); } + AiDifficulty::from_label(s) } From 62a84e2b2eb6a8cc3a8113186e2237ce806f547e Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 19 Jul 2026 02:16:30 -0700 Subject: [PATCH 3/5] feat(ai-commander): print per-seat resolved deck counts after name resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_deck_list silently skips any card name the database doesn't recognize, and the only existing print (pre-resolution, from the raw feed JSON) happens before that resolution runs — so no prior output could prove a deck actually loaded full-size (plan §3.9). A harness driving this binary at scale had no way to detect silent-skip drift short of re-deriving counts from engine internals. Adds a per-seat "Resolved deck sizes (post name-resolution...)" block right after resolve_deck_list, summing each PlayerDeckPayload's main_deck and commander DeckEntry counts. Purely additive — the existing pre-resolution print is untouched, and pod-lab's runner.py doesn't currently parse either line, so this carries no stdout-compatibility risk. Co-Authored-By: Claude Fable 5 --- crates/phase-ai/src/bin/ai_commander.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index 3878a0dceb..ee5aaf9082 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -155,6 +155,28 @@ fn main() { }; let payload: DeckPayload = resolve_deck_list(&db, &deck_list); + // Post-resolution deck-count line (plan §3.9): `resolve_deck_list` silently + // skips any name the card database doesn't recognize, so the pre-resolution + // "main: N cards" print above can't prove a deck actually loaded full-size. + // Print the resolved per-seat counts too, so a harness parsing stdout can + // detect silent-skip drift (a resolved count short of the pre-resolution + // one) without needing engine internals. + println!("Resolved deck sizes (post name-resolution, 0-indexed by seat):"); + for (i, seat) in [ + &payload.player, + &payload.opponent, + &payload.ai_decks[0], + &payload.ai_decks[1], + ] + .into_iter() + .enumerate() + { + let main_count: u32 = seat.main_deck.iter().map(|e| e.count).sum(); + let commander_count: u32 = seat.commander.iter().map(|e| e.count).sum(); + println!(" P{i} main={main_count:>3} commander={commander_count}"); + } + println!(); + let mut state = GameState::new(FormatConfig::commander(), 4, seed); load_deck_into_state(&mut state, &payload); From a656879bb236843485c91006c8101dc9dac1c000 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:18:13 -0700 Subject: [PATCH 4/5] fix(ai-commander): capture break_reason from every AI-action batch, not only empty ones `run_ai_actions` can complete one or more actions and still stop on ApplyFailed/ChooseActionNone before the safety cap. The driver previously only inspected `break_reason` when the returned batch was empty, silently discarding the diagnostic whenever a batch was non-empty and looping again until a later, unrelated empty batch produced a misleading NoActor/unknown reason in the stall report. Extract `driver_step` in auto_play.rs as the single place that decides whether a batch's break_reason should stop the driver, and use it in ai_commander's loop. Add regression coverage: a unit test on `driver_step` itself, and an integration test forcing `run_ai_actions` to return a real non-empty batch with a break_reason (DeclareBlockers applies, then priority reaches an AI seat with no registered config). Co-Authored-By: Claude Fable 5 --- crates/phase-ai/src/auto_play.rs | 95 +++++++++++++++++++++++++ crates/phase-ai/src/bin/ai_commander.rs | 21 ++++-- crates/phase-ai/tests/ai_quality.rs | 72 ++++++++++++++++++- 3 files changed, 181 insertions(+), 7 deletions(-) diff --git a/crates/phase-ai/src/auto_play.rs b/crates/phase-ai/src/auto_play.rs index 3639e6c707..835b8253a8 100644 --- a/crates/phase-ai/src/auto_play.rs +++ b/crates/phase-ai/src/auto_play.rs @@ -197,3 +197,98 @@ pub fn run_ai_actions( break_reason, } } + +/// Driver-relevant outcome of processing one `run_ai_actions` batch: how many +/// actions to add to a caller's running total, and the break reason to stop +/// and report at this batch boundary, if the batch carries one. +/// +/// phase#6080 follow-up: a batch can complete one or more actions (`results` +/// non-empty) and *still* carry a `break_reason` — e.g. it applies two +/// actions, then the third choice is `ChooseActionNone` or the fourth +/// `apply()` call fails. A driver that only inspects `break_reason` when +/// `results.is_empty()` silently discards the diagnostic for exactly that +/// case, loops again, and may report a misleading `NoActor`/unknown reason +/// once a later, unrelated batch happens to come back empty. `driver_step` +/// is the single place that decision is made, so callers (and tests) don't +/// re-derive it ad hoc. +pub struct DriverStep { + pub actions_taken: usize, + pub break_reason: Option, +} + +/// Extracts the [`DriverStep`] for one batch. Callers should process +/// `results`'s individual `AiActionResult`s (logging, animation, dumps) +/// before or after calling this — it only reports the count/stop decision. +pub fn driver_step(results: AiActionsRun) -> DriverStep { + DriverStep { + actions_taken: results.results.len(), + break_reason: results.break_reason, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn dummy_result(state: &GameState) -> AiActionResult { + AiActionResult { + action: GameAction::PassPriority, + state: state.clone(), + events: Vec::new(), + log_entries: Vec::new(), + } + } + + #[test] + fn driver_step_preserves_break_reason_from_non_empty_batch() { + // The exact regression: a batch that completed an action must not + // have its break_reason discarded just because `results` isn't + // empty. + let state = GameState::new_two_player(1); + let run = AiActionsRun { + results: vec![dummy_result(&state)], + break_reason: Some(AiActionsBreakReason::ChooseActionNone { + player: PlayerId(1), + }), + }; + let step = driver_step(run); + assert_eq!(step.actions_taken, 1); + assert!( + matches!( + step.break_reason, + Some(AiActionsBreakReason::ChooseActionNone { .. }) + ), + "break_reason from a non-empty batch must survive driver_step" + ); + } + + #[test] + fn driver_step_empty_batch_behavior_is_unchanged() { + // Existing behavior (empty batch + break reason) must still work. + let run = AiActionsRun { + results: Vec::new(), + break_reason: Some(AiActionsBreakReason::NoActor), + }; + let step = driver_step(run); + assert_eq!(step.actions_taken, 0); + assert!(matches!( + step.break_reason, + Some(AiActionsBreakReason::NoActor) + )); + } + + #[test] + fn driver_step_ordinary_batch_is_unaffected() { + // Ordinary caller path: batch completed actions and hit no break + // door (e.g. hit the safety cap) — driver_step must not fabricate a + // stop signal. + let state = GameState::new_two_player(1); + let run = AiActionsRun { + results: vec![dummy_result(&state), dummy_result(&state)], + break_reason: None, + }; + let step = driver_step(run); + assert_eq!(step.actions_taken, 2); + assert!(step.break_reason.is_none()); + } +} diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index ee5aaf9082..796f97e20c 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -21,7 +21,7 @@ use engine::game::deck_loading::{ use engine::types::format::FormatConfig; use engine::types::game_state::{GameState, WaitingFor}; use engine::types::player::PlayerId; -use phase_ai::auto_play::run_ai_actions; +use phase_ai::auto_play::{driver_step, run_ai_actions}; use phase_ai::config::{create_config_for_players, AiConfig, AiDifficulty, Platform}; use rand::rngs::StdRng; use rand::SeedableRng; @@ -215,10 +215,6 @@ fn main() { &mut ai_rng, &ai_session, ); - if results.is_empty() { - last_break_reason = results.break_reason; - break; - } if dump_log_path.is_some() { for r in &mut results { game_log.extend(std::mem::take(&mut r.log_entries)); @@ -229,7 +225,6 @@ fn main() { actions_log.push(format!("{:?}", r.action)); } } - total_actions += results.len(); if state.turn_number != last_turn_reported { last_turn_reported = state.turn_number; @@ -249,6 +244,20 @@ fn main() { let _ = std::io::stdout().flush(); } + // phase#6080 follow-up: a batch can complete one or more actions and + // still carry a break_reason (e.g. ApplyFailed/ChooseActionNone hits + // after earlier actions in the same batch applied cleanly). Capture + // that reason from EVERY batch — not only empty ones — and stop the + // driver at this batch boundary, after this batch's completed + // actions are already retained above, so the stall report below + // reflects the original break door instead of a later, unrelated one. + let step = driver_step(results); + total_actions += step.actions_taken; + if step.break_reason.is_some() { + last_break_reason = step.break_reason; + break; + } + if total_actions >= MAX_TOTAL_ACTIONS { aborted = true; println!(); diff --git a/crates/phase-ai/tests/ai_quality.rs b/crates/phase-ai/tests/ai_quality.rs index cd75361ddf..c75905896e 100644 --- a/crates/phase-ai/tests/ai_quality.rs +++ b/crates/phase-ai/tests/ai_quality.rs @@ -18,7 +18,7 @@ use engine::types::game_state::{PlayerDeckPool, WaitingFor}; use engine::types::mana::ManaCost; use engine::types::phase::Phase; use engine::types::player::PlayerId; -use phase_ai::auto_play::run_ai_actions; +use phase_ai::auto_play::{driver_step, run_ai_actions, AiActionsBreakReason}; use phase_ai::choose_action; use phase_ai::config::{create_config, AiDifficulty, Platform}; use phase_ai::score_candidates; @@ -263,6 +263,76 @@ fn ai_vs_ai_completes_combat_sequence() { ); } +#[test] +fn run_ai_actions_non_empty_batch_carries_break_reason() { + // phase#6080 follow-up (PR #6194 review): `run_ai_actions` can complete + // one or more actions and *still* stop on a break door (here: P1 is + // nominally AI-controlled via `ai_players` but has no entry in + // `ai_configs` — a caller wiring gap distinct from the ordinary "no + // actor" case). The old `ai_commander` driver only checked + // `break_reason` when the returned batch was empty, so this exact + // shape (non-empty batch + Some(break_reason)) got silently discarded. + // This asserts `run_ai_actions` reports it, and that `driver_step` — the + // helper the driver now uses — preserves it and signals a stop. + let mut scenario = GameScenario::new(); + scenario.with_life(P0, 5); + let attacker = scenario.add_creature(P1, "Attacker", 6, 6).id(); + let blocker = scenario.add_creature(P0, "Blocker", 2, 2).id(); + + let mut runner = scenario.build(); + { + let state = runner.state_mut(); + state.phase = Phase::DeclareBlockers; + state.active_player = P1; + state.combat = Some(CombatState { + attackers: vec![AttackerInfo::attacking_player(attacker, P0)], + ..Default::default() + }); + state.waiting_for = WaitingFor::DeclareBlockers { + player: P0, + valid_blocker_ids: vec![blocker], + valid_block_targets: HashMap::from([(blocker, vec![attacker])]), + block_requirements: HashMap::new(), + blocker_constraints: Default::default(), + }; + } + + // Both P0 and P1 are declared AI-controlled, but only P0 has a config. + // P0's DeclareBlockers action applies successfully; priority then moves + // to P1 (the active player), whose missing config stops the batch — + // after that one action already completed. + let ai_players: HashSet = [P0, P1].into_iter().collect(); + let config = create_config(AiDifficulty::Medium, Platform::Native); + let ai_configs = HashMap::from([(P0, config)]); + let mut ai_rng = SmallRng::seed_from_u64(42); + let ai_session = phase_ai::session::AiSession::arc_from_game(runner.state()); + + let results = run_ai_actions( + runner.state_mut(), + &ai_players, + &ai_configs, + &mut ai_rng, + &ai_session, + ); + + assert!( + !results.is_empty(), + "P0's DeclareBlockers action should have applied before the batch stopped" + ); + assert!( + matches!(results.break_reason, Some(AiActionsBreakReason::NoActor)), + "expected NoActor once priority reaches P1's missing config" + ); + + let step = driver_step(results); + assert_eq!(step.actions_taken, 1); + assert!( + step.break_reason.is_some(), + "driver_step must preserve the break reason from a non-empty batch \ + so the driver stops at this boundary instead of discarding it" + ); +} + #[test] fn declare_blockers_never_produces_pass_priority() { // Regression test: the AI must return DeclareBlockers even when From 68ea9098ea1e5a7223b20e7306ab036b4e27e846 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 19 Jul 2026 14:49:46 -0700 Subject: [PATCH 5/5] fix(ai): split MissingAiConfig out of NoActor in AiActionsBreakReason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The missing-configuration break door recorded `NoActor`, so `ai_commander` reported a caller wiring gap (an AI seat in `ai_players` with no `ai_configs` entry) as "no actor" — two stalls with different remediation printed as one. Adds `MissingAiConfig { player }` and records it at that door. `NoActor` stays fieldless and now documents the two causes it still covers: `acting_players()` returned empty (`GameOver`, or an empty pending set), or it returned players none of whom map to an AI seat. It carries no `PlayerId` because the first cause has no player at all, and `MulliganDecision`/`OpeningHandBottomCards` can pend several at once, so naming one would be arbitrary. The regression expectation now asserts `MissingAiConfig { player: P1 }` via `matches!`, which pins the missing-config door specifically — `NoActor` has no field and cannot match it. Also addresses two bot notes: corrects the `ApplyFailed` doc comment (only `action` is boxed; `EngineError` is four variants with a `String` at worst, so `large_enum_variant` does not fire on it), and replaces hardcoded `ai_decks[0]`/`[1]` indexing in the deck-count print with seat iteration. Co-Authored-By: Claude Opus 4.8 --- crates/phase-ai/src/auto_play.rs | 33 +++++++++++++++---------- crates/phase-ai/src/bin/ai_commander.rs | 12 +++------ crates/phase-ai/tests/ai_quality.rs | 13 +++++++--- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/crates/phase-ai/src/auto_play.rs b/crates/phase-ai/src/auto_play.rs index 835b8253a8..243ae6d868 100644 --- a/crates/phase-ai/src/auto_play.rs +++ b/crates/phase-ai/src/auto_play.rs @@ -26,7 +26,7 @@ pub struct AiActionResult { pub log_entries: Vec, } -/// Which of `run_ai_actions`'s three break doors (see its doc comment) ended +/// Which of `run_ai_actions`'s four break doors (see its doc comment) ended /// the batch before the safety cap. `None` means the loop ran out AI actions /// to take for a benign reason the caller already distinguishes elsewhere /// (hit `MAX_AI_ACTIONS_PER_SEQUENCE`, or the very first iteration found no @@ -39,18 +39,29 @@ pub struct AiActionResult { /// caller like `ai_commander` print it instead of installing a subscriber. #[derive(Debug, Clone)] pub enum AiActionsBreakReason { - /// No acting player was found for the current `waiting_for`, or none of - /// the acting players maps (via `turn_control::authorized_submitter_for_player`) - /// to an AI-controlled seat. + /// No AI seat can currently act. Two causes are still folded together + /// here: `WaitingFor::acting_players()` returned empty (`GameOver`, or an + /// empty pending set), or it returned one or more players and none of + /// their `turn_control::authorized_submitter_for_player` mappings is in + /// `ai_players` (a human seat, or a human turn-decision controller). + /// Deliberately carries no `PlayerId`: the first cause has no player at + /// all, and the simultaneous-decision variants (`MulliganDecision`, + /// `OpeningHandBottomCards`) can pend several at once, so naming one + /// would be arbitrary. A missing AI *configuration* is `MissingAiConfig`. NoActor, + /// `player` is in `ai_players` but has no entry in `ai_configs`. Distinct + /// from `NoActor`: an actor *was* found and *is* AI-controlled, so the + /// remedy is caller wiring (register a config for this seat), not "wait + /// for a human" or "the game ended". + MissingAiConfig { player: PlayerId }, /// `choose_action_with_session` returned `None` for `player` — the AI /// policy stack produced no legal action for a decision it was asked to /// make. ChooseActionNone { player: PlayerId }, - /// `apply()` rejected `player`'s chosen `action`. Boxed: `GameAction` and - /// `EngineError` are large relative to the other variants (clippy - /// `large_enum_variant`), and this variant is a diagnostic path taken - /// only when a batch breaks early, not a hot allocation. + /// `apply()` rejected `player`'s chosen `action`. `action` is boxed because + /// `GameAction` is large relative to the other variants (clippy + /// `large_enum_variant`); `EngineError` is four small variants (largest + /// payload a `String`) and needs no box. ApplyFailed { player: PlayerId, action: Box, @@ -143,11 +154,7 @@ pub fn run_ai_actions( Some(c) => c, None => { tracing::warn!(player = ?actor, "AI seat has no config — stopping AI loop"); - // No dedicated break-reason variant: a missing config is a - // caller wiring bug (every AI seat must be registered), not - // one of the three doors phase#6080 instruments. Falls back - // to the closest existing category. - break_reason = Some(AiActionsBreakReason::NoActor); + break_reason = Some(AiActionsBreakReason::MissingAiConfig { player: actor }); break; } }; diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index 796f97e20c..95e9dbc6bc 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -162,14 +162,10 @@ fn main() { // detect silent-skip drift (a resolved count short of the pre-resolution // one) without needing engine internals. println!("Resolved deck sizes (post name-resolution, 0-indexed by seat):"); - for (i, seat) in [ - &payload.player, - &payload.opponent, - &payload.ai_decks[0], - &payload.ai_decks[1], - ] - .into_iter() - .enumerate() + for (i, seat) in [&payload.player, &payload.opponent] + .into_iter() + .chain(payload.ai_decks.iter()) + .enumerate() { let main_count: u32 = seat.main_deck.iter().map(|e| e.count).sum(); let commander_count: u32 = seat.commander.iter().map(|e| e.count).sum(); diff --git a/crates/phase-ai/tests/ai_quality.rs b/crates/phase-ai/tests/ai_quality.rs index c75905896e..53fc766e72 100644 --- a/crates/phase-ai/tests/ai_quality.rs +++ b/crates/phase-ai/tests/ai_quality.rs @@ -268,8 +268,9 @@ fn run_ai_actions_non_empty_batch_carries_break_reason() { // phase#6080 follow-up (PR #6194 review): `run_ai_actions` can complete // one or more actions and *still* stop on a break door (here: P1 is // nominally AI-controlled via `ai_players` but has no entry in - // `ai_configs` — a caller wiring gap distinct from the ordinary "no - // actor" case). The old `ai_commander` driver only checked + // `ai_configs`). That door reports `MissingAiConfig { player }`: an actor + // was found and is AI-controlled, so it is a caller wiring gap, not the + // `NoActor` stall. The old `ai_commander` driver only checked // `break_reason` when the returned batch was empty, so this exact // shape (non-empty batch + Some(break_reason)) got silently discarded. // This asserts `run_ai_actions` reports it, and that `driver_step` — the @@ -320,8 +321,12 @@ fn run_ai_actions_non_empty_batch_carries_break_reason() { "P0's DeclareBlockers action should have applied before the batch stopped" ); assert!( - matches!(results.break_reason, Some(AiActionsBreakReason::NoActor)), - "expected NoActor once priority reaches P1's missing config" + matches!( + results.break_reason, + Some(AiActionsBreakReason::MissingAiConfig { player: P1 }) + ), + "expected MissingAiConfig(P1): P1 is an AI seat with no ai_configs entry, \ + which is not the same stall as NoActor" ); let step = driver_step(results);