fix(ai): make the duel-suite comparator see the regressions it was blind to

[lgray]

🤖 AI text below 🤖

Summary

The duel-suite comparison gate returned PASS on a recorded run that had two independent regressions in it. In that run (.ab/noC-1.json, the A+B+D leg of #6969), enchantress-mirror had 8 of 10 games stop having a winner and carried status: "Fail" with fail_reason: "mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50". The compare section printed 0 FAIL, 0 WARN, 3 PASS and the gate exited 0.

Two causes, both in classify_row's paired branch:

1. paired_seed_shift classified only Some(0)→Some(1) and Some(1)→Some(0); every Some(_) → None fell into a _ => unchanged wildcard. A branch that made the AI stall or loop every game — the exact failure this gate exists to catch — produced flips=0, sign_test_p=None, and a green comparison.
2. The paired branch read game outcomes and nothing else. It never looked at SuiteStatus, so an existing matchup that newly failed its own suite check did not fail the comparison. Only the new-matchup arm ever read that field.

This makes the draw axis and the suite-status axis first-class, severity-ordered peers of the win/loss axis, and removes the wildcard that hid the first one.

Files changed

• crates/phase-ai/src/duel_suite/compare.rs — the substance of the change, of which roughly 85% is tests and comments. decisive_to_draw / draw_to_decisive / draw_sign_test_p on PairedSeedShift and CompareRow; exhaustive classification match; four new verdict arms (draw Fail/Warn, status Fail/Warn); avg_turn_delta and suite_status_shift carried on the row; render_markdown extracted from print_markdown over a COLUMNS constant, with seven new columns; twenty-seven new tests (10 → 37).
• crates/phase-ai/src/bin/ai_gate.rs — main routed through gate_verdict, so the stdout body and the exit code are decided in one place the tests can reach (round-2 review).

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: not-applicable — the change is confined to crates/phase-ai/src/duel_suite/compare.rs, the duel-suite report comparator (CI gate tooling). No crates/engine/ game logic, parser, effect, resolver, or targeting code is touched, and no AI decision-making behavior changes: this alters how two recorded reports are compared, not how any game is played.

Flow used: measured-defect probe → written plan → implementation → mutant discharge → real-data acceptance → five rounds of independent review-impl. Rounds 1-4 each returned FAIL, as did round 5's first pass; round 5's re-review returned PASS with one LOW, which this head closes rather than discloses. The findings and their fixes are described inline below rather than summarised away, because two of them were false claims in this PR's own documentation and three were defects in fixes for earlier findings.

CR references

None. This is CI comparison tooling, not game rules.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.
• Gate A output below is for the current committed head.
• Final review-impl below is clean for the current committed head.
• Both anchors cite existing analogous code at the same seam.

Measured at the current head edc1c1ea3, after rebase onto 562e7b5d2:

• cargo test -p phase-ai --lib duel_suite::compare — 37 passed, 0 failed (10 before this change)
• cargo test -p phase-ai --lib — 2049 passed, 0 failed, 8 ignored
• cargo test -p phase-ai --test gate_cli — 2 passed, 0 failed (real process, no card data, 0.02s)
• cargo clippy -p phase-ai --all-targets -- -D warnings — rc=0
• cargo fmt --all --check — rc=0
• ai-gate --games 10 (CI's exact gate invocation, server-release, against the committed baseline) — compare: 0 FAIL, 0 WARN, 3 PASS, 0 NEW, 0 REMOVED, rc=0, zero refusals

The live gate stays green, structurally

matchup	W→L	L→W	sign p	dec→draw	draw→dec	draw sign p	unpaired base	unpaired cur	Δ avg turns	suite status	status
affinity-mirror	0	0	—	0	0	—	0	0	+0.8	Pass	PASS
enchantress-mirror	2	2	0.5000	0	0	—	0	0	-1.5	Pass	PASS
red-mirror	0	0	—	0	0	—	0	0	+0.0	Pass	PASS

Every row has equal draw counters and no Fail on either side — which is exactly the precondition of the invariant stated at the site, so every new guard is false and the chain takes the pre-change path. The live run is an instance of the invariant, not merely consistent with it.

All three new columns carry real values through the actual binary rather than only in tests, and the table is well-formed after the render_markdown extraction, so the refactor did not disturb the real output path.

Per-seed cross-check of the only draws in the whole run: affinity seeds 10592735 and 10592736 drew in the current run and are null in the committed baseline, so both classify (None, None) => unchanged — which is what dec→draw=0 / draw→dec=0 reports. The instrument agrees with the hand count.

Disclosure: this is not an identity comparison. The committed baseline records card_data_hash=56b033662b27ea926a2db73a8b83007b6352ff35, and no card-data.json reachable from this checkout hashes to it — the input the baseline was recorded against no longer exists, so an identity re-run is not achievable. This ran against current card data, which makes the result stronger for the question that matters: the new axes stay silent across genuine data drift.

The failure this fixes, measured on real recorded data

The acceptance run is the real comparator over two recorded reports — committed crates/phase-ai/baselines/suite-baseline.json versus .ab/noC-1.json, a real gate run (the A+B+D leg of #6969). Not synthetic:

arm	enchantress-mirror	red-mirror	affinity-mirror	any_fail()
BASE (this PR's two verdict tiers removed)	PASS — despite decisive→draw=8	PASS 0/0	PASS 0/0	false
POST (this PR)	FAIL — paired draw regression: decisive→draw=8 draw→decisive=0 sign-test p=0.0020	PASS 0/0	PASS 0/0	true

red-mirror and affinity-mirror are the built-in negative controls on real data: both measure 0 on the draw axis and neither moves. The BASE and POST artifacts were diffed and differ, so this is not a stale-binary false pass. draw_sign_test_p = 0.001953125 is exactly the hand-derived 1/512, produced by the already-existing sign_test_mid_p_upper_tail — reused, not reimplemented.

Mutant evidence — 30 mutants over the module's 26 tests

The verdict chain, each mutant flipping a distinct set: dropping either draw tier, dropping the status arm, reordering either Fail arm below the W/L Warn arm (1 test each — precisely the edit the "Do not reorder" comment warns against), restoring the literal pre-fix classification, firing on any movement regardless of direction or significance, making Fail→Fail silent again, and reading the draw statistic in one fixed direction.

Review then measured, twice, that the reporting layer was pinned far more weakly than the chain. Sixteen mutations survived a whole green suite across rounds 3 and 4; all now fail a test. The most consequential:

mutant	what it corrupted
AVG-FROZEN	Δ avg turns cell frozen to a constant
DEC-SWAP	dec→draw / draw→dec cells transposed — the recorded incident would print its counters backwards
SEP-EMPTY	separator fill emptied — right pipe count, renders the whole table as one paragraph
REC-EMPTY	recovery message emptied
DRAW-FAIL-SWAP / DRAW-WARN-SWAP	counters transposed inside either draw reason string
STATUS-NO-SHIFT	status reason drops the Pass → Fail it names
MERGED-KEY	merged arm re-keyed != Pass instead of == Fail, mislabelling Fail → Open
WL-FAIL-SWAP / WL-WARN-SWAP	counters transposed inside either win/loss reason — a 10-0 regression printed as W→L=0 L→W=10
SHIFT-CELL	the suite status column's shift branch — the one the status axis exists to surface — replaced by a constant
NONE-DASH	the — fallbacks that New and Removed rows print
MIRROR-GUARD	the avg-turn arm's Expected::Mirror guard deleted (pre-existing arm; every fixture was a mirror)

Cells are now asserted by column name against a fixture whose four counters are pairwise distinct (2, 3, 4, 1), so no transposition can hide; the separator is checked to be a valid markdown rule; every reason string is pinned to its counters in order.

Two of these fixes were themselves defective, and are disclosed rather than quietly corrected. The natural spelling of the separator check — .split('|').filter(|s| !s.is_empty()) — drops every segment exactly when the fill is empty, so SEP-EMPTY survived a second time. And the first cell-transposition fixture rendered 45% in both win-rate columns, so swapping them changed nothing; the fixture now differs them via a current-only seed, which moves the rate without touching a flip counter. A guard is not a guard until its mutant dies.

The working tree was restored and verified byte-identical after every mutant run.

What the tier order actually guarantees — swept, not argued

Two earlier drafts of this invariant were false, both caught by review compiling the base and running candidate inputs through both versions. The claim is now stated to its exact edge and verified over 1,327,104 paired inputs against the compiled base, comparing (status, reason) on each. The domain covers the counters, both statuses ∈ {Pass, Fail, Open}², every Expected variant, seeds present in only one report, duplicate seeds within a matchup, empty game vectors, zero-matchup reports, and NaN/infinite turn deltas:

• Clause 1 — nothing that reaches Fail today changes verdict. The W/L Fail arm is still first with byte-identical counters. Measured: 0 violations; every base-Fail input keeps verdict and reason byte-identical.
• Clause 2 — a row whose draw counters are EQUAL and where NEITHER report's status is Fail takes precisely the pre-change path. That is the full precondition: every new guard needs either unequal draw counters or a Fail on one side. Measured: 0 violations.
• Clause 3 — a W/L Warn DOES escalate to Fail, via the draw Fail arm on a significant draw regression and via the status Fail arm when the matchup newly fails its own check. Intended: those are the failures this gate exists to catch, and suppressing either because the win/loss axis also wobbled insignificantly would reintroduce the same blindness one case narrower.

Zero violations of either clause, zero Fail→X, zero X→Pass. Head severity is monotone ≥ base on every input, and all 287,712 escalations to Fail are attributable to a significant draw regression or a non-Fail→Fail status shift — nothing this PR does can make a comparison quieter than it was.

Each clause names the test that pins it, because all three false claims in this PR's history had the same shape: a claim about an arm that no test exercises.

The two directions on each axis are deliberately asymmetric. decisive→draw dominating is the regression signal and can reach Fail; draw→decisive dominating is an improvement and can never Fail through that arm — it requires decisive_to_draw > draw_to_decisive — but still Warns, because a comparator silent about games that started resolving would be hiding a behavior change. Same shape on the status axis.

Reason strings are first-match-wins, so every axis gets a column

Review also measured that the new draw WARN arm shadows the mirror avg-turn WARN arm: same input, pre-change → Warn "mirror avg-turn drift +6.0 turns", this PR → Warn "paired draw shift: …". Status unchanged, but the drift magnitude then surfaced nowhere, because the table had no avg-turns column and the reason string was its only surface — the same argument used to justify the draw columns, turned against this change.

Fixed by carrying avg_turn_delta on CompareRow and adding a Δ avg turns column, which generalizes to the invariant now written at print_markdown: every axis the verdict chain can decide on owns a column, because the chain is first-match-wins and columns are the only surface that survives suppression. Adding a verdict arm means adding a column. Pinned by mirror_drift_magnitude_survives_a_shadowing_reason.

No baseline refresh. baselines/suite-baseline.json is untouched.

Review response (maintainer CHANGES_REQUESTED, 2026-08-05)

Two further commits. Both findings reproduced independently at the reviewed head before being fixed.

HIGH — partial comparison could report PASS. Confirmed: compare validated only schema_version, and paired_seed_shift continued past baseline seeds missing from current while never traversing extra current games at all. Two reports sharing no seeds scored zero on every counter and passed.

The remedy splits, because two defects hid behind one symptom. A different base_seed or difficulty makes a seed number denote a different game, so compare now returns CompareError::WorkloadMismatch { field, baseline, current } — one parameterized variant mirroring the PerfCompareError::WorkloadMismatch you pointed at, rather than a sibling per field. A different games_per_matchup is not that: it changes how many seeds exist, not what one means, so the samples that do pair remain comparable. It is counted rather than gated — see the disclosure below. card_data_hash is likewise not gated: the committed baseline's hash matches no card-data present on a current checkout, and perf.rs carries card-data hashes as informational fields for the same reason.

PairedSeedShift/CompareRow gain unpaired_baseline and unpaired_current, each with a column, and a Warn arm fires when either is non-zero. The arm sits last among the Warns so a row with real drift keeps its drift headline and reports the coverage gap through its columns instead; promoting it flips exactly one test.

LOW — unescaped cells. Confirmed, and it indicted an existing test of mine: markdown_rows_are_rectangular asserted precisely this property and passed, because every one of its fixtures was pipe-free. md_cell now encodes every report-supplied cell, with a pipe-bearing reachability fixture.

Disclosure — the nightly. This change makes the discarded samples visible as unpaired cur and Warn-level. That was false, and round 2 below replaces it. A Warn exits 0, so the nightly step succeeded, so the step that publishes the report never ran — the counters went to a file nobody read. The correction is in the round-2 section.

Verification at the new head. 2044 lib tests pass; clippy -D warnings and cargo check --workspace --all-targets both clean; the live gate is green against the committed baseline (0 FAIL, 0 WARN, 3 PASS, rc=0) with both new columns reading 0. Twelve mutants across the two commits, tree restored byte-identical after each — including a control arm so a reject-everything mutant cannot pass as a working guard, and a mutant pinning the deliberate games_per_matchup non-gate against a later reader "fixing" it.

Review response (maintainer round 2, 2026-08-05 11:07Z)

HIGH — the workload mismatch was a green, unpublished partial comparison. Confirmed, and it falsified a claim in the section above.

Reproduced at the reviewed head before changing anything, because the disclosure I wrote in round 1 said this change "makes it visible as unpaired cur and Warn-level" and that the nightly "starts saying out loud that it discards 90 of every 100 samples". Both are false where it matters:

• ai-gate.yml:87 — cargo ai-gate --full-suite --games 100 > target/ai-gate-report.md. stdout, and only stdout, becomes the issue body.
• ai-gate.yml:90 — Open or update drift issue is guarded by if: steps.gate.outcome == 'failure'.
• CompareReport::any_fail counts only Fail, so a Warn exits 0, the step succeeds, and that guard is false.

The columns were therefore written to a file nobody reads. Producing a diagnostic is not surfacing it. I had verified the counters reached the report and stopped one layer short of the consumer.

Remedy: hard incompatibility with preserved diagnostics — the first of the two you offered. The second (align the nightly baseline or workload) needs either a baseline refresh or a workflow edit, and I do not take either unilaterally.

compare now returns WorkloadMismatch for games_per_matchup, and render_error_markdown writes the field, both values and the remedy to stdout, which is what the workflow captures. Printing the refusal only to stderr would have traded a green false-pass for a red job with an empty issue body (:95 aborts with "AI gate failed without a drift report") — a different failure, not a fix. gate_verdict returns the stdout body and the exit code together because the issue posts only when the exit is non-zero and the file is non-empty; a test pinning either alone cannot see this mode. main now only prints and exits.

The stdout byte stream for a successful run is unchanged by that refactor, which matters because the nightly posts it verbatim: println!() + print!(table) + println!("\ncompare: …") and format!("\n{table}\ncompare: …\n") emit the same bytes, and the live run's file confirms it — leading \n, table, then | PASS |\n\ncompare: 0 FAIL, 0 WARN, 3 PASS, 0 NEW, 0 REMOVED\n.

The unpaired columns stay. A same-workload pair can still fail to pair — a crashed matchup, a filter change — and that remainder is still counted rather than skipped.

Consequence, stated up front: the nightly will fail every night until baseline or workflow is aligned. It runs --games 100 against a games_per_matchup: 10 baseline. The required PR gate runs --games 10 (ai-gate.yml:50), matches the baseline header exactly, and is unaffected — the live run below is that invocation. I will do whichever alignment you prefer as a follow-up; it is one line either way, but one line in a file I do not edit unasked.

MED — md_cell escape order. Confirmed, and the test that was supposed to catch it had the same bug in mirror image. Escaping | before \ turns a report-supplied \| into \\| — an escaped backslash followed by a live separator — so the input that looks already-safe is the one that breaks the row. Backslashes are escaped first now. The in-test separator counter used "the previous character is a backslash", which is right for \| and wrong for \\|; it is now md_separator_count, keyed on run-length parity, shared by both pipe tests. The new fixture carries both parities and inlines the previous encoder as a control, asserting it leaks a separator where the fixed one does not — without that arm the assertion could pass for an unrelated reason.

LOW — copied doc paragraph. Confirmed; the two paragraphs describing a distinct-value fixture belonged to markdown_cells_carry_their_own_column_values. Removed, with a one-line note saying where they went so the next reader does not re-copy them.

Mutants for this round — 8, all killed, tree sha256-restored after each.

mutant	killed by
drop the games_per_matchup guard	2
render_error_markdown → empty string	3
gate_verdict always exits 0	2
gate_verdict always exits 2	1 — only the three-outcome control
refusal body drops both values	2 (the exit-code assertion survives)
md_cell → pipes-first	1
md_cell → identity	2
separator counter → previous-character rule	1

The last four rows are the point. A reject-everything gate_verdict satisfies every refusal assertion and is caught only by the control arm. Emptying the body kills the body assertions while the exit-code assertion survives, and forcing exit 0 does the reverse — so the two halves you asked to see asserted end-to-end are pinned separately rather than by one test that could be passing for either reason. And reverting the counter kills the same test as reverting the encoder, which is the measurement that would otherwise have hidden the encoder bug.

Answers to the three CodeRabbit nitpicks are in a separate comment, with the measurements behind each.

Review response (round 3, 2026-08-05 12:06Z) — one blocker is not mine to close

HIGH — the nightly workload is permanently incompatible with the committed baseline. Confirmed, and it invalidates the remedy I offered in round 2.

Measured independently before accepting it, and my own reviewer had reached it first:

• ai-gate.yml:50 — PR gate, cargo ai-gate --games 10, quick filter (3 matchups), no continue-on-error.
• ai-gate.yml:87 — nightly, cargo ai-gate --full-suite --games 100 (32 matchups).
• Both compare against the same default baseline. Neither passes --baseline. It records games_per_matchup: 10 and 3 results.

With a symmetric guard no single value satisfies both: at 10 the nightly refuses, at 100 the required PR gate refuses on every pull request. So the remedy I offered last round — regenerate the baseline at 100 — would have traded a red nightly for a red required check. That advice was also in the shipped error text; it is rewritten, and the text now carries the constraint that makes the choice non-local: a baseline is shared, so whichever workload is chosen, every invocation comparing against it must use that one.

Worth adding to the picture: the mismatch is not only games. The nightly runs 32 matchups against a 3-matchup baseline, so 29 report NEW every night. That baseline was never the nightly's baseline; this PR made a long-standing mismatch fatal instead of quiet.

Both of your suggested fixes need something I don't do unasked, so I am offering rather than choosing. Committing a workload-specific 100-game baseline means editing .github/workflows/ai-gate.yml to select it and recording 32 × 100 games — measured at ~15–39 s/game on this box, so hundreds of CPU-hours, not something to produce unreviewed. Using one workload consistently is a one-line workflow edit. I don't edit workflow files without being asked to; say which and I will, including the recording run with the paired-seed report attached so the refresh is auditable. My weak preference and its downside are in a separate comment.

Until then this PR leaves the nightly failing loudly with an accurate explanation, which is the outcome your previous review asked for, rather than green while comparing a tenth of its sample.

MED — coverage stopped at gate_verdict, not the process boundary. Fixed, not disclosed.

You were right that a regression in main could re-split exit from output with every unit test green. emit_gate_verdict now owns both statements and all three comparing binaries end in it — ai-gate, ai-duel compare, and ai-perf-gate through its own renderer. ai-duel compare turned out to have the identical defect and is fixed by routing rather than patched in place.

crates/phase-ai/tests/gate_cli.rs drives the contract through a real process. It runs ai-duel compare because that is the only one of the three needing no card database and playing no games, so it costs 0.02 s and adds no card-data load for scripts/check-test-card-data-load.sh to object to. Both halves are asserted on the same invocation — the issue posts only when the step failed and the file is non-empty — with a control arm so a refuse-everything binary cannot pass.

Its fixture is built from the real structs and serialised. The first draft hand-wrote the JSON, got Expected's internally-tagged encoding wrong, and both arms failed on a parse error rather than on the contract; running it is what caught that.

Also fixed this round, from an independent review pass that returned FAIL on the round-2 head:

• The round-2 claim that a stderr-only refusal would leave the report file empty was false: run_suite prints the suite table to stdout before the baseline is loaded, so [ ! -s ] is unreachable on that path — my own end-to-end log showed the refusal below a rendered table and I read past it. The real consequence is a red job whose issue body is a table of PASSing rows and no statement of what failed. Both doc comments corrected.
• The empty-report failure was real in the other binary. ai-perf-gate writes nothing to stdout before compare, so a PerfCompareError produced a zero-byte report and "Decision-cost perf gate failed without a drift report" — exactly what round 2 claimed it had refused to ship, in the gate I cited as prior art without checking its caller. Fixed on both its bail-outs.
• render_error_markdown's I/O arm was unreachable (compare does no I/O) while the real baseline-read failure was still stderr-only. That call site is wired, which fixes the gap and makes the arm live.
• contains("compare: 0 FAIL") was degenerate — four of five counters are zero in that fixture, so it survived transposing counters, zeroing them, and deleting the tally loop. Now pins the whole line, plus a fixture with two distinct non-zero counts.

Mutants this round: 10, all killed, files sha256-restored after each. Seven on the library fixes (dropping the shared remedy fails 3 tests; reducing it to the bare error line fails 4; emptying the perf renderer fails its own; deleting remedy 2 fails the refusal test; zeroing the tally and transposing pass/new each fail the verdict test the old substring could not see; reverting md_cell now also fails on trailing-run parity) and three at the process boundary (stderr instead of stdout fails both CLI tests; always-return-0 fails only the exit assertion; reverting ai-duel to stderr-only fails only the refusal test — so each half is independently load-bearing where CI actually reads them).

Gate A

Gate A PASS head=8eaee5fe919a97c4a9ffd024bebf505158ac2f41 base=c44a4512e6f91684068c259a028eb44b4d801340

Re-recorded at the current head after rebasing onto upstream/main. Base passed explicitly rather
than left to the script's default, which resolves against origin/main — a fork ref that lags.
Range non-empty (8 commits, 7 files) but contains zero parser paths, so the parser-specific
verdict is vacuous by construction and is labelled rather than presented as a pass. Gate G also PASS.

Anchored on

• crates/phase-ai/src/duel_suite/compare.rs:352 — the existing paired win/loss FAIL arm (dominance test plus sign_test_p < 0.05). The new draw FAIL tier is the same shape at the same seam: same predicate form, same reason-string format, same if/else if chain in classify_row.
• crates/phase-ai/src/duel_suite/compare.rs:606 — sign_test_mid_p_upper_tail, the existing significance statistic. The draw axis reuses it verbatim with n = decisive_to_draw + draw_to_decisive and k = max(...), exactly as the win/loss axis calls it. No new statistic was written.

Both line numbers were re-derived at this head with grep -n after the rebase, not carried over: the
previously recorded :259 and :449 had drifted and now point at unrelated lines.

Final review-impl

Final review-impl PENDING head=8eaee5fe919a97c4a9ffd024bebf505158ac2f41

Deliberately not a PASS line, because a PASS here would be false. The most recent certification is
cert-r7 FAIL head=3c9646266, and no independent pass has run against this head. The maintainer's
latest review (2026-08-05T16:13:23Z) also carries an open HIGH blocker — the repository invokes the
same default 10-game baseline from two incompatible workloads (ai-gate.yml:49-50 at --games 10,
:78-87 at --full-suite --games 100, against a suite-baseline.json recording
games_per_matchup: 10) — which is unaddressed at this head: no commit on this branch postdates that
review. The rebase above changes only the base (diff byte-identical across it, md5 a12bb582741c);
it does not answer the blocker.

This section is expected to keep the artifact gate red on the review-impl axis, and that is the
correct signal rather than something to paper over.

Claimed parse impact

None.

Scope Expansion

Two items, both deliberate and both reviewed.

1. The suite-status axis. Review found that an existing matchup whose own SuiteStatus goes to Fail did not fail the comparison — proven from this PR's own evidence artifact, which carries status: "Fail" on enchantress-mirror while its compare section read 0 FAIL, 0 WARN, 3 PASS. The incident this PR is built on had two independent holes; shipping a fix for one and leaving a known-latent sibling in a required gate would make more work later, so it is closed here. New: a Fail arm (b.status != Fail && c.status == Fail, carrying the matchup's own fail_reason), and one Warn arm parameterized by c.status rather than two siblings: recovery (Fail → non-Fail) and still-failing (Fail → Fail) are both reported, never failed — the same asymmetry as the draw axis. Plus suite_status_shift on the row, a suite status column, and tests including noc1_enchantress_row_carries_both_holes, which pins the recorded incident whole by asserting both holes so a future edit that closes one and reopens the other cannot pass it.

The still-failing case exists because review showed it is reachable, not theoretical: nothing revalidates a committed baseline when it is loaded, so a baseline that already sanctions a failure keeps sanctioning it and that matchup exits 0 forever. It Warns rather than Fails deliberately — the exit code answers "did this change make things worse", and the baseline, however it got that way, already sanctions that state. Whether a baseline may bless a failure at all is a policy question, and its other half is a guard on --refresh-baseline in bin/ai_gate.rs; that is now #7029 rather than smuggled in here.

(Phrased about the baseline rather than about --refresh-baseline writing without a verdict check. That was the mechanism, and #7029 adds the missing check — so naming it as the reason would make this paragraph, and the two source comments quoting it, false the day #7029 lands. Reachability does not depend on it: a baseline blessed before that guard, or hand-edited, is unaffected by any guard on the write path.)

Keyed on Fail specifically rather than any status change, and not by preference: Fail is the only status this file already acts on — the new-matchup arm matches SuiteStatus::Fail and treats Pass/Open alike. Same authority, same vocabulary, extended from new matchups to existing ones. Inert on current data (all three live matchups hold their own verdict, so the suite status column reads Pass on every row of the live run).

2. Table columns. print_markdown gained dec→draw, draw→dec, draw sign p, Δ avg turns, and suite status; CompareRow carries avg_turn_delta and suite_status_shift. The verdict chain is first-match-wins, so a row that fires on one axis prints only that axis's reason and hides every other axis's magnitude — review measured exactly that regression in an earlier revision of this PR, where the new draw Warn arm shadowed the mirror avg-turn reason and the drift magnitude then surfaced nowhere. bin/ai_gate.rs and bin/ai_duel.rs are the only callers of print_markdown, neither reads the columns, and no CI step parses the table — .github/workflows/ai-gate.yml cats the whole file into an issue body. Verified through the real binary, not only tests: the 13-column table renders correctly and every new column carries real values. render_markdown was split out of print_markdown so this invariant is enforced by tests rather than asserted in a comment — while the table was inlined in println!, the new columns could be deleted with the whole suite green.

Disclosed non-change: (Some(a), Some(b)) pairs with seats other than 0/1 still count as unchanged, exactly as before. The duel suite is two-player; reclassifying them is out of scope.

Validation Failures

None.

CI Failures

None.

Summary by CodeRabbit

• New Features
  • Added richer comparison reports with more detailed verdicts, mismatch handling, and additional summary metrics.
  • Comparison errors now produce readable markdown output for easier CI troubleshooting.
• Bug Fixes
  • Comparison results now properly flag incomplete pairing instead of silently skipping unmatched samples.
  • Output formatting is now consistent and rectangular, with improved escaping for special characters.
• Tests
  • Expanded coverage for comparison outcomes, refusal cases, and markdown formatting guarantees.

[coderabbitai]

📝 Walkthrough

Walkthrough

The duel-suite comparator rejects incompatible workloads, tracks expanded matchup metrics, applies additional verdict rules, and renders rectangular Markdown output. Gate commands emit refusal reports and use centralized exit-code handling. Tests cover comparison, rendering, and CLI behavior.

Changes

Duel comparison analysis

Layer / File(s)	Summary
Workload validation and comparison metrics crates/phase-ai/src/duel_suite/compare.rs	The comparator rejects mismatched workload fields. CompareRow and paired-seed analysis track draw transitions, unpaired samples, average-turn deltas, suite-status changes, and draw significance.
Matchup verdict classification crates/phase-ai/src/duel_suite/compare.rs	Verdict precedence adds failure handling for significant decisive-to-draw regressions. It adds warnings for draw shifts, suite-status changes, mirror turn drift, and incomplete pairing.
Markdown output and refusal rendering crates/phase-ai/src/duel_suite/compare.rs, crates/phase-ai/src/duel_suite/mod.rs, crates/phase-ai/src/duel_suite/perf.rs	Shared columns render escaped, rectangular comparison tables. Refusal reports include error details and remediation text.
Gate integration and validation crates/phase-ai/src/bin/*.rs, crates/phase-ai/tests/gate_cli.rs, crates/phase-ai/src/duel_suite/compare.rs	Gate commands centralize verdict output and exit codes. Tests cover metrics, verdict ordering, refusal behavior, Markdown invariants, and CLI stdout contracts.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GateCommand
  participant Comparator
  participant PairedSeedAnalysis
  participant MarkdownOutput
  GateCommand->>Comparator: load and compare reports
  Comparator->>Comparator: validate workload compatibility
  Comparator->>PairedSeedAnalysis: calculate matchup metrics
  PairedSeedAnalysis-->>Comparator: return metrics and verdicts
  Comparator->>MarkdownOutput: render comparison or refusal report
  MarkdownOutput-->>GateCommand: return stdout body and exit code

Loading

Possibly related PRs

• phase-rs/phase#6969: Both changes modify comparison-related reporting in perf.rs.

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: the duel-suite comparator now detects regressions that previously produced false passes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@crates/phase-ai/src/duel_suite/compare.rs`:
- Around line 409-412: Update the paired comparison flow around
paired_seed_shift and CompareRow so baseline games whose seeds are absent from
current are counted as unpaired instead of skipped silently. Include that
counter in the verdict chain with a Warn outcome when unpaired > 0, and add the
corresponding verdict column in render_markdown; alternatively, make compare
return CompareError when the reports’ base_seed values differ.
- Around line 574-583: Escape pipe characters in the reason continuation emitted
by the row-rendering logic before interpolating reason into the markdown cell,
so free-form fail_reason text cannot add columns. Update the code around the
reason handling in the comparison table formatter; preserve the existing
styling, continuation layout, and column padding.

🪄 Autofix

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: 9c2d218c-3580-492e-9ca3-521e55d5aba6

📥 Commits

Reviewing files that changed from the base of the PR and between 0d04771 and f4ee39e.

📒 Files selected for processing (1)

• crates/phase-ai/src/duel_suite/compare.rs

[matthewevans]

Blocked — the comparator can still report PASS for incompatible paired-seed workloads.

🔴 Blocker

[HIGH] The paired-seed gate still accepts incomparable reports and can return PASS without comparing the full sample. Evidence: crates/phase-ai/src/duel_suite/compare.rs:127-132 rejects only schema, while :400-412 maps current games by seed and silently skips unmatched baseline seeds; SuiteReport carries the omitted workload contract in crates/phase-ai/src/duel_suite/run.rs:78-88 (base_seed, games_per_matchup). A changed seed makes the same numeric seed represent a different randomized game; fewer current games skips baseline samples, and extra current games are never traversed, so the counters/sign tests at compare.rs:253-361 can stay zero and return Pass. The analogous perf comparator rejects workload mismatches at crates/phase-ai/src/duel_suite/perf.rs:579-600. This is a false-green in the CI gate this PR is hardening. Add a typed workload-mismatch guard before row classification for at least base_seed and games_per_matchup (and every other pairing-defining configuration); test both missing-baseline and extra-current samples through compare/any_fail.

🟡 Non-blocking

[LOW] Markdown report cells are not escaped. Evidence: compare.rs:557-580 interpolates report-provided matchup/exercise/reason text into pipe-delimited rows; fail_reason is deserialized and included at :190-195, :293-298, and :344-348, while ai_duel compare accepts JSON reports at bin/ai_duel.rs:721-753. A | makes the generated diagnostics non-rectangular. Use one markdown-cell encoder for every report-provided cell and add a pipe-bearing fixture.

✅ Clean

The decisive↔draw classifications are exhaustive and reuse the existing sign-test path; CodeRabbit’s two current-head findings were independently confirmed.

Recommendation: request changes. Address workload compatibility first, then add the markdown cell encoding hardening.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lgray]

🤖 AI text below 🤖

Both findings reproduced independently at f4ee39e98 before being fixed, and both are addressed at f9a8a2f8f.

HIGH — partial comparison could report PASS

Confirmed exactly as described. compare validated only schema_version; paired_seed_shift built a seed→game map from the current report, continued past any baseline seed missing from it, and — because pairing walks the baseline — never traversed extra current games at all. Two reports sharing no seeds scored zero on every counter and returned Pass.

Worth stating plainly: I knew about that skip. An earlier revision of this PR deliberately leaned on it, giving a rendering fixture a current-only seed so the two win-rate cells would differ "without touching a flip counter". I used the hole as a convenience and never asked what else it admitted. That fixture is re-derived here onto matched seed sets, and now asserts both unpaired counters are zero so the dependency cannot quietly return. A census of all 27 test fns confirmed it was the only fixture with asymmetric seed sets.

Two defects hid behind one symptom, and they need opposite remedies.

A different base_seed or difficulty makes the same seed number denote a different game, so pairing by it compares unrelated samples and calls the difference drift. Nothing downstream rescues that, so compare now returns CompareError::WorkloadMismatch { field, baseline, current } before any row is classified — one parameterized variant rather than a sibling per field, mirroring the PerfCompareError::WorkloadMismatch you pointed at. Thank you for that pointer; it turned the fix into reuse rather than invention.

A different games_per_matchup is not that. It changes how many seeds exist, not what a seed means, so the samples that do pair remain comparable. PairedSeedShift/CompareRow therefore carry unpaired_baseline and unpaired_current, each with its own column, and a Warn arm fires when either is non-zero. Per your instruction to test both directions, they are pinned separately — transposing the two counters fails both direction tests, which is why there are two fixtures rather than one asserting "something was unpaired".

card_data_hash is deliberately not gated: the committed baseline's hash matches no card-data present on a current checkout, so gating it would fail every run immediately. perf.rs reaches the same conclusion, carrying card-data hashes as informational fields rather than as a guard.

Disclosure — the nightly, and a decision that is yours

ai-gate.yml:87 runs --full-suite --games 100 against a games_per_matchup: 10 baseline. It therefore discards 90 of every 100 samples per matchup today, silently. This change does not alter that behaviour — it makes it visible as unpaired cur and Warn-level.

I did not gate games_per_matchup hard, and the reason is concrete rather than cautious: a compare error exits before printing, and because the report goes to stdout while errors go to stderr, the workflow's [ ! -s target/ai-gate-report.md ] branch would fail the nightly outright instead of opening the drift issue it exists to open. Making the workload mismatch a hard error is the more rigorous end state, but getting there requires either aligning the baseline to the nightly's workload or changing the workflow. Both are your call, not mine — say which you prefer and I will draft it.

LOW — unescaped cells

Confirmed, and it indicted one of my own tests. markdown_rows_are_rectangular asserted precisely this property and passed the whole time, because every one of its fixtures was pipe-free — a confident test of the wrong thing, which made the table look guarded. md_cell now encodes every report-supplied cell (matchup_id, exercises, and the reason continuation row), and report_supplied_pipes_cannot_add_columns is the reachability arm it never had.

That test needed fixing before it proved anything: its first measurement counted line.split('|'), which cannot distinguish \| from |, so it reported correctly-escaped output as broken. It now counts separators the way a markdown parser does. One gap is disclosed rather than counted: the exercises site renders format!("{f:?}") of a fieldless enum, so no fixture can put a pipe there and reverting that one site alone leaves the suite green. It is defense against a future data-carrying FeatureKind, not a defect being fixed.

Verification at f9a8a2f8f

Rebased onto 5c8d12bf5. 2044 lib tests pass; clippy -D warnings clean; cargo check --workspace --all-targets clean; Gate A PASS head=f9a8a2f8f base=5c8d12bf5. The live gate is green against the committed baseline — 0 FAIL, 0 WARN, 3 PASS, rc=0 — with both new columns reading 0, so the guard does not fire on a matched workload.

Twelve mutants across the two commits, tree restored byte-identical after each. Notably: refusing every pair fails 29 tests including matching_workloads_are_compared_normally, a control arm that exists so a reject-everything mutant cannot masquerade as a working guard; and adding a games_per_matchup gate fails a_different_games_per_matchup_is_reported_not_refused, so the deliberate non-gate is pinned and a later reader cannot quietly "fix" it without confronting the nightly question above.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

crates/phase-ai/src/duel_suite/compare.rs (3)

541-550: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

The decisive→decisive arms enumerate seats 0 and 1, so any other seat pair is classified as unchanged.

(Some(0), Some(1)) and (Some(1), Some(0)) are the only recognized flips. Every other decisive→decisive pair falls into the (Some(_), Some(_)) arm and increments unchanged. If a winner value other than 0 or 1 ever appears, a real winner change reports as no movement — the same blind spot this rewrite closes on the draw axis.

The classification does not need to know seat numbers. Compare the two winners directly and derive the direction from the baseline seat. This handles the class rather than the two-player special case, and it removes the Some(_)/Some(_) catch-all that the seat gap hides behind.

♻️ Seat-agnostic classification

         match (baseline_game.winner, current_game.winner) {
-            (Some(0), Some(1)) => flipped_w_to_l += 1,
-            (Some(1), Some(0)) => flipped_l_to_w += 1,
             (Some(_), None) => decisive_to_draw += 1,
             (None, Some(_)) => draw_to_decisive += 1,
-            // Same winner, or drawn on both sides. Non-0/1 seat pairs land here as they always
-            // have — the duel suite is two-player, and changing that classification is out of
-            // this change's scope.
-            (Some(_), Some(_)) | (None, None) => unchanged += 1,
+            // Winner changed seats. `w_to_l` is measured from p0's perspective: p0 held the win
+            // and lost it, or p0 did not hold it and gained it. Seat-agnostic, so a third seat
+            // cannot be swept into `unchanged`.
+            (Some(before), Some(after)) if before != after => {
+                if before == P0_SEAT {
+                    flipped_w_to_l += 1;
+                } else if after == P0_SEAT {
+                    flipped_l_to_w += 1;
+                }
+            }
+            // Same winner, or drawn on both sides.
+            (Some(_), Some(_)) | (None, None) => unchanged += 1,
         }

If winner is guaranteed to be Some(0), Some(1), or None by construction, encode that guarantee in the type instead. A Option<Seat> with Seat::{P0, P1} makes the match exhaustive over a known enum and lets the compiler reject the third-seat case at the source.

🤖 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/compare.rs` around lines 541 - 550, Update the
winner classification match in the duel comparison logic to compare decisive
winners directly rather than enumerating only seats 0 and 1. For any
`(Some(baseline), Some(current))` pair, increment unchanged when the seats
match; otherwise derive the flip direction from the baseline winner, so non-0/1
seat changes are classified correctly and the `Some(_)`/`Some(_)` catch-all is
removed.

Source: Coding guidelines

---

97-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a typed field discriminant instead of &'static str.

WorkloadMismatch.field is stringly typed. A small enum (for example WorkloadField::{BaseSeed, Difficulty}) carries the same information, makes callers match exhaustively, and prevents a typo from creating an unmatchable error. The doc comment cites PerfCompareError::WorkloadMismatch as precedent; if that type already uses a typed discriminant, this site should follow it.

♻️ Proposed typed discriminant

+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum WorkloadField {
+    BaseSeed,
+    Difficulty,
+}
+
+impl WorkloadField {
+    const fn as_str(self) -> &'static str {
+        match self {
+            Self::BaseSeed => "base_seed",
+            Self::Difficulty => "difficulty",
+        }
+    }
+}

     WorkloadMismatch {
-        field: &'static str,
+        field: WorkloadField,
         baseline: String,
         current: String,
     },

#!/bin/bash
# Check whether PerfCompareError::WorkloadMismatch uses a typed discriminant or a string field.
rg -nP -C 6 '\bWorkloadMismatch\b' --type=rust

🤖 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/compare.rs` around lines 97 - 101, Replace the
&'static str field in WorkloadMismatch with a dedicated typed discriminant enum,
such as WorkloadField with variants for each supported workload field. Update
all constructors, pattern matches, formatting, and comparisons involving
WorkloadMismatch to use the enum and ensure matches are exhaustive, following
the existing PerfCompareError::WorkloadMismatch representation if it already
defines an equivalent type.

---

331-357: 🧹 Nitpick | 🔵 Trivial

Two independent sign tests at α=0.05 raise the per-row false-Fail rate.

Each paired game contributes to exactly one axis: a win/loss flip, a draw transition, or unchanged. The win/loss sign test and the draw sign test therefore run on disjoint samples and are independent under the null hypothesis. A row that has flips on both axes now has a family-wise false-Fail probability of about 9.75% rather than 5%. Across the three matchups in the live gate, that compounds.

Most rows have a flat draw axis today, so draw_sign_test_p is None and the second arm cannot fire. The rate only rises on rows that move on both axes. Consider whether the draw arm should use a tighter α, or whether the two axes should share one corrected threshold, before the gate becomes noisy.

🤖 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/compare.rs` around lines 331 - 357, Adjust the
paired regression decision logic in the comparison flow around the
`paired.sign_test_p` and `paired.draw_sign_test_p` checks to control family-wise
false-Fail errors across the two independent sign tests. Use a shared
multiple-comparison correction or a stricter per-axis threshold, while
preserving the existing directional requirements and failure messages in both
`CompareStatus::Fail` branches.

Source: Path instructions

🤖 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 `@crates/phase-ai/src/duel_suite/compare.rs`:
- Around line 1550-1561: Remove the copied introductory documentation from the
test’s doc comment, including the claims about column values, frozen or
transposed cells, and distinct numeric axes. Keep the test-specific “Condition:
only unpaired_baseline moves” description and its explanation of the zero-valued
axes unchanged.
- Around line 735-737: Update md_cell to escape backslashes before escaping pipe
characters, preserving the required ordering so existing backslashes cannot
neutralize pipe escaping. Extend report_supplied_pipes_cannot_add_columns with
an input containing a backslash before a pipe and ensure its separator-counting
logic correctly handles escaped backslashes.

---

Nitpick comments:
In `@crates/phase-ai/src/duel_suite/compare.rs`:
- Around line 541-550: Update the winner classification match in the duel
comparison logic to compare decisive winners directly rather than enumerating
only seats 0 and 1. For any `(Some(baseline), Some(current))` pair, increment
unchanged when the seats match; otherwise derive the flip direction from the
baseline winner, so non-0/1 seat changes are classified correctly and the
`Some(_)`/`Some(_)` catch-all is removed.
- Around line 97-101: Replace the &'static str field in WorkloadMismatch with a
dedicated typed discriminant enum, such as WorkloadField with variants for each
supported workload field. Update all constructors, pattern matches, formatting,
and comparisons involving WorkloadMismatch to use the enum and ensure matches
are exhaustive, following the existing PerfCompareError::WorkloadMismatch
representation if it already defines an equivalent type.
- Around line 331-357: Adjust the paired regression decision logic in the
comparison flow around the `paired.sign_test_p` and `paired.draw_sign_test_p`
checks to control family-wise false-Fail errors across the two independent sign
tests. Use a shared multiple-comparison correction or a stricter per-axis
threshold, while preserving the existing directional requirements and failure
messages in both `CompareStatus::Fail` branches.

🪄 Autofix

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: 6f254bde-8fa5-4a51-bdcb-6a3fc0b52693

📥 Commits

Reviewing files that changed from the base of the PR and between 5c8d12b and f9a8a2f.

📒 Files selected for processing (1)

• crates/phase-ai/src/duel_suite/compare.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The doc comment's first paragraph describes a different test.

"Every column carries the value it claims to", the Δ avg turns freeze, the dec→draw/draw→dec transposition, and "the fixture gives every numeric axis a DISTINCT value (2, 3, 4, 1, and two different p-values)" all describe markdown_cells_carry_their_own_column_values at Line 1741. This test uses a four-game fixture in which every axis except unpaired_baseline reads zero, which is what the closing "Condition:" sentence states.

A reader who trusts the first paragraph could delete markdown_cells_carry_their_own_column_values and believe this test still covers cell-value binding. Remove the copied paragraph and keep the condition sentence.

🤖 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/compare.rs` around lines 1550 - 1561, Remove
the copied introductory documentation from the test’s doc comment, including the
claims about column values, frozen or transposed cells, and distinct numeric
axes. Keep the test-specific “Condition: only unpaired_baseline moves”
description and its explanation of the zero-valued axes unchanged.

[matthewevans]

Changes requested — the workload mismatch remains a successful, unsurfaced partial comparison in the actual gate.

🔴 Blocker

[HIGH] The comparator still exits successfully for a mismatched sample workload, so its CI gate can declare no regression after comparing only a subset of the run. Evidence: crates/phase-ai/src/duel_suite/compare.rs:166-171 deliberately excludes games_per_matchup from WorkloadMismatch; :438-461 turns unmatched samples into Warn; CompareReport::any_fail at :77-80 ignores Warn; and crates/phase-ai/src/bin/ai_gate.rs:102-112 exits nonzero only on any_fail. run.rs:659-662 uses games_per_matchup as the averaging/classification denominator. The pinned test at compare.rs:1722-1737 requires this mismatch to remain non-failing. Nightly runs 100 games against the committed 10-game baseline (.github/workflows/ai-gate.yml:78-87) and publishes its report only on failure (:89-105), so the partial-comparison warning is neither gate-failing nor visible in the drift issue. Make games_per_matchup mismatch actionable (hard incompatibility with preserved diagnostics, or align nightly baseline/workload) and add end-to-end exit/report assertions.

🟡 Non-blocking

[MED] md_cell mishandles a backslash immediately before a pipe. Evidence: compare.rs:735-737 escapes only |; a \\| input becomes \\\\|, leaving a Markdown separator after backslash consumption. The test :1851-1856 lacks that fixture and :1873-1876 treats every pipe preceded by any backslash as safe. Escape backslashes before pipes and test odd/even backslash runs with parser-equivalent separator counting.

[LOW] The docstring at compare.rs:1551-1558 describes a different test than :1559-1594 exercises. Remove the copied distinct-axis description and retain the unpaired-specific explanation.

✅ Clean

The original unmatched-seed and ordinary pipe findings are addressed; current CodeRabbit backslash/stale-documentation findings were independently confirmed.

Recommendation: request changes. The workload mismatch must not remain a green, hidden partial comparison.

[lgray]

🤖 AI text below 🤖

Answers to the three CodeRabbit nitpicks. The two actionable comments from the same review (md_cell backslash ordering, and the copied doc paragraph) are fixed in the pushed commit — the proposed md_cell diff and mine are the same ordering.

Seat-agnostic classification (541-550) — deliberate non-change, and changing it in this file alone would make things worse.

Measured: the suite builds exactly two AI players — run.rs:874, let ai_players: HashSet<PlayerId> = [PlayerId(0), PlayerId(1)].into_iter().collect() — so winner: Option<u8> has no third seat to produce. More decisively, the report side already collapses non-zero seats: run.rs:652 aggregates with Some(0) => p0_wins += 1, Some(_) => p1_wins += 1. A seat-agnostic comparator sitting over a seat-collapsing aggregator would disagree with the very numbers it compares — the p0% columns come from that aggregation. The right fix is the one the comment's second half proposes: give winner a Seat enum so the compiler rejects the third case at the source, in run.rs and compare.rs together. That is a type change across the report format, not a match-arm edit, and it is out of scope for a comparator correctness fix.

Typed discriminant for WorkloadMismatch.field (97-101) — following the precedent the comment names.

The comment's own condition is "if that type already uses a typed discriminant, this site should follow it." It does not: perf.rs:504-508 is WorkloadMismatch { field: &'static str, baseline: String, current: String }, constructed with string literals at :580 and :587. Mirroring it was the point — the two comparators are read side by side. Introducing an enum on one side only splits the precedent rather than improving it. Worth doing as a single change across both, which is a separate PR.

Family-wise error rate across the two sign tests (331-357) — the arithmetic is right; the change is a gate-policy change and needs evidence this PR cannot produce.

Confirmed, not conceded on trust: each paired game lands on exactly one axis (a W/L flip, a draw transition, or unchanged), so the two sign tests do run on disjoint samples, and 1 - 0.95² = 0.0975 is the family-wise false-Fail rate on a row where both axes move. This PR added the second test, so the increase is mine.

Exposure measured rather than guessed: in the live gate run at this head (ai-gate --games 10, committed baseline, all three matchups) the draw sign p column reads — on every row, so the second test does not fire anywhere today and the family size is 1 on every live row. The 9.75% figure is the ceiling on a row that moves on both axes, and no such row currently exists.

Two further facts bound the remedy. Both Fail arms are directionally gated as well as p-gated (flipped_w_to_l > flipped_l_to_w, decisive_to_draw > draw_to_decisive), so each is one-sided; and the correction is not a constant, because the family size per row is 1 or 2 depending on whether both axes moved — a flat α/2 over-corrects the common single-axis row, which is the opposite of what this PR is for.

Tightening α trades false-Fails for false-greens, and false-greens are the failure class this PR exists to close. Picking the threshold needs the paired-seed report the repo requires for any gate-threshold change, against a baseline refresh — neither of which is in this PR's scope. Filed rather than guessed. If you want it here instead, say so and I will do the Holm variant keyed on the number of active axes, with the report attached.

[matthewevans]

Changes requested — this current head makes the existing nightly workload permanently incompatible with its committed baseline.

🔴 Blocker

[HIGH] The new games_per_matchup refusal correctly prevents a partial comparison, but the repository's own nightly now always supplies incompatible reports. Evidence: crates/phase-ai/src/duel_suite/compare.rs:205-211 rejects differing workloads; crates/phase-ai/baselines/suite-baseline.json records games_per_matchup: 10; and .github/workflows/ai-gate.yml:78-87 runs cargo ai-gate --full-suite --games 100 without selecting a matching baseline. gate_verdict returns exit code 2 on that error (compare.rs:833-838), so the nightly will fail and publish a refusal rather than measure drift. The one default baseline refresh path cannot make both the 10-game PR gate and 100-game nightly comparable. Suggested fix: commit a workload-specific 100-game baseline and select it in nightly, or use one workload/baseline consistently; cover both workflow invocations against their selected baseline.

🟡 Non-blocking

[MED] The new exit/report coverage stops at gate_verdict, not the command/process boundary that redirects stdout in CI. Evidence: compare.rs:1843-1853 calls gate_verdict directly, while crates/phase-ai/src/bin/ai_gate.rs:104-115 is responsible for printing and exiting. A regression in main can again split nonzero exit from report output while every added unit test remains green. Suggested fix: add a CLI/integration test that runs the binary with deliberately mismatched report inputs and asserts both nonzero status and the refusal body on stdout.

Recommendation: request changes. Reconcile the nightly and PR workloads with their baselines, then bind the actual binary's status/output contract.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lgray]

🤖 AI text below 🤖

Correction to the remedy I was about to offer — a baseline refresh cannot fix this, and I should have checked before proposing it.

I was going to offer to regenerate suite-baseline.json at --games 100 so the nightly goes green. That would have made things worse, and the reason is worth stating because it also shaped the error text this PR ships.

Both jobs compare against the same default baseline at different workloads:

• ai-gate.yml:50 — cargo ai-gate --games 10 (quick filter, 3 matchups). No continue-on-error.
• ai-gate.yml:87 — cargo ai-gate --full-suite --games 100 (32 matchups).

With a symmetric games_per_matchup guard, no single baseline value satisfies both. At 10 the nightly refuses — today's state. At 100 the PR gate refuses, on every pull request, in a job with no continue-on-error. So remedy-by-refresh trades a red nightly for a red required check. A baseline refresh is not a fix here; it is a relocation.

That constraint is not something this PR can settle, because every way out of it edits .github/workflows/ai-gate.yml, and I do not edit workflow files unasked. The two that work:

1. Align the two jobs' --games. One value, both invocations, baseline recorded to match. The nightly loses sample size or the PR gate gains runtime; which trade is right is a call about what each job is for.
2. Give the nightly its own baseline via --baseline, recorded at --games 100 --full-suite. Both jobs then compare like-for-like. Costs a second baseline file and a second refresh path to maintain — the honest downside, since a baseline nobody refreshes rots into a false green, which is the failure this whole PR is about.

I'll implement either the moment you say which, including the recording run with the paired-seed report attached so the refresh is auditable rather than a blind overwrite. My weak preference is 2, because it lets the nightly keep the larger sample that makes it worth running at all — but it adds a file, and you own that trade.

What this PR does in the meantime is deliberate and I am not asking you to soften it: the nightly fails, loudly, with a body naming both directions and the constraint above. It was previously green while comparing 10 of every 100 games it played. A red job with an accurate explanation is the better of the two, and it is what your review asked for.

The error text carries the shared-baseline warning generically ("whichever workload is chosen, every invocation that compares against this baseline has to use that one"), so the first person to read a failing nightly does not have to rediscover this the way I did — by being told.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@crates/phase-ai/src/bin/ai_duel.rs`:
- Around line 746-756: Update both load_report error arms in the report-loading
flow near compare_reports to call render_error_markdown(&e) before returning
exit code 2, ensuring unreadable or invalid baseline/current reports produce the
required stdout artifact. Add a gate_cli test case covering a missing or
malformed report and verify the failure output is rendered to stdout.

🪄 Autofix

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: b7e6055d-93f5-4986-9355-f9efc286bbbd

📥 Commits

Reviewing files that changed from the base of the PR and between 562e7b5 and edc1c1e.

📒 Files selected for processing (7)

• crates/phase-ai/src/bin/ai_duel.rs
• crates/phase-ai/src/bin/ai_gate.rs
• crates/phase-ai/src/bin/ai_perf_gate.rs
• crates/phase-ai/src/duel_suite/compare.rs
• crates/phase-ai/src/duel_suite/mod.rs
• crates/phase-ai/src/duel_suite/perf.rs
• crates/phase-ai/tests/gate_cli.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/phase-ai/src/duel_suite/compare.rs

[matthewevans]

Changes requested — the current head makes the scheduled AI gate permanently incompatible with its committed baseline, and one stdout-refusal path remains untested and unwired.

🔴 Blockers

[HIGH] The new games_per_matchup refusal is correct for avoiding a partial comparison, but the repository's own scheduled gate now always supplies an incompatible workload. Evidence: .github/workflows/ai-gate.yml:50 runs the required quick gate with --games 10; .github/workflows/ai-gate.yml:87 runs the nightly with --full-suite --games 100; the sole committed default baseline declares games_per_matchup: 10; and compare.rs:155-185 now rejects that mismatch. Why it matters: every nightly fails before producing a drift comparison; changing the shared baseline to 100 would instead make the required 10-game PR gate fail. Suggested fix: make workload selection/baseline selection compatible for both callers (for example, workload-specific baselines selected by the workflow, or one shared workload), and add paired coverage that exercises both invocations against the chosen baselines.

[MED] ai-duel compare bypasses the shared stdout renderer when either load_report call fails. Evidence: ai_duel.rs:731-744 returns after eprintln!, while ai_duel.rs:752-756 alone routes comparison errors through emit_gate_verdict; gate_cli.rs:100-147 covers only workload mismatch and a comparable pair. Why it matters: redirected stdout remains empty for unreadable or malformed baseline/current reports, violating the documented gate-report contract. Suggested fix: route both load failures through render_error_markdown/the same output authority and add missing- and malformed-report process tests asserting non-zero exit plus a non-empty stdout refusal body.

No direct build was run; CI status is not sufficient to resolve these behavioral incompatibilities.

[lgray]

🤖 AI text below 🤖

Taking your first option: a workload-specific 100-game baseline, selected by the nightly. Recording is underway; the workflow change and the baseline land together, baseline first.

Two corrections to what I said earlier in this PR, both to my own numbers.

My cost estimate was wrong by an order of magnitude. I wrote "hundreds of CPU-hours" for a 32×100 recording. My own measured rate — 15–39 s/game — multiplies out to 13–35 CPU-hours, not hundreds. I asserted that figure without doing the multiplication, which is exactly the kind of claim I have no business making in a PR that argues for measurement.

And the corrected figure is itself built on an unrepresentative sample. The 15–39 s/game came from red-mirror alone, the fast aggro smoke. A one-game-per-matchup sample across all 32 matchups is running now under a 6-core cap, and it has already exceeded the wall-clock that rate predicted — so the real per-matchup spread is wider than one matchup could show, and slower. I will post the measured per-matchup projection before committing to the full recording rather than quote either number again.

That sample doubles as the thing that decides the shape of the fix: if the full recording is not affordable, a 100-game nightly baseline is not the right answer regardless of how tidy it looks, and I would come back to you with the alternative rather than quietly run something for a day.

What is already done and pushed (head edc1c1ea3): the exit/report contract is now bound at the process boundary — emit_gate_verdict owns print-and-exit for all three comparing binaries, and crates/phase-ai/tests/gate_cli.rs drives it through a real process with fabricated report files, asserting non-zero exit and the refusal body on stdout in the same invocation, plus a control arm. It uses ai-duel compare because that is the only one of the three needing no card database, so it costs milliseconds and adds no card-data load to the test suite.

Retracted: my earlier offer to regenerate the existing default baseline at --games 100. That would have made the required PR gate refuse on every pull request, since both jobs read that one file at different workloads. Giving the nightly its own baseline — your suggestion — is what avoids that, and it is why the workflow needs the --baseline selection rather than the baseline alone.

[lgray]

🤖 AI text below 🤖

Before recording the 100-game baseline I measured what it would cost, and the measurement says the option cannot work — for a reason that is not about the baseline at all. Your nightly has not completed a run in at least five days.

The nightly times out every night

scheduled run	duration	outcome
2026-08-04	300.6 min	cancelled
2026-08-03	300.3 min	cancelled
2026-08-02	300.5 min	cancelled
2026-08-01	300.5 min	cancelled
2026-07-31	300.6 min	cancelled

ai-gate.yml sets timeout-minutes: 300 on ai-gate-nightly. Every one of those runs is the timeout, not a verdict. The 2026-08-05 run started 11:15:05Z and was still going three hours later. Before this stretch (07-27 → 07-30) the job failed in ~45 s instead — a different, earlier mode.

The workflow already has an arm for it — if: steps.gate.outcome == 'cancelled' → exit 1 — so this has been firing nightly.

Why that makes a 100-game baseline unusable

A baseline is only worth what the run compared against it is worth. If the nightly cannot finish --full-suite --games 100, it cannot produce a report comparable to a 32×100 baseline no matter how correct that baseline is. Committing one would add a file that nothing can ever match.

It also means the consequence I disclosed in this PR — "the nightly will now fail every night until the workloads are aligned" — was overstated in the direction that flatters my change. It was already not producing a drift signal. My refusal would have replaced a silent timeout with a loud refusal, which is still an improvement, but not the one I described.

What it would cost to record, measured

One game per matchup across all 32, six workers, on a loaded box:

• 30 of 32 games: sum 2097 s, mean 69.9 s, median 37.5 s, max 228 s
• 2 games never finished in the 33-minute run — niv-mirror and delver-vs-green, so ≥1680 s each for a single game

Extrapolating the 30 that finished gives ~62 CPU-hours for 32×100. Including a lower bound for the two that didn't gives ≥151 CPU-hours and no upper bound, because I still do not know when they end. The distribution is heavy-tailed, so my earlier figures — "hundreds of CPU-hours", then the corrected "13–35" — were both projections from red-mirror, the fast smoke. Neither was a measurement of the suite.

A single game of niv-mirror running 28+ minutes looks like a search worst case rather than a normal cost, and may be worth a look on its own.

What I am asking you to decide

The real defect is the nightly's workload, not its baseline — fixing the baseline while leaving --games 100 --full-suite in place fixes nothing. So the choice is yours, and I did not want to burn a day of CPU pre-empting it:

1. Cut the nightly's workload to something that finishes, then record a baseline at exactly that workload. From the sample, --full-suite --games 1 should land in 23–45 min on a runner; --games 10 is roughly 15 CPU-hours (~3.8 h at 4 cores), which is marginal against a 5-hour budget and is dominated by the two outlier matchups.
2. Keep 100 games but cut the matchup set to those that finish in reasonable time, and record that.
3. Raise the timeout and fix the outliers first, treating niv-mirror / delver-vs-green as the actual bug.

I will implement whichever you pick, including the recording and the --baseline selection, in this PR. What I have not done is guess.

Meanwhile the rest of this PR stands on its own: the comparator no longer returns a green verdict from a partial sample, and the exit/report contract is bound at the process boundary. The required PR gate passes on CI at this head — the failing check on this branch is the decision-cost perf gate, which fails identically on main-based branches unrelated to mine (e.g. ship/perf-baseline-card-data-hash) and was already failing here before I touched ai_perf_gate.rs.

[lgray]

🤖 AI text below 🤖

MED fixed and pushed (375a5a84ae6b). HIGH is answered by the measurement in my previous comment and now needs a decision only you can make — details below.

MED — ai-duel compare bypassed the shared renderer on a load failure

Confirmed exactly as described, and it was the third instance of this defect in one PR: the same shape was fixed at compare's error arm and again in ai-perf-gate, and these two arms were the ones the sweep missed. Both now render through render_error_markdown. The path stays on stderr, because CompareError carries the cause but not the file, and a refusal reading "I/O error" without naming which of two inputs it was reading is not actionable.

Writing the current-side test found a second, smaller thing you did not flag: the Io | Parse remedy said "The baseline could not be read", which becomes false as soon as the current-report arm renders it. It now says "report" and points at the stderr line for the side.

tests/gate_cli.rs covers four sub-cases — {missing, malformed} × {baseline, current} — because the two inputs are separate arms in the source and the two failure modes carry different CompareError variants into the same renderer. Four mutants, both files restored byte-identical and verified with diff -q:

mutant	result
revert the baseline-side fix only	killed
revert the current-side fix only	killed
publish the body to stderr (the original defect's shape)	killed
empty the remedy, leaving the envelope	killed

The first two are why both inputs are covered rather than one.

Scope boundary, stated because "the same recipe" has an edge. run_repro_report in ai-perf-gate also exits 2 from a stderr-only load failure and is deliberately unchanged: its stdout is print_repro_margin — the margin gate's own report, not the drift-issue body this contract governs. Putting a comparison refusal there would emit it into an output nothing parses as one. The two usage errors in run_compare are left alone for the same kind of reason: a mistyped argument has no report to publish.

HIGH — the workload incompatibility

Your diagnosis is right and I am not arguing it. What I want to put beside it is the measurement from my previous comment, because it changes which of your two remedies is even available: the nightly has ended cancelled at its 300-minute timeout for five consecutive nights and has never once completed --full-suite --games 100.

So "every nightly fails before producing a drift comparison" is already true, and has been for at least five days, for a reason that predates this PR. What this PR changes is that the failure becomes legible — a refusal naming the mismatched field instead of a verdict quietly built from 10% of the sample. That is the unmasking, not the cause.

Both remedies you name need a judgement that is yours, not mine:

1. Workload-specific baselines. Needs a 32×100 baseline to exist. I measured what recording one costs before attempting it and stopped: the distribution is heavy-tailed, two matchups did not finish a single game in 33 minutes, and any mean-based projection off the fast matchups is invalid. I am bounding those two now (details below) so the number is a measurement rather than a floor.
2. One shared workload. Needs someone to decide what the nightly is for — if it exists to be a deeper sample than the PR gate, cutting it to --games 10 makes it a slower duplicate of the required check.

I will execute either as soon as you pick, including the ai-gate.yml change, baseline first. I am not picking for you: it is your workflow and the question is what it is meant to measure.

In flight: the two straggler matchups (niv-mirror, delver-vs-green) are running as single games under a 4-hour bound. At the time of writing both are past 90 minutes for one game. I will post the completed figure rather than the current floor — quoting a floor as though it were a measurement is what made my first cost estimate wrong, and I would rather be late than repeat it. That single-game duration also looks like a pre-existing AI-search worst case worth its own issue, independent of this PR.

[matthewevans]

Changes requested — this current head makes the scheduled AI gate incompatible with its committed baseline.

🔴 Blocker

[HIGH] games_per_matchup is now correctly treated as a comparison incompatibility, but the repository invokes the same default 10-game baseline with two incompatible workloads. Evidence: crates/phase-ai/src/duel_suite/compare.rs:205-211 returns WorkloadMismatch when the reports differ; crates/phase-ai/baselines/suite-baseline.json records games_per_matchup: 10; .github/workflows/ai-gate.yml:49-50 runs the quick gate with --games 10; and .github/workflows/ai-gate.yml:78-87 runs nightly with --full-suite --games 100 without choosing another baseline. Why it matters: the 100-game nightly now exits with a refusal instead of measuring drift, while changing the only default baseline to 100 would make the required 10-game gate incompatible. Suggested fix: select workload-compatible baselines for both invocations, or use one common workload/baseline, and add coverage that exercises each invocation against its selected baseline.

Recommendation: request changes. Resolve the workload/baseline contract for both the quick gate and nightly before this comparator refusal lands.

[lgray]

🤖 AI text below 🤖

A second, independent line of evidence that nothing is watching main — and it also settles whether this PR's perf-gate failure is mine.

The Decision-cost perf gate is red on this PR with 8 FAIL / 21 PASS. Before claiming that was inherited I went looking for a controlled measurement, and there is exactly one: the scheduled run's Nightly decision-cost perf drift monitor completed on 2026-08-05 at 11:14Z against main at 5c8d12bf5fb1 (run 31000694717). Every other scheduled run in the window was cancelled at the timeout, so this is the only completed main-line perf measurement that exists.

main alone already fails all eight counters

counter	baseline	main @ 5c8d12bf5fb1	main/baseline
attackable_player_sweeps	830	2640	3.18x
crew_eligibility_scans	7337	12027	1.64x
layers_full_eval	3495	15877	4.54x
legend_rule_mode_gate_scans	10274	20926	2.04x
mana_aura_trigger_scans	14286	26841	1.88x
restriction_static_mode_gate_scans	46421	89324	1.92x
sba_battlefield_snapshot_builds	10205	20853	2.04x
state_clone_for_legality	6489	17786	2.74x

The gate's band is base*1.05 + 64. Every row above clears it several times over, so the perf gate fails on main itself, with no PR involved.

How much of this PR's failure is inherited

Three of the eight are byte-identical between main and this PR — attackable_player_sweeps 2640, crew_eligibility_scans 12027, layers_full_eval 15877. Those are inherited in full, measured rather than argued.

The other five read higher on this branch than on that main snapshot. I am not claiming those are proven inherited, because the snapshot is from 11:14Z and this branch is based on a later main, so the difference is consistent with continued drift in between but is not demonstrated by this instrument. What I can say structurally is that this PR's diff touches the suite comparator, ai-duel's stdout, and one remedy string — all of which run after games are simulated, while these are engine counters recorded during simulation. A cross-branch control is in flight on #7029 (an unrelated change to the same crate); if it fails the same rows, neither PR caused them.

Why this belongs in this thread

You asked, reasonably, how the workload and the baseline should be made compatible. This is the same question arriving from a second direction: the counters have drifted 1.9x-4.5x past the committed baseline on main, and the only reason nobody has had to act on it is that the job which would have said so has not completed a run in five days. Earlier in this thread I showed the nightly timing out; this shows what it would have been reporting had it run.

One counter is worth singling out for whoever picks up the perf regression: attackable_player_sweeps was 1265 in a CI run on 2026-08-03 and is 2640 on main now. I flag it as indicative rather than controlled — the earlier figure came from a PR-branch run, not a main-line one — but a counter roughly doubling in two days against a static baseline is the shape of something still moving, not something that settled.

I have no fix to offer here: this is a pre-existing engine/AI-decision cost regression, and my earlier bisect attributed the bulk of it to two merged commits (31c260f50 #6826, 8121fd1c6 #6842) rather than to anything in this PR. It is raised so the workload decision is made with it visible.

[lgray]

🤖 AI text below 🤖

The cross-branch control landed, and it closes the part of my previous comment I said was unproven.

I claimed three of the eight counters were provably inherited (byte-identical to main) and explicitly declined to claim the other five, because the only main-line snapshot available was from 11:14Z while this branch sits on a later main — consistent with drift, but not demonstrated.

#7029 has now run the same gate. It is an unrelated change to the same crate (a baseline-write transaction fix; no comparator, no ai-duel, no shared file with this PR's diff except the crate itself). Result:

counter	baseline	#7026	#7029
attackable_player_sweeps	830	2640	2640
crew_eligibility_scans	7337	12027	12027
layers_full_eval	3495	15877	15877
legend_rule_mode_gate_scans	10274	27535	27535
mana_aura_trigger_scans	14286	53208	53208
restriction_static_mode_gate_scans	46421	155747	155747
sba_battlefield_snapshot_builds	10205	27462	27462
state_clone_for_legality	6489	19342	19342

Byte-identical on all eight, and the same tally on both: perf compare: 8 FAIL, 21 PASS, 0 NEW, 0 REMOVED.

Two different diffs producing identical values across all 29 counters means the counters are invariant to both diffs. All eight failures are inherited, not caused by either PR — the five I would not previously assert are now settled by measurement rather than by the structural argument I offered as a substitute.

Taken with the earlier finding that main at 5c8d12bf5fb1 already clears the band on all eight, the position is: the perf gate is red on main, has been for some time, and every PR touching crates/phase-ai inherits that red. Nothing in this PR moves it in either direction.

[lgray]

🤖 AI text below 🤖

The tail measurement concluded — by hitting its bound, not by finishing. Neither game completed in four hours.

I said earlier I would post a completed figure rather than the >=1680s floor I had. There is still no completed figure, and I would rather say that plainly than dress up the bound as one.

matchup	seed	start	killed	elapsed	outcome
delver-vs-green	10586729	14:15:40Z	18:20:23Z	14683s = 4.08h	did not finish
niv-mirror	10597729	14:18:27Z	18:20:23Z	14516s = 4.03h	did not finish

One game each. The other 30 matchups completed their single game in the same run; these two were still running when the 4-hour watchdog killed the process (EXIT=143).

Slow, not hung, established two ways so it is not an inference: at a direct sample partway through, each worker thread had accrued 2h04m of CPU at roughly 90% duty — so they were computing, not starved — and MAX_TOTAL_ACTIONS = 10_000 caps actions per game, so these games must terminate eventually. Four hours simply was not enough.

What this means for the nightly, with no extrapolation

I can make one claim here that requires no projection at all.

matchup_seed = base_seed + idx * 1000 and seed = matchup_seed + game_idx. Both are independent of games_per_matchup. My run and the nightly both use --full-suite, the default seed, and the default Medium difficulty. So --games 100 plays game_idx 0 for every matchup — exactly the two seeds above.

The nightly's 3200-game workload therefore provably contains at least two individual games that each need more than four hours, against a 300-minute total budget (ai-gate.yml:61). Those two games alone want more than eight hours of the five it has — before the other 3198 are considered.

A correction to something I nearly posted. I had drafted a much larger figure on the basis that games within a matchup run sequentially, which was true when I first read that code. It is not true now — run_games_parallel is merged, so games within a matchup are distributed across workers, and the projection would have been wrong. I checked before publishing rather than after. The seed-overlap argument above replaces it and needs no assumption about scheduling at all.

Caveat, running in the conservative direction: this was measured on a contended box under nice -19 pinned to 6 cores. The 90%-duty measurement shows contention is not what made these slow, and a 4-core CI runner would not be faster.

What I am not claiming

The honest answer to "how long does one niv-mirror game take" is still more than four hours, upper bound unknown. I bounded it below rather than measuring it, because measuring it means letting a single game run indefinitely, which needs your call on whether the answer is worth the machine time.

This looks like a pre-existing AI-search worst case rather than anything to do with either of my PRs, and it deserves its own issue. I am happy to open one with the seeds, the durations, and the bisect context — say the word.

[lgray]

🤖 AI text below 🤖

Cross-link: the decision-cost investigation referenced in this thread is now posted on #6967 (#6967 (comment)) — it independently confirms the break window from the profiling comment there, credits #6998's wall-clock fix, and defers all baseline questions to #7044.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lgray]

🤖 AI text below 🤖

@matthewevans — answering the HIGH blocker as a design question, since a rebase can't resolve it.
Nothing here is committed or pushed; this is a proposal for your call.

1. You already named the resolution, so this is a sketch of your preference, not a menu

All three of your reviews say the same thing, and the first-named option is the same each time:

(12:06Z) "Suggested fix: commit a workload-specific 100-game baseline and select it in nightly,
or use one workload/baseline consistently; cover both workflow invocations against their selected
baseline."

(14:08Z) "make workload selection/baseline selection compatible for both callers (for example,
workload-specific baselines selected by the workflow, or one shared workload), and add paired
coverage that exercises both invocations against the chosen baselines."

(16:13Z) "select workload-compatible baselines for both invocations, or use one common
workload/baseline, and add coverage that exercises each invocation against its selected baseline."

So: Option A = workload-specific baselines selected by the workflow (your lead option, given as the
worked example twice). Option B = one shared workload. In all three, the coverage clause is not
optional — it is part of the fix, not a follow-up.

2. What the code supports today (measured, at 8eaee5fe919a)

The load-bearing finding: Option A requires no Rust change at all.

capability	where	state
ai-gate accepts a baseline path	crates/phase-ai/src/bin/ai_gate.rs:141 — "--baseline" => baseline = next_path(...)	already exists
default when the flag is absent	ai_gate.rs:19 — DEFAULT_BASELINE = "crates/phase-ai/baselines/suite-baseline.json"	this is what both callers silently get today
what counts as an incompatible workload	compare.rs:198-211 — WorkloadMismatch on difficulty and games_per_matchup	two fields, not one
--refresh-baseline target	ai_gate.rs:76-77 — writes to args.baseline	honours the same --baseline selection
refresh script arg passthrough	scripts/refresh-ai-baseline.sh:7 — exec ai-gate.sh --refresh-baseline "$@", and ai-gate.sh:20 — exec …/ai-gate "$@"	fully forwarded

Consequence: a second baseline can be produced today with

scripts/refresh-ai-baseline.sh --full-suite --games 100 \
  --baseline crates/phase-ai/baselines/suite-baseline-nightly.json

and the nightly selects it with one added flag. No comparator, CLI, or script change.

The two callers as they stand: .github/workflows/ai-gate.yml:50 — cargo ai-gate --games 10
(required, PR-level); :87 — cargo ai-gate --full-suite --games 100 (nightly). Both resolve to the
one 10-game default, which is exactly the collision.

3. Why Option B is not viable, with numbers rather than taste

Option B means one shared workload. Both directions fail, and the timing half is measured, not
estimated:

• Raise the PR gate to 100 games. The paired-seed job completed in 11.4 min at --games 10
  (run 31050404179). Games scale the
  play phase roughly linearly and compile is a stable ~9 min cold / ~2 warm (your own §2 table), so
  ~10× the play phase lands far outside the 60-minute wall this issue exists about. Non-starter on a
  required check.
• Drop the nightly to 10 games. Legal, but it discards the nightly's entire reason to exist: the
  sign test at compare.rs:606 (sign_test_mid_p_upper_tail) gets its power from sample count, and
  a 10-game nightly is just the PR gate on a timer.

So Option B trades away either the wall-clock budget or the statistical power. Option A keeps both.

4. Recommendation — Option A, concretely

1. Commit crates/phase-ai/baselines/suite-baseline-nightly.json, recorded with the nightly's own
   workload (--full-suite --games 100) via the command in §2.
2. Select it in the nightly step (ai-gate.yml:87):
   cargo ai-gate --full-suite --games 100 --baseline crates/phase-ai/baselines/suite-baseline-nightly.json.
   The quick gate at :50 keeps the default and is untouched.
   This line is yours to land, not ours. .github/workflows/** is your domain and we don't edit
   it uninvited — so of the three pieces, two are ours to deliver (the committed nightly baseline
   artifact with full provenance, and the paired coverage) and the one-line workflow selection is
   yours, or ours only if you'd rather bless us landing it.
3. Paired coverage, per your clause. A test per invocation asserting each is comparable against
   the baseline it actually selects — i.e. that compare() does not return WorkloadMismatch for
   (quick workload × default baseline) and (nightly workload × nightly baseline). The discriminating
   form is to assert on the workload fields the comparator keys on, so the test fails if either
   workflow line or either baseline's difficulty/games_per_matchup drifts. A test that merely
   ran the comparator on hand-built reports would pass while the workflow said something else — the
   coverage has to read the workflow invocations, or it is not covering the thing that broke.

Trade-offs, stated

• Two baselines to refresh, not one. Real cost: a workload-affecting AI change now needs two
  refreshes, and forgetting the nightly one produces a nightly refusal — the same failure mode as
  today, just less often. The §3-style coverage is what catches it.
• Nightly baseline is expensive to regenerate (100 games × full suite), and "can be produced
  today" means mechanically unblocked, not cheap — its wall time is unmeasured and I won't imply
  otherwise. The only full-suite figures anyone holds are debug-profile watchdog tails from before
  fix(ai): gate projection wall-clock cap on measurement mode + unify AI gate build profiles #6998 unified the build profiles (≥4 h per matchup, wrong profile, 0 of 2 games completed), and
  there is no completed post-fix(ai): gate projection wall-clock cap on measurement mode + unify AI gate build profiles #6998 nightly to cite — the most recent scheduled run,
  31000694717, was cancelled. The
  cheap settling instrument is one timed server-release full-suite run at --games 10, scaled by
  game count; that number should exist before anyone budgets the real recording. Offer: this box
  has ~1 TB free and 16 cores, so we can host the recording under the refresh script's own provenance
  contract and hand you the artifact plus its timing — or you produce it yourself if you'd rather own
  the provenance end to end. Either way the figure gets measured before it gets promised.
• Sanctioned-failure interaction. Per fix(ai): make the duel-suite comparator see the regressions it was blind to #7026's own Scope Expansion note, a committed baseline can
  bless a failing matchup and nothing revalidates it on load. A second baseline doubles the number of
  places that can happen. fix(ai): refuse to refresh the duel-suite baseline from a failing run #7029's --refresh-baseline guard reduces it on the write path for both.

5. One thing your review does not cover, surfaced rather than folded in

--full-suite changes the matchup set, not just the game count, and compare() does not treat
a differing matchup set as a workload mismatch — the check at compare.rs:198-211 keys only on
difficulty and games_per_matchup. Extra matchups arrive as new-matchup rows instead. Option A
resolves your stated blocker either way, because each baseline is recorded with its own caller's
matchup set. But if your intent is that "incompatible workload" means the whole workload, then the
matchup set arguably belongs in the same check. Flagging as a question for you, not proposing it —
it is a scope expansion on a PR whose scope expansion is already the thing under review.

[matthewevans]

[MED] Duplicate baseline seeds can suppress a distinct unpaired current seed. Evidence: crates/phase-ai/src/duel_suite/compare.rs:537-561 deduplicates current games into current_by_seed but increments paired once for every baseline row with that key; :587-591 then computes unpaired_current as current_by_seed.len().saturating_sub(paired) without validating duplicate seeds. With baseline seeds [1, 1] and current seeds [1, 2], the loop sets paired = 2, the map length is 2, and the distinct unmatched current seed 2 is reported as zero. Why it matters: the comparator can return a clean fully-paired result while omitting real current-game coverage, recreating a false-green partial comparison. Suggested fix: validate duplicate seeds as malformed reports before comparison, or count pairing as a set intersection of distinct seeds; add the [1,1] versus [1,2] regression with a discriminating unmatched-current assertion.