feat(ai): per-seat difficulty, elimination/timing lines, action-cap flag for ai-commander

[mcbradd]

Previously STACKED on #6194 — that PR has now MERGED (squashed as 45c6b30b8), and this branch is rebased onto current main. This PR is now independently reviewable. Its diff is scoped to its own commits only; it does not re-list PR1's changes.

Summary

Three additions to ai-commander for pod-lab's mixed-skill gauntlet tables:

• Per-seat difficulty — --difficulty still sets the table-wide default, and new --difficulty-p0..--difficulty-p3 flags override a single seat (parsed through the existing single-authority parse_difficulty, so an override can never drift from the table-wide flag's validation). Each seat's config is still built through create_config_for_players (unchanged, reused per seat rather than once for the whole table), and the resolved per-seat difficulty is echoed at startup before the game loop starts, mirroring the existing anti-silent-downgrade philosophy parse_difficulty already established.
• Elimination lines — the engine already flips player.is_eliminated per CR 800.4 when a player leaves the game; the runner now watches that flag every batch and prints exactly one ELIMINATED: P{n} turn=… actions=… elapsed=… line the moment it flips, so a harness can tell an expected early elimination apart from a stall.
• Timing lines — the per-turn snapshot line already printed now also carries wall-clock elapsed=, and the elimination line above carries the same, so slow turns / perf regressions are visible from stdout alone instead of only the final Elapsed: summary.
• Action-cap flag — the previously-hardcoded MAX_TOTAL_ACTIONS constant is now --action-cap <N> (default unchanged, 200,000), parsed through a new parse_action_cap that hard-fails a non-positive/non-numeric value rather than silently keeping the default — the same silent-poison-failure class the difficulty parsing already guards against.

Plus, from review on this round:

• Centralized difficulty labels — the accepted --difficulty label list is hoisted out of ai_commander.rs into a module-level pub const ACCEPTED_DIFFICULTY_LABELS in config.rs, next to the AiDifficulty::from_label it mirrors, and consumed by the CLI validator. from_label itself is unchanged — it is an exhaustive match producing enum values, not a membership test, and driving it off a &[&str] would trade a compiler-checked match for a silently-driftable positional lookup. Instead, two tests in config.rs pin the const and the enum together in both directions: accepted_difficulty_labels_round_trip_through_from_label (every listed label round-trips: list → enum → list) and every_difficulty_variant_appears_in_accepted_labels (every enum variant, enumerated through a wildcard-free match that fails to compile when a variant is added, appears in the list and round-trips: enum → list → enum). One comment on the per-seat difficulty echo is also corrected; see the review comment for that and two disclosed follow-ups.
• Resurrected the ai-commander test target — dropped test = false from its [[bin]] in crates/phase-ai/Cargo.toml, so the bin's #[cfg(test)] region actually compiles and runs. It never did before, which meant the existing parse_action_cap test (and any bin-local test) was dead. The region now compiles for the first time under -D warnings.
• CLI failure-discipline (the resurrected tests cover these):
  • parse_action_cap split into a Result-returning parse_action_cap_checked core + a thin exiting wrapper, so both the accept and the reject paths are unit-tested (the wrapper's exit(1) branch was previously untestable).
  • --difficulty-pN no longer silently drops a mistyped flag: a non-numeric or out-of-range (>= 4) seat index, or a missing label, now hard-fails at startup like its parse_difficulty/parse_action_cap siblings, via a pure, unit-tested parse_seat_override. The label value arg is now consumed unconditionally, so a bad index can no longer leak its label into the arg catch-all.
• Epilogue classifier — the end-of-run aborted/stalled/completed decision (from aborted + waiting_for) is extracted into a pure, unit-tested classify_run_outcome; main matches on it for both the printed outcome and the exit code. Stdout is byte-identical on every reachable common path (verified by diffing the println! literals before/after) — pod-lab parses these lines. One narrowly-reachable corner does change: (aborted, GameOver) — the game ends on the last action of a batch and then the cap fires — now prints the abort line rather than the old inline code's contradictory "Game ended cleanly" (exit code 2 either way); Aborted is the honest label for a run the cap cut short.

Test plan

Re-run after the rebase onto main @ 45c6b30b8 (the previous run's results were voided by the rebase):

• cargo fmt --all clean
• cargo clippy -p phase-ai --all-targets -- -D warnings clean (now includes the resurrected bin cfg(test))
• cargo test -p phase-ai clean — new/resurrected tests: accepted_difficulty_labels_round_trip_through_from_label, every_difficulty_variant_appears_in_accepted_labels, parse_action_cap_accepts_positive_integer, parse_action_cap_rejects_zero_and_non_numeric, parse_seat_override_rejects_non_numeric_index, parse_seat_override_rejects_out_of_range_index, parse_seat_override_requires_a_label_value, parse_seat_override_accepts_valid_seat_and_label, classify_run_outcome_completed_only_on_clean_gameover, classify_run_outcome_aborted_when_cap_hit, classify_run_outcome_stalled_when_parked_off_gameover
• parser combinator gate clean, run with an explicit base (./scripts/check-parser-combinators.sh $(git merge-base upstream/main HEAD)) — Gate A + Gate G PASS
• stdout println! literals byte-identical to the pre-change branch on every reachable common path (per-run output unchanged; the one intended change is the (aborted, GameOver) corner disclosed above)

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces structured diagnostic tracking for AI action loop breaks to help identify and debug driver stalls in ai_commander. It wraps the results of run_ai_actions in an AiActionsRun struct containing a detailed AiActionsBreakReason, and updates ai_commander to detect stalls, log diagnostic context, and exit with a distinct status code. Additionally, it adds per-seat difficulty overrides, custom action caps, post-resolution deck size logging, and stricter CLI argument validation. The review feedback highlights a maintenance concern regarding the duplication of accepted difficulty labels in ai_commander.rs and recommends centralizing this source of truth in config.rs.

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.

[matthewevans]

Maintainer triage — held as a stacked dependency on #6194.

This branch contains #6194's commits plus its own changes, and the PR description explicitly asks for #6194 to be reviewed first. #6194 currently has a blocking review finding in its stall-diagnostic path, so this PR is not independently reviewable or approvable at its current base. I will resume review after #6194 is corrected and lands/rebases.

[mcbradd]

Addressed a review finding: the action-cap abort path (aborted=true, game not over) fell into the same non-GameOver match arm as a real driver stall and was printing the full softlock STALL block — including a factually wrong STALL: break_reason=unknown (... stall detected on a later empty batch this process did not observe) line, since last_break_reason is never set on the abort door. Fixed by guarding the STALL-reporting block with if !aborted; on abort, the ABORT: hit action cap=... line plus the existing Game did NOT reach GameOver. line (unchanged, printed in both cases) are sufficient. Exit-code precedence (2 for abort checked before 3 for stall) is unchanged.

For the record: as part of this same commit, ABORT: hit MAX_TOTAL_ACTIONS= became ABORT: hit action cap=. Downstream consumers pinning the old literal should update on their next fold-in — the known consumer's classification is unaffected since it matches on Game did NOT reach GameOver. first.

🤖 Generated with Claude Code

[matthewevans]

Maintainer status — processed current head 99915c0cc; stack hold remains.

This head still contains the explicitly stacked #6194 work. #6194 has an unresolved formal changes-requested review for loss of the original ApplyFailed/ChooseActionNone diagnostic after a non-empty batch. This PR remains held, rather than silently left unreviewed, until that dependency is corrected and lands/rebases.

[matthewevans]

Reprocessed current head 99915c0cc63ed60639ad8a6439ebb689a7deabd1 after its green CI.

This stacked PR remains held, not independently approved or enqueued: its base PR #6194 is still open at 62a84e2b2eb6a8cc3a8113186e2237ce806f547e with formal requested changes and an unresolved diagnostic-correctness blocker. Please resolve and land #6194, then rebase this branch for a fresh review.

[matthewevans]

[MED] --action-cap does not bound the configured number of actions. Evidence: crates/phase-ai/src/bin/ai_commander.rs:313-324 calls run_ai_actions before testing total_actions >= action_cap, while crates/phase-ai/src/auto_play.rs:19,136 permits a batch of 200 actions. Why it matters: --action-cap 1 (and every non-multiple of 200) can execute up to 199 actions beyond the operator-specified budget, so a harness cannot use the new flag to cap expensive or looping runs as advertised. Suggested fix: thread the remaining action budget into the driver/batch boundary (or execute no more than the remaining number of actions) and add a regression that proves a small cap is never exceeded.

[mcbradd]

Rebased and ready for a fresh review.

• Base fix(ai): surface ai-commander stalls + strict difficulty parse + post-resolution deck counts #6194 resolved and merged per your standing instruction. It landed squash-merged as 45c6b30b8, so this branch is rebased onto current main (which contains it). Because the merge was a squash, fix(ai): surface ai-commander stalls + strict difficulty parse + post-resolution deck counts #6194's commits aren't ancestors of main; I rebased with git rebase --onto upstream/main <bundle-a-tip> podlab/bundle-b so the stacked content is shed rather than replayed. The feature commit's diff-of-diffs against its base is byte-identical to the pre-rebase state (same diffstat, only context shifted). This PR now contains only its own work and is independently reviewable.
• Abort-path finding fixed and now tested. The end-of-run outcome logic is extracted into a pure classify_run_outcome. Its exit-code/print contract is unit-tested, including the narrowly-reachable (aborted, GameOver) corner (game ends on the last action of a batch, then the cap fires), which is deliberately reported as an abort. Those tests actually run now: crates/phase-ai/Cargo.toml dropped test = false on the ai-commander bin, whose #[cfg(test)] region had never compiled — so the existing parse_action_cap test was dead. The resurrected region compiles under -D warnings for the first time.
• Failure-discipline hardened on the new flags. parse_action_cap split into a Result core + exiting wrapper (both paths tested); --difficulty-pN now hard-fails on a non-numeric/out-of-range index or a missing label instead of silently dropping the flag and leaking its value into the arg catch-all — via a pure, unit-tested parse_seat_override.
• Difficulty-label centralization (the bot note): the accepted-label list is a single pub const ACCEPTED_DIFFICULTY_LABELS in config.rs next to from_label, consumed by the CLI validator, pinned by two drift tests that fail in either direction.

Stdout is byte-identical on every reachable common path (pod-lab parses these lines); the one intended change is the (aborted, GameOver) corner noted above. Verified: cargo fmt --all, cargo clippy -p phase-ai --all-targets -- -D warnings, cargo test -p phase-ai (all pass), and the parser gate with an explicit merge-base.

Note on CI: the failing Card data check is repo-wide baseline staleness from tonight's Hobbit-set auto-unlock — Bard, King of Dale entered the pool at 00:00Z and added a row to the frozen draw-replacement corpus — so it fails identically on main and every open PR; it is unrelated to this diff. A one-line re-freeze PR from podlab/corpus-freeze-bard is incoming.

[matthewevans]

Review status — the current-head blocker remains unresolved.

787f330ad9407f47e3b3a71e0a507ecd990938e8 is unchanged from the formal review. The new classify_run_outcome/CLI parsing work does not address the requested action-budget boundary: run_ai_actions can still complete 200 actions before ai_commander compares total_actions with --action-cap. Please thread the remaining budget into that batch boundary and add the small-cap regression requested in the review. The current Card-data failure is separately reported as baseline drift and is not the reason for this hold.

[matthewevans]

Approved: the bounded AI action runner enforces the remaining --action-cap at the production batch boundary, and the long-stream regression proves a small cap cannot be exceeded.

[matthewevans]

Request changes — the cap implementation is at the right batch boundary, but its regression does not protect the commander driver that exposed the bug.

🔴 Blocker

[MED] The action-cap regressions do not exercise the ai_commander production loop. Evidence: crates/phase-ai/tests/scenarios.rs:283-345 invokes run_ai_actions_bounded and reproduces the budget loop locally, while the production call site is crates/phase-ai/src/bin/ai_commander.rs:251-333. Why it matters: changing that production call back to run_ai_actions would reintroduce the up-to-199-action overshoot from the prior review, yet both new tests would still pass. Suggested fix: extract the commander batch/remaining-budget driver boundary into a testable helper used by main, then assert a small-cap long stream through that helper never exceeds—and reaches—the cap.

✅ Clean

The implementation itself now passes action_cap - total_actions into run_ai_actions_bounded, and the bounded primitive preserves the existing 200-action safety ceiling (crates/phase-ai/src/bin/ai_commander.rs:251-267, crates/phase-ai/src/auto_play.rs:145-173). That resolves the prior production overrun by inspection.

Recommendation: add a discriminating commander-driver regression, then I can re-review the current implementation.

[matthewevans]

🔴 Blocking

The new action-cap regression still does not drive the production ai_commander loop. crates/phase-ai/tests/scenarios.rs:312-341 duplicates the remaining-budget loop, while crates/phase-ai/src/bin/ai_commander.rs:251-333 owns the actual driver and only has parser/outcome-helper tests later in the file. A regression in the CLI loop’s budget plumbing would therefore keep this test green.

Extract the production driver loop into a testable function used by main, then exercise a small action cap through that function. That gives the regression coverage its claimed protection without maintaining a mirrored implementation.

Recommendation: request changes — add a discriminating production-driver action-cap test before this is reconsidered.

[matthewevans]

Approved: the remaining-budget loop is now a production run_driver_loop boundary, and the two-batch regression proves a small --action-cap is exact rather than a batch-boundary approximation.