From dc9f7f98f319f8c1b821ca4c2bd7fafbb6dd2d62 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 04:05:33 -0500 Subject: [PATCH 1/4] fix(ai): refuse to refresh the duel-suite baseline from a failing run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--refresh-baseline` ran the suite and wrote the result to the baseline path without ever looking at the result's own verdicts. Because the baseline is what every later run is compared against, one refresh from a red run blesses that failure permanently: the next run compares equal to the blessed report, the comparison reports no drift, and the gate exits 0 forever while the matchup is still broken. Nothing in the file inspected `SuiteStatus` on the refresh path. 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 that PR filed rather than smuggled in is whether a baseline may bless a failure at all. It may not. `SuiteReport::failing_matchups` reports the 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` — the call site would otherwise have to re-filter to say anything useful. Only `SuiteStatus::Fail` disqualifies a run. `Open` does not, and the distinction is load-bearing rather than incidental: `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, which is why there is a test whose only job is to fail against it. `Open` in fact has two producers — `grep`ed to confirm exactly two construction sites outside tests — and review caught the doc comment claiming one: `classify` returns `Open` for any matchup with zero games before it ever inspects `Expected`. That exposed a second way to write an unfit baseline, from the opposite side — a run that measured nothing. Comparison pairs by seed, so a gameless baseline scores zero on every axis forever and the drift signal dies as quietly as a blessed-red one. `SuiteReport::recorded_games` disqualifies it, and deliberately counts games rather than testing for all-`Open`: 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`", which is true here but false on #7026, cited two paragraphs above, where the paired arm gains status tiers. Zero games is what makes a baseline inert; all-`Open` is not. The routes are enumerated without claiming the enumeration is complete, because an earlier draft said "two routes" and review found a third. `--games 0` is rejected at parse time, since the existing error string already promised a positive integer and `usize` alone does not. A `--suite-filter` selecting no matchups is caught on the report, because `run_suite` does not reject an empty selection. `failed_result` yields an empty `games` vector alongside `SuiteStatus::Fail`, which the failure guard reports first because it names the actual setup error. And `SuiteOptions::new` does not validate `games_per_matchup`, so a library caller can build a zero-game run without touching the CLI at all. No override flag. The escape hatch already exists one layer up and is the correct layer, so adding 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 is nil, measured rather than assumed: `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 motivated it rather than to invented data: `the_recorded_failing_run_is_disqualified_as_a_baseline` transcribes the recorded gate run (`.ab/noC-1.json`, the A+B+D leg of #6969) — 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. Evidence. Nine mutants, tree restored byte-identical after each. Kill counts are transcribed from the runs, not summarised — an earlier draft of this paragraph asserted three of them from memory and review measured all three wrong. On `failing_matchups`: dropping the filter is killed by 5 tests; returning nothing by 1 (`the_recorded_failing_run_is_disqualified_as_a_baseline`); and the plausible `!= SuiteStatus::Pass` by 3 — every test that asserts an `Open` matchup is not a failure. That last one had been written up as killing only `an_open_matchup_is_not_a_failure` "surviving the other two entirely", which was simply false: the gameless and all-`Open` fixtures kill it too. The named test is still the one that states the intent, but it is not the only thing standing between that mutant and green, and claiming otherwise oversold a single test. On `recorded_games`, with the constant stated because it decides the answer: replacing the body with `0` is killed by 1 test, with `1` by 3, with `2` by 3. The earlier draft said "making it constant fails all three of its tests" — true only for `1`, and false for `0`, which is precisely the value the guard tests. Counting non-`Open` matchups instead of games is killed by 1, the fixture written for that conflation. Two further mutants were found SURVIVING and are the reason `an_all_open_run_that_played_games_is_still_a_usable_baseline` now carries an uneven fixture. Counting matchups-with-games, and summing `games.len().min(1)`, both returned the right answer for every fixture in the suite, because each matchup carried exactly one game — so the total always equalled the matchup count and "sum of games" was never distinguished from "number of matchups that played". Against the real committed baseline (3 matchups x 10 games) those mutants return 3 where the contract says 30. The fixture now plays two games in one matchup and one in the other, and asserts 3; both mutants die. One further mutation is reported precisely rather than counted, because review caught an earlier draft overstating it: 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 to `&str`, but it is not predicate coverage and is not counted as such. Because no unit test can reach a binary's `main`, the wiring was proven end to end, two-sided, against a temporary baseline — the committed baseline was never written. Positive control: a clean run still refreshes (red-mirror `PASS`, exit 0, baseline written at sha256 `42701dbeac46b015…`), so the guard does not false-positive. True positive: with `classify`'s mirror arm forced to Fail and the binary rebuilt, the same command printed `refusing to refresh …: 1 matchup(s) failed their own suite check` followed by the matchup and its reason, exited 1, and left that baseline at sha256 `42701dbeac46b015…` — byte-identical, the overwrite prevented. Both new refusals were then exercised through the CLI alone, with no source mutation, which is stronger evidence than the forced-Fail arm: `--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 as the positive control. The binary under test was verified to contain both refusal strings first, so no arm can pass against a stale build. 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 — a `--suite-filter` matching nothing — has no failing matchups to report. One surface is deliberately left uncovered and is stated rather than implied: no test executes the binary, so the refusal block itself — as opposed to the two predicates behind it — can be deleted or inverted with the whole suite green. That also means the ordering above is argued from the code path rather than pinned by a test; producing a broken deck-payload tree to exercise it end to end was judged not worth the fixture. The end-to-end runs above were performed against this tree but are not committed as tests. A process-spawning integration test would have to run a real suite to reach the guard, which is minutes of CI for a local-only human command, so the trade is made knowingly rather than overlooked. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 58 ++++++++- crates/phase-ai/src/duel_suite/run.rs | 167 ++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 3 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index 8099c203fe..b8774ff83a 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -72,6 +72,53 @@ fn main() { }; if args.refresh_baseline { + // A baseline is what every later run is judged against, so refreshing from a run + // that failed its own `Expected` check blesses that failure permanently: the next + // run compares equal to it and exits 0 forever, and the gate goes quiet about a + // matchup that is still broken. Refuse. A matchup that genuinely has no verdict + // yet says so with `Expected::Open` in the suite definition — that is the place + // to express it, not a red baseline. + // + // ORDER MATTERS, and this one is strictly better on every input. The two conditions + // are not exclusive: `failed_result` builds a matchup with an empty `games` vector + // AND `SuiteStatus::Fail`, so a run whose deck payloads all failed to load satisfies + // both. Reporting the failures names each matchup and its `setup error: …`; reporting + // gamelessness first would replace that with a sentence about seeds. Nothing is lost + // by checking failures first, because a run that is merely gameless — a + // `--suite-filter` matching nothing — has no failing matchups to report. + let failing: Vec<_> = current.failing_matchups().collect(); + if !failing.is_empty() { + eprintln!( + "refusing to refresh {}: {} matchup(s) failed their own suite check", + args.baseline.display(), + failing.len() + ); + for result in failing { + eprintln!( + " {}: {}", + result.matchup_id, + result + .fail_reason + .as_deref() + .unwrap_or("no reason recorded") + ); + } + eprintln!( + "fix the regression, or declare the matchup `Expected::Open` if it has no verdict yet" + ); + std::process::exit(1); + } + // A run that measured nothing is unfit for the same reason a red one is, reached from + // the other side: comparison pairs by seed, so a gameless baseline scores zero on the + // outcome axes forever and the drift signal dies quietly. Reached by a `--suite-filter` + // that selects no matchups; `--games 0` is refused earlier, at parse time. + if current.recorded_games() == 0 { + eprintln!( + "refusing to refresh {}: the run recorded no games, so every later comparison would score zero", + args.baseline.display() + ); + std::process::exit(1); + } if args.baseline.exists() { match load_report(&args.baseline) .and_then(|baseline| compare(&baseline, ¤t, &CompareOptions)) @@ -137,9 +184,14 @@ fn parse_args() -> Result { current_output = next_path(&mut iter, "--current-output")?; } "--games" => { - games = next_value(&mut iter, "--games")? - .parse() - .map_err(|_| "--games must be a positive integer".to_string())?; + // `usize` alone accepts 0, which the error string already promised it would + // not. A zero-game run classifies every matchup `Open` and produces a + // baseline that can never detect drift, so reject it here rather than + // burning a whole suite run to refuse it later. + games = match next_value(&mut iter, "--games")?.parse() { + Ok(0) | Err(_) => return Err("--games must be a positive integer".to_string()), + Ok(value) => value, + }; } "--seed" => { seed = next_value(&mut iter, "--seed")? diff --git a/crates/phase-ai/src/duel_suite/run.rs b/crates/phase-ai/src/duel_suite/run.rs index 43b66fbf88..c62ebdc9e3 100644 --- a/crates/phase-ai/src/duel_suite/run.rs +++ b/crates/phase-ai/src/duel_suite/run.rs @@ -146,6 +146,51 @@ impl SuiteReport { .collect(), } } + + /// Matchups that failed their own `Expected` check, judged without reference to any + /// baseline. + /// + /// This is a different question from `CompareReport::any_fail`, which asks "did this + /// change make things worse than the baseline". This one asks "is this run fit to + /// *become* the baseline" — and only `SuiteStatus::Fail` disqualifies it. + /// + /// `SuiteStatus::Open` does not, and it has two producers, not one: `Expected::Open` + /// is how a matchup declares it has no verdict yet, and `classify` also returns `Open` + /// for any matchup with zero games before it ever inspects `Expected`. Neither is a + /// failure, so anything keyed on `!= Pass` would conflate a declared no-verdict with a + /// regression. The zero-game case is disqualifying for a different reason and is caught + /// by `recorded_games`, not here. + pub fn failing_matchups(&self) -> impl Iterator { + self.results + .iter() + .filter(|result| result.status == SuiteStatus::Fail) + } + + /// Games actually recorded across every matchup. + /// + /// A report with none is unfit to become a baseline whatever its statuses say: + /// comparison pairs by seed, so a baseline holding no games makes every later + /// comparison score zero on every axis and the drift signal dies silently — the same + /// false-green this guard exists to prevent, arrived at from the other side. + /// + /// Deliberately counts games rather than checking for all-`Open`: a suite whose + /// matchups are all declared `Expected::Open` still records real games, and such a + /// report *is* a usable baseline, because the paired-comparison arm decides on `games`, + /// not on `status` — an all-`Open` pair therefore still detects outcome drift. (Stated + /// as "decides on games" rather than "never reads status": the paired arm gained status + /// tiers in #7026, so the stronger wording would have been false the day that merged.) + /// Zero games is the property that makes a baseline inert; all-`Open` is not. + /// + /// Not an exhaustive list of routes, so it does not claim to be one: `--games 0` is + /// refused at parse time, a `--suite-filter` matching no matchups lands here, and + /// `failed_result` yields an empty `games` vector alongside `SuiteStatus::Fail`, which + /// `failing_matchups` reports first because it names the actual setup error. And this is + /// a `pub` method, so its audience includes library callers: `SuiteOptions::new` does not + /// validate `games_per_matchup`, so a caller can construct a zero-game run without going + /// through the CLI at all. + pub fn recorded_games(&self) -> usize { + self.results.iter().map(|result| result.games.len()).sum() + } } /// Controls decision-trace attribution capture during a suite run. When set @@ -1082,6 +1127,128 @@ mod tests { } } + /// Reuses `report_with_timing`'s matchup as the field template so these tests state + /// only the axes they exercise: each matchup's status and reason. + fn report_with_statuses(statuses: &[(&str, SuiteStatus, Option<&str>)]) -> SuiteReport { + let mut report = report_with_timing(1, 100); + let template = report.results[0].clone(); + report.results = statuses + .iter() + .map(|(id, status, reason)| MatchupResult { + matchup_id: (*id).to_string(), + status: *status, + fail_reason: reason.map(str::to_string), + ..template.clone() + }) + .collect(); + report + } + + #[test] + fn a_clean_run_has_no_failing_matchups() { + let report = report_with_statuses(&[ + ("red-mirror", SuiteStatus::Pass, None), + ("affinity-mirror", SuiteStatus::Pass, None), + ]); + + assert_eq!(report.failing_matchups().count(), 0); + } + + /// Transcribed from the recorded gate run that motivated this guard (`.ab/noC-1.json`, + /// the A+B+D leg of #6969): the statuses and the verbatim `fail_reason` are that run's, + /// not invented. Refreshing the baseline from this exact report is what would have + /// blessed a broken matchup permanently. The artifact is untracked, so it is + /// transcribed rather than loaded — a test that read the file would fail in CI. + #[test] + fn the_recorded_failing_run_is_disqualified_as_a_baseline() { + let report = report_with_statuses(&[ + ("red-mirror", SuiteStatus::Pass, None), + ("affinity-mirror", SuiteStatus::Pass, None), + ( + "enchantress-mirror", + SuiteStatus::Fail, + Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50"), + ), + ]); + + let failing: Vec<_> = report.failing_matchups().collect(); + assert_eq!(failing.len(), 1, "the one Fail, not every matchup"); + assert_eq!(failing[0].matchup_id, "enchantress-mirror"); + // The reason travels with the matchup: the refusal is only actionable if it can say + // *why* the run is unfit, not merely that it is. + assert_eq!( + failing[0].fail_reason.as_deref(), + Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50") + ); + } + + /// The discriminating case for `recorded_games`: a suite whose matchups are all + /// declared `Expected::Open` still played real games, and that report IS a usable + /// baseline, because seed-paired drift detection never consults `status`. An + /// implementation that disqualified all-`Open` reports instead of gameless ones would + /// pass every other test here and fail this one. + #[test] + fn an_all_open_run_that_played_games_is_still_a_usable_baseline() { + let mut report = report_with_statuses(&[ + ("experimental-a", SuiteStatus::Open, None), + ("experimental-b", SuiteStatus::Open, None), + ]); + // Uneven on purpose. With one game each, the total (2) equals the MATCHUP count, so + // "sum of games" and "number of matchups that played" are indistinguishable — two + // mutants with that wrong contract survived the earlier version of this test. Three + // games across two matchups separates them. + let extra = report.results[0].games[0].clone(); + report.results[0].games.push(GameResult { + seed: extra.seed + 1, + ..extra + }); + + assert_eq!(report.failing_matchups().count(), 0); + assert_eq!( + report.recorded_games(), + 3, + "two games in the first matchup plus one in the second — a SUM, not a matchup count" + ); + } + + #[test] + fn a_gameless_run_records_nothing_however_many_matchups_it_has() { + let mut report = report_with_statuses(&[ + ("red-mirror", SuiteStatus::Open, None), + ("affinity-mirror", SuiteStatus::Open, None), + ]); + // What `--games 0` produces: matchups exist, none of them played anything. + for result in &mut report.results { + result.games.clear(); + } + + assert_eq!(report.recorded_games(), 0); + // And it is NOT a failure — the two disqualifiers are independent, so a guard that + // conflated them would let one of the two holes back open. + assert_eq!(report.failing_matchups().count(), 0); + } + + #[test] + fn a_run_that_selected_no_matchups_records_nothing() { + // What a `--suite-filter` matching nothing produces: no matchups at all. + let report = report_with_statuses(&[]); + + assert_eq!(report.recorded_games(), 0); + } + + #[test] + fn an_open_matchup_is_not_a_failure() { + // `Expected::Open` classifies as `SuiteStatus::Open`: a matchup that has no verdict + // yet, which must not block a refresh. This is the test that dies if the filter is + // ever written as the plausible `!= SuiteStatus::Pass` — the other two survive it. + let report = report_with_statuses(&[ + ("red-mirror", SuiteStatus::Pass, None), + ("experimental-mirror", SuiteStatus::Open, None), + ]); + + assert_eq!(report.failing_matchups().count(), 0); + } + #[test] fn deterministic_core_excludes_wall_clock_fields() { let first = report_with_timing(1, 100); From 69cd170fa047f6d501a47f4f6d2dbfe957391d02 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 08:51:42 -0500 Subject: [PATCH 2/4] fix(ai): never let a rejected run modify the baseline it was judged against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the guard this branch adds could fire after the damage it exists to prevent. `run_suite` writes its report to `options.output_path` before any guard runs, and `--baseline` and `--current-output` are independent flags, so pointing them at the same file truncated the baseline and only then printed "refusing to refresh". Reproduced on the built binary at the reviewed head: baseline in at 116 bytes, sha256 985133e3…; out at 250 bytes, sha256 bc9649fd…; exit 1. A safeguard whose subject is already destroyed is not a safeguard. Verifying it surfaced a second instance on the compare path, and that one is worse because it is silent. There the same aliasing makes `load_report(&args.baseline)` read back the report the suite has just written over it, so the gate compares the run to ITSELF and reports no drift. Measured against a baseline recording p0 at 100% and a run that scored 0%: `| red-mirror | … | 0% | 0% | 0 | 0 | — | PASS |`, exit 0. An honest comparison of those two inputs is a win-rate collapse. This is the false-green class the whole branch exists to close, reached by destroying the evidence rather than by misreading it. Three layers, each doing a job the others cannot. The root cause on the compare path is WHEN the baseline is read, not which paths were passed, so the baseline is now loaded before the suite runs and held in memory. That is immune to aliasing by construction — including aliases no path comparison can see. A hard link resolves to a different name and the same inode, so canonicalising calls the two paths distinct while a write to either truncates both; ordering does not care how the alias was built. Measured through a hard link, which defeats the argument check below: `| red-mirror | … | 100% | 0% | 1 | 0 | 0.2500 | WARN |`. The pre-fix binary called the same inputs `0% | 0%`, PASS. It also fails a missing or corrupt baseline in a second rather than after a full suite run. The refresh path is made transactional. The suite writes to `.staging.json` and the baseline is replaced by renaming that file only once every guard has passed; every refusal removes it. The staging file is a SIBLING because `rename` is atomic only within one filesystem — a staging path in `/tmp` could land on another mount and silently degrade to copy-then-truncate, reintroducing the half-written baseline it exists to prevent. `write_report` is deleted rather than left beside this: it was the last writer that could produce a baseline outside the transaction. `--current-output` is unused on the refresh path now, since the refreshed baseline IS the run's report; no workflow passes `--refresh-baseline`, so nothing in CI depends on the old behaviour. Aliased paths are still refused outright, before the card database and before a single game. With the two layers above, correctness no longer depends on it — but aliasing still DESTROYS the user's baseline file as a side effect of a run that had no business writing there, and losing the file quietly is its own defect. Evidence, and one piece of it is a test that had to be fixed before it was worth having. The first version of `tests/refresh_baseline_cli.rs` asserted exactly what review asked for — non-zero exit and unchanged baseline bytes — and mutation showed it was vacuous: 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, so the pair was satisfied for a reason unrelated to the hazard. Adding an assertion that the refusal actually names the aliasing makes both mutants fail two tests each; a third mutant, `same_file` always true, is caught only by the control arm that proves distinct paths still run. The module comment states the limitation rather than hiding it. 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, because reaching them needs a card database and a real suite run. The five runs are recorded in the PR. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 226 +++++++++++++++--- crates/phase-ai/tests/refresh_baseline_cli.rs | 149 ++++++++++++ 2 files changed, 348 insertions(+), 27 deletions(-) create mode 100644 crates/phase-ai/tests/refresh_baseline_cli.rs diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index b8774ff83a..80f210d707 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -4,15 +4,13 @@ #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; -use std::fs::File; -use std::io::BufWriter; use std::path::{Path, PathBuf}; use std::process::Command; use engine::database::CardDatabase; use phase_ai::config::AiDifficulty; use phase_ai::duel_suite::compare::{compare, load_report, print_markdown, CompareOptions}; -use phase_ai::duel_suite::run::{run_suite, SuiteOptions, SuiteReport}; +use phase_ai::duel_suite::run::{run_suite, SuiteOptions}; const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/suite-baseline.json"; const DEFAULT_CURRENT: &str = "target/ai-gate-current.json"; @@ -45,6 +43,28 @@ fn main() { } }; + // Refuse before ANY work — before the card database, before a single game — when the + // suite's output path and the baseline are the same file. Review found this defeats the + // refusal this PR adds: `run_suite` writes its report to `options.output_path` before any + // guard runs, so an aliased pair truncated the baseline and only THEN printed "refusing to + // refresh". Measured on the real binary at the reviewed head: 116 bytes in, 250 bytes out, + // different sha256, exit 1. + // + // The compare path is worse and is why this check is not confined to `--refresh-baseline`. + // There the same aliasing makes `load_report(&args.baseline)` read back the run that just + // overwrote it, so the gate compares the run to ITSELF and exits 0. Measured: a baseline + // recording p0 at 100% against a run that scored 0% printed `0% | 0%`, zero flips, `0 FAIL`, + // exit 0. A gate that reports no drift because it destroyed its own reference is the exact + // false-green this branch exists to close, and it is silent where the refresh case is loud. + if same_file(&args.baseline, &args.current_output) { + eprintln!( + "--baseline and --current-output are the same file ({}); the suite would overwrite \ + the baseline before it could be compared or validated", + args.baseline.display() + ); + std::process::exit(2); + } + let db_path = args.data_root.join("card-data.json"); let db = match CardDatabase::from_export(&db_path) { Ok(db) => db, @@ -58,11 +78,50 @@ fn main() { }; let mut options = SuiteOptions::new(args.difficulty, args.games, args.seed); - options.output_path = args.current_output.clone(); + // On a refresh, the suite writes to a staging file BESIDE the baseline rather than to + // `--current-output`, and the baseline is replaced by renaming that file only after every + // guard has passed. The alias check above already refuses the one path that reached this + // bug, but a check on argument values cannot be the whole answer: the property that has to + // hold is that a rejected run never modifies the baseline, and that is a property of the + // write ordering, not of the flags. Staging + rename gives it unconditionally. + // + // Beside the baseline because `rename` is only atomic within one filesystem; a staging file + // in `/tmp` could land on a different mount and silently degrade to copy-then-truncate. + // `--current-output` is therefore unused on the refresh path — the refreshed baseline IS + // the run's report. No workflow passes `--refresh-baseline`, so nothing in CI depends on + // the old behaviour. + let staging = args.refresh_baseline.then(|| staging_path(&args.baseline)); + options.output_path = staging + .clone() + .unwrap_or_else(|| args.current_output.clone()); options.filter = args.suite_filter.clone(); options.git_sha = command_output("git", &["rev-parse", "--short=12", "HEAD"]); options.card_data_hash = command_output("git", &["hash-object", path_str(&db_path)]); + // Read the baseline BEFORE the suite runs, and keep it in memory. + // + // This is the root-cause half of the aliasing fix, and it is what the argument check above + // cannot give: reading first makes the comparison independent of ANYTHING the suite writes, + // including aliases this process cannot detect from paths at all — a hard link resolves to + // a different name and the same inode, so `same_file` calls it distinct while a write to + // one truncates the other. Order does not care how the alias was constructed. + // + // It also fails a missing or corrupt baseline in a second instead of after a full suite run, + // which is the difference between a typo costing nothing and costing a hundred games. + let baseline = match load_report(&args.baseline) { + Ok(report) => Some(report), + // On a refresh there may be no baseline yet, and that is the normal first-run case. + Err(_) if args.refresh_baseline && !args.baseline.exists() => None, + Err(err) if args.refresh_baseline => { + eprintln!("could not read the old baseline for comparison: {err}"); + None + } + Err(err) => { + eprintln!("failed to load baseline {}: {err}", args.baseline.display()); + std::process::exit(2); + } + }; + let current = match run_suite(&db, &options) { Ok(report) => report, Err(err) => { @@ -86,6 +145,16 @@ fn main() { // gamelessness first would replace that with a sentence about seeds. Nothing is lost // by checking failures first, because a run that is merely gameless — a // `--suite-filter` matching nothing — has no failing matchups to report. + let staging = staging.expect("staging path is set whenever refresh_baseline is"); + // Every exit below leaves the staging file behind otherwise, and a stale + // `*.staging.json` next to a baseline is exactly the kind of artefact someone later + // mistakes for a real one. + let refuse = |message: &str| -> ! { + let _ = std::fs::remove_file(&staging); + eprintln!("{message}"); + std::process::exit(1); + }; + let failing: Vec<_> = current.failing_matchups().collect(); if !failing.is_empty() { eprintln!( @@ -103,31 +172,32 @@ fn main() { .unwrap_or("no reason recorded") ); } - eprintln!( - "fix the regression, or declare the matchup `Expected::Open` if it has no verdict yet" + refuse( + "fix the regression, or declare the matchup `Expected::Open` if it has no verdict yet", ); - std::process::exit(1); } // A run that measured nothing is unfit for the same reason a red one is, reached from // the other side: comparison pairs by seed, so a gameless baseline scores zero on the // outcome axes forever and the drift signal dies quietly. Reached by a `--suite-filter` // that selects no matchups; `--games 0` is refused earlier, at parse time. if current.recorded_games() == 0 { - eprintln!( + refuse(&format!( "refusing to refresh {}: the run recorded no games, so every later comparison would score zero", args.baseline.display() - ); - std::process::exit(1); + )); } - if args.baseline.exists() { - match load_report(&args.baseline) - .and_then(|baseline| compare(&baseline, ¤t, &CompareOptions)) - { + // Informational old-vs-new diff, from the copy read before the suite ran. + if let Some(old) = &baseline { + match compare(old, ¤t, &CompareOptions) { Ok(report) => print_markdown(&report), Err(err) => eprintln!("could not compare old baseline: {err}"), } } - if let Err(err) = write_report(¤t, &args.baseline) { + // The run is accepted: promote the staging file. `rename` replaces the baseline in one + // step, so a reader never observes a half-written baseline and a failure here leaves the + // previous one intact. + if let Err(err) = std::fs::rename(&staging, &args.baseline) { + let _ = std::fs::remove_file(&staging); eprintln!( "failed to write baseline {}: {err}", args.baseline.display() @@ -138,13 +208,8 @@ fn main() { return; } - let baseline = match load_report(&args.baseline) { - Ok(report) => report, - Err(err) => { - eprintln!("failed to load baseline {}: {err}", args.baseline.display()); - std::process::exit(2); - } - }; + let baseline = + baseline.expect("the non-refresh path exits above when the baseline is unreadable"); let report = match compare(&baseline, ¤t, &CompareOptions) { Ok(report) => report, @@ -248,12 +313,47 @@ fn path_str(path: &Path) -> &str { path.to_str().unwrap_or("") } -fn write_report(report: &SuiteReport, path: &Path) -> Result<(), std::io::Error> { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent)?; +/// Whether two paths designate the same file, including through symlinks and `..`. +/// +/// `canonicalize` is the authority when a path exists, because it is the only thing that +/// resolves symlinks — a plain string or `absolute()` comparison calls `baselines/x.json` and a +/// symlink pointing at it different files, and then the write lands on the baseline anyway. The +/// current-output side usually does NOT exist yet, so it falls back to canonicalizing the parent +/// directory (which has to be real for the write to land) and rejoining the file name. +/// +/// Returns false when neither resolution is possible, which is the right default: an +/// unresolvable path cannot be shown to alias, and refusing to run on a path we cannot inspect +/// would break invocations that are fine. +fn same_file(a: &Path, b: &Path) -> bool { + fn resolved(path: &Path) -> Option { + if let Ok(canonical) = std::fs::canonicalize(path) { + return Some(canonical); + } + let parent = match path.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + _ => Path::new("."), + }; + Some(std::fs::canonicalize(parent).ok()?.join(path.file_name()?)) + } + match (resolved(a), resolved(b)) { + (Some(x), Some(y)) => x == y, + _ => false, } - let file = File::create(path)?; - serde_json::to_writer_pretty(BufWriter::new(file), report).map_err(std::io::Error::other) +} + +/// Where a refresh run stages its report before it earns the right to be the baseline. +/// +/// Beside the baseline, so the later `rename` is a same-filesystem atomic replace. +fn staging_path(baseline: &Path) -> PathBuf { + let name = baseline + .file_name() + .map(|n| { + let mut s = n.to_os_string(); + s.push(".staging.json"); + s + }) + .unwrap_or_else(|| "baseline.staging.json".into()); + baseline.with_file_name(name) } fn print_usage() { @@ -262,3 +362,75 @@ fn print_usage() { eprintln!(" [--suite-filter STR[,STR...] | --full-suite]"); eprintln!(" [--data-root DIR] [--baseline PATH] [--current-output PATH]"); } + +#[cfg(test)] +mod tests { + use super::*; + + /// The staging file must be a SIBLING of the baseline. `rename` is only atomic within one + /// filesystem, so a staging path that drifted to `/tmp` (or anywhere else the baseline is + /// not) would silently degrade the final replace into copy-then-truncate — reintroducing the + /// half-written baseline this staging exists to prevent, and doing it invisibly. + /// + /// Asserted as "same parent, different file name", which is the property atomicity needs, + /// rather than as a literal string, which would pin a spelling nobody depends on. + #[test] + fn the_staging_file_is_a_sibling_of_the_baseline_it_will_replace() { + for baseline in [ + "crates/phase-ai/baselines/suite-baseline.json", + "/abs/path/base.json", + "relative.json", + "/weird/no-extension", + ] { + let baseline = Path::new(baseline); + let staging = staging_path(baseline); + assert_eq!( + staging.parent(), + baseline.parent(), + "staging must sit beside {}, got {}", + baseline.display(), + staging.display() + ); + assert_ne!( + staging, + baseline, + "staging must not BE the baseline: {}", + baseline.display() + ); + } + } + + /// A path and a symlink to it are the same file, and a string comparison cannot see that. + /// This is the case that makes `same_file` more than `a == b`: the write lands on the + /// baseline's bytes either way. + #[test] + fn same_file_sees_through_a_symlink() { + let dir = std::env::temp_dir().join(format!("phase-same-file-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let real = dir.join("baseline.json"); + std::fs::write(&real, "{}").expect("write"); + let link = dir.join("link.json"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + #[cfg(unix)] + { + assert!(same_file(&real, &link), "symlinked alias must be detected"); + // PREMISE: the two paths really are textually different, so the assertion above is + // about resolution rather than about a trivially equal comparison. + assert_ne!(real, link); + } + + // Control: two genuinely distinct files must not be called aliases, or the guard would + // refuse every legitimate invocation. + let other = dir.join("current.json"); + std::fs::write(&other, "{}").expect("write"); + assert!(!same_file(&real, &other)); + // And a path that does not exist yet still resolves through its parent, which is the + // normal case for `--current-output` on a clean tree. + assert!(!same_file(&real, &dir.join("not-created-yet.json"))); + assert!(same_file(&real, &dir.join("baseline.json"))); + + std::fs::remove_dir_all(&dir).ok(); + } +} diff --git a/crates/phase-ai/tests/refresh_baseline_cli.rs b/crates/phase-ai/tests/refresh_baseline_cli.rs new file mode 100644 index 0000000000..82c906765b --- /dev/null +++ b/crates/phase-ai/tests/refresh_baseline_cli.rs @@ -0,0 +1,149 @@ +//! The baseline's file-state contract, asserted against the real binary. +//! +//! Review found that the refusal this crate's `--refresh-baseline` guard adds could fire *after* +//! the damage it exists to prevent: `run_suite` writes its report to the suite's output path +//! before any guard runs, so pointing `--current-output` at the baseline truncated the baseline +//! and only then printed "refusing to refresh". Measured on the pre-fix binary: 116 bytes in, +//! 250 out, different sha256, exit 1. A guard whose subject is already destroyed is not a guard. +//! +//! Every assertion here is on the pair (exit status, baseline bytes), because either alone is +//! satisfied by a broken implementation: exiting non-zero while having clobbered the file is the +//! bug itself, and leaving the file alone while exiting 0 would bless the run. +//! +//! These run the real CLI and deliberately never reach the card database — the argument check +//! under test rejects before any of that — so they cost milliseconds and add no card-data load +//! for `scripts/check-test-card-data-load.sh` to object to. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// Not a valid suite report, and that is deliberate: these tests must fail if the binary gets +/// far enough to parse it, because getting that far means it also ran the suite. +const SENTINEL: &str = r#"{"this":"is the trusted baseline, byte for byte"}"#; + +fn scratch(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("phase-refresh-cli-{tag}-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create scratch dir"); + dir +} + +fn seed_baseline(dir: &Path) -> PathBuf { + let path = dir.join("suite-baseline.json"); + std::fs::write(&path, SENTINEL).expect("seed baseline"); + path +} + +fn run(args: &[&str]) -> (Option, String) { + let out = Command::new(env!("CARGO_BIN_EXE_ai-gate")) + .args(args) + .output() + .expect("spawn ai-gate"); + ( + out.status.code(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) +} + +/// The recorded defect, in the direction that destroyed data: a refresh whose output path is the +/// baseline must be refused with the baseline still byte-identical. +#[test] +fn an_aliased_refresh_is_refused_before_the_baseline_can_be_overwritten() { + let dir = scratch("refresh"); + let baseline = seed_baseline(&dir); + let path = baseline.display().to_string(); + + let (code, stderr) = run(&[ + "--refresh-baseline", + "--baseline", + &path, + "--current-output", + &path, + "--suite-filter", + "no-such-matchup", + ]); + + assert_ne!( + code, + Some(0), + "an aliased refresh must fail; stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + SENTINEL, + "the baseline was modified by a run that was refused" + ); + // LOAD-BEARING — see the module note. Measured: with the alias check deleted, or with + // `same_file` forced to false, the two assertions above still pass, because the binary then + // fails at the card database with the baseline equally untouched. This assertion is the only + // one of the three that dies to those mutants. + assert!( + stderr.contains("same file"), + "the refusal must be about aliasing, not something later; stderr:\n{stderr}" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// The same aliasing on the COMPARE path, which is the quieter half: there it made the gate read +/// back the run that had just overwritten the baseline and compare it to itself, reporting no +/// drift and exiting 0. Measured on the pre-fix binary with a baseline recording p0 at 100% +/// against a run that scored 0%: `0% | 0%`, zero flips, `0 FAIL`, exit 0. +#[test] +fn an_aliased_comparison_is_refused_before_the_baseline_can_be_overwritten() { + let dir = scratch("compare"); + let baseline = seed_baseline(&dir); + let path = baseline.display().to_string(); + + let (code, stderr) = run(&["--baseline", &path, "--current-output", &path]); + + assert_ne!( + code, + Some(0), + "an aliased comparison must fail; stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + SENTINEL, + "the baseline was modified by a run that was refused" + ); + // LOAD-BEARING — see the module note. Measured: with the alias check deleted, or with + // `same_file` forced to false, the two assertions above still pass, because the binary then + // fails at the card database with the baseline equally untouched. This assertion is the only + // one of the three that dies to those mutants. + assert!( + stderr.contains("same file"), + "the refusal must be about aliasing, not something later; stderr:\n{stderr}" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// Control arm. Every assertion above is satisfied by a binary that refuses everything, which +/// would break the gate far worse than the bug being fixed. Distinct paths must get past the +/// argument check — proven by the failure being about something LATER in the run (the card +/// database or the suite), never about aliasing. +#[test] +fn distinct_paths_are_not_treated_as_aliases() { + let dir = scratch("distinct"); + let baseline = seed_baseline(&dir); + let current = dir.join("current.json"); + + let (_code, stderr) = run(&[ + "--baseline", + &baseline.display().to_string(), + "--current-output", + ¤t.display().to_string(), + "--data-root", + &dir.join("no-such-data-root").display().to_string(), + ]); + + assert!( + !stderr.contains("same file"), + "distinct paths must not be rejected as aliases; stderr:\n{stderr}" + ); + // PREMISE: the run really did proceed past the argument check, so the assertion above is + // about aliasing rather than about the process dying even earlier for some other reason. + assert!( + stderr.contains("failed to load card database"), + "expected the run to proceed to the card database; stderr:\n{stderr}" + ); + std::fs::remove_dir_all(&dir).ok(); +} From 8edf5411e3ba5e488a8e900d08f17b80b6321d9b Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 10:35:10 -0500 Subject: [PATCH 3/4] fix(ai): refuse hard-linked and unreadable baselines before the suite runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found two ways the baseline could still be destroyed by a run that had no right to replace it. Both are closed here, and both get CLI coverage that executes the real binary. A hard link is one inode under two names. `canonicalize` faithfully preserves both names, so the previous path-string check called them distinct, let the run proceed, and `write_report` truncated the shared inode with `File::create`. Compare mode is the quiet half: it then read back the report the run had just written over the baseline and reported no drift. `same_file` now asks the filesystem for identity — `(dev, ino)` when both paths exist — and keeps the canonicalized-parent fallback for the usual case where the output does not exist yet. Identity subsumes the path comparison where it applies; the fallback still resolves symlinks and `..` where it does not. The second is narrower and worse. The refresh path caught EVERY `load_report` error, logged it, and continued — so a baseline that was corrupt, truncated or unreadable was overwritten by the staged report rather than kept for diagnosis, and the new reference was established from a prior state nobody had examined. "I could not read it" is not evidence that it was worthless. Only `ErrorKind::NotFound` now proceeds, which is the genuine first-refresh case. Matched on the error kind rather than `!baseline.exists()`: the old form asked the filesystem a second, later question and answered the wrong one under a permission error, where the file exists but `exists()` reports false. The read-before-run ordering is kept and its comment rewritten, because the comment had become false — it claimed `same_file` cannot see hard links, which this commit is precisely the end of. It 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. Reading first makes the comparison independent of whatever the check missed. It cannot save the bytes — only refusing does that. Two fixtures had to be rebuilt rather than extended. `a_gameless_refresh_...` and `an_accepted_refresh_...` used an invalid sentinel as the baseline, which the malformed-baseline fix now refuses BEFORE either test reaches its subject — while still satisfying a naive "bytes unchanged" assertion, so they would have passed for the wrong reason. They now record a real baseline first, which costs about two milliseconds because an empty card database still yields one degenerate playable matchup. 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 needs two files on different filesystems whose inode numbers collide, and inode allocation is not controllable enough to construct that deterministically. `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, not a destroyed baseline. `two_existing_but_different_files_are_not_aliases` exists because no prior test had BOTH paths existing and distinct — `distinct_paths_are_not_treated_as_aliases` leaves the output nonexistent, so `same_inode` returns `None` and the path fallback answers. Without it, an implementation calling every existing pair identical would pass the whole file while refusing every real gate invocation. `a_missing_baseline_is_still_the_first_refresh_case` is the matching two-sided control: 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 tree: 9 CLI tests green in 0.05s, 2026 lib tests green, `clippy -p phase-ai --all-targets -D warnings` clean. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 77 +++- crates/phase-ai/tests/refresh_baseline_cli.rs | 372 +++++++++++++++++- 2 files changed, 428 insertions(+), 21 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index 80f210d707..e518aec5dc 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -4,12 +4,15 @@ #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; +use std::io::ErrorKind; use std::path::{Path, PathBuf}; use std::process::Command; use engine::database::CardDatabase; use phase_ai::config::AiDifficulty; -use phase_ai::duel_suite::compare::{compare, load_report, print_markdown, CompareOptions}; +use phase_ai::duel_suite::compare::{ + compare, load_report, print_markdown, CompareError, CompareOptions, +}; use phase_ai::duel_suite::run::{run_suite, SuiteOptions}; const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/suite-baseline.json"; @@ -100,20 +103,31 @@ fn main() { // Read the baseline BEFORE the suite runs, and keep it in memory. // - // This is the root-cause half of the aliasing fix, and it is what the argument check above - // cannot give: reading first makes the comparison independent of ANYTHING the suite writes, - // including aliases this process cannot detect from paths at all — a hard link resolves to - // a different name and the same inode, so `same_file` calls it distinct while a write to - // one truncates the other. Order does not care how the alias was constructed. + // This is the root-cause half of the aliasing fix. The check above now recognises hard links + // too, so it is no longer the case that an alias can slip past it — but the two guards answer + // different questions and only one of them survives being wrong. `same_file` enumerates the + // ways two names can mean one file, and any such enumeration is a claim about the filesystem + // that a future filesystem can falsify. Reading first makes the COMPARISON independent of + // anything the suite writes, whatever the check missed. It cannot save the bytes — only + // refusing does that — so this is defence in depth, not a substitute. // // It also fails a missing or corrupt baseline in a second instead of after a full suite run, // which is the difference between a typo costing nothing and costing a hundred games. let baseline = match load_report(&args.baseline) { Ok(report) => Some(report), - // On a refresh there may be no baseline yet, and that is the normal first-run case. - Err(_) if args.refresh_baseline && !args.baseline.exists() => None, - Err(err) if args.refresh_baseline => { - eprintln!("could not read the old baseline for comparison: {err}"); + // ABSENT is the only error a refresh may proceed through, and the narrowness is the + // point. Review found the earlier `Err(_) if refresh_baseline` arm logged every failure + // and carried on to `run_suite`, which then 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 replacement was established from an unexamined prior state. + // "I could not read it" is not evidence that it was worthless. + // + // Matched on `ErrorKind::NotFound` rather than `!args.baseline.exists()`: the old form + // asked a second, later question of the filesystem and answered the wrong one under a + // permission error, where the file exists but `exists()` reports false. + Err(CompareError::Io(err)) + if args.refresh_baseline && err.kind() == ErrorKind::NotFound => + { None } Err(err) => { @@ -313,18 +327,28 @@ fn path_str(path: &Path) -> &str { path.to_str().unwrap_or("") } -/// Whether two paths designate the same file, including through symlinks and `..`. +/// Whether two paths designate the same file, including through symlinks, `..`, and hard links. +/// +/// Two layers, because they answer different questions and the cheap one is not sufficient. /// -/// `canonicalize` is the authority when a path exists, because it is the only thing that -/// resolves symlinks — a plain string or `absolute()` comparison calls `baselines/x.json` and a -/// symlink pointing at it different files, and then the write lands on the baseline anyway. The -/// current-output side usually does NOT exist yet, so it falls back to canonicalizing the parent -/// directory (which has to be real for the write to land) and rejoining the file name. +/// **Identity first.** When both paths exist, `(dev, ino)` is the only thing that sees a hard +/// link: `canonicalize` faithfully preserves two distinct names for one inode, so a path +/// comparison calls them different files while `File::create` on either truncates both +/// (`duel_suite::run::write_report`). Review found this and it is not hypothetical — it is the +/// one alias a check on argument strings can never catch, and the destructive one. +/// +/// **Paths second.** The current-output side usually does NOT exist yet, so there is no inode to +/// compare; that case falls back to `canonicalize`, which still resolves symlinks and `..`, on +/// the parent directory (which has to be real for the write to land) with the file name rejoined. /// /// Returns false when neither resolution is possible, which is the right default: an /// unresolvable path cannot be shown to alias, and refusing to run on a path we cannot inspect /// would break invocations that are fine. fn same_file(a: &Path, b: &Path) -> bool { + // Both sides exist => filesystem identity is decisive, and it subsumes the path check. + if let Some(identical) = same_inode(a, b) { + return identical; + } fn resolved(path: &Path) -> Option { if let Ok(canonical) = std::fs::canonicalize(path) { return Some(canonical); @@ -341,6 +365,27 @@ fn same_file(a: &Path, b: &Path) -> bool { } } +/// `Some(true)` when both paths exist and name one inode, `Some(false)` when both exist and do +/// not, `None` when the question cannot be answered — either side missing, or a platform with no +/// inode concept, both of which leave the decision to the path-based fallback. +/// +/// Split out rather than inlined so the `cfg` seam is one function with one contract, instead of +/// a conditional block inside a predicate whose meaning would then differ per platform. +#[cfg(unix)] +fn same_inode(a: &Path, b: &Path) -> Option { + use std::os::unix::fs::MetadataExt; + let (a, b) = (std::fs::metadata(a).ok()?, std::fs::metadata(b).ok()?); + Some(a.dev() == b.dev() && a.ino() == b.ino()) +} + +/// Non-Unix has no portable inode, so identity is unanswerable and the path check stands alone. +/// The gate is a developer/CI tool that runs on Linux; this arm exists so the crate still builds +/// elsewhere, not because the weaker guarantee is considered acceptable there. +#[cfg(not(unix))] +fn same_inode(_a: &Path, _b: &Path) -> Option { + None +} + /// Where a refresh run stages its report before it earns the right to be the baseline. /// /// Beside the baseline, so the later `rename` is a same-filesystem atomic replace. diff --git a/crates/phase-ai/tests/refresh_baseline_cli.rs b/crates/phase-ai/tests/refresh_baseline_cli.rs index 82c906765b..1076c3e5d9 100644 --- a/crates/phase-ai/tests/refresh_baseline_cli.rs +++ b/crates/phase-ai/tests/refresh_baseline_cli.rs @@ -10,15 +10,19 @@ //! satisfied by a broken implementation: exiting non-zero while having clobbered the file is the //! bug itself, and leaving the file alone while exiting 0 would bless the run. //! -//! These run the real CLI and deliberately never reach the card database — the argument check -//! under test rejects before any of that — so they cost milliseconds and add no card-data load -//! for `scripts/check-test-card-data-load.sh` to object to. +//! These run the real CLI. The aliasing tests are rejected at the argument check and never reach +//! the card database at all. The ones that must run the suite to reach what they test supply +//! their own database — the literal `{}`, which `CardDatabase::from_export` accepts as an empty +//! map because it deserialises a map of name→entry. Nothing here loads the ~90 MB export, so the +//! file stays in the millisecond range and adds no per-test parse. use std::path::{Path, PathBuf}; use std::process::Command; -/// Not a valid suite report, and that is deliberate: these tests must fail if the binary gets -/// far enough to parse it, because getting that far means it also ran the suite. +/// Not a valid suite report, and that is deliberate: a test using this as the baseline must fail +/// if the binary ever parses it, because parsing it means it got further than the check under +/// test. Usable only where the refusal precedes the baseline read — since the malformed-baseline +/// fix, a refresh that reaches `load_report` refuses on this rather than running the suite. const SENTINEL: &str = r#"{"this":"is the trusted baseline, byte for byte"}"#; fn scratch(tag: &str) -> PathBuf { @@ -33,6 +37,45 @@ fn seed_baseline(dir: &Path) -> PathBuf { path } +/// A valid, empty card database: the export deserialises a map from card name to entry, so `{}` +/// is a legal database with no cards. Returns the `--data-root` argument. +fn empty_card_db(dir: &Path) -> String { + let root = dir.join("cards"); + std::fs::create_dir_all(&root).expect("create data root"); + std::fs::write(root.join("card-data.json"), "{}").expect("write card data"); + root.display().to_string() +} + +/// Record a real, parseable baseline by running the binary once. +/// +/// Tests whose subject lies PAST the baseline read can no longer use `SENTINEL`: a refresh now +/// refuses an existing baseline it cannot parse, so an invalid fixture would stop the run before +/// it reached the thing under test — and would do so while still satisfying a naive +/// "bytes unchanged" assertion. Recording a real one costs about two milliseconds, because an +/// empty card database still yields one degenerate but playable matchup. +fn record_baseline(dir: &Path, data_arg: &str, name: &str) -> PathBuf { + let path = dir.join(name); + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + data_arg, + "--baseline", + &path.display().to_string(), + "--current-output", + &dir.join("record-current.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + assert_eq!( + code, + Some(0), + "recording a baseline failed; stderr:\n{stderr}" + ); + path +} + fn run(args: &[&str]) -> (Option, String) { let out = Command::new(env!("CARGO_BIN_EXE_ai-gate")) .args(args) @@ -116,6 +159,325 @@ fn an_aliased_comparison_is_refused_before_the_baseline_can_be_overwritten() { std::fs::remove_dir_all(&dir).ok(); } +/// The refusal block itself, executed. +/// +/// An earlier round disclosed this as uncovered: the two predicates behind the refusal were unit +/// tested, but the block that acts on them lived in a binary no test ran, so deleting or +/// inverting it left the suite green. Reaching it looked like it needed a card database and a +/// real suite run — minutes of CI for a local-only command. +/// +/// It does not. `CardDatabase::from_export` deserialises a map, so `{}` is a VALID empty +/// database, and a `--suite-filter` matching nothing selects no matchups, builds no decks and +/// plays no games. The run therefore reaches `recorded_games() == 0` and refuses, in about five +/// milliseconds, with no card data on disk. +/// +/// The exit code is asserted as exactly 1 rather than merely non-zero, and that is what makes +/// this test see the block instead of an early death: argument and database failures exit 2, so +/// a mutant that stops the binary before the suite runs changes 1 to 2 and fails here. +#[test] +fn a_gameless_refresh_is_refused_by_the_block_and_leaves_the_baseline_untouched() { + let dir = scratch("gameless"); + let data_arg = empty_card_db(&dir); + let baseline = record_baseline(&dir, &data_arg, "suite-baseline.json"); + let trusted = std::fs::read_to_string(&baseline).expect("read baseline"); + + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current.json").display().to_string(), + "--suite-filter", + "no-such-matchup", + ]); + + assert_eq!( + code, + Some(1), + "expected the refusal block's exit 1, not an earlier failure; stderr:\n{stderr}" + ); + assert!( + stderr.contains("recorded no games"), + "the refusal must be the gameless one; stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + trusted, + "a refused refresh must leave the baseline byte-identical" + ); + // The staging file is an implementation detail of the transaction, but a leftover one is an + // artefact someone later mistakes for a real baseline. + let staging = baseline.with_file_name("suite-baseline.json.staging.json"); + assert!( + !staging.exists(), + "a refused refresh must not leave {} behind", + staging.display() + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// The other side of the transaction: an ACCEPTED refresh must actually promote the staged +/// report over the old baseline. +/// +/// Without this, "a refused run leaves the baseline alone" is satisfied by a binary that never +/// writes a baseline at all — measured: deleting the promotion entirely left every other test in +/// this file green. The two directions have to be asserted together or the guard is +/// indistinguishable from a break. +/// +/// Reachable at the same near-zero cost as the refusal tests, for a reason worth recording: an +/// empty card database still yields a playable (degenerate) matchup, so a single game completes +/// in about two milliseconds and the run is accepted. The baseline written here is meaningless +/// as a baseline — that is fine, because what is under test is the file transaction, not the +/// contents. +#[test] +fn an_accepted_refresh_promotes_the_staged_report_over_the_old_baseline() { + let dir = scratch("accept"); + let data_arg = empty_card_db(&dir); + let baseline = record_baseline(&dir, &data_arg, "suite-baseline.json"); + // Mark the recorded baseline so "was it replaced?" is answered by content, not by mtime. + // A marked-but-valid file is required: an invalid one is now refused before the suite runs. + let mut old: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&baseline).expect("read baseline")) + .expect("baseline is JSON"); + old["git_sha"] = serde_json::json!("OLD-BASELINE-MARKER"); + let marked = serde_json::to_string(&old).expect("serialize baseline"); + std::fs::write(&baseline, &marked).expect("write baseline"); + + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + + assert_eq!( + code, + Some(0), + "an accepted refresh must succeed; stderr:\n{stderr}" + ); + let written = std::fs::read_to_string(&baseline).expect("read baseline"); + assert_ne!( + written, marked, + "the accepted run was never promoted over the old baseline" + ); + // PREMISE: what landed is the run's report, not any other file the process might have + // written — so "changed" cannot pass by truncation or by a stray write. + let report: serde_json::Value = serde_json::from_str(&written).expect("baseline is JSON"); + assert_eq!(report["games_per_matchup"], 1); + assert_eq!(report["results"][0]["matchup_id"], "red-mirror"); + assert_eq!( + report["results"][0]["games"].as_array().map(Vec::len), + Some(1), + "the promoted baseline must carry the game that was played" + ); + let staging = baseline.with_file_name("suite-baseline.json.staging.json"); + assert!( + !staging.exists(), + "staging must not survive a successful refresh" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// The alias no path comparison can see: one inode, two names. +/// +/// `canonicalize` faithfully resolves a hard link to its own name, so the pre-fix path check +/// called these distinct and let the run proceed — and `write_report` opens `--current-output` +/// with `File::create`, which truncates the shared inode. Reading the baseline before the suite +/// runs saves the *verdict* from being computed against the run's own output, but it does not +/// save the *bytes*; only refusing does. Both halves are asserted here for that reason. +/// +/// Compare mode rather than refresh, because it is the quieter half: refresh at least prints a +/// refusal, while compare would have gone on to report no drift and exit 0 against a baseline it +/// had just destroyed. +#[test] +fn a_hard_linked_output_is_refused_and_the_baseline_survives() { + let dir = scratch("hardlink"); + let baseline = seed_baseline(&dir); + let link = dir.join("current-link.json"); + std::fs::hard_link(&baseline, &link).expect("hard link"); + + // PREMISE: the fixture really is one file under two names. Without this the test would still + // pass on a platform or filesystem that silently copied instead, having proven nothing. + assert_ne!( + std::fs::canonicalize(&baseline).expect("canonicalize baseline"), + std::fs::canonicalize(&link).expect("canonicalize link"), + "a hard link must present two distinct paths, or this test is not about hard links" + ); + + let (code, stderr) = run(&[ + "--baseline", + &baseline.display().to_string(), + "--current-output", + &link.display().to_string(), + ]); + + assert_eq!( + code, + Some(2), + "a hard-linked output must be refused at the argument check; stderr:\n{stderr}" + ); + assert!( + stderr.contains("same file"), + "the refusal must name the aliasing; stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + SENTINEL, + "the baseline was truncated through its hard link" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// A baseline that exists but cannot be parsed must stop the refresh, not be overwritten by it. +/// +/// The earlier code logged every read failure and carried on, so the run continued to `run_suite` +/// and renamed its staged report over the file. That destroys the evidence needed to work out WHY +/// the baseline was unreadable, and establishes the replacement from an unexamined prior state. +/// +/// The exit code is asserted as exactly 2, which is what separates this from the gameless +/// refusal's 1: the point is that the run stops BEFORE the suite, not that it stops eventually. +/// A mutant that let the run proceed and refused later would leave the bytes intact on this +/// fixture too, and the code is the only assertion that tells the two apart. +#[test] +fn a_malformed_existing_baseline_is_not_replaced_by_a_refresh() { + let dir = scratch("malformed"); + let data_arg = empty_card_db(&dir); + let baseline = seed_baseline(&dir); + + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + + assert_eq!( + code, + Some(2), + "a malformed baseline must stop the run before the suite; stderr:\n{stderr}" + ); + assert!( + stderr.contains("failed to load baseline"), + "the refusal must name the baseline load; stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + SENTINEL, + "an unreadable baseline must be preserved for diagnosis, not overwritten" + ); + let staging = baseline.with_file_name("suite-baseline.json.staging.json"); + assert!( + !staging.exists(), + "a refused refresh must not leave {} behind", + staging.display() + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// The other side of that refusal: a MISSING baseline is the ordinary first-refresh case and must +/// still be allowed through. +/// +/// Without this, narrowing the accepted error to `NotFound` is indistinguishable from refusing +/// every unreadable baseline including the absent one — which would make the first refresh on a +/// fresh checkout impossible. Measured: this is the arm that fails if the `NotFound` guard is +/// dropped in the refusing direction. +#[test] +fn a_missing_baseline_is_still_the_first_refresh_case() { + let dir = scratch("firstrun"); + let data_arg = empty_card_db(&dir); + let baseline = dir.join("does-not-exist-yet.json"); + assert!(!baseline.exists(), "fixture must start with no baseline"); + + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + + assert_eq!( + code, + Some(0), + "a first refresh with no baseline must succeed; stderr:\n{stderr}" + ); + assert!( + baseline.exists(), + "the first refresh must create the baseline" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +/// Control arm for the identity check specifically: two files that BOTH exist and are genuinely +/// different must not be called aliases. +/// +/// `distinct_paths_are_not_treated_as_aliases` does not cover this — its output path does not +/// exist, so `same_inode` returns `None` and the path fallback answers. This is the only case +/// that reaches the inode comparison and expects `Some(false)`, so without it an implementation +/// that reported every existing pair as identical would pass the whole file while refusing every +/// real invocation of the gate. +/// +/// Known gap, stated rather than implied: this does not pin the `dev` half of `(dev, ino)`. A +/// mutant comparing only `ino` survives every test here, because killing it needs two files on +/// different filesystems whose inode numbers collide, and inode allocation is not controllable +/// enough to construct that deterministically. The `dev` term is kept on correctness grounds — +/// dropping it can only ever produce a false ALIAS report, whose consequence is a refused run +/// rather than a destroyed baseline, so the unpinned direction is the safe one. +#[test] +fn two_existing_but_different_files_are_not_aliases() { + let dir = scratch("distinct-existing"); + let baseline = seed_baseline(&dir); + let current = dir.join("current.json"); + std::fs::write(¤t, "{}").expect("seed current"); + + // PREMISE: both really exist, so the run reaches the inode comparison rather than the + // path fallback this test is not about. + assert!(baseline.exists() && current.exists()); + + let (_code, stderr) = run(&[ + "--baseline", + &baseline.display().to_string(), + "--current-output", + ¤t.display().to_string(), + "--data-root", + &dir.join("no-such-data-root").display().to_string(), + ]); + + assert!( + !stderr.contains("same file"), + "two distinct existing files must not be called aliases; stderr:\n{stderr}" + ); + // PREMISE: the run proceeded past the argument check, so the assertion above is about + // aliasing rather than about the process dying even earlier. + assert!( + stderr.contains("failed to load card database"), + "expected the run to proceed to the card database; stderr:\n{stderr}" + ); + std::fs::remove_dir_all(&dir).ok(); +} + /// Control arm. Every assertion above is satisfied by a binary that refuses everything, which /// would break the gate far worse than the bug being fixed. Distinct paths must get past the /// argument check — proven by the failure being about something LATER in the run (the card From b71e0a3db506fb38d1a7fe9e217f7e20dbb4af4b Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 11:14:17 -0500 Subject: [PATCH 4/4] fix(ai): reserve the staging file before the suite can write through it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third route to the same data loss, and the only one no argument check could have caught: the staging path is derived internally, so `same_file` never sees it — that check compares `--baseline` against `--current-output`, and the staging file is neither. `staging_path` is deterministic (`.staging.json`) and `write_report` opens it with `File::create`, which FOLLOWS a symlink to its target and SHARES a hard link's inode. An entry already sitting at that path is therefore written straight through, truncating whatever it points at, before any refresh guard runs. Reproduced on the binary before the fix: with a symlink pre-placed at the staging path, a refresh that should have been accepted reported **exit 0** and the baseline's bytes were gone. The fix reserves the path with `create_new` rather than checking it. That is deliberate and is the difference between a fix and a smaller window: testing first and opening second leaves a gap between the two, 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 subsequently truncates is a regular file this process just created with a link count of one. Refusing rather than reusing is the point. The path is derived, so anything already there was not put there by this run, and a leftover from a killed run is precisely the artefact the refusal paths delete. The message names the file and says what would have happened had it been followed. Reserving creates an obligation the previous code did not have: every exit after the reservation must release it, or one diagnosable failure becomes two — the refusal, and then a next refresh blocked by the file the refusal left. The baseline-load and suite-run exits now call `release_staging`, 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 distinguishes 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 must 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. Measured at this tree: 10 CLI tests green, 2026 lib tests green, `clippy -p phase-ai --all-targets -D warnings` clean, `cargo fmt --check` clean. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 56 ++++++++++++ crates/phase-ai/tests/refresh_baseline_cli.rs | 87 +++++++++++++++++++ 2 files changed, 143 insertions(+) diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index e518aec5dc..9ec1d99d42 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -94,6 +94,47 @@ fn main() { // the run's report. No workflow passes `--refresh-baseline`, so nothing in CI depends on // the old behaviour. let staging = args.refresh_baseline.then(|| staging_path(&args.baseline)); + // RESERVE the staging path before the suite can write a byte to it. + // + // Third route to the same destruction, and the only one no argument check could ever catch: + // this path is derived internally, so `same_file` never sees it — it compares `--baseline` + // against `--current-output`, and the staging file is neither. `write_report` opens it with + // `File::create`, which FOLLOWS a symlink to its target and SHARES a hard link's inode, so an + // entry already sitting there truncates whatever it points at, before any refresh guard runs. + // Measured on the binary before this reservation existed: with a symlink pre-placed at the + // staging path, the refresh reported success and the baseline's bytes were gone. + // + // `create_new` is the whole fix, and it is deliberately a reservation rather than a check. + // 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` fails on ANY existing entry — regular + // file, hard link, live or dangling symlink — in one atomic step. What the suite then + // truncates is a regular file this process just created, with a link count of one. + if let Some(path) = &staging { + if let Some(parent) = path.parent() { + if let Err(err) = std::fs::create_dir_all(parent) { + eprintln!("failed to create {}: {err}", parent.display()); + std::process::exit(2); + } + } + if let Err(err) = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + { + // Refusing rather than reusing is the point. The path is derived, so anything already + // there was not put there by this run, and a leftover from a killed run is exactly + // the artefact the refusal paths below delete. Naming it and stopping is better than + // writing through something whose provenance is unknown. + eprintln!( + "failed to reserve the staging file {}: {err}\n\ + if a previous run was interrupted, remove that file and retry; if it is a \ + symlink or hard link, it would have been written straight through into whatever \ + it points at", + path.display() + ); + std::process::exit(2); + } + } options.output_path = staging .clone() .unwrap_or_else(|| args.current_output.clone()); @@ -131,6 +172,9 @@ fn main() { None } Err(err) => { + // Every exit after the reservation has to release it, or a refused run leaves a file + // that blocks the next refresh — turning one diagnosable failure into two. + release_staging(&staging); eprintln!("failed to load baseline {}: {err}", args.baseline.display()); std::process::exit(2); } @@ -139,6 +183,7 @@ fn main() { let current = match run_suite(&db, &options) { Ok(report) => report, Err(err) => { + release_staging(&staging); eprintln!("suite run failed: {err}"); std::process::exit(1); } @@ -365,6 +410,17 @@ fn same_file(a: &Path, b: &Path) -> bool { } } +/// Release a reserved staging file, if this run reserved one. +/// +/// A no-op on the compare path, where `staging` is `None`. Errors are ignored deliberately: this +/// only ever runs on a path that is already exiting with a diagnosis of its own, and a failure to +/// remove a scratch file is not worth displacing that diagnosis. +fn release_staging(staging: &Option) { + if let Some(path) = staging { + let _ = std::fs::remove_file(path); + } +} + /// `Some(true)` when both paths exist and name one inode, `Some(false)` when both exist and do /// not, `None` when the question cannot be answered — either side missing, or a platform with no /// inode concept, both of which leave the decision to the path-based fallback. diff --git a/crates/phase-ai/tests/refresh_baseline_cli.rs b/crates/phase-ai/tests/refresh_baseline_cli.rs index 1076c3e5d9..9a5b91d4ad 100644 --- a/crates/phase-ai/tests/refresh_baseline_cli.rs +++ b/crates/phase-ai/tests/refresh_baseline_cli.rs @@ -215,6 +215,27 @@ fn a_gameless_refresh_is_refused_by_the_block_and_leaves_the_baseline_untouched( "a refused refresh must not leave {} behind", staging.display() ); + // The consequence of leaving it, asserted rather than inferred: the staging path is now + // RESERVED with `create_new`, so a leftover does not merely look like an artefact — it blocks + // every later refresh. One refused run must not poison the next. + let (next_code, next_stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current2.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + assert_eq!( + next_code, + Some(0), + "a refused refresh must not block the next one; stderr:\n{next_stderr}" + ); std::fs::remove_dir_all(&dir).ok(); } @@ -478,6 +499,72 @@ fn two_existing_but_different_files_are_not_aliases() { std::fs::remove_dir_all(&dir).ok(); } +/// The third route to the same destruction, and the one no argument check can see: the staging +/// path the process derives for ITSELF. +/// +/// `staging_path` is deterministic — `.staging.json` — and `write_report` opens it with +/// `File::create` before any refresh guard runs. So an entry already sitting at that path is +/// followed (symlink) or shared (hard link), and the baseline is truncated by a write nobody +/// passed on the command line. `same_file` cannot help: it compares `--baseline` against +/// `--current-output`, and the staging path is neither. +/// +/// Both alias kinds are exercised because they fail differently — `File::create` FOLLOWS a +/// symlink to its target, while a hard link IS the target — and a fix that reserved the path +/// against only one of them would leave the other live. +#[cfg(unix)] +#[test] +fn a_pre_existing_staging_alias_cannot_truncate_the_baseline() { + for kind in ["symlink", "hardlink"] { + let dir = scratch(&format!("staging-{kind}")); + let data_arg = empty_card_db(&dir); + let baseline = record_baseline(&dir, &data_arg, "suite-baseline.json"); + let trusted = std::fs::read_to_string(&baseline).expect("read baseline"); + let staging = baseline.with_file_name("suite-baseline.json.staging.json"); + if kind == "symlink" { + std::os::unix::fs::symlink(&baseline, &staging).expect("symlink"); + } else { + std::fs::hard_link(&baseline, &staging).expect("hard link"); + } + + // PREMISE: the alias really points at the baseline, so a write through it would land on + // the file under test. Without this the fixture could be inert and the test vacuous. + assert_eq!( + std::fs::read_to_string(&staging).expect("read through alias"), + trusted, + "{kind}: the staging alias must resolve to the baseline" + ); + + // A refresh that would otherwise be ACCEPTED — the destructive case, because an accepted + // run is the one that goes on to rename over the baseline. + let (code, stderr) = run(&[ + "--refresh-baseline", + "--data-root", + &data_arg, + "--baseline", + &baseline.display().to_string(), + "--current-output", + &dir.join("current.json").display().to_string(), + "--suite-filter", + "red-mirror", + "--games", + "1", + ]); + + assert_ne!( + code, + Some(0), + "{kind}: a run that cannot reserve its staging file must not report success; \ + stderr:\n{stderr}" + ); + assert_eq!( + std::fs::read_to_string(&baseline).expect("read baseline"), + trusted, + "{kind}: the baseline was truncated through the staging alias" + ); + std::fs::remove_dir_all(&dir).ok(); + } +} + /// Control arm. Every assertion above is satisfied by a binary that refuses everything, which /// would break the gate far worse than the bug being fixed. Distinct paths must get past the /// argument check — proven by the failure being about something LATER in the run (the card