perf(ai): make the AI gates finish and leave evidence - #6969
Conversation
Both gates were silent for their entire run and wrote every artifact only after the last unit of work finished, so a CI timeout kill left nothing to diagnose with. Measured: workflow run 30782543973 shows 37m59s of zero output before the kill, and across the last 200 `ai-gate.yml` runs (2026-07-26 .. 2026-08-03) 86 PR runs were killed at a 60.4m median by `timeout-minutes: 60` — on 2026-08-02, 22 of 22. - `duel_suite/run.rs`: one `start` line and one `done` line per game, carrying `HH:MM:SS` UTC, matchup id, game index, seed, winner, turn count and elapsed ms. Each line is self-identifying so worker interleaving is a grep problem, not an ordering problem. `utc_hms()` is seconds-of-day arithmetic — `phase-ai` depends on neither `chrono` nor `time`, and a bare elapsed counter cannot be lined up against a killed job's own timeline. - `duel_suite/perf.rs`: one `start` and one `done` line per perf scenario, the `done` line carrying that scenario's full counter payload as a single JSON line, so a killed sample still leaves a machine-readable partial result for every scenario that did complete. All new output is stderr. That is load-bearing in both gates: the perf parent runs its children with `Stdio::null()` on stdout to keep its markdown table clean (`bin/ai_perf_gate.rs`), and both nightly jobs pipe the gate's stdout into a GitHub issue body. The per-scenario lines are emitted after `run_perf_scenario` returns, i.e. outside the `perf_counters::reset()` / `snapshot()` window, so no counter can move. Assisted-by: ClaudeCode:claude-opus-4.8
…atchup `run_matchups_parallel` capped its worker count at the number of selected matchups (`n_workers = available_parallelism().min(run_total)`), and the games inside a matchup ran strictly sequentially. The PR gate selects three matchups (`ai_gate.rs`'s `DEFAULT_QUICK_FILTER`), so it ran three-wide no matter how many cores were available, and its wall clock was the longest single matchup's serial game time — not the total work divided by the core count. The cursor now hands out `(matchup, game)` pairs. A game is a pure function of `(payload, seed, difficulty, action_cap)` and its seed is still `base_seed + matchup_idx*1000 + game_idx` derived from the matchup's ORIGINAL index, so nothing about a game's result depends on which worker picked it up or when. Supporting extractions, each now the single authority for its concern so the sequential (attribution / harvest) runner and the parallel runner cannot drift apart: - `play_reported_game` — seed derivation, `catch_unwind`, harvest side-channel and progress lines for one game. - `assemble_matchup_result` — the win/draw tally and the `classify` verdict, computed from the games vector alone. - `game_tasks` — the work list; a matchup whose decks failed to resolve contributes zero tasks and keeps its `failed_result`. - `regroup_games` — regroups by matchup and restores `game_idx` order. Deck payloads are resolved once per matchup on the calling thread before the fan-out, so a deck-resolution failure is DETECTED before any game starts and no game re-resolves a deck the sequential runner resolved once. (It is not *printed* any earlier: `build_payload` is silent, and the failure still reaches the operator through `failed_result` in the post-join row loop.) Ordering is not cosmetic: `MatchupResult::games` is a `deterministic_core` field, so completion order leaking into it would be a baseline diff on every parallel run. `regroup_games_restores_game_index_order_from_shuffled_completion` feeds a deliberately interleaved, out-of-order completion list; deleting the `sort_by_key` makes it fail. `game_tasks_skip_matchups_whose_payload_failed` fails if the `is_ok()` filter is dropped. Output contract change, in both directions. Gained: per-game lines stream live and interleave across matchups, each tagged with its matchup id. Lost: the per-matchup verdict rows now all print after the join, so a run killed at `timeout-minutes: 60` emits ZERO verdict rows where the old completion-order printer emitted one per finished matchup. That is a real regression in kill-resilience at the matchup level and it is disclosed rather than buried — it is a net gain only because each per-game `done` line carries id, seed, winner and turns, so the tally and the `classify` input are reconstructible from the log. The post-join order is also selection order, which is deterministic; the old completion-order counter was not. `play_reported_game` also reads the wall clock (for `elapsed_ms` and the progress timestamps), but nothing it reads there enters the returned `GameResult`. The suite is NOT wall-clock-free further down, and the module docs now say where: `projection.rs`'s 15 ms `TIME_CAP` is not gated on measurement mode. That is pre-existing and orthogonal to this commit. The sequential branch is deliberately untouched. Attribution installs a thread-local `tracing` dispatcher via `with_default`, so events raised on scoped worker threads would bypass `CaptureLayer` and `drain()` would return nothing — parallelising it would silently empty the attribution, not merely interleave it. Harvesting joins it because one `HarvestSink` owns one append stream. Neither is on the CI path: `ai_gate.rs` sets neither option. Assisted-by: ClaudeCode:claude-opus-4.8
`bin/ai_gate.rs` and `bin/ai_perf_gate.rs` both install
`#[global_allocator] mimalloc::MiMalloc`, and both are invoked through
`.cargo/config.toml` aliases that carry no profile flag — so CI runs them in
the dev profile. The workspace had no `[profile.dev.package]` overrides, so
mimalloc's entire C core (one unity translation unit,
`libmimalloc-sys-0.1.49/c_src/mimalloc/v3/src/static.c`) inherited
`opt-level = 0`: every allocation in a gate run walked an unoptimized malloc.
Measured on the linked artifact, both ways:
readelf --debug-dump=info target/debug/ai-gate | grep -A1 'GNU C23'
before: `GNU C23 16.1.1 ... -g -gdwarf-4 -O0 -ffunction-sections ...`
after: `GNU C23 16.1.1 ... -g -gdwarf-4 -O2 -ffunction-sections ...`
followed in both cases by
`DW_AT_name: .../libmimalloc-sys-0.1.49/c_src/mimalloc/v3/src/static.c`.
Same flip on `ai-perf-gate`. Package-profile propagation into the `cc` build
is therefore confirmed, not assumed.
This is NOT provably payload-neutral, and saying so is worth more than a clean
claim. Implementation review found a live wall-clock branch on the AI's
decision path: `phase-ai/src/projection.rs:110` is `TIME_CAP = 15ms`, `:139-142`
bails on it, and unlike `search.rs:2012` and `planner/mod.rs:691` it is NOT
gated on measurement mode. It is reachable at the gate's default
`AiDifficulty::Medium` — `policies/registry.rs` registers
`EvasionRemovalPriorityPolicy` unconditionally and its `velocity_score` calls
`AiSession::get_or_project` → `project_to`, with the `projection_min_budget_ms`
floor bypassed because a measurement-mode `Deadline::none()` reports
`expired() == false` and `remaining() == None`. A bail scores 0.0 where a
completed projection scores up to +3.0, and that term selects the removal
target. Making allocation faster therefore lets more projections finish, which
can move a target, a board, a winner, and every counter downstream.
That hazard is pre-existing and host-speed-wide: it fires on any faster or
slower machine, and this override neither creates it nor can avoid it. The fix
— gating `TIME_CAP` on measurement mode the way `search.rs:2012` already does —
is a behavior change that needs its own baseline sign-off and does not belong
in a build-profile commit. What IS established here is only what `readelf`
shows. Scoped to the one package so
nothing else loses debug fidelity. `debug` is deliberately left alone —
`libmimalloc-sys`'s build script keys `MI_BUILD_RELEASE`/`NDEBUG` off it, and
it is also what keeps `DW_AT_producer` available as the evidence channel above.
Assisted-by: ClaudeCode:claude-opus-4.8
… in debug `search::fallback_action`'s pending-cast branch is a HANDLED condition: it logs and returns `CancelCast` (CR 601.2), which is what every release build has always done. It also carried `debug_assert!(false, ...)`, which made the two profiles disagree about the game *result*, not just about diagnostics: - both AI gates run the dev profile (`.cargo/config.toml`: `ai-gate = "run --bin ai-gate --"`, no profile flag), so CI panics here; - `duel_suite/run.rs` catches that panic per game and scores the seed `(None, 0)` — a DRAW release would have played out; - the committed `suite-baseline.json` it is compared against is release-generated (`scripts/refresh-ai-baseline.sh` execs `scripts/ai-gate.sh`, which does `cargo build --release --bin ai-gate`). So CI was comparing a debug run against a release-cut baseline across an assert that only exists in one of them. Removing it makes the CI run agree with the baseline's own profile. No baseline is refreshed by this commit. Verified the committed baseline is unaffected: it contains zero `turns == 0` games, and its only two `winner: null` rows are affinity-mirror seeds 10592735/10592736 at `turns: 8` — genuine draws, not panic artifacts. The diagnostic guidance moves verbatim into the `tracing::error!` that was already there; the gap the assert guarded is still a real bug, and the error event is still the way to find it. The two Claws-of-Gix scenarios named that assert as their detector. Their backing assertion, `assert!(results.len() <= 200)`, is structurally vacuous: `run_ai_actions` is hard-capped at `MAX_AI_ACTIONS_PER_SEQUENCE = 200` (`auto_play.rs`), so the predicate holds for every possible run. Replaced with `assert_no_fallback_cancel`, which is sound because `tactical_gate.rs` rejects `CancelCast` from the strategic pool — an APPLIED `CancelCast` can only have come from `fallback_action`. Unlike the assert it replaces, it also holds in release builds. Measured while strengthening those two tests, and disclosed rather than quietly absorbed: on the WITNESS board the AI does not activate the Claws at all — driving the loop yields exactly `[PassPriority]`. So that test was vacuous for the scenario its name describes, both before this change (the removed `debug_assert` could never fire on a board the AI passes on) and after. Its load-bearing assertion is therefore the new `activation_legal_for` precondition, which fails the moment a Metalcraft/cost regression stops `legal_actions` surfacing the activation; its doc now states exactly that instead of claiming a completion it does not observe. The positive `ActivateAbility` assertion went to the mana-first sibling, which does complete the activation (measured: `[ActivateAbility, SelectCards, PassPriority]`). The same assertion failing on the witness board is that assertion's own discriminating control. Why the AI declines a legal, witnessed Claws activation is a separate question and is not in this commit's scope. Disclosed trade: in dev, a seed that reaches this branch used to abort the game at `(None, 0)` immediately. It will now churn cast → CancelCast until `MAX_TOTAL_ACTIONS = 10_000` (`duel_suite/run.rs`) instead. Latent today — the release baseline's 30 games run 8-20 turns with no capped game, so no quick-gate seed reaches the branch — but it is a known cost, not a surprise, and it is the correct trade: a handled path must not be a panic in one profile only. `assert_no_fallback_cancel` checks both outcomes. `run_ai_actions` only records an action once `apply_interaction` succeeded, and a dead-end reached through the bare `allows_cancel_cast` disjunct is not enumerated by `candidate_actions_broad_with_probe`, so that shape lands in `break_reason` rather than in `results` and would slip past a results-only assertion. Assisted-by: ClaudeCode:claude-opus-4.8
📝 WalkthroughWalkthroughThe PR optimizes ChangesDuel suite execution and reporting
Pending-cast fallback behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run_parallel_suite
participant play_reported_game
participant assemble_matchup_result
run_parallel_suite->>play_reported_game: execute one matchup game
play_reported_game-->>run_parallel_suite: return game result and duration
run_parallel_suite->>assemble_matchup_result: aggregate ordered game results
assemble_matchup_result-->>run_parallel_suite: return matchup summary
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — repository hard-stop review is required for this build-profile change before this PR can receive implementation review.
[HIGH] This PR changes workspace build configuration. Evidence: Cargo.toml:47-75 adds [profile.dev.package.libmimalloc-sys] opt-level = 2; the current policy packet classifies Cargo.toml as a hard_stop path. The change applies to the native AI binaries and the PR itself documents that it can change live, time-capped projection completion and therefore downstream AI decisions. Why it matters: this is not an isolated harness refactor; it alters dependency compilation and decision behavior across the dev profile, so it needs a dedicated maintainer-approved performance/baseline review before the remaining implementation can be evaluated or merged. Suggested fix: split or otherwise obtain the required maintainer review for the Cargo profile override, with the decision-impact/baseline evidence scoped to that change; then request a fresh review of the resulting current head.
No implementation verdict is implied by this hard-stop disposition.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/phase-ai/src/duel_suite/run.rs (4)
578-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTag the abort lines with the timestamp and matchup id.
Under the game-level runner, games from different matchups interleave on stderr. The two abort lines print only the seed, so an operator cannot attribute an aborted game to a matchup without cross-referencing seed arithmetic. Every other progress line in this function carries
utc_hms()andspec.id. Make the abort lines match.Both branches also repeat the same
catch_unwind+ abort-print shape. A small local closure for the abort report would keep the two in sync.♻️ Proposed change
- Err(_) => { - eprintln!(" seed {seed} aborted: AI panic during suite game"); - (None, 0) - } + Err(_) => { + eprintln!( + "{ts} [{id}] game {n}/{total} seed={seed} aborted: AI panic during suite game", + ts = utc_hms(), + id = spec.id, + n = game_idx + 1, + total = options.games_per_matchup, + ); + (None, 0) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/duel_suite/run.rs` around lines 578 - 613, Update both AI-panic abort reports in the harvest and non-harvest branches of the game-level runner to include utc_hms() and spec.id alongside the seed, matching the surrounding progress lines. Extract the shared abort-report formatting into a small local closure and reuse it from both catch_unwind Err branches.
1109-1231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for seed derivation from the original matchup index.
The tests cover task filtering, regrouping, empty slots, and the aggregation denominator. They do not cover the one contract that the restructuring most easily breaks: the worker derives
matchup_seedfromidx(the matchup's original position inall_matchups()), not frompos(its position inselected). With an id filter active, those two differ. If a later edit usespos, every seed shifts and only a full baseline comparison detects it.Extract the derivation into a small helper, then assert it against a filtered selection.
🧪 Suggested helper and test
fn matchup_seed(base_seed: u64, original_idx: usize) -> u64 { base_seed.wrapping_add(original_idx as u64 * 1_000) } #[test] fn game_seed_follows_original_matchup_index_not_selection_position() { // `selected` position 0 holding original index 3 must seed from 3, not 0. assert_eq!(matchup_seed(7, 3).wrapping_add(2), 3_009); }As per path instructions: "Strengthen tests around observable legality, fallback cancellation, aggregation denominators, task filtering, regrouping, and empty slots; avoid vacuous assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/duel_suite/run.rs` around lines 1109 - 1231, Extract matchup seed calculation into a helper such as matchup_seed, using the original matchup index rather than the selected-position index, while preserving wrapping arithmetic. Add a focused test with a filtered selection where original index 3 is at selected position 0, and assert the derived game seed reflects index 3.Source: Path instructions
319-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a named task type instead of
(usize, usize).The pair
(usize, usize)flows throughgame_tasks, the worker loop, andregroup_gamesas(matchup position, game index). A smallstruct GameTask { matchup_pos: usize, game_idx: usize }would make each field self-describing and remove the risk of swapping the two indices in a later edit. The same applies to the 4-tuple(usize, usize, GameResult, u128).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/duel_suite/run.rs` around lines 319 - 335, Replace the positional `(usize, usize)` game-task representation with a named `GameTask` struct containing `matchup_pos` and `game_idx`, and update `game_tasks`, the worker loop, and `regroup_games` to use its fields. Also introduce a named type for the `(usize, usize, GameResult, u128)` value passed through those paths, updating construction and destructuring without changing behavior.
476-485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProgress numbering now differs between the two runners.
The sequential branch labels rows with the matchup's original index over
matchups.len()(Line 293-298). This loop labels rows with the selection position over the selected count. With an id filter active, the same matchup gets a different label depending on which runner ran it. Reuseselected's original index andtotalhere so log lines stay comparable across branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/duel_suite/run.rs` around lines 476 - 485, The selected-results loop should use each matchup’s original index and the overall matchup total, matching the sequential runner’s progress labels. Update the loop around result.matchup_id to retain or derive the corresponding original index from selected and format progress with that index and total instead of the local enumerate position and results.len().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 73-74: Validate the opt-level change for libmimalloc-sys under the
authoritative dev profile by generating before/after paired-seed AI evidence and
comparing projection completion, target selection, winners, and counters against
the 15ms cap. Do not rely on --release-generated suite-baseline.json; if the
optimization is intended only for the PR gate, move it into a dedicated profile
instead of [profile.dev.package.libmimalloc-sys].
---
Nitpick comments:
In `@crates/phase-ai/src/duel_suite/run.rs`:
- Around line 578-613: Update both AI-panic abort reports in the harvest and
non-harvest branches of the game-level runner to include utc_hms() and spec.id
alongside the seed, matching the surrounding progress lines. Extract the shared
abort-report formatting into a small local closure and reuse it from both
catch_unwind Err branches.
- Around line 1109-1231: Extract matchup seed calculation into a helper such as
matchup_seed, using the original matchup index rather than the selected-position
index, while preserving wrapping arithmetic. Add a focused test with a filtered
selection where original index 3 is at selected position 0, and assert the
derived game seed reflects index 3.
- Around line 319-335: Replace the positional `(usize, usize)` game-task
representation with a named `GameTask` struct containing `matchup_pos` and
`game_idx`, and update `game_tasks`, the worker loop, and `regroup_games` to use
its fields. Also introduce a named type for the `(usize, usize, GameResult,
u128)` value passed through those paths, updating construction and destructuring
without changing behavior.
- Around line 476-485: The selected-results loop should use each matchup’s
original index and the overall matchup total, matching the sequential runner’s
progress labels. Update the loop around result.matchup_id to retain or derive
the corresponding original index from selected and format progress with that
index and total instead of the local enumerate position and results.len().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e68cebd0-87f1-4c99-9ade-49f752c1edeb
📒 Files selected for processing (5)
Cargo.tomlcrates/phase-ai/src/duel_suite/perf.rscrates/phase-ai/src/duel_suite/run.rscrates/phase-ai/src/search.rscrates/phase-ai/tests/scenarios.rs
|
Maintainer-side review. I had independently profiled this from the CI side (#6967) before this PR landed, so this is a cross-check against a second data set plus four concrete proposals. Short answer: yes, this helps, and the direction is right — but the arithmetic says it does not reach green on a hosted runner by itself. What I verified at source (not taken on trust)All three of your disclosed pre-existing findings are real. I checked each against
That third one is the most interesting thing in this PR and I don't think the summary gives it enough weight: it means some fraction of the existing baseline was never comparable to what the gate measures. The per-game telemetry is already earning its keepI could compute everything below only because of commit A. Your PR's own gate run ( Where I disagree: this is not sufficient on a 4-vCPU runnerFrom your own run's per-game lines:
Extrapolating the 8 unfinished games at the observed mean gives ~269 CPU-minutes of work.
This is not an argument against the PR — game-level parallelism is strictly better than matchup-level, and the skew in that table (30.1 min vs 2.3 min games) is exactly the case matchup-level parallelism handles worst. It is an argument that one more lever is needed in the same change, or the gate stays red and all of this good work reads as "still broken." ProposalsP1 — Add the missing multiplier: optimize the compute crates, not just the allocator
[profile.dev.package.phase-engine]
opt-level = 2
[profile.dev.package.phase-ai]
opt-level = 2Two things make this safe, and they are the same two your PR already relies on:
Please measure rather than assume the multiplier — a compute-bound Rust workload usually gains far more from this than from an allocator change, but the honest number is whatever your A/B harness reports, and you already have the harness built. Worth reporting the compile-time cost alongside it, since it lands on every dev build. P2 — Land the comparator fix with this, not after itI understand the instinct to keep it separate, and normally I'd agree. Here the sequencing is the problem:
Shipping throughput first converts a loudly-broken gate into a quietly-wrong one, and the quiet version will be trusted. The fix is small and mechanical: make the match exhaustive over P3 — Gate
|
|
Correction to my P1 — and it changes the ordering, so please read before acting on my previous comment. I justified adding I've now walked the chain you describe and it holds at every step:
So speed is an input to the verdict on this path. That is precisely the hazard your commit C comment declines to hide, and you were right to state it rather than claim allocator changes are invisible. The consequence for my proposal is worse than for yours, not better: if a 1.16× allocator speedup can move a target through Revised ordering. P3 is not a companion to P1 — it is a prerequisite for it:
This also means the honest reading of your commit C is that it carries a real, disclosed risk today — and the fix for that risk is step 1, not more caution about allocators. What does not change: commits A (per-game progress) and B (game-level parallelism) are unaffected by any of this — B's neutrality rests on paired seeds and you demonstrated it with Apologies for the churn — my P1 was reasoned from the wrong half of the workflow's comment, and your |
|
🤖 AI text below 🤖 Flagging this now rather than after the fact, since you're actively working this area in #6967 and it affects the window you'd build on. Your P4 fast-reference anchor It resolves to The consequence is for the inference, not the experiment. You wrote that the last This is the third off-
Three independent instances turns "re-cut baselines only from a commit that is an Sequencing note: we're running the A/B after the Same courtesy you extended by verifying our disclosed defects at source before building |
2 participants
🤖 AI text below 🤖
Summary
Makes the two AI gates finish and leave evidence: per-game/per-scenario progress to
stderr, game-level (not matchup-level) parallelism in the duel suite, mimalloc's C core
compiled at
-O2in the dev profile the gates actually run under, and removal of adebug_assert!(false, …)that made the debug gate score a seed differently from therelease build its own baseline was cut with.
Context for why this matters now: across the last 200
ai-gate.ymlruns(2026-07-26 … 2026-08-03) 86 PR runs were killed by
timeout-minutes: 60at a 60.4 mmedian — 22 of 22 on 2026-08-02 — and all 3 scheduled nightly runs died at their 300 m
ceiling, so the drift issue never opens. A killed run currently produces nothing: the
suite prints only per-matchup lines and writes every artifact after the last matchup
returns. Run
30782543973shows 37m59s of silence before the kill.Files changed
Cargo.toml—[profile.dev.package.libmimalloc-sys] opt-level = 2crates/phase-ai/src/duel_suite/run.rs— per-game progress; game-level parallel runnercrates/phase-ai/src/duel_suite/perf.rs— per-scenario progress with counter payloadcrates/phase-ai/src/search.rs— remove the pending-castdebug_assert!(false, …)crates/phase-ai/tests/scenarios.rs— replace the two vacuous Claws assertionsTrack
Developer
LLM
Model: claude-opus-4.8
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — no
crates/engine/game-logic change. This is AI-gate harness(
crates/phase-ai/src/duel_suite/), one workspace build-profile override, and onecrates/phase-ai/src/search.rsassertion removal that does not alter any resolution path.Plan, plan-review and two rounds of implementation-review were still run as independent
agents.
CR references
None new.
search.rs's existing CR 601.2 rationale for theCancelCastrecovery ispreserved and re-stated in the comment that replaces the assert.
Verification
All numbers below are transcribed from named logs in the branch worktree; nothing is recalled.
Static gates at the exact committed head (
04185c70a, rebased onto1738d5c91)cargo fmt --all --checkcargo clippy -p phase-ai --all-targets -- -D warningscargo test -p phase-ai --lib duel_suitecargo test -p phase-ai --test scenarios claws./scripts/check-parser-combinators.shhead=04185c70a173999aff3ab5e1cd0d3664727057c0./scripts/check-prelowered-ratchet.shCommit C's propagation, measured both ways —
readelf --debug-dump=info <bin> | grep -A1 'GNU C23'reports
-O0forlibmimalloc-sys-0.1.49/c_src/mimalloc/v3/src/static.con the pre-overridebinaries and
-O2on the post-override binary. Verified on bothai-gateandai-perf-gate, andre-verified on the three A/B binaries below (
PRE-O0,noC-O0,POST-O2) as thediscriminating control for that leg.
Behavioural A/B,
--games 10(the exact CI PR-gate invocation), pinned card-data, one leg at atime, 16-core host. All three binaries are built from base
c9daf66e3, so the pair isolates exactlythese four commits and nothing upstream.
.bins/ai-gate-PREc9daf66e3)PASS p0=4/10 turns=13.9; enchantress-mirrorFAIL p0=1/10 turns=11.8; no report JSON written at all.bins/ai-gate-noC--config).bins/ai-gate-POSTThroughput: PRE
20/4811s = 0.0042games/s → noC30/1883s = 0.0159games/s (3.8x) → POST30/1629s = 0.0184games/s (4.4x), same box, same workload, one leg at a time. The noC→POSTgap is a single sample of one seed set and is not claimed as commit C's speedup; commit C's
claim is the
DW_AT_producerpropagation above, nothing more.Verdict-neutrality, two independent controls.
Commits A+B+D vs the committed (release-cut) baseline: seeds match one-for-one with
differing winners: 0on red-mirror and affinity-mirror, and noC matches PRE's own partial rowsdigit-for-digit where PRE got that far. Game-level parallelism moved no verdict.
Commit C in isolation:
noCandPOSTdiffer by exactly commit C (same base, same source,C neutralised on
noCvia--config,-O0vs-O2confirmed in each binary's DWARF). All30 of 30 games agree on both
winnerandturns:Commit D is visible in the report. PRE panic-aborted affinity-mirror seed
10592730(
search.rs:921, "AI fallback reached during pending cast (variant ManaPayment, spell KappaCannoneer)") and scored it
(None, 0); noC's affinity-mirror turns are[15,12,9,12,14,13,8,8,11,12]— noturns: 0. Its disclosed cost is real too: that one seedconsumed roughly 26 of noC's 31 minutes.
Pre-existing, not caused by this PR:
enchantress-mirrorFAILs identically on the fix-free PREbinary. And the compare stage reports that FAIL as
PASS— see the disclosure below.Test non-vacuity. Each new assertion was revert-probed or has a natural control:
assemble_matchup_result_…_divides_by_games_per_matchup,avg_turns/avg_duration_msgames.len()flips1.625 → 6.5; test FAILSstatus/fail_reasonclassify's third argument togames.len()flips PASS↔FAIL; test FAILS (independently of the averages)regroup_games_restores_game_index_order_from_shuffled_completionsort_by_keyyields[2,0,1]; test FAILSgame_tasks_skip_matchups_whose_payload_failedis_ok()filter adds(1,0)/(1,1); test FAILSassert_no_fallback_cancelpositive half (mana-first)[PassPriority], which is how the witness test's vacuity was discoveredactivation_legal_forprecondition (witness)Gate A
./scripts/check-parser-combinators.shat the committed head:Gate A PASS head=04185c70a173999aff3ab5e1cd0d3664727057c0(
Gate G PASS,Gate P PASSin the same run.)Anchored on
upstream/mainat1738d5c91("fix(parser): keep the card-type gate on "from among them" casts(#6880) (#6959)"). The four commits were rebased onto it cleanly (4/4, no conflicts); the only
overlap with the three intervening upstream commits was the
release: v0.44.0version bump in thesame
Cargo.tomlthis PR touches, in a different table.Final review-impl
Two independent review rounds (separate agents, no shared context), both on the committed diff.
Round 1 — 2 MED + 4 LOW. Fixed: doc paragraphs asserting a purity the crate elsewhere denies
(now hedged "modulo #4878");
assemble_matchup_result's test rewritten so thegames_per_matchupdenominator is observable;
assert_no_fallback_cancelextended tobreak_reason; theCargo.tomlcomment de-overclaimed. Round 1 also surfaced two clippy errors of my own
(
needless_option_as_deref,unused_mut) that would have redded CI.Round 2 — 1 HIGH + 5 MED/LOW, all addressed, every claim re-verified by me at the source before
being accepted:
payloads" was false.
projection.rs's ungated 15 msTIME_CAPis the mechanism (chainre-derived independently:
registry.rs→velocity_score→can_afford_projection→deadline.rs:33-43Deadline::none()→context.rs:96-98is_none_or). Both the comment andcommit C's message now name it instead of denying it.
scenario_claws_of_gix_witness_board_does_not_dead_endwas vacuous, and strengthening itproved it — adding an
ActivateAbilityassertion made it FAIL, because the AI's entire actionlist on that board is
[PassPriority]. Resolution: the discriminating content is the newactivation_legal_forprecondition; the positive assertion moved to the mana-first sibling, whichdoes complete (
[ActivateAbility, SelectCards, PassPriority]). Disclosed in commit D's message.drive_gamepurity overclaims the round-1 hedge pointed at;"reads no wall clock" (it reads it four times — nothing it reads enters the
GameResult); acitation naming
candidate_actions_broad_with_probewhere the guard is insemantic_candidate_actions_with_probe; two test docs describing the deleted panic, one quoting astring with 0 occurrences in
search.rs; a sort rationale citing a stability guarantee that isnot load-bearing; and commit B's undisclosed loss of per-matchup rows under a timeout kill.
Every fix was folded into the commit that owns it — there are no "fix the previous commit" commits
in this branch.
Claimed parse impact
None. No parser file is touched; the pre-commit parser combinator gate (Gate G) and the
PreLowered ratchet (Gate P) both passed on every commit.
Scope Expansion
None.
Validation Failures
None from this branch. Three things found while measuring are pre-existing on
mainand aredisclosed rather than fixed here — each moves gate verdicts, so each wants its own change and its
own baseline sign-off:
compare.rs:269-273classifies
Some(_) → Noneasunchanged, so a run in which 8 of 10 enchantress-mirror gameswent from a decisive result to a draw reported
flips W→L 0 | flips L→W 0 | PASSwhile thesuite section of the same report said
FAIL, and the process exited 0.enchantress-mirrorFAILs on unmodifiedmain(FAIL p0=1/10), reproduced on the fix-freePRE binary — so it is not introduced by anything here. Scope caveat: that is measured on my
host. CI's run of this PR finished only 2 of 10 enchantress games before its timeout and both
matched the baseline, so how much of this reproduces on a hosted runner is not established.
See the cross-environment note under CI Failures.
projection.rs:110's 15 msTIME_CAPis not gated onmeasurement mode (unlike
search.rs:2012/planner/mod.rs:691) and is reachable at the gate'sdefault
MediumviaEvasionRemovalPriorityPolicy::velocity_score; theprojection_min_budget_msfloor is bypassed becauseDeadline::none()returnsremaining() == Noneandcontext.rs:96-98isis_none_or. A bail scores 0.0 where a completedprojection scores up to +3.0, and that term picks the removal target. This is why commit C's
comment states a mechanism instead of claiming allocator changes are invisible.
CI Failures
Everything except the two AI-gate jobs is green, including the required
Rust (fmt, clippy, test, coverage-gate)aggregator:timeout-minutes: 60timeout-minutes: 60This PR does not make the gate fit inside CI's 60-minute budget, and the run above is
the measurement that proves it. Stating that plainly is the point: run
30829742995isthe first
ai-gaterun in this repo that can be diagnosed rather than guessed at.From the job log (
Paired-seed AI gate, job91740407535):cargo ai-gate --games 10→Finished dev profile [unoptimized] in 2m 04s, gate started15:58:08Z,##[error]The operation was canceledat16:55:31Z— 57m23s of gate run.2/10), each with a timestamped
start/done winner=… turns=… …msline. Onmainthesame kill produces per-matchup lines only — run
30782543973shows 37m59s of unbrokensilence before its kill and no artifact of any kind.
3.43x effective parallelism on a 4-core hosted runner, with 8 more games still in
flight at the kill. Commit B is doing its job; the runner is simply too small.
--games 10against a4 core × 60 min = 240 core-minute budget. And the single slowest game alone is
30.1 min (affinity-mirror seed
10592730) — a floor no amount of parallelism beats.So the remaining fix is CI-side (runner size,
timeout-minutes, or--games), notsource-side. That is written up separately for the maintainer rather than committed here,
since
.github/workflows/**is out of scope for this PR.Cross-environment divergence, disclosed. Of the 22 games CI finished, 21 match my
local
POSTrun seed-for-seed on bothwinnerandturns(red-mirror 10/10,affinity-mirror 10/10 — including the 30-minute seed
10592730, which completes ratherthan panicking, commit D visible in CI). The one exception is enchantress-mirror seed
10593729: CI reportsp1, 10 turns(matching the baseline) where my box reportsdraw, 8 turns.I could not close that out, and I am not going to guess at it. What I did exclude is local
machine load: replaying that seed alone on an idle box takes 125s instead of 277s — a 2.2x
speed swing — and returns the identical
draw, 8 turns. Unexcluded candidates remainthe card-data version (CI restores a
cardgencache; my legs pincard_data_hash 0acb81c9…) and the ungated 15 msTIME_CAPunder Validation Failures item 3. Note thisdivergence is orthogonal to this PR's commits — it reproduces on
noC, which does notcontain commit C.