fix(ai): surface ai-commander stalls + strict difficulty parse + post-resolution deck counts

[mcbradd]

Summary

Three fixes to ai-commander, the AI-only 4-player Commander sanity-check runner pod-lab's gauntlet harness drives at scale:

• Surface driver stalls instead of exiting as normal game end (0ec55a611). An empty run_ai_actions batch while waiting_for != GameOver was previously conflated with a clean game end at the driver, hiding the whole p0-softlock issue family (Everflowing Chalice — When attempting to cast, I am prompted to kick it or stop kicking it. #5250, Stuck decision: CombatTaxPayment #4345, Treasure cruise froze game — The Ai cast it and since the it has been frozen. #5958, Stuck decision: CastOffer #6172, Stuck decision: ReplacementChoice #3886, Stuck decision: ReplacementChoice #3919, Stuck decision: DeclareAttackers #3233) behind a silent success exit. run_ai_actions now returns AiActionsRun (Deref'd to the existing Vec<AiActionResult> for source compatibility) carrying a typed AiActionsBreakReason naming which of its three break doors fired (no AI actor, choose_action returned None, or apply() returned Err). 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 — converting the whole silent-softlock family into loud, diagnosable failures. The existing "Game did NOT reach GameOver" substring pod-lab's runner.py already matches on is preserved verbatim. Addresses [Card Bug] Native AI stalls on OptionalCostChoice (kicker / alternative cost) — ai-commander never reaches GameOver #6080 (driver-side surfacing; card-level fallback tracked separately).
• Hard-fail unknown --difficulty labels instead of silently downgrading (ebe4f7bb5). parse_difficulty previously matched five exact-case strings and silently fell back to Easy for anything else (typos, or CEDH, which had no arm at all) — poisoning an entire batch of games with the wrong skill tier with no indication anything went wrong. It now routes through the crate's single-authority AiDifficulty::from_label, but layers a hard startup error on top of any label from_label would otherwise silently downgrade, and lists every accepted value (VeryEasy, Easy, Medium, Hard, VeryHard, CEDH) in the error message.
• Print per-seat resolved deck counts after name resolution (62a84e2b2). resolve_deck_list silently skips any card name the database doesn't recognize, and the only prior print happened before that resolution ran, so nothing could prove a deck actually loaded full-size. Adds a per-seat "Resolved deck sizes (post name-resolution)" block summing each seat's resolved main_deck/commander counts, so a harness can detect silent-skip drift without reaching into engine internals.

Test plan

• cargo fmt --all clean
• cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings clean (pre-push hook)
• cargo test -p engine --features proptest parser::oracle::tests clean (pre-push hook)
• cargo test -p phase-ai --lib clean (pre-push hook)
• cargo check --release --bin card-data-validate clean (pre-push hook)
• parser combinator gate (scripts/check-parser-combinators.sh) clean (pre-push hook)
• coverage regression check clean (pre-push hook)
• pnpm lint / pnpm run type-check clean (pre-push hook)

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces structured diagnostic tracking for AI action execution stalls and improves command-line validation in ai_commander. Specifically, it defines an AiActionsBreakReason enum to capture why an AI execution batch broke early, integrates this tracking into the main loop, and adds strict validation for the --difficulty argument. The review feedback highlights opportunities to optimize the new enum's memory footprint by boxing EngineError, avoid brittle hardcoded indexing when printing resolved deck sizes, and prevent overwriting valuable diagnostic break reasons on subsequent loop iterations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

[MEDIUM] Unboxed large enum variant field. Evidence: crates/phase-ai/src/auto_play.rs:50-54. Why it matters: The comment states that both GameAction and EngineError are large and should be boxed to avoid clippy's large_enum_variant lint, but EngineError remains unboxed, leaving the enum size sub-optimal. Suggested fix: Box the EngineError field as well.

    ApplyFailed {
        player: PlayerId,
        action: Box<GameAction>,
        error: Box<EngineError>,
    },

[gemini-code-assist]

[MEDIUM] Missing box allocation for EngineError. Evidence: crates/phase-ai/src/auto_play.rs:178-182. Why it matters: To align with boxing EngineError in the AiActionsBreakReason enum definition, the error instance must be boxed here. Suggested fix: Wrap e in Box::new(e).

Suggested change

	break_reason = Some(AiActionsBreakReason::ApplyFailed {
	player: actor,
	action: Box::new(action),
	error: e,
	});
	break_reason = Some(AiActionsBreakReason::ApplyFailed {
	player: actor,
	action: Box::new(action),
	error: Box::new(e),
	});

[gemini-code-assist]

[MEDIUM] Brittle hardcoded indexing of deck payload. Evidence: crates/phase-ai/src/bin/ai_commander.rs:165-177. Why it matters: Directly indexing payload.ai_decks[0] and payload.ai_decks[1] is brittle and will panic at runtime if the deck list structure or player count is ever modified. Suggested fix: Use an iterator chain to dynamically traverse all seats.

Suggested change

	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}");
	}
	let seats = std::iter::once(&payload.player)
	.chain(std::iter::once(&payload.opponent))
	.chain(payload.ai_decks.iter());
	for (i, seat) in seats.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}");
	}

[gemini-code-assist]

[MEDIUM] Stale diagnostic break reason on non-empty batches. Evidence: crates/phase-ai/src/bin/ai_commander.rs:218-221. Why it matters: If a batch of AI actions takes some successful actions but then breaks early due to a high-fidelity failure (like ApplyFailed or ChooseActionNone), the current loop continues because results.is_empty() is false. In the next iteration, run_ai_actions will immediately return an empty result with a generic NoActor reason, overwriting the highly valuable original diagnostic reason. Suggested fix: Break the loop immediately whenever results.break_reason is Some(_).

Suggested change

	if results.is_empty() {
	last_break_reason = results.break_reason;
	break;
	}
	if results.break_reason.is_some() {
	last_break_reason = results.break_reason;
	break;
	}

[matthewevans]

Changes requested — the reported stall reason can be lost after real AI actions.

🔴 Blocker

crates/phase-ai/src/bin/ai_commander.rs:216-219 stores results.break_reason only when the returned action list is empty. But run_ai_actions can execute one or more actions and then stop on ApplyFailed or ChooseActionNone (crates/phase-ai/src/auto_play.rs:156-183). In that case this loop discards the diagnostic, invokes another batch, and may later print NoActor or unknown instead of the actual failed action/reason. That defeats the PR's stated purpose: making the original stall diagnosable.

Capture a non-None break reason from every batch and stop the driver at that batch boundary (after retaining its completed actions), then add a discriminating regression that exercises a non-empty batch followed by a failing apply/choice and asserts the emitted reason.

🟡 Non-blocking

The public run_ai_actions return-type change is currently untested across its diagnostic paths. The required regression above should also demonstrate the intended no-regression behavior for ordinary callers that consume action results.

Recommendation: request changes; please preserve the original break door before retrying the AI loop.

[mcbradd]

Fixed the review blocker: break_reason is now captured from every run_ai_actions batch, not only empty ones.

Previously the driver only checked results.break_reason inside the results.is_empty() branch, so a batch that completed one or more actions and then stopped on ApplyFailed/ChooseActionNone had its diagnostic silently discarded — the driver looped again and could later report NoActor/unknown on an unrelated empty batch instead of the real cause.

Fix: extracted driver_step (crates/phase-ai/src/auto_play.rs) as the single place that decides, from a batch's action count and break_reason, whether the driver should stop. ai_commander's loop now calls it after retaining/logging that batch's completed actions, and stops at that batch boundary with the original break reason preserved for the stall report.

Regression coverage added:

• driver_step_preserves_break_reason_from_non_empty_batch, driver_step_empty_batch_behavior_is_unchanged, driver_step_ordinary_batch_is_unaffected (unit tests in auto_play.rs) — cover the discriminating cases directly.
• run_ai_actions_non_empty_batch_carries_break_reason (crates/phase-ai/tests/ai_quality.rs) — forces run_ai_actions itself to return a real non-empty batch with a break_reason (DeclareBlockers applies, then priority reaches an AI seat with no registered config) and asserts driver_step handles it correctly, demonstrating ordinary callers are unchanged.

cargo fmt --all, cargo clippy -p phase-ai --all-targets -- -D warnings, and cargo test -p phase-ai (1354 unit + all integration tests) all pass.

The stacked branch (#6195) has been rebased on top of this fix — both the abort-path STALL-block guard (if !aborted) and the new break-reason capture are preserved together.

🤖 Generated with Claude Code

[matthewevans]

The non-empty-batch regression is now covered, but the diagnostic is still inaccurate for the case the new test exercises.

run_ai_actions finds P1 as an AI-controlled actor, then takes the ai_configs.get(&actor) == None branch, but records AiActionsBreakReason::NoActor. The commander prints that value as the stall cause, so a missing AI configuration is reported as "no actor" even though those require different remediation.

Please add a distinct missing-config reason (including the player) or otherwise preserve that distinction, and update the regression expectation. The completed CI is green; this is a correctness issue in the newly added diagnostic contract.

[mcbradd]

You're right — that door reported a caller wiring gap as "no actor", and those need different remediation. Fixed.

The fix

run_ai_actions takes the ai_configs.get(&actor) == None branch only after it has already found an actor and confirmed the seat is in ai_players. That door now records its own reason instead of borrowing 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 },

            None => {
                tracing::warn!(player = ?actor, "AI seat has no config — stopping AI loop");
                break_reason = Some(AiActionsBreakReason::MissingAiConfig { player: actor });
                break;
            }

And the regression expectation in run_ai_actions_non_empty_batch_carries_break_reason now pins that specific door:

    assert!(
        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"
    );

Why NoActor stays fieldless

Deliberate, and I want to be explicit about the reasoning rather than have it read as an oversight. NoActor fires when the find() over acting_players() yields nothing, and that has two shapes — one of which has no player to name at all:

• crates/engine/src/types/game_state.rs:7341 — WaitingFor::GameOver { .. } => None in acting_player(), which acting_players() funnels through at :7361 (_ => self.acting_player().into_iter().collect()). GameOver, and an empty pending set, both produce an empty Vec<PlayerId>. There is no player to attach.
• crates/engine/src/types/game_state.rs:7355-7360 — MulliganDecision and OpeningHandBottomCards both pending.iter().map(|e| e.player).collect(), so they can pend several players simultaneously. If the batch stops because none of them is an AI seat, picking one of the several to name would be arbitrary.

Attaching a PlayerId to NoActor would therefore either be a lie (empty case) or an arbitrary pick (simultaneous case) — reproducing the same imprecision you filed, one layer down. The distinction you asked for is carried by the new variant instead.

Disclosures

• NoActor still covers both the empty-acting_players() case and the actor-not-in-ai_players case; only the missing-configuration cause you named is split out here, and separating that second conflation is filed as follow-up rather than folded into the fix you asked for.
• The regression expectation asserts via matches! on the variant's player field, so no new derives were needed on the reason enum.
• The STALL printer already formats the reason with {:?}, so the new variant surfaces with no commander change; a hand-written Display was not added.
• The player: P1 in the updated expectation is what pins the missing-config door specifically — NoActor carries no field and cannot match it — so no second test was added.
• EngineError is four variants with a String at worst, so the comment was corrected rather than the type boxed; clippy's large_enum_variant does not fire on it.

Bot notes

• Deck-count indexing (ai_commander.rs:165-177): addressed. [&payload.player, &payload.opponent].into_iter().chain(payload.ai_decks.iter()).enumerate() replaces the hardcoded ai_decks[0]/[1]. ai_decks is built with exactly two entries, and both forms visit player → opponent → ai_decks[0] → ai_decks[1] in that order, so output is byte-identical for the 4-seat case while no longer being able to panic if the seat count changes.
• EngineError boxing: declined, with the comment corrected instead — see the disclosure above. The comment was the inaccurate part, not the type.

Proof

Revert the None => arm to AiActionsBreakReason::NoActor and run_ai_actions_non_empty_batch_carries_break_reason fails on the matches! assertion with expected MissingAiConfig(P1) — NoActor is fieldless, so it cannot satisfy the player: P1 pattern. (P1 is pub const P1: PlayerId = PlayerId(1) from engine::game::scenario, and PlayerId derives PartialEq + Eq, so that is a real value check and not a catch-all binding.)

Verification

cargo fmt --all, cargo clippy -p phase-ai --all-targets -- -D warnings (clean, zero warnings), and cargo test -p phase-ai — 1402 passed, 0 failed, 14 ignored. Pre-push hook (full workspace clippy + card-data pipeline + coverage regression + frontend lint/type-check) run, not bypassed.

🤖 Generated with Claude Code

[matthewevans]

Re-reviewed after the maintainer branch update.

The current head contains a clean merge of main; the implementation diff remains the same three reviewed phase-ai files. The missing-configuration diagnostic is now represented as MissingAiConfig { player }, and the non-empty-batch regression exercises the production run_ai_actions path. No additional implementation finding from this follow-up.

Required checks restarted on the updated head, so approval/enqueue remains pending their completion.

[matthewevans]

Approved after current-head re-review and the completed post-update checks: the AI stall diagnostic preserves non-empty-batch break reasons, distinguishes missing configuration from no actor, and has production-path regression coverage.