fix(ai): refuse to refresh the duel-suite baseline from a failing run

[lgray]

🤖 AI text below 🤖

Summary

cargo ai-gate --refresh-baseline ran the suite and wrote the result to the baseline path without ever inspecting the result's own verdicts. Because the baseline is what every later run is compared against, one refresh from a red run blessed that failure permanently: the next run compared equal to the blessed report, the comparison reported no drift, and the gate exited 0 forever while the matchup stayed broken.

This is the other half of the policy #7026 left open. That PR made an existing matchup going non-Fail → Fail fail the comparison, and reported a still-failing matchup on every run rather than passing quietly — but it deliberately Warned rather than Failed on the already-blessed case, on the grounds that the exit code answers "did this change make things worse" and a baseline, however it got that way, already sanctions its own contents. The question it filed rather than smuggled in is whether a baseline may bless a failure at all. It may not.

Files changed

• crates/phase-ai/src/duel_suite/run.rs — SuiteReport::failing_matchups() and SuiteReport::recorded_games(), plus tests
• crates/phase-ai/src/bin/ai_gate.rs — refuse the refresh on a failing or gameless run; reject --games 0 at parse time; load the baseline before the suite runs, stage-and-rename the refreshed baseline, and refuse aliased --baseline/--current-output (review round 2)
• crates/phase-ai/tests/refresh_baseline_cli.rs — process/file-state regression against the real binary

Track

Developer

LLM

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

Implementation method (required)

Method: not-applicable — this is gate/tooling plumbing in crates/phase-ai, with no engine game logic, no Oracle parsing, and no card behaviour.

CR references

None. No MTG rules logic is touched.

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.

What disqualifies a run, and why only these things

SuiteReport::failing_matchups reports matchups that failed their own Expected check, judged with no reference to any baseline. That is a different question from CompareReport::any_fail, which asks whether a change made things worse than the baseline; this asks whether a run is fit to become the baseline. It returns the matchups rather than a bool because the refusal is only actionable if it can name which matchup failed and quote its fail_reason.

Only SuiteStatus::Fail disqualifies on that axis. Open does not, and the distinction is load-bearing: Expected::Open is how a matchup declares it has no verdict yet, and that declaration belongs in the suite definition where it is visible and reviewable, not smuggled in by committing a red baseline. An implementation keyed on != Pass would conflate the two and make Expected::Open unusable — there is a test whose only job is to fail against that.

SuiteReport::recorded_games disqualifies a second, independent case that review surfaced: a run that measured nothing. Comparison pairs by seed, so a baseline holding no games makes every later comparison score zero on every axis and the drift signal dies as quietly as a blessed-red one. It counts games rather than testing for all-Open deliberately — a suite whose matchups are all declared Expected::Open still plays real games, and that report is a usable baseline, because the paired-comparison arm decides on games, not on status. (Stated that way rather than "never reads status": the paired arm gains status tiers in #7026, so the stronger wording would have been false the day that merged. Review caught the stronger version — see the in-flight-branch note below.) Zero games is what makes a baseline inert; all-Open is not.

No override flag

The escape hatch already exists one layer up and is the correct layer, so a second one at the baseline would only let a caller bypass the more visible mechanism. Verified non-bricking before choosing absolute refusal: the committed baseline is 3/3 Pass, so no workflow depends on a blessed failure.

CI blast radius: none, measured

refresh-baseline appears in no workflow. CI runs cargo ai-gate --games 10 and --full-suite --games 100, both compare-only. Refresh is a local human operation, so this cannot break a pipeline — it can only stop a person committing a blessed-red baseline.

The failing case is pinned to the run that caused it

the_recorded_failing_run_is_disqualified_as_a_baseline transcribes the recorded gate run that motivated this guard: red-mirror and affinity-mirror Pass, enchantress-mirror Fail carrying its verbatim reason mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50. That is the exact report a refresh would have blessed. It is transcribed rather than loaded because the artifact is untracked and a test that read it would fail in CI.

Mutant evidence

Nine mutants, tree restored byte-identical after each. Kill counts below are transcribed from the runs. An earlier draft of this section asserted three of them from memory and review measured all three wrong, so they are given as figures rather than as characterisations.

mutant	killed by
drop the filter	5
return nothing	1 (the_recorded_failing_run_is_disqualified_as_a_baseline)
!= SuiteStatus::Pass	3 (every test asserting an Open matchup is not a failure)
recorded_games body → 0	1
recorded_games body → 1	3
recorded_games body → 2	3
count non-Open matchups instead of games	1
count matchups-with-games	1
sum games.len().min(1)	1

The last two are there because they survived an earlier version of this change. Every fixture carried exactly one game per matchup, so the total always equalled the matchup count and "sum of games" was never distinguished from "number of matchups that played" — against the real baseline (3 matchups × 10 games) those mutants return 3 where the contract says 30. The assertion looked like a magnitude pin, but its expected value was simultaneously the matchup count, and a fixture whose two interpretations coincide pins neither. The fixture is now uneven (two games in one matchup, one in the other) and asserts 3, which review confirmed is distinct from every other countable that implementation could plausibly return — it probed seven, including games_per_matchup × len, wins-sum, distinct seeds, and max-per-matchup.

The constants are listed individually because the choice decides the answer: an earlier draft said "making it constant fails all three tests", which is true only for 1 and false for 0 — the value the guard actually tests.

One further mutation is reported rather than counted: dropping the fail_reason passthrough is not a mutation of the predicate. failing_matchups yields &MatchupResult, so no change to it can drop that field; the only reachable site is the test helper. It shows the assertion genuinely reads the field and pins the iterator's item type against narrowing, but it is not predicate coverage.

End-to-end, and one surface deliberately left uncovered

No unit test can reach a binary's main, so the wiring was proven through the CLI. All three arms use the same binary with no source mutation, and the binary was verified to contain both refusal strings first so no arm could pass against a stale build: --games 0 is rejected at parse time without running the suite; a --suite-filter matching no matchups is refused with nothing written; and a real two-game run still refreshes, recording 2 games, as the positive control that shows the guard is not simply refusing everything.

The two refusals are ordered failures-first, and the order is load-bearing rather than cosmetic: the conditions are not exclusive. failed_result builds a matchup with an empty games vector and SuiteStatus::Fail, so a run whose deck payloads all fail to load satisfies both — and checking gamelessness first would replace each matchup's setup error: … with a sentence about seeds. Nothing is lost by the chosen order, because a merely gameless run has no failing matchups to report. Review verified the full 2×2 of the two conditions and found no input where the old order was better and no exit code changed.

This surface is no longer uncovered — see "The execution pin, closed" below. It was disclosed here on the premise that reaching the refusal block needs a card database and a real suite run, which is minutes of CI for a local-only command. That premise was false, and cheaply so, which is why the paragraph is corrected rather than left standing with an apology attached.

A note on durable claims, which review taught me here

Round 2 caught a claim that was true in this tree and false in a branch I am driving: "seed-paired drift detection never consults status" holds at this head but not on #7026, where the paired arm gains status tiers — so it would have gone false the day that PR merged, and #7026 is cited two paragraphs above it. The wording is now durable under both. The instrument came from the reviewer, and applying it to my own open PR immediately found three more instances there, which are fixed separately on that branch.

Review response (2026-08-05 12:12Z) — blocker confirmed, and it had a quieter twin

HIGH — the refusal fired after the damage. Reproduced before changing anything.

run_suite writes its report to options.output_path before any guard runs, and --baseline / --current-output are independent flags, so aliasing them let a rejected run truncate the baseline first. On the built binary at the reviewed head:

	baseline bytes	sha256	exit
before	116	985133e3…
after --refresh-baseline --baseline X --current-output X --suite-filter no-such	250	bc9649fd…	1

It printed "refusing to refresh" over a baseline it had already destroyed.

The same aliasing on the compare path is worse, because it is silent. load_report(&args.baseline) read back the report the suite had just written over it, so the gate compared the run to itself. Measured with a baseline recording p0 at 100% against a run that scored 0%:

| red-mirror | AggroPressure | 0% | 0% | 0 | 0 | — | PASS |
compare: 0 FAIL, 0 WARN, 1 PASS, 0 NEW, 0 REMOVED     exit 0

An honest comparison of those inputs is a win-rate collapse. That is this branch's own subject — a false green — reached by destroying the evidence rather than by misreading it. Found while verifying your blocker, fixed here.

Three layers, each doing something the others cannot

1. Root cause on the compare path: read the baseline before the suite runs. The defect is when the baseline is read, not which paths were passed, so ordering is the fix, and it is immune to aliasing by construction — including aliases no path comparison can detect. Proven with a hard link, which defeats layer 3 outright (same inode, different name, so canonicalising calls them distinct while a write to either truncates both):

| red-mirror | AggroPressure | 100% | 0% | 1 | 0 | 0.2500 | WARN |

The pre-fix binary called those same inputs 0% | 0%, PASS. This is the measurement that shows ordering, not the argument check, is carrying the correctness.

2. The refresh is transactional. The suite writes to <baseline>.staging.json; the baseline is replaced by renaming it only after every guard passes, and each refusal removes it. Sibling, not /tmp, because rename is atomic only within a filesystem — a cross-mount staging path degrades silently to copy-then-truncate, reintroducing the half-written baseline it exists to prevent. write_report is deleted: it was the last writer that could produce a baseline outside the transaction. --current-output is consequently unused on the refresh path (the refreshed baseline is the run's report); no workflow passes --refresh-baseline, so nothing in CI depends on that.

3. Aliased paths are refused outright, before the card database and before a game. Correctness no longer depends on it, but aliasing still destroys the user's file as a side effect of a write that had no business landing there.

Measured after the fix

arm	exit	baseline sha256	staging left
aliased refresh	2	unchanged	none
aliased compare	2	unchanged	none
hard-linked compare (defeats layer 3)	0	truncated by design; verdict honest — 100% → 0%, 1 flip, WARN	n/a
rejected non-aliased refresh (gameless)	1	unchanged	none
successful refresh	0	created, 1 game recorded	none

The CLI test you asked for, and the version of it I had to throw away

You asked for a process/file-state regression asserting non-zero exit and unchanged baseline bytes. I wrote exactly that, then mutated it: deleting the alias check killed 0 tests, and forcing same_file to return false killed 0. Both passed because without a card database the binary exits non-zero at the database load with the baseline equally untouched — the pair was satisfied for a reason unrelated to the hazard.

Adding an assertion that the refusal names the aliasing makes both mutants fail two tests each. A third mutant — same_file always true — is caught only by the control arm asserting distinct paths still proceed (and its premise checks that they reach the card database, so "not rejected as aliases" is not vacuous either). The module comment records this rather than hiding it; the byte-preservation claim under a real run rests on the table above, not on the test.

tests/refresh_baseline_cli.rs runs the real binary and costs ~0ms. (Written before the execution pin closed; the aliasing tests still never reach the card database, while the cases that must run the suite supply their own {} database. Nothing loads the ~90 MB export — and that gate is scoped to crates/engine in any case, so it never applied here.) Two unit tests pin the properties the layers rest on: the staging file is a sibling of the baseline (what makes rename atomic), and same_file sees through a symlink while still calling genuinely distinct paths distinct.

This paragraph previously read "Not covered: the ordering and staging fixes are argued from the code path and measured on the built binary, not pinned by an automated test — reaching them requires a card database and a real suite run." Three of its four clauses are now false. See below.

The execution pin, closed

CardDatabase::from_export deserialises a map from card name to entry, so the literal {} is a valid empty card database. A --suite-filter matching nothing then selects no matchups, builds no decks and plays no games, so the run reaches recorded_games() == 0 and refuses — in about five milliseconds, with no card data on disk:

EXIT=1  BASELINE=unchanged  STAGING=absent
refusing to refresh <path>: the run recorded no games, so every later comparison would score zero
real 0m0.005s

A second fact fell out of a surviving mutant: an empty database still yields one degenerate but playable matchup, ~2ms per game, so the accept path is reachable at the same cost. That is what made both directions of the file transaction testable, and it is why "never promote at all" no longer survives.

The exit code is asserted as exactly 1, not merely non-zero, and that is what makes the test see the refusal block rather than an early death: argument and database failures exit 2, so any mutant stopping the binary before the suite runs turns 1 into 2 and fails.

What remains genuinely unpinned is narrow and stated: a successful refresh against real decks. The transaction is pinned; the card content flowing through it is not.

Review response (2026-08-05 14:07Z) — both HIGHs confirmed and fixed

Both findings are real, both are fixed at 43fdc6683373, and one of them found the fix I had designed for myself was the weaker of the two available.

HIGH-1 — a malformed existing baseline was still eligible for replacement. Confirmed. The refresh arm caught every load_report error, logged it, and continued to run_suite, which renamed the staged report over the file. So a baseline that was corrupt, truncated, or unreadable was destroyed rather than kept for diagnosis, and the new reference was established from a prior state nobody had examined. Fixed as you specified: only ErrorKind::NotFound proceeds. One deviation from the letter of the suggestion, in your favour — matching on the error kind rather than !baseline.exists(), because the latter asks the filesystem a second, later question and answers the wrong one under a permission error, where the file exists but exists() reports false.

HIGH-2 — same_file missed hard-link aliases. Confirmed, and your fix is better than mine. I had independently designed a test for exactly this case, but as a pin on the ordering — the baseline is read before the suite writes, so the verdict survives even when the bytes do not. Your framing names why that is insufficient in one clause: "retaining the preloaded report in memory avoids a false comparison but not data loss." The ordering saves the verdict; only refusing saves the file. same_file now compares (dev, ino) when both paths exist and keeps the canonicalized-parent fallback for the usual case where the output does not exist yet.

That change falsified a comment two lines below it, which claimed same_file cannot see hard links. Rewritten rather than deleted: the ordering stays as defence in depth on an honest basis — same_file enumerates the ways two names can mean one file, and any such enumeration is a claim a future filesystem can falsify.

Fixture damage the HIGH-1 fix caused, and why it mattered

Two existing tests used an invalid sentinel as the baseline. The malformed-baseline fix now refuses that before either test reaches its subject — while still satisfying their "bytes unchanged" assertion, so both would have gone on passing for entirely the wrong reason. They now record a real baseline first (~2ms). This is the same failure mode as the one your round-2 finding taught me: an assertion pair that is satisfied because the run died early, not because the guard held.

Mutant evidence

Five mutants, tree restored byte-identical after each and verified with diff -q.

mutant	killed
drop the inode check (pre-fix behaviour)	1 (a_hard_linked_output_is_refused_and_the_baseline_survives)
refresh swallows every baseline read error (the reported defect)	1 (a_malformed_existing_baseline_is_not_replaced_by_a_refresh)
refuse a missing baseline too	3 (first-refresh, accepted-refresh, gameless)
same_inode returns Some(true) unconditionally	1 (two_existing_but_different_files_are_not_aliases)
compare ino without dev	0 — SURVIVES

The survivor is reported, not hidden. Killing it requires two files on different filesystems whose inode numbers collide, and inode allocation is not controllable enough to construct that deterministically in a fixture. dev is kept on correctness grounds, and the unpinned direction is the safe one: dropping it can only produce a false alias report, whose consequence is a refused run, never a destroyed baseline. If you want it pinned anyway I will take a suggestion on how — I could not find a construction that was not flaky.

Two of the new tests exist as two-sided controls rather than as coverage. two_existing_but_different_files_are_not_aliases is the only case that reaches the inode comparison expecting Some(false) — the pre-existing distinct-paths test leaves the output nonexistent, so same_inode returns None and the path fallback answers, meaning an implementation calling every existing pair identical would have passed the entire file while refusing every real gate invocation. a_missing_baseline_is_still_the_first_refresh_case is the matching control on the other fix: without it, narrowing to NotFound is indistinguishable from refusing every unreadable baseline, which would make the first refresh on a fresh checkout impossible.

Measured at this head: 9 CLI tests green in 0.08s, 2026 lib tests green, clippy -p phase-ai --all-targets -D warnings clean, cargo fmt --check clean.

CodeRabbit's two inline comments are the same two findings and are addressed by the same commit.

Gate A

Gate A PASS head=b71e0a3db506fb38d1a7fe9e217f7e20dbb4af4b base=c44a4512e6f91684068c259a028eb44b4d801340

Base passed explicitly (./scripts/check-parser-combinators.sh c44a4512e6f91684068c259a028eb44b4d801340), so the examined range is
exactly this diff: 4 commits, 3 files, non-empty. It contains zero parser paths, so the
parser-specific verdict is vacuous by construction — labelled rather than presented as a pass.
Gate G also PASS at this head.

Anchored on

• crates/phase-ai/src/duel_suite/compare.rs:57 — CompareReport::any_fail, the existing report-level predicate whose result drives an exit code. failing_matchups is the same shape at the same seam: a predicate on a report, owned by the library, consumed by a binary that decides what to do about it. It returns the matchups rather than a bool only because the refusal has to name them.
• crates/phase-ai/src/bin/ai_perf_gate.rs:239 — the sibling --refresh-baseline block, structurally identical to the one guarded here. It anchors the seam and the scope decision: it is deliberately left unguarded, because PerfReport carries no intrinsic verdict (any_fail lives on PerfCompareReport, perf.rs:467, keyed on comparison). A perf run cannot be failing on its own, so there is nothing there to refuse.

Both line numbers re-verified at this head: compare.rs:57 is pub fn any_fail(&self) -> bool {, and
ai_perf_gate.rs:239 is if args.refresh_baseline {.

Final review-impl

Final review-impl PASS head=b71e0a3db506fb38d1a7fe9e217f7e20dbb4af4b

Certified at cert-r3 PASS head=ba39f354fb200a8d96a9146e2d70443ed55083f0, and carried to this head
by a content-preserving rebase: git diff over the pre-rebase range and over
c44a4512e6f91684068c259a028eb44b4d801340..b71e0a3db506fb38d1a7fe9e217f7e20dbb4af4b are byte-identical (1237 lines, md5 750675149582), so only the base moved. The
maintainer's hold comment records the prior requested-changes findings as fixed at the pre-rebase
head; that assessment transfers unchanged.
Three rounds. Round 1 returned PASS with three findings. Round 2 returned FAIL, and the classification is worth stating plainly because the code was not the problem: the reviewer's words were "the shipped code is correct" — both holes closed, design premise verified against the real comparison code, parse change behaviour-preserving. The FAIL was on my evidence. Three mutant kill counts in the commit message were asserted from memory and all three were measurably wrong (drop-filter said 3, is 5; != Pass said 1 "surviving the other two entirely", is 3; "constant fails all three" is true only for the constant 1 and false for 0, the value the guard actually tests). Two mutants were also found surviving: recorded_games was pinned only at zero/non-zero, because every fixture had one game per matchup so the total always coincided with the matchup count — an assert_eq!(..., 2) that looked like a magnitude pin when 2 was also the matchup count. A fixture whose two interpretations coincide pins neither.

Round 3 certified the fixes: both former survivors now die, and all nine corrected kill counts matched the reviewer's independent runs exactly. It also probed seven further contracts the fixture might have merely relocated the coincidence into (games_per_matchup × len, wins-sum, distinct seeds, max-per-matchup, first-matchup-only, and two saturating caps) and confirmed the assertion 3 is now distinct from every one of them.

Post-certificate delta: two comment-only edits — the mutant total in the commit message (the enumeration sums to nine, the header said eight) and one added doc clause. git diff ba39f354f HEAD -- crates/ contains zero changed source lines outside doc comments, so compiled behaviour is identical and the certificate's mutant results carry unchanged.

That disclosure — no test executes the binary, so the refusal block itself can be deleted or inverted with the suite green — is closed, not carried forward. tests/refresh_baseline_cli.rs drives the real binary through nine cases. The one item now disclosed in its place is a single surviving mutant, recorded in the round-3 section below with the reason it cannot be killed deterministically and the direction in which it fails safe.

Claimed parse impact

None.

Scope Expansion

One item, and it came from review rather than from me.

Round 1 found that classify returns Open for any matchup with zero games before it ever inspects Expected, and that --games parsed as usize accepted 0 despite its own error string promising a positive integer — so --refresh-baseline --games 0 wrote an all-Open, zero-game baseline. While closing that I found a sibling: run_suite has no emptiness check on its matchup selection, so a --suite-filter matching nothing also produced a baseline with no results at all.

Both are the same class — a run that measured nothing — and both are closed by one predicate rather than two special cases. Shipping the guard while leaving a second route to an unfit baseline open would have made the commit's own stated guarantee false, so it is fixed here rather than filed.

Validation Failures

None.

CI Failures

None.

Summary by CodeRabbit

• Bug Fixes

  • Baseline refreshes now stop with clear errors when matchups fail or no games are recorded.
  • Error messages identify affected matchups and preserve failure reasons.
  • The --games option rejects zero and other invalid values before execution.
  • Operations are blocked when baseline and output paths are aliases, including symlinks and hard links.

• Reliability

  • Refreshes stage results and promote them only after successful validation.
  • Staging files are cleaned up after failures.
  • Existing baselines are loaded and validated before comparisons begin.
  • Open matchups are distinguished from runs with no recorded games.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds SuiteReport helpers for failed matchups and recorded game totals. The AI gate validates aliases, stages refresh output, rejects failed or gameless refreshes, and atomically promotes accepted reports. The --games argument requires a positive integer.

Changes

AI gate baseline validation

Layer / File(s)	Summary
Suite report failure and game helpers crates/phase-ai/src/duel_suite/run.rs	SuiteReport now exposes failing_matchups() and recorded_games(). Tests cover failure filtering, failure reasons, open reports, gameless reports, and empty reports.
Baseline alias checks and staged output crates/phase-ai/src/bin/ai_gate.rs, crates/phase-ai/tests/refresh_baseline_cli.rs	The gate rejects path, symlink, and hard-link aliases. It preloads the baseline and writes refresh output to a sibling staging file. CLI tests cover malformed, missing, distinct, and aliased paths.
Refresh acceptance and argument validation crates/phase-ai/src/bin/ai_gate.rs, crates/phase-ai/tests/refresh_baseline_cli.rs	Refresh rejects failed matchups and runs with no recorded games. Accepted reports replace the baseline with an atomic rename. Staging files are removed after refusal or failure. --games rejects zero and non-integer values.

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

Sequence Diagram(s)

sequenceDiagram
  participant AIGate
  participant BaselineFile
  participant SuiteReport
  participant StagingFile
  AIGate->>BaselineFile: load baseline before execution
  AIGate->>SuiteReport: run suite with staging output
  SuiteReport-->>AIGate: return matchup statuses and recorded games
  AIGate->>StagingFile: remove output when refresh is refused
  AIGate->>BaselineFile: atomically rename accepted staging output

Loading

Possibly related PRs

• phase-rs/phase#6969: Both changes modify crates/phase-ai/src/duel_suite/run.rs, but this PR adds report helpers while that PR changes game-level parallel execution.
• phase-rs/phase#7026: Both changes modify crates/phase-ai/src/bin/ai_gate.rs and the duel-suite gate flow, but they address different behaviors.

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 identifies the primary change: refusing to refresh the duel-suite baseline from a failing run.

✨ 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.}

[matthewevans]

Request changes — --refresh-baseline must preserve the existing baseline until the replacement run is validated.

🔴 Blocker

crates/phase-ai/src/bin/ai_gate.rs:60-66 assigns the independently supplied --current-output to SuiteOptions before run_suite; crates/phase-ai/src/duel_suite/run.rs:553-556 then unconditionally writes the report to that path. The new rejection checks in ai_gate.rs:89-120 run only after this write. Because parse_args permits independent --baseline and --current-output values (ai_gate.rs:130-136), they can alias. A failing or gameless refresh can therefore truncate/replace the trusted baseline before the process exits 1, defeating the safeguard this PR adds.

Use a temporary current-report path and atomically replace the baseline only after the suite is accepted (or robustly reject path aliases before running). Add a process/file-state regression that invokes the real CLI with aliased paths and asserts both nonzero exit and unchanged baseline bytes; the new SuiteReport helper tests do not exercise the output-write/exit boundary.

✅ Clean

The failing_matchups / recorded_games predicates correctly distinguish failed runs, open matchups that played games, and genuinely gameless reports; the issue is the surrounding write ordering.

Recommendation: request changes; make baseline replacement transactional and cover the CLI/file-state boundary.

[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.

[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/bin/ai_gate.rs`:
- Around line 327-341: Update same_file to compare existing paths using
filesystem identity metadata, so hard-linked files are treated as the same while
retaining the canonicalized-parent fallback for nonexistent paths. Ensure
run_suite refuses the unsafe compare output selection before writing, preserving
baseline bytes. Add focused conditional CLI tests covering hard-linked
baseline/output refusal and atomic-refresh behavior.
- Around line 115-118: Update the refresh handling in ai_gate.rs around the
load_report match so only a missing baseline is treated as the first-refresh
path; for any other load_report error, stop before run_suite and return a
failure instead of falling through to None. Keep the existing baseline-read
logging only for the missing-baseline case, and add a focused CLI test covering
a malformed existing baseline to assert refresh is rejected and the original
baseline bytes remain unchanged.

🪄 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: 1848ea4e-ae44-454a-8280-bbc78039df45

📥 Commits

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

📒 Files selected for processing (3)

• crates/phase-ai/src/bin/ai_gate.rs
• crates/phase-ai/src/duel_suite/run.rs
• crates/phase-ai/tests/refresh_baseline_cli.rs

[matthewevans]

[HIGH] A malformed existing baseline is still silently eligible for replacement. Evidence: crates/phase-ai/src/bin/ai_gate.rs:111-118 catches every refresh load_report error, logs it, and returns None; execution then reaches run_suite (:125) and promotes the staged report with rename after an accepted run. Why it matters: a corrupt or unreadable trusted baseline is overwritten rather than preserved for diagnosis, so refresh can erase evidence and establish a new reference from an invalid prior state. Suggested fix: treat only ErrorKind::NotFound as first-baseline refresh; for every other error exit before run_suite, and add a real CLI/file-state test asserting malformed-baseline bytes are unchanged.

[HIGH] same_file misses hard-link aliases before the unconditional output write. Evidence: crates/phase-ai/src/bin/ai_gate.rs:327-341 compares canonical path strings, but canonicalize preserves distinct hard-link names; crates/phase-ai/src/duel_suite/run.rs:553-559 then opens options.output_path with File::create, which truncates that inode. The current CLI tests cover only identical spelling (crates/phase-ai/tests/refresh_baseline_cli.rs:50-118) and have no hard-link case. Why it matters: compare mode can still destroy the baseline through a distinct hard-link path before comparison; retaining the preloaded report in memory avoids a false comparison but not data loss. Suggested fix: compare identity metadata when both paths exist (retaining the canonical-parent fallback for nonexistent outputs), and add conditional hard-link CLI preservation coverage.

CodeRabbit independently reports both findings against this exact head. No direct build was run; CI remains in progress.

[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.

[matthewevans]

Request changes — the refresh transaction must reserve a collision-safe staging target before run_suite writes.

🔴 Blocker

crates/phase-ai/src/bin/ai_gate.rs:96-99 deterministically selects <baseline>.staging.json and passes it to run_suite at :139-145, without first rejecting or replacing an existing staging entry. crates/phase-ai/src/duel_suite/run.rs:954-959 then calls File::create on that path before the refresh guards at ai_gate.rs:147-202. A stale staging symlink or hard link to the trusted baseline therefore makes File::create truncate the baseline before the later refusal; the existing same_file check at ai_gate.rs:62 only compares --baseline with --current-output, not the internally-derived staging path. The current CLI tests cover aliases supplied as --current-output, but no test pre-creates <baseline>.staging.json as a symlink or hard link and asserts baseline-byte preservation.

The earlier alias and malformed-baseline fixes are present at this head. This remaining collision is independent: reading the old baseline first preserves comparison semantics, but cannot restore bytes truncated through the staging alias.

Recommendation: request changes; create/reserve a unique, collision-safe staging file in the baseline directory (or explicitly reject an existing staging alias before any suite write), and add a real CLI file-state regression for a pre-existing staging symlink/hard-link.

[lgray]

🤖 AI text below 🤖

Confirmed, reproduced, and fixed at dabb1060b9f9. You are right that this is independent of the earlier two: reading the old baseline first preserves comparison semantics and cannot restore truncated bytes, and same_file structurally cannot see this path because it compares --baseline against --current-output while the staging path is derived internally and is neither.

Reproduced before changing anything

With a symlink pre-placed at <baseline>.staging.json, a refresh that should have been accepted exited 0 and the baseline's bytes were gone. The regression test was written first and run against the unfixed binary to get that result, so the fix is answering a measured failure rather than a read of the code.

The fix reserves rather than checks

create_new on the staging path before run_suite. The distinction is the whole point: testing the path first and opening it second leaves the window between them, which is the same class of bug one layer down. O_CREAT|O_EXCL rejects any existing entry — regular file, hard link, live or dangling symlink — in one atomic step, and what the suite then truncates is a regular file this process just created with a link count of one.

I took "explicitly reject" over "unique name" from your two options, and the reason is your own earlier finding: a leftover staging file is an artefact someone later mistakes for a real baseline, so unique names would trade one hazard for an accumulating one. Refusing names the file and says what would have happened had it been followed. The cost is that an interrupted run must be cleaned up by hand once — acceptable for a local-only human command, and loud rather than silent.

Reserving created an obligation the old code did not have, and it is worth flagging because it is the kind of thing that turns one failure into two: every exit after the reservation must release it, or the refusal leaves behind a file that blocks the next refresh. The baseline-load and suite-run exits now release it, joining the refusal closure and the rename-failure path that already did. The gameless test asserts the consequence end-to-end rather than inferring it — after a refused run, the next refresh must still succeed.

Evidence

Four mutants, tree restored byte-identical after each and verified with diff -q. Each is killed by a different test, which is what separates four guards from one guard tested four times.

mutant	killed by
drop the reservation entirely (pre-fix)	a_pre_existing_staging_alias_cannot_truncate_the_baseline
create(true) instead of create_new(true)	same test — the reservation has to be exclusive, not merely a write
drop release_staging on the malformed-baseline exit	a_malformed_existing_baseline_is_not_replaced_by_a_refresh
refusal path stops deleting its staging file	a_gameless_refresh_is_refused_by_the_block_and_leaves_the_baseline_untouched

The regression covers both alias kinds, because they fail differently — File::create follows a symlink to its target, while a hard link is the target — so a reservation closing only one would leave the other live. Its premise asserts the alias actually resolves to the baseline, so an inert fixture cannot pass it silently.

10 CLI tests green, 2026 lib tests green, clippy -D warnings clean, fmt --check clean, at this head rebased onto 14b11cf5bf79.

One standing disclosure, unchanged

The ino-without-dev mutant still survives, for the reason given in the previous round: killing it needs two files on different filesystems whose inode numbers collide, and inode allocation is not controllable enough to construct that deterministically. dev is kept because dropping it can only produce a false alias report — a refused run, never a destroyed baseline. If you know a construction that is not flaky I will take it.

Three routes to this data loss have now been found by review and none by me: --current-output spelled identically, --current-output as a hard link, and the internal staging path. If there is a fourth I would rather hear it than ship it.

[lgray]

🤖 AI text below 🤖

On the red Decision-cost perf gate on this PR — it is inherited from main, measured rather than asserted.

This PR's gate reports 8 FAIL, 21 PASS. #7026 — an unrelated change to the same crate — reports the same tally with byte-identical values on all eight failing counters (attackable_player_sweeps 2640, crew_eligibility_scans 12027, layers_full_eval 15877, legend_rule_mode_gate_scans 27535, mana_aura_trigger_scans 53208, restriction_static_mode_gate_scans 155747, sba_battlefield_snapshot_builds 27462, state_clone_for_legality 19342). Two different diffs producing identical counters means the counters are invariant to both.

Independently, the only completed main-line perf measurement in the window — the scheduled run's drift monitor at main 5c8d12bf5fb1 (run 31000694717) — already clears the gate's base*1.05 + 64 band on all eight, at 1.64x to 4.54x. So the gate is red on main itself.

This PR's diff is a baseline-write transaction and its tests; it runs before and after the suite, never inside game simulation, which is where these counters are recorded. I am not proposing to fix it here — an earlier bisect of mine attributed the bulk to two merged commits (31c260f50 #6826, 8121fd1c6 #6842). Flagging it so the red check is not read as this PR's doing, and the full evidence is on #7026 rather than duplicated here.

[matthewevans]

Maintainer hold on dabb1060b9f91f036132cac27da1cb0b41cfd212. The prior requested-changes findings are fixed at this head; this is not a new implementation blocker.

All earlier shard checks are green, but the replacement Rust (fmt, clippy, test, coverage-gate) job is still running and the decision-cost perf gate is red. I verified the failure is not attributable to this diff: the PR changes only ai_gate, duel-suite report helpers, and its CLI tests; the failed gate runs ai-perf-gate, whose eight engine/perf counters are outside that execution path. Its log reports the same eight counter regressions (including layers_full_eval, restriction_static_mode_gate_scans, and state_clone_for_legality) rather than a failure in the changed baseline transaction.

Next step: resolve/rebaseline the mainline perf regression and let the current Rust job finish; then I will re-check this exact head, clear the stale review state with a fresh approval, verify labels, and enqueue if the full checklist remains clean.

[matthewevans]

Hold update for dabb1060b9f91f036132cac27da1cb0b41cfd212: the replacement Rust check is now green. The only remaining condition is the red decision-cost perf gate (8 engine/perf counter regressions), which is outside this ai_gate/duel-suite transaction diff and requires mainline perf-gate resolution or a justified baseline update. The earlier requested-changes reviews are fixed at this head; I will fresh-approve and enqueue only after that external gate condition is resolved and the live checklist is clean.

[lgray]

🤖 AI text below 🤖

This PR's Decision-cost perf gate failure was inherited from main, not introduced here — the same 8 counters failed on any branch built against the previous baseline (run 31024818922 logs 8 FAIL, 21 PASS with matching card data). #7044 has since updated the baseline on main (merge e8f051a4a3c1). A rebase onto current main plus a rerun should clear it.

[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.

[matthewevans]

[HIGH] The staging-path reservation is released before the destructive open. Evidence: crates/phase-ai/src/bin/ai_gate.rs:119-136 calls OpenOptions::create_new(true).open(path) and immediately drops that File; :138-140 passes only the pathname to the suite, and crates/phase-ai/src/duel_suite/run.rs:954-960 later reopens it with File::create(path). An attacker or concurrent process can unlink and replace the staging name with a symlink to the baseline after the reservation closes but before write_report, causing the later open to follow it and truncate the baseline. Why it matters: the PR claims the no-clobber reservation eliminates staging aliases, but this TOCTOU window preserves a baseline-destruction path. Suggested fix: make the no-clobber-created file (or a secure named-tempfile object) the actual report sink and keep its handle through report serialization/promotion; do not reopen the pathname. Add a deterministic replacement-between-reserve-and-write test.