Skip to content

Commit 643f322

Browse files
committed
fix(ai): refuse to refresh the duel-suite baseline from a failing run
`--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 phase-rs#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 phase-rs#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 phase-rs#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
1 parent f7c4469 commit 643f322

2 files changed

Lines changed: 222 additions & 3 deletions

File tree

crates/phase-ai/src/bin/ai_gate.rs

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,53 @@ fn main() {
7272
};
7373

7474
if args.refresh_baseline {
75+
// A baseline is what every later run is judged against, so refreshing from a run
76+
// that failed its own `Expected` check blesses that failure permanently: the next
77+
// run compares equal to it and exits 0 forever, and the gate goes quiet about a
78+
// matchup that is still broken. Refuse. A matchup that genuinely has no verdict
79+
// yet says so with `Expected::Open` in the suite definition — that is the place
80+
// to express it, not a red baseline.
81+
//
82+
// ORDER MATTERS, and this one is strictly better on every input. The two conditions
83+
// are not exclusive: `failed_result` builds a matchup with an empty `games` vector
84+
// AND `SuiteStatus::Fail`, so a run whose deck payloads all failed to load satisfies
85+
// both. Reporting the failures names each matchup and its `setup error: …`; reporting
86+
// gamelessness first would replace that with a sentence about seeds. Nothing is lost
87+
// by checking failures first, because a run that is merely gameless — a
88+
// `--suite-filter` matching nothing — has no failing matchups to report.
89+
let failing: Vec<_> = current.failing_matchups().collect();
90+
if !failing.is_empty() {
91+
eprintln!(
92+
"refusing to refresh {}: {} matchup(s) failed their own suite check",
93+
args.baseline.display(),
94+
failing.len()
95+
);
96+
for result in failing {
97+
eprintln!(
98+
" {}: {}",
99+
result.matchup_id,
100+
result
101+
.fail_reason
102+
.as_deref()
103+
.unwrap_or("no reason recorded")
104+
);
105+
}
106+
eprintln!(
107+
"fix the regression, or declare the matchup `Expected::Open` if it has no verdict yet"
108+
);
109+
std::process::exit(1);
110+
}
111+
// A run that measured nothing is unfit for the same reason a red one is, reached from
112+
// the other side: comparison pairs by seed, so a gameless baseline scores zero on the
113+
// outcome axes forever and the drift signal dies quietly. Reached by a `--suite-filter`
114+
// that selects no matchups; `--games 0` is refused earlier, at parse time.
115+
if current.recorded_games() == 0 {
116+
eprintln!(
117+
"refusing to refresh {}: the run recorded no games, so every later comparison would score zero",
118+
args.baseline.display()
119+
);
120+
std::process::exit(1);
121+
}
75122
if args.baseline.exists() {
76123
match load_report(&args.baseline)
77124
.and_then(|baseline| compare(&baseline, &current, &CompareOptions))
@@ -137,9 +184,14 @@ fn parse_args() -> Result<Args, String> {
137184
current_output = next_path(&mut iter, "--current-output")?;
138185
}
139186
"--games" => {
140-
games = next_value(&mut iter, "--games")?
141-
.parse()
142-
.map_err(|_| "--games must be a positive integer".to_string())?;
187+
// `usize` alone accepts 0, which the error string already promised it would
188+
// not. A zero-game run classifies every matchup `Open` and produces a
189+
// baseline that can never detect drift, so reject it here rather than
190+
// burning a whole suite run to refuse it later.
191+
games = match next_value(&mut iter, "--games")?.parse() {
192+
Ok(0) | Err(_) => return Err("--games must be a positive integer".to_string()),
193+
Ok(value) => value,
194+
};
143195
}
144196
"--seed" => {
145197
seed = next_value(&mut iter, "--seed")?

crates/phase-ai/src/duel_suite/run.rs

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,51 @@ impl SuiteReport {
146146
.collect(),
147147
}
148148
}
149+
150+
/// Matchups that failed their own `Expected` check, judged without reference to any
151+
/// baseline.
152+
///
153+
/// This is a different question from `CompareReport::any_fail`, which asks "did this
154+
/// change make things worse than the baseline". This one asks "is this run fit to
155+
/// *become* the baseline" — and only `SuiteStatus::Fail` disqualifies it.
156+
///
157+
/// `SuiteStatus::Open` does not, and it has two producers, not one: `Expected::Open`
158+
/// is how a matchup declares it has no verdict yet, and `classify` also returns `Open`
159+
/// for any matchup with zero games before it ever inspects `Expected`. Neither is a
160+
/// failure, so anything keyed on `!= Pass` would conflate a declared no-verdict with a
161+
/// regression. The zero-game case is disqualifying for a different reason and is caught
162+
/// by `recorded_games`, not here.
163+
pub fn failing_matchups(&self) -> impl Iterator<Item = &MatchupResult> {
164+
self.results
165+
.iter()
166+
.filter(|result| result.status == SuiteStatus::Fail)
167+
}
168+
169+
/// Games actually recorded across every matchup.
170+
///
171+
/// A report with none is unfit to become a baseline whatever its statuses say:
172+
/// comparison pairs by seed, so a baseline holding no games makes every later
173+
/// comparison score zero on every axis and the drift signal dies silently — the same
174+
/// false-green this guard exists to prevent, arrived at from the other side.
175+
///
176+
/// Deliberately counts games rather than checking for all-`Open`: a suite whose
177+
/// matchups are all declared `Expected::Open` still records real games, and such a
178+
/// report *is* a usable baseline, because the paired-comparison arm decides on `games`,
179+
/// not on `status` — an all-`Open` pair therefore still detects outcome drift. (Stated
180+
/// as "decides on games" rather than "never reads status": the paired arm gained status
181+
/// tiers in #7026, so the stronger wording would have been false the day that merged.)
182+
/// Zero games is the property that makes a baseline inert; all-`Open` is not.
183+
///
184+
/// Not an exhaustive list of routes, so it does not claim to be one: `--games 0` is
185+
/// refused at parse time, a `--suite-filter` matching no matchups lands here, and
186+
/// `failed_result` yields an empty `games` vector alongside `SuiteStatus::Fail`, which
187+
/// `failing_matchups` reports first because it names the actual setup error. And this is
188+
/// a `pub` method, so its audience includes library callers: `SuiteOptions::new` does not
189+
/// validate `games_per_matchup`, so a caller can construct a zero-game run without going
190+
/// through the CLI at all.
191+
pub fn recorded_games(&self) -> usize {
192+
self.results.iter().map(|result| result.games.len()).sum()
193+
}
149194
}
150195

151196
/// Controls decision-trace attribution capture during a suite run. When set
@@ -1082,6 +1127,128 @@ mod tests {
10821127
}
10831128
}
10841129

1130+
/// Reuses `report_with_timing`'s matchup as the field template so these tests state
1131+
/// only the axes they exercise: each matchup's status and reason.
1132+
fn report_with_statuses(statuses: &[(&str, SuiteStatus, Option<&str>)]) -> SuiteReport {
1133+
let mut report = report_with_timing(1, 100);
1134+
let template = report.results[0].clone();
1135+
report.results = statuses
1136+
.iter()
1137+
.map(|(id, status, reason)| MatchupResult {
1138+
matchup_id: (*id).to_string(),
1139+
status: *status,
1140+
fail_reason: reason.map(str::to_string),
1141+
..template.clone()
1142+
})
1143+
.collect();
1144+
report
1145+
}
1146+
1147+
#[test]
1148+
fn a_clean_run_has_no_failing_matchups() {
1149+
let report = report_with_statuses(&[
1150+
("red-mirror", SuiteStatus::Pass, None),
1151+
("affinity-mirror", SuiteStatus::Pass, None),
1152+
]);
1153+
1154+
assert_eq!(report.failing_matchups().count(), 0);
1155+
}
1156+
1157+
/// Transcribed from the recorded gate run that motivated this guard (`.ab/noC-1.json`,
1158+
/// the A+B+D leg of #6969): the statuses and the verbatim `fail_reason` are that run's,
1159+
/// not invented. Refreshing the baseline from this exact report is what would have
1160+
/// blessed a broken matchup permanently. The artifact is untracked, so it is
1161+
/// transcribed rather than loaded — a test that read the file would fail in CI.
1162+
#[test]
1163+
fn the_recorded_failing_run_is_disqualified_as_a_baseline() {
1164+
let report = report_with_statuses(&[
1165+
("red-mirror", SuiteStatus::Pass, None),
1166+
("affinity-mirror", SuiteStatus::Pass, None),
1167+
(
1168+
"enchantress-mirror",
1169+
SuiteStatus::Fail,
1170+
Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50"),
1171+
),
1172+
]);
1173+
1174+
let failing: Vec<_> = report.failing_matchups().collect();
1175+
assert_eq!(failing.len(), 1, "the one Fail, not every matchup");
1176+
assert_eq!(failing[0].matchup_id, "enchantress-mirror");
1177+
// The reason travels with the matchup: the refusal is only actionable if it can say
1178+
// *why* the run is unfit, not merely that it is.
1179+
assert_eq!(
1180+
failing[0].fail_reason.as_deref(),
1181+
Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50")
1182+
);
1183+
}
1184+
1185+
/// The discriminating case for `recorded_games`: a suite whose matchups are all
1186+
/// declared `Expected::Open` still played real games, and that report IS a usable
1187+
/// baseline, because seed-paired drift detection never consults `status`. An
1188+
/// implementation that disqualified all-`Open` reports instead of gameless ones would
1189+
/// pass every other test here and fail this one.
1190+
#[test]
1191+
fn an_all_open_run_that_played_games_is_still_a_usable_baseline() {
1192+
let mut report = report_with_statuses(&[
1193+
("experimental-a", SuiteStatus::Open, None),
1194+
("experimental-b", SuiteStatus::Open, None),
1195+
]);
1196+
// Uneven on purpose. With one game each, the total (2) equals the MATCHUP count, so
1197+
// "sum of games" and "number of matchups that played" are indistinguishable — two
1198+
// mutants with that wrong contract survived the earlier version of this test. Three
1199+
// games across two matchups separates them.
1200+
let extra = report.results[0].games[0].clone();
1201+
report.results[0].games.push(GameResult {
1202+
seed: extra.seed + 1,
1203+
..extra
1204+
});
1205+
1206+
assert_eq!(report.failing_matchups().count(), 0);
1207+
assert_eq!(
1208+
report.recorded_games(),
1209+
3,
1210+
"two games in the first matchup plus one in the second — a SUM, not a matchup count"
1211+
);
1212+
}
1213+
1214+
#[test]
1215+
fn a_gameless_run_records_nothing_however_many_matchups_it_has() {
1216+
let mut report = report_with_statuses(&[
1217+
("red-mirror", SuiteStatus::Open, None),
1218+
("affinity-mirror", SuiteStatus::Open, None),
1219+
]);
1220+
// What `--games 0` produces: matchups exist, none of them played anything.
1221+
for result in &mut report.results {
1222+
result.games.clear();
1223+
}
1224+
1225+
assert_eq!(report.recorded_games(), 0);
1226+
// And it is NOT a failure — the two disqualifiers are independent, so a guard that
1227+
// conflated them would let one of the two holes back open.
1228+
assert_eq!(report.failing_matchups().count(), 0);
1229+
}
1230+
1231+
#[test]
1232+
fn a_run_that_selected_no_matchups_records_nothing() {
1233+
// What a `--suite-filter` matching nothing produces: no matchups at all.
1234+
let report = report_with_statuses(&[]);
1235+
1236+
assert_eq!(report.recorded_games(), 0);
1237+
}
1238+
1239+
#[test]
1240+
fn an_open_matchup_is_not_a_failure() {
1241+
// `Expected::Open` classifies as `SuiteStatus::Open`: a matchup that has no verdict
1242+
// yet, which must not block a refresh. This is the test that dies if the filter is
1243+
// ever written as the plausible `!= SuiteStatus::Pass` — the other two survive it.
1244+
let report = report_with_statuses(&[
1245+
("red-mirror", SuiteStatus::Pass, None),
1246+
("experimental-mirror", SuiteStatus::Open, None),
1247+
]);
1248+
1249+
assert_eq!(report.failing_matchups().count(), 0);
1250+
}
1251+
10851252
#[test]
10861253
fn deterministic_core_excludes_wall_clock_fields() {
10871254
let first = report_with_timing(1, 100);

0 commit comments

Comments
 (0)