From ae37fe16ccc60f19eb476693b53be58e106ab01a Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 00:55:54 -0500 Subject: [PATCH 1/8] fix(ai): make the duel-suite comparator see the regressions it was blind to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `paired_seed_shift` classified only `Some(0)→Some(1)` and `Some(1)→Some(0)`; every `Some(_) → None` fell into a `_ => unchanged` wildcard. Measured on the committed baseline versus a real recorded gate run (`.ab/noC-1.json`, the A+B+D leg of #6969): `enchantress-mirror` went 4/6/0 to 1/1/8 — eight of ten games stopped having a winner — and the comparison reported `unchanged=10`, `flips=0`, `sign_test_p=None`, `CompareStatus::Pass`. A branch that made the AI stall or loop every game would pass this gate silently. The same recorded run carried a SECOND hole: its enchantress-mirror row has `status: "Fail"` with `fail_reason: "mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50"`, and the comparison still printed `0 FAIL, 0 WARN, 3 PASS`. `classify_row`'s paired branch read the game outcomes and nothing else, so an existing matchup that newly failed its own suite check did not fail the gate. Only the new-matchup arm ever looked at `SuiteStatus`. The draw axis and the suite-status axis both become first-class peers of the win/loss axis: - `PairedSeedShift` and `CompareRow` gain `decisive_to_draw`, `draw_to_decisive`, and `draw_sign_test_p`, the last computed by the existing `sign_test_mid_p_upper_tail` on the draw axis — reused, not reimplemented. - The classification match is exhaustive with no `_` arm, so a future `winner` representation breaks the build instead of silently rejoining `unchanged`. That wildcard is how this defect survived. - Two verdict tiers: draw FAIL after W/L FAIL, draw WARN after W/L WARN. The tier order is load-bearing, and the invariant it buys is narrower than "nothing moves": nothing that reaches Fail today changes verdict, and nothing with a flat draw axis changes verdict at any severity — but a W/L *Warn* does escalate to Fail when the draw axis is significantly negative, because the draw Fail arm sits above the W/L Warn arm. That escalation is intended; suppressing a significant draw regression because the win/loss axis also wobbled insignificantly would reintroduce the same blindness one case narrower. `draw_regression_escalates_an_insignificant_win_loss_warn` pins it, and a reorder mutant that demotes it to Warn flips that test and only it. - Asymmetric by construction: `decisive→draw` dominating is the regression signal and can reach Fail; `draw→decisive` is an improvement and can never Fail (tier 2 requires decisive→draw to dominate) but still Warns, because a comparator silent about games that started resolving would hide a behavior change. - The suite-status axis: an existing matchup going non-Fail → Fail is a Fail here, carrying the matchup's own `fail_reason`; recovery (Fail → non-Fail) is a Warn, never a Fail, the same asymmetry as the draw axis. Keyed on `Fail` specifically rather than on any status change, because `Fail` is the only status this file already acts on — the new-matchup arm matches `SuiteStatus::Fail` and treats `Pass`/`Open` alike. Same authority, same vocabulary, extended from new matchups to existing ones. The Fail arm sits below both outcome Fail arms so everything failing today keeps its more specific reason string. A matchup that was already failing in the baseline and is still failing Warns every run rather than passing quietly — reachable, not theoretical, since `--refresh-baseline` writes the current report verbatim with no `any_fail` check. Warn and not Fail is deliberate: the exit code answers "did this change make things worse", and the baseline already sanctions that state; whether a baseline may bless a failure at all belongs with the refresh guard in `bin/ai_gate.rs`. - The table gains `dec→draw`, `draw→dec`, `draw sign p`, `Δ avg turns`, and `suite status` columns; `CompareRow` carries `avg_turn_delta` and `suite_status_shift`. Rendering moves out of `println!` into `render_markdown() -> String` over a `COLUMNS` constant, so the invariant "every axis the chain can decide on owns a column" is enforced by tests instead of asserted in a comment — while it was inlined, both new columns could be deleted with the whole suite green. The verdict chain is first-match-wins, so a firing arm suppresses every other arm's reason string: a W/L Warn used to hide the row's draw movement, and a draw Warn hides the mirror avg-turn drift the same way. Columns are the only surface that survives that suppression, so every axis the chain can decide on now owns one. `mirror_drift_magnitude_survives_a_shadowing_reason` pins the shadowed case. Evidence. Thirty mutants, each flipping a distinct set of the module's 26 tests, tree restored byte-identical after every run. The verdict chain: dropping either draw tier, dropping the status arm, reordering either Fail arm below the W/L Warn arm, restoring the pre-fix classification, firing on any movement regardless of direction, making `Fail->Fail` silent again, reading the draw statistic in one fixed direction. Then four rounds of review, each finding the previous round's blind spot one layer down. Round 3 showed the RENDERING was pinned by header label only — freezing the `Δ avg turns` cell, or transposing the `dec→draw`/`draw→dec` cells so the recorded incident prints its counters backwards, survived the whole suite. Round 4 showed the same for the two WIN/LOSS reason strings (transposing them reports a 10-0 regression as `W→L=0 L→W=10`), for the `suite status` column's shift branch, for the `—` fallbacks that New and Removed rows print, and for the avg-turn arm's `Expected::Mirror` guard. Round 5 showed the `PASS` label itself was unbound — WARN, FAIL, NEW and REMOVED are each pinned by a cell assertion, and mutating `status_str`'s `Pass` arm survived the whole suite, so a gate that rendered every row as garbage would still read as reviewed. All now die. Three of my own fixes were themselves defective and are recorded rather than quietly corrected: the separator check `.split('|').filter(|s| !s.is_empty())` drops every segment exactly when the fill is empty, so its mutant survived a second time; the first cell-transposition fixture rendered `45%` in both win-rate columns, so swapping them changed nothing; and the `—` fallback sweep pinned five of its six sites, missing `draw sign p` — which is the fallback the real gate renders on every row today, since a matchup with a flat draw axis has no statistic to report. A guard is not a guard until its mutant dies, and a sweep by recipe is not a sweep until every site of that recipe is covered. The invariant the tier order buys is stated to its exact edge and verified over **1,327,104 paired inputs** against the compiled base — including seeds present in only one report, duplicate seeds, empty game vectors, zero-matchup reports, NaN and infinite turn deltas, and every `Expected` variant. Zero violations of either clause, zero `Fail->X`, zero `X->Pass`: head severity is monotone >= base on every input, and all 287,712 escalations are attributable to a significant draw regression or a non-Fail->Fail status shift. The acceptance run is the real comparator over the two recorded reports — `enchantress-mirror` moves PASS -> FAIL (`decisive→draw=8 draw→decisive=0 sign-test p=0.0020`, the exact 1/512), while `red-mirror` and `affinity-mirror` measure 0 on the draw axis and do not move. BASE and POST artifacts were diffed and differ, so this is not a stale binary. `noc1_enchantress_row_carries_both_holes` pins the recorded incident whole, asserting BOTH holes are present so a future edit that closes one and reopens the other cannot pass it. The live gate stays green: `ai-gate --games 10` against the committed baseline reports `0 FAIL, 0 WARN, 3 PASS`, and structurally so — all three matchups have a flat draw axis and an unchanged suite status, so every new guard is false and the chain takes exactly the pre-change path. `baselines/suite-baseline.json` is untouched. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/duel_suite/compare.rs | 1262 ++++++++++++++++++++- 1 file changed, 1243 insertions(+), 19 deletions(-) diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index 925d19e217..1fa7021a88 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -38,10 +38,24 @@ pub struct CompareRow { pub baseline: Option, pub current: Option, pub delta_p0_pp: Option, + /// `current.avg_turns - baseline.avg_turns`. Carried on the row, not only inside a reason + /// string: the verdict chain is first-match-wins, so whichever arm fires suppresses every + /// other arm's message. Every axis the chain can decide on therefore gets a column of its + /// own, and this was the last one without. + pub avg_turn_delta: Option, + /// `Some((baseline, current))` when the matchup's own suite verdict changed, `None` when it + /// held. The third axis this chain decides on, and like the other two it needs a surface of + /// its own — a row that fails on paired outcomes prints only that reason. + pub suite_status_shift: Option<(SuiteStatus, SuiteStatus)>, pub flipped_w_to_l: usize, pub flipped_l_to_w: usize, + /// `Some(_) → None` — games that stopped resolving. See `PairedSeedShift`. + pub decisive_to_draw: usize, + /// `None → Some(_)` — games that started resolving. + pub draw_to_decisive: usize, pub unchanged: usize, pub sign_test_p: Option, + pub draw_sign_test_p: Option, pub status: CompareStatus, pub reason: Option, } @@ -159,10 +173,15 @@ fn classify_row( baseline: Some(b.clone()), current: None, delta_p0_pp: None, + avg_turn_delta: None, + suite_status_shift: None, flipped_w_to_l: 0, flipped_l_to_w: 0, + decisive_to_draw: 0, + draw_to_decisive: 0, unchanged: 0, sign_test_p: None, + draw_sign_test_p: None, status: CompareStatus::Removed, reason: Some("matchup removed from current report".to_string()), }, @@ -183,10 +202,15 @@ fn classify_row( baseline: None, current: Some(c.clone()), delta_p0_pp: None, + avg_turn_delta: None, + suite_status_shift: None, flipped_w_to_l: 0, flipped_l_to_w: 0, + decisive_to_draw: 0, + draw_to_decisive: 0, unchanged: 0, sign_test_p: None, + draw_sign_test_p: None, status, reason, } @@ -198,6 +222,34 @@ fn classify_row( let paired = paired_seed_shift(b, c); let avg_turn_delta = c.avg_turns - b.avg_turns; + // TIER ORDER IS LOAD-BEARING. Two earlier drafts of this comment overclaimed what it + // buys — the first said "no input that reaches Fail or Warn today can change verdict" + // (false: Warn escalates), the second said "nothing with a flat draw axis changes + // verdict" (false once the status axis landed in the same commit). Both were caught by + // review running the claim through the compiled base. So, by construction and stated + // to its exact edge: + // + // 1. Nothing that reaches Fail today changes verdict. The W/L Fail arm is still + // first and its counters are byte-identical to before, so it wins every input it + // used to win. + // 2. A row whose draw counters are EQUAL and where NEITHER report's status is + // `Fail` takes precisely the pre-change path. That is the full precondition, not + // a draw-axis-only one: every new guard below needs either unequal draw counters + // or a `Fail` on one side, so with both absent none can fire. This is what keeps + // identity comparisons and every unaffected regression bit-for-bit unchanged. + // 3. A W/L *Warn* DOES escalate to Fail — via the draw Fail arm when the draw axis + // is significantly negative, and via the status Fail arm when the matchup newly + // fails its own suite check. Both escalations are intended: those are the + // failures this gate exists to catch, and suppressing either because the win/loss + // axis also wobbled insignificantly would reintroduce the same blindness one case + // narrower. Pinned by `draw_regression_escalates_an_insignificant_win_loss_warn` + // and `status_regression_escalates_an_insignificant_win_loss_warn` — one per + // escalating arm, because a claim about an arm that no test exercises is how the + // first two drafts of this comment stayed wrong. + // + // Do not reorder to group same-axis arms together: moving either Fail arm below the + // W/L Warn arm silently demotes its clause-3 escalation back to Warn. Both reorders + // are covered by the two tests named above. let (status, reason) = if paired.flipped_w_to_l > paired.flipped_l_to_w && paired.sign_test_p.is_some_and(|p| p < 0.05) { @@ -210,6 +262,41 @@ fn classify_row( paired.sign_test_p.unwrap_or(1.0), )), ) + } else if paired.decisive_to_draw > paired.draw_to_decisive + && paired.draw_sign_test_p.is_some_and(|p| p < 0.05) + { + // Games that used to resolve stopped resolving: the signature of a stalled or + // looping AI. Asymmetric BY CONSTRUCTION — this arm requires decisive→draw to + // dominate, so the improvement direction can never reach Fail through it. + ( + CompareStatus::Fail, + Some(format!( + "paired draw regression: decisive→draw={} draw→decisive={} sign-test p={:.4}", + paired.decisive_to_draw, + paired.draw_to_decisive, + paired.draw_sign_test_p.unwrap_or(1.0), + )), + ) + } else if b.status != SuiteStatus::Fail && c.status == SuiteStatus::Fail { + // The matchup newly fails its OWN suite check (mirror imbalance, expectation + // violation). Without this arm the comparison read the paired game outcomes and + // nothing else, so a run could carry `status: "Fail"` on an existing matchup and + // still exit 0 — measured on `.ab/noC-1.json`, whose enchantress-mirror row is + // `status: "Fail"` while its compare section reported `0 FAIL, 0 WARN, 3 PASS`. + // + // Keyed on `Fail` specifically, not on any status change, because `Fail` is the + // only status this file already acts on: the new-matchup arm above matches + // `SuiteStatus::Fail` and treats `Pass`/`Open` alike. Same authority, same + // vocabulary, extended from new matchups to existing ones. + ( + CompareStatus::Fail, + Some(format!( + "matchup status regressed {:?} → {:?}: {}", + b.status, + c.status, + c.fail_reason.as_deref().unwrap_or("no reason"), + )), + ) } else if paired.flipped_w_to_l != paired.flipped_l_to_w { ( CompareStatus::Warn, @@ -220,6 +307,49 @@ fn classify_row( paired.sign_test_p.unwrap_or(1.0), )), ) + } else if paired.decisive_to_draw != paired.draw_to_decisive { + // Any imbalance on the draw axis is reported, in either direction. The improvement + // direction (draw→decisive dominating) lands HERE and never above: a comparator + // that stayed silent about games that started resolving would be hiding a + // behavior change, which is the same reason the win/loss tier warns on L→W too. + ( + CompareStatus::Warn, + Some(format!( + "paired draw shift: decisive→draw={} draw→decisive={} sign-test p={:.4}", + paired.decisive_to_draw, + paired.draw_to_decisive, + paired.draw_sign_test_p.unwrap_or(1.0), + )), + ) + } else if b.status == SuiteStatus::Fail { + // The baseline already recorded this matchup as failing. One arm, two messages, + // rather than two sibling arms: the axis is `b.status == Fail` and `c.status` is + // the parameter. + // + // Neither case can reach Fail — the arm above requires the regression direction — + // but neither may be silent either: + // * Recovery is a behavior change, and a comparator that hid it would be as + // wrong as one that hid a regression (the same reason the W/L tier warns on + // L→W and the draw tier warns on draw→decisive). + // * STILL failing is reported every run rather than passing quietly. Review + // showed this is reachable, not theoretical: `--refresh-baseline` writes the + // current report verbatim with no `any_fail` check, so one refresh from a + // failing run would otherwise make that matchup exit 0 forever. + // + // It is a Warn and not a Fail deliberately: the exit code answers "did this change + // make things worse", and the baseline — however it got that way — already + // sanctions this state. Making a baseline-sanctioned failure red is a policy call + // about whether a baseline may bless a failure at all, which belongs with the + // `--refresh-baseline` guard in `bin/ai_gate.rs`, not here. + let reason = if c.status == SuiteStatus::Fail { + format!( + "matchup still failing (baseline also Fail): {}", + c.fail_reason.as_deref().unwrap_or("no reason") + ) + } else { + format!("matchup status recovered {:?} → {:?}", b.status, c.status) + }; + (CompareStatus::Warn, Some(reason)) } else if matches!(c.expected, Expected::Mirror { .. }) && avg_turn_delta.abs() > MIRROR_AVG_TURN_WARN_DELTA { @@ -237,10 +367,15 @@ fn classify_row( baseline: Some(b.clone()), current: Some(c.clone()), delta_p0_pp: Some(delta_pp), + avg_turn_delta: Some(avg_turn_delta), + suite_status_shift: (b.status != c.status).then_some((b.status, c.status)), flipped_w_to_l: paired.flipped_w_to_l, flipped_l_to_w: paired.flipped_l_to_w, + decisive_to_draw: paired.decisive_to_draw, + draw_to_decisive: paired.draw_to_decisive, unchanged: paired.unchanged, sign_test_p: paired.sign_test_p, + draw_sign_test_p: paired.draw_sign_test_p, status, reason, } @@ -251,8 +386,15 @@ fn classify_row( struct PairedSeedShift { flipped_w_to_l: usize, flipped_l_to_w: usize, + /// `Some(_) → None`: a game that used to resolve no longer does. The regression signal — + /// the signature of a stalled or looping AI. + decisive_to_draw: usize, + /// `None → Some(_)`: a game that used to stall now resolves. The improvement signal. + draw_to_decisive: usize, unchanged: usize, sign_test_p: Option, + /// Sign test on the draw axis, computed exactly like `sign_test_p` is on the win/loss axis. + draw_sign_test_p: Option, } fn paired_seed_shift(baseline: &MatchupResult, current: &MatchupResult) -> PairedSeedShift { @@ -260,16 +402,28 @@ fn paired_seed_shift(baseline: &MatchupResult, current: &MatchupResult) -> Paire current.games.iter().map(|game| (game.seed, game)).collect(); let mut flipped_w_to_l = 0; let mut flipped_l_to_w = 0; + let mut decisive_to_draw = 0; + let mut draw_to_decisive = 0; let mut unchanged = 0; for baseline_game in &baseline.games { let Some(current_game) = current_by_seed.get(&baseline_game.seed) else { continue; }; + // EXHAUSTIVE, no `_` fallback. The wildcard this replaces is how the decisive→draw class + // became invisible: it swept `Some(_) → None` into `unchanged`, so a matchup could lose + // most of its winners and the comparison would report no movement at all. Listing every + // shape means a future `winner` representation breaks the build instead of silently + // rejoining `unchanged`. match (baseline_game.winner, current_game.winner) { (Some(0), Some(1)) => flipped_w_to_l += 1, (Some(1), Some(0)) => flipped_l_to_w += 1, - _ => unchanged += 1, + (Some(_), None) => decisive_to_draw += 1, + (None, Some(_)) => draw_to_decisive += 1, + // Same winner, or drawn on both sides. Non-0/1 seat pairs land here as they always + // have — the duel suite is two-player, and changing that classification is out of + // this change's scope. + (Some(_), Some(_)) | (None, None) => unchanged += 1, } } @@ -277,11 +431,18 @@ fn paired_seed_shift(baseline: &MatchupResult, current: &MatchupResult) -> Paire let sign_test_p = (flips > 0).then(|| sign_test_mid_p_upper_tail(flips, flipped_w_to_l.max(flipped_l_to_w))); + let draw_flips = decisive_to_draw + draw_to_decisive; + let draw_sign_test_p = (draw_flips > 0) + .then(|| sign_test_mid_p_upper_tail(draw_flips, decisive_to_draw.max(draw_to_decisive))); + PairedSeedShift { flipped_w_to_l, flipped_l_to_w, + decisive_to_draw, + draw_to_decisive, unchanged, sign_test_p, + draw_sign_test_p, } } @@ -320,11 +481,48 @@ fn status_str(s: CompareStatus) -> &'static str { } } -/// Render a markdown table of the comparison to stdout + emit a summary line. -pub fn print_markdown(report: &CompareReport) { - println!(); - println!("| matchup | exercises | baseline p0% | current p0% | flips W→L | flips L→W | sign p | status |"); - println!("|---------|-----------|--------------|-------------|-----------|-----------|--------|--------|"); +/// The column headers, in order. Named once so tests can assert that every axis the verdict chain +/// decides on has a surface here — see `COLUMNS` usage in `render_markdown` and the invariant below. +const COLUMNS: &[&str] = &[ + "matchup", + "exercises", + "baseline p0%", + "current p0%", + "flips W→L", + "flips L→W", + "sign p", + "dec→draw", + "draw→dec", + "draw sign p", + "Δ avg turns", + "suite status", + "status", +]; + +/// Build the markdown table as a string. +/// +/// INVARIANT: every axis the verdict chain can decide on has a column here. The chain is +/// first-match-wins, so whichever arm fires suppresses every other arm's reason string — a row that +/// warns on the win/loss axis printed only the W/L reason and hid its draw movement, and a row that +/// warns on the draw axis hid its mirror avg-turn drift the same way. Columns are the only surface +/// that survives that suppression, so each axis owns one: W/L flips + sign p, draw counters + draw +/// sign p, avg-turn delta, and suite status. **Adding a verdict arm means adding a column.** +/// +/// Separated from `print_markdown` so that invariant is enforced by tests rather than asserted in a +/// comment: `markdown_has_a_column_for_every_verdict_axis` and `markdown_rows_are_rectangular` read +/// this string. When it was inlined in a `println!`, both new columns could be deleted with the +/// whole suite still green. +fn render_markdown(report: &CompareReport) -> String { + let mut out = String::new(); + out.push_str(&format!("| {} |\n", COLUMNS.join(" | "))); + out.push_str(&format!( + "|{}|\n", + COLUMNS + .iter() + .map(|c| "-".repeat(c.chars().count() + 2)) + .collect::>() + .join("|") + )); for row in &report.rows { let exercises: Vec = row.exercises.iter().map(|f| format!("{f:?}")).collect(); let baseline_cell = match &row.baseline { @@ -339,23 +537,58 @@ pub fn print_markdown(report: &CompareReport) { Some(p) => format!("{p:.4}"), None => "—".to_string(), }; - println!( - "| {} | {} | {} | {} | {} | {} | {} | {} |", - row.matchup_id, + let draw_sign_p_cell = match row.draw_sign_test_p { + Some(p) => format!("{p:.4}"), + None => "—".to_string(), + }; + let avg_turn_cell = match row.avg_turn_delta { + Some(d) => format!("{d:+.1}"), + None => "—".to_string(), + }; + // Falls back to the CURRENT status when nothing shifted, so the column is populated on + // every row that has a current report — including new matchups, whose verdict is decided + // from `c.status` but which have no shift to show. A blank cell under a status-decided + // verdict would break the invariant above. + let suite_status_cell = match (row.suite_status_shift, row.current.as_ref()) { + (Some((before, after)), _) => format!("{before:?}→{after:?}"), + (None, Some(c)) => format!("{:?}", c.status), + (None, None) => "—".to_string(), + }; + let cells = [ + row.matchup_id.clone(), exercises.join(", "), baseline_cell, current_cell, - row.flipped_w_to_l, - row.flipped_l_to_w, + row.flipped_w_to_l.to_string(), + row.flipped_l_to_w.to_string(), sign_p_cell, - status_str(row.status), - ); + row.decisive_to_draw.to_string(), + row.draw_to_decisive.to_string(), + draw_sign_p_cell, + avg_turn_cell, + suite_status_cell, + status_str(row.status).to_string(), + ]; + debug_assert_eq!(cells.len(), COLUMNS.len()); + out.push_str(&format!("| {} |\n", cells.join(" | "))); if let Some(reason) = &row.reason { if !matches!(row.status, CompareStatus::Pass) { - println!("| ↳ _{reason}_ | | | | | | | |"); + // Reason spans one labelled cell plus blanks for the rest, so the row stays + // rectangular no matter how many columns the table has. + out.push_str(&format!( + "| ↳ _{reason}_ |{}\n", + " |".repeat(COLUMNS.len() - 1) + )); } } } + out +} + +/// Render the comparison table to stdout + emit a summary line. +pub fn print_markdown(report: &CompareReport) { + println!(); + print!("{}", render_markdown(report)); let mut pass = 0usize; let mut warn = 0usize; @@ -427,6 +660,982 @@ mod tests { } } + /// Build a matchup from explicit `(seed, winner, turns)` rows, so a fixture can carry a + /// REAL recorded run instead of the synthetic win/loss ladder `mk_result` generates. + fn mk_result_from_games(id: &str, rows: &[(u64, Option, u32)]) -> MatchupResult { + let games: Vec = rows + .iter() + .map(|(seed, winner, turns)| GameResult { + seed: *seed, + winner: *winner, + turns: *turns, + }) + .collect(); + let p0_wins = games.iter().filter(|g| g.winner == Some(0)).count(); + let p1_wins = games.iter().filter(|g| g.winner == Some(1)).count(); + let draws = games.iter().filter(|g| g.winner.is_none()).count(); + MatchupResult { + matchup_id: id.into(), + exercises: vec![FeatureKind::AggroPressure], + p0_label: "A".into(), + p1_label: "B".into(), + expected: Expected::Mirror { tolerance: 0.15 }, + p0_wins, + p1_wins, + draws, + games, + total_turns: 0, + total_duration_ms: 0, + avg_turns: 10.0, + avg_duration_ms: 1000.0, + status: SuiteStatus::Pass, + fail_reason: None, + attribution: None, + } + } + + /// **P2 — the paired-seed shift is blind to decisive→draw.** + /// + /// HISTORICAL, not synthetic. These ten rows are the committed `suite-baseline.json`'s + /// `enchantress-mirror` games paired by seed against a real recorded gate run + /// (`.ab/noC-1.json`, the A+B+D leg of #6969). Eight of the ten stopped having a winner — + /// baseline 4 p0 / 6 p1 / 0 draws became 1 / 1 / **8**. + /// + /// `paired_seed_shift` only recognizes `Some(0)→Some(1)` and `Some(1)→Some(0)`; every + /// `Some(_)→None` falls into the `_` arm and is tallied as UNCHANGED. So `flips == 0`, + /// `sign_test_p == None`, and the comparison PASSES a matchup in which 80% of the games + /// stopped resolving. That is the exact signature of a stalled or looping AI, and a branch + /// that drew every game would pass this gate. + #[test] + fn paired_seed_shift_counts_decisive_to_draw_as_a_shift() { + let baseline = mk_result_from_games( + "enchantress-mirror", + &[ + (10593729, Some(1), 10), + (10593730, Some(0), 18), + (10593731, Some(1), 12), + (10593732, Some(1), 14), + (10593733, Some(1), 11), + (10593734, Some(0), 15), + (10593735, Some(0), 15), + (10593736, Some(0), 14), + (10593737, Some(1), 18), + (10593738, Some(1), 19), + ], + ); + let current = mk_result_from_games( + "enchantress-mirror", + &[ + (10593729, None, 8), + (10593730, None, 12), + (10593731, Some(1), 12), + (10593732, None, 11), + (10593733, None, 9), + (10593734, None, 12), + (10593735, Some(0), 15), + (10593736, None, 13), + (10593737, None, 17), + (10593738, None, 9), + ], + ); + + // PREMISE: the fixture really carries the shift it claims (8 lost winners, 2 kept). + assert_eq!( + baseline.draws, 0, + "premise: the baseline matchup had no draws" + ); + assert_eq!(current.draws, 8, "premise: the recorded run drew 8 of 10"); + + let shift = paired_seed_shift(&baseline, ¤t); + + // Before this change these eight landed in a `_ => unchanged` arm and `unchanged` read 10. + assert_eq!( + shift.unchanged, 2, + "eight games stopped having a winner; they are a SHIFT, not 'unchanged'" + ); + assert_eq!( + (shift.decisive_to_draw, shift.draw_to_decisive), + (8, 0), + "the shift is eight decisive→draw, none back" + ); + // The W/L axis is genuinely silent here, and that is CORRECT — no game changed which + // player won. Pinned so the fix is read as adding a second axis, not as repairing the + // first: a fix that started reporting W→L flips for these rows would be wrong. + assert_eq!( + (shift.flipped_w_to_l, shift.flipped_l_to_w), + (0, 0), + "PIN: no directional win/loss flip occurred; the shift is entirely decisive→draw" + ); + } + + /// The end-to-end consequence: the matchup must NOT pass. + /// + /// This is the claim that matters to a reviewer — not that a private counter is wrong, but + /// that `cargo ai-gate`'s comparison reported no problem for a run in which 8 of 10 games + /// stopped resolving. Both matchups are marked `Pass` in their own right (the suite-status + /// half is a separate check), so this row isolates the COMPARISON's verdict. + /// + /// Measured before the fix: `CompareStatus::Pass`. + #[test] + fn compare_fails_a_matchup_that_lost_eight_of_ten_winners() { + let rows_before: &[(u64, Option, u32)] = &[ + (10593729, Some(1), 10), + (10593730, Some(0), 18), + (10593731, Some(1), 12), + (10593732, Some(1), 14), + (10593733, Some(1), 11), + (10593734, Some(0), 15), + (10593735, Some(0), 15), + (10593736, Some(0), 14), + (10593737, Some(1), 18), + (10593738, Some(1), 19), + ]; + let rows_after: &[(u64, Option, u32)] = &[ + (10593729, None, 8), + (10593730, None, 12), + (10593731, Some(1), 12), + (10593732, None, 11), + (10593733, None, 9), + (10593734, None, 12), + (10593735, Some(0), 15), + (10593736, None, 13), + (10593737, None, 17), + (10593738, None, 9), + ]; + let baseline = mk_report(vec![mk_result_from_games( + "enchantress-mirror", + rows_before, + )]); + let current = mk_report(vec![mk_result_from_games("enchantress-mirror", rows_after)]); + + let result = compare(&baseline, ¤t, &CompareOptions).unwrap(); + assert_eq!(result.rows.len(), 1, "premise: exactly one paired matchup"); + + // n=8, k=8 ⇒ mid-p = 1/512 ≈ 0.00195 < 0.05, so this reaches Fail, not merely Warn. + assert_eq!( + result.rows[0].status, + CompareStatus::Fail, + "8 of 10 games stopped having a winner — the comparison must not report Pass; reason={:?}", + result.rows[0].reason + ); + // The counters and their ORDER are pinned, not just the word "draw": transposing them in + // the reason string would report the recorded incident as an improvement. + assert!( + result.rows[0] + .reason + .as_deref() + .is_some_and(|r| r.contains("decisive→draw=8 draw→decisive=0")), + "reason={:?}", + result.rows[0].reason + ); + assert!(result.any_fail(), "the compare exit code must reflect it"); + } + + /// TRIVIALIZE control. A fix that failed on ANY draw-axis movement, ignoring direction and + /// significance, would pass every other test in this module — including both demonstration + /// rows — and would be wrong. Games that STARTED resolving are an improvement. + /// + /// Same magnitude as the regression row (8 of 10), opposite direction. Must be Warn: reported + /// because any imbalance is worth surfacing, never Fail. + #[test] + fn draw_to_decisive_improvement_warns_but_never_fails() { + let stalled: &[(u64, Option, u32)] = &[ + (1, None, 8), + (2, None, 12), + (3, Some(1), 12), + (4, None, 11), + (5, None, 9), + (6, None, 12), + (7, Some(0), 15), + (8, None, 13), + (9, None, 17), + (10, None, 9), + ]; + let resolving: &[(u64, Option, u32)] = &[ + (1, Some(1), 10), + (2, Some(0), 18), + (3, Some(1), 12), + (4, Some(1), 14), + (5, Some(1), 11), + (6, Some(0), 15), + (7, Some(0), 15), + (8, Some(0), 14), + (9, Some(1), 18), + (10, Some(1), 19), + ]; + let baseline = mk_report(vec![mk_result_from_games("enchantress-mirror", stalled)]); + let current = mk_report(vec![mk_result_from_games("enchantress-mirror", resolving)]); + + let result = compare(&baseline, ¤t, &CompareOptions).unwrap(); + assert_eq!( + result.rows[0].draw_to_decisive, 8, + "premise: this fixture really is the improvement direction" + ); + assert_eq!( + result.rows[0].decisive_to_draw, 0, + "premise: nothing regressed on the draw axis" + ); + // The statistic is computed on `max(decisive_to_draw, draw_to_decisive)`, so it must read + // the DOMINANT direction whichever one that is. Replacing the `max` with `decisive_to_draw` + // leaves the verdict correct (the Fail arm's dominance guard protects it) but prints + // 0.9980 instead of 0.0020 in the reason string and the `draw sign p` column. + assert!( + result.rows[0].draw_sign_test_p.is_some_and(|p| p < 0.05), + "the reported statistic must describe the 8-0 shift, not its complement; got {:?}", + result.rows[0].draw_sign_test_p + ); + assert_eq!( + result.rows[0].status, + CompareStatus::Warn, + "improvement is reported, never failed; reason={:?}", + result.rows[0].reason + ); + assert!(!result.any_fail(), "an improvement must not fail the gate"); + } + + /// **The escalation the tier order actually produces.** An independent review measured this + /// input against both the pre-change and post-change comparator and found the earlier + /// "no input that reaches Fail or Warn today can change verdict" claim FALSE: the draw Fail + /// arm sits above the W/L Warn arm, so a Warn escalates to Fail. + /// + /// The escalation is intended — 8 of 11 games ceasing to resolve is the failure this gate + /// exists to catch, and suppressing it because the win/loss axis also wobbled insignificantly + /// would reintroduce the same blindness one case narrower. It is pinned here because the + /// claim that it *couldn't* happen was the reason it went untested. + /// + /// The two premise asserts below are what make this a proof of escalation without compiling + /// the old code: `sign_test_p > 0.05` means the W/L Fail arm cannot fire, and + /// `flipped_w_to_l != flipped_l_to_w` means the old chain fell to the W/L Warn arm. Warn + /// before, Fail now. + #[test] + fn draw_regression_escalates_an_insignificant_win_loss_warn() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(0), 10), + (3, Some(0), 10), + (4, Some(1), 10), + (5, Some(1), 10), + (6, Some(1), 10), + (7, Some(1), 10), + (8, Some(1), 10), + (9, Some(1), 10), + (10, Some(1), 10), + (11, Some(1), 10), + ]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(1), 10), + (2, Some(1), 10), + (3, Some(1), 10), + (4, None, 10), + (5, None, 10), + (6, None, 10), + (7, None, 10), + (8, None, 10), + (9, None, 10), + (10, None, 10), + (11, None, 10), + ]; + let baseline = mk_report(vec![mk_result_from_games("m", before)]); + let current = mk_report(vec![mk_result_from_games("m", after)]); + let row = &compare(&baseline, ¤t, &CompareOptions).unwrap().rows[0]; + + // PREMISE 1: the W/L Fail arm cannot be what fired — 3 flips one way is p=0.0625. + assert_eq!((row.flipped_w_to_l, row.flipped_l_to_w), (3, 0)); + assert!( + row.sign_test_p.is_some_and(|p| p > 0.05), + "premise: the win/loss axis is INSIGNIFICANT, so the W/L Fail arm is out; got {:?}", + row.sign_test_p + ); + // PREMISE 2: the old chain therefore reached the W/L Warn arm (`w2l != l2w`). + assert_ne!( + row.flipped_w_to_l, row.flipped_l_to_w, + "premise: this input used to land on the win/loss Warn arm" + ); + + assert_eq!( + row.status, + CompareStatus::Fail, + "a significant draw regression escalates a W/L Warn to Fail; reason={:?}", + row.reason + ); + assert!( + row.reason.as_deref().is_some_and(|r| r.contains("draw")), + "the draw arm won, not the W/L Warn arm below it; reason={:?}", + row.reason + ); + } + + /// First-match-wins means a firing arm suppresses every other arm's reason string. Here the + /// draw Warn arm shadows the mirror avg-turn Warn arm: same status, different message, and + /// the drift magnitude would vanish entirely if the row did not carry it. + /// + /// That is why `avg_turn_delta` is a field and a column rather than only a reason string — + /// the same argument that put the draw counters in the table. + #[test] + fn mirror_drift_magnitude_survives_a_shadowing_reason() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, Some(1), 10), + ]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, None, 22), + ]; + let baseline = mk_report(vec![mk_result_from_games("mirror", before)]); + let mut current_result = mk_result_from_games("mirror", after); + current_result.avg_turns = 16.0; // baseline is 10.0 → +6.0, past MIRROR_AVG_TURN_WARN_DELTA + let current = mk_report(vec![current_result]); + let row = &compare(&baseline, ¤t, &CompareOptions).unwrap().rows[0]; + + // PREMISE: the mirror avg-turn arm WOULD have fired — it is genuinely shadowed, not absent. + assert!(matches!( + row.current.as_ref().unwrap().expected, + Expected::Mirror { .. } + )); + assert_eq!(row.avg_turn_delta, Some(6.0)); + assert!( + row.avg_turn_delta.unwrap().abs() > MIRROR_AVG_TURN_WARN_DELTA, + "premise: the drift is past the warn threshold" + ); + // PREMISE: one game stopped resolving, but not significantly (n=1 ⇒ p=0.25). + assert_eq!((row.decisive_to_draw, row.draw_to_decisive), (1, 0)); + assert!(row.draw_sign_test_p.is_some_and(|p| p > 0.05)); + + assert_eq!(row.status, CompareStatus::Warn); + assert!( + row.reason + .as_deref() + .is_some_and(|r| r.contains("decisive→draw=1 draw→decisive=0")), + "the draw arm shadows the avg-turn arm's message, with its counters in order; reason={:?}", + row.reason + ); + assert!( + !row.reason.as_deref().unwrap().contains("avg-turn"), + "premise of this test: the avg-turn message really is suppressed" + ); + } + + /// The no-draw-shift control: with the draw counters equal, both draw guards (`>` and `!=`) + /// are false and the chain falls through as it did before this change. + /// + /// Scope, stated precisely because an earlier version of this doc overclaimed: the fixture is + /// an identity comparison, so ALL axes are flat, and it therefore pins only the draw guards' + /// inertness — not the full precondition of invariant 2, and not the tier order, which + /// `draw_regression_escalates_an_insignificant_win_loss_warn` pins. + #[test] + fn compare_without_draw_shift_is_unaffected() { + let rows: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(0), 11), + (3, Some(1), 12), + (4, Some(1), 13), + ]; + let report = mk_report(vec![mk_result_from_games("red-mirror", rows)]); + let result = compare(&report, &report, &CompareOptions).unwrap(); + + assert_eq!( + ( + result.rows[0].decisive_to_draw, + result.rows[0].draw_to_decisive + ), + (0, 0), + "premise: the draw axis is flat, so the new tiers must be inert" + ); + assert_eq!(result.rows[0].draw_sign_test_p, None); + assert_eq!(result.rows[0].status, CompareStatus::Pass); + assert!(!result.any_fail()); + } + + /// **The status axis, isolated.** An existing matchup that newly fails its OWN suite check + /// must fail the comparison. Before this arm existed, `classify_row`'s paired branch read the + /// game outcomes and nothing else, so a run could carry `status: "Fail"` on an existing + /// matchup and still exit 0. + /// + /// Every outcome axis is held FLAT here (premise-asserted below), so the status axis is the + /// only thing that can produce a verdict — which is what makes this the drop-mutant's target. + #[test] + fn status_regression_to_fail_flags_a_matchup_with_unchanged_outcomes() { + let games: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, Some(1), 10), + ]; + let baseline = mk_report(vec![mk_result_from_games("enchantress-mirror", games)]); + let mut failing = mk_result_from_games("enchantress-mirror", games); + failing.status = SuiteStatus::Fail; + // Verbatim from `.ab/noC-1.json`'s enchantress-mirror row. + failing.fail_reason = + Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50".into()); + let current = mk_report(vec![failing]); + let row = &compare(&baseline, ¤t, &CompareOptions).unwrap().rows[0]; + + // PREMISE: every outcome axis is flat, so nothing above the status arm can fire. + assert_eq!((row.flipped_w_to_l, row.flipped_l_to_w), (0, 0)); + assert_eq!((row.decisive_to_draw, row.draw_to_decisive), (0, 0)); + assert_eq!(row.avg_turn_delta, Some(0.0)); + assert_eq!( + row.suite_status_shift, + Some((SuiteStatus::Pass, SuiteStatus::Fail)) + ); + + assert_eq!( + row.status, + CompareStatus::Fail, + "a matchup that started failing its own suite check must fail the comparison; reason={:?}", + row.reason + ); + assert!( + row.reason + .as_deref() + .is_some_and(|r| r.contains("Pass → Fail") && r.contains("mirror imbalance")), + "the reason names the shift AND carries the matchup's own fail_reason; got {:?}", + row.reason + ); + } + + /// **The recorded incident, whole — and it had TWO independent holes.** + /// + /// This is `.ab/noC-1.json`'s `enchantress-mirror` row as recorded: the eight games that + /// stopped resolving AND `status: "Fail"` with its Wilson-CI reason. The run's compare section + /// nonetheless printed `0 FAIL, 0 WARN, 3 PASS`, because neither the draw axis nor the suite + /// status axis existed in the comparison. Both are asserted present so a future edit that + /// closes one and reopens the other cannot pass this row. + /// + /// The reason string is the draw one: the draw Fail arm sits above the status Fail arm, so the + /// more specific outcome diagnosis wins. The status hole is still visible on the row via + /// `suite_status_shift` — which is exactly why every axis carries a field and a column instead + /// of relying on the first-match-wins reason. + #[test] + fn noc1_enchantress_row_carries_both_holes() { + let before: &[(u64, Option, u32)] = &[ + (10593729, Some(1), 10), + (10593730, Some(0), 18), + (10593731, Some(1), 12), + (10593732, Some(1), 14), + (10593733, Some(1), 11), + (10593734, Some(0), 15), + (10593735, Some(0), 15), + (10593736, Some(0), 14), + (10593737, Some(1), 18), + (10593738, Some(1), 19), + ]; + let after: &[(u64, Option, u32)] = &[ + (10593729, None, 8), + (10593730, None, 12), + (10593731, Some(1), 12), + (10593732, None, 11), + (10593733, None, 9), + (10593734, None, 12), + (10593735, Some(0), 15), + (10593736, None, 13), + (10593737, None, 17), + (10593738, None, 9), + ]; + let baseline = mk_report(vec![mk_result_from_games("enchantress-mirror", before)]); + let mut recorded = mk_result_from_games("enchantress-mirror", after); + recorded.status = SuiteStatus::Fail; + recorded.fail_reason = + Some("mirror imbalance: p0=0.10, Wilson 95% CI [0.02, 0.40] excludes 0.50".into()); + let current = mk_report(vec![recorded]); + let report = compare(&baseline, ¤t, &CompareOptions).unwrap(); + let row = &report.rows[0]; + + // HOLE 1: eight games stopped resolving. + assert_eq!((row.decisive_to_draw, row.draw_to_decisive), (8, 0)); + // HOLE 2: the matchup failed its own suite check. + assert_eq!( + row.suite_status_shift, + Some((SuiteStatus::Pass, SuiteStatus::Fail)) + ); + + assert_eq!(row.status, CompareStatus::Fail); + assert!(report.any_fail(), "the recorded run must not exit 0"); + assert!( + row.reason.as_deref().is_some_and(|r| r.contains("draw")), + "the outcome diagnosis is the more specific one and wins; reason={:?}", + row.reason + ); + } + + /// Status-axis asymmetry, same shape as the draw axis: a matchup that STOPPED failing is an + /// improvement. It is reported, because a comparator silent about it would hide a behavior + /// change — but it can never Fail, since the Fail arm requires the regression direction. + #[test] + fn status_recovery_warns_but_never_fails() { + let games: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, Some(1), 10), + ]; + let mut was_failing = mk_result_from_games("enchantress-mirror", games); + was_failing.status = SuiteStatus::Fail; + was_failing.fail_reason = Some("mirror imbalance".into()); + let baseline = mk_report(vec![was_failing]); + let current = mk_report(vec![mk_result_from_games("enchantress-mirror", games)]); + let report = compare(&baseline, ¤t, &CompareOptions).unwrap(); + let row = &report.rows[0]; + + assert_eq!( + row.suite_status_shift, + Some((SuiteStatus::Fail, SuiteStatus::Pass)), + "premise: this fixture really is the recovery direction" + ); + assert_eq!( + row.status, + CompareStatus::Warn, + "recovery is reported, never failed; reason={:?}", + row.reason + ); + // The merged arm's two branches share a status, so only the message distinguishes them. + // Without this the recovery branch had no output assertion at all and could be emptied. + assert!( + row.reason + .as_deref() + .is_some_and(|r| r.contains("recovered") && r.contains("Fail → Pass")), + "reason={:?}", + row.reason + ); + assert!(!report.any_fail(), "an improvement must not fail the gate"); + + // Second branch case: `Fail → Open` is also recovery. Keying the still-failing branch on + // `c.status != Pass` instead of `== Fail` would mislabel this row as still failing, and + // the `Fail → Pass` case above cannot tell the two keyings apart. + let mut was_failing = mk_result_from_games("enchantress-mirror", games); + was_failing.status = SuiteStatus::Fail; + let mut now_open = mk_result_from_games("enchantress-mirror", games); + now_open.status = SuiteStatus::Open; + let report = compare( + &mk_report(vec![was_failing]), + &mk_report(vec![now_open]), + &CompareOptions, + ) + .unwrap(); + assert!( + report.rows[0] + .reason + .as_deref() + .is_some_and(|r| r.contains("recovered") && r.contains("Fail → Open")), + "reason={:?}", + report.rows[0].reason + ); + } + + /// The status analogue of `draw_regression_escalates_an_insignificant_win_loss_warn`, and it + /// exists because round 2 of review showed the status Fail arm's placement was UNPINNED: + /// moving it below the W/L Warn arm left the whole suite green. The concrete consequence — + /// measured — was `any_fail() == false` on a matchup that newly fails its own suite check. + /// + /// Same premise structure: the W/L axis is insignificant (p=0.0625) so the W/L Fail arm cannot + /// fire, and `w2l != l2w` so the pre-change chain landed on the W/L Warn arm. Warn before, + /// Fail now. + #[test] + fn status_regression_escalates_an_insignificant_win_loss_warn() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(0), 10), + (3, Some(0), 10), + (4, Some(1), 10), + (5, Some(1), 10), + (6, Some(1), 10), + (7, Some(1), 10), + (8, Some(1), 10), + (9, Some(1), 10), + (10, Some(1), 10), + (11, Some(1), 10), + ]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(1), 10), + (2, Some(1), 10), + (3, Some(1), 10), + (4, Some(1), 10), + (5, Some(1), 10), + (6, Some(1), 10), + (7, Some(1), 10), + (8, Some(1), 10), + (9, Some(1), 10), + (10, Some(1), 10), + (11, Some(1), 10), + ]; + let baseline = mk_report(vec![mk_result_from_games("m", before)]); + let mut failing = mk_result_from_games("m", after); + failing.status = SuiteStatus::Fail; + failing.fail_reason = Some("mirror imbalance".into()); + let report = compare(&baseline, &mk_report(vec![failing]), &CompareOptions).unwrap(); + let row = &report.rows[0]; + + // PREMISE: the W/L axis is insignificant, so the W/L Fail arm is out... + assert_eq!((row.flipped_w_to_l, row.flipped_l_to_w), (3, 0)); + assert!(row.sign_test_p.is_some_and(|p| p > 0.05)); + // ...and the draw axis is flat, so neither draw arm can fire either. + assert_eq!((row.decisive_to_draw, row.draw_to_decisive), (0, 0)); + + assert_eq!( + row.status, + CompareStatus::Fail, + "a matchup that newly fails its own check escalates a W/L Warn to Fail; reason={:?}", + row.reason + ); + assert!(report.any_fail(), "and the gate must exit non-zero"); + assert!( + row.reason + .as_deref() + .is_some_and(|r| r.contains("status regressed")), + "the status arm won, not the W/L Warn arm below it; reason={:?}", + row.reason + ); + } + + /// A matchup that was ALREADY failing in the baseline and is still failing is reported every + /// run, not passed over in silence. + /// + /// Reachable, not theoretical: `--refresh-baseline` writes the current report verbatim with no + /// `any_fail` check, so a single refresh from a failing run would otherwise make that matchup + /// exit 0 forever. Warn rather than Fail is deliberate — see the arm's comment. + #[test] + fn a_matchup_still_failing_is_reported_every_run() { + let games: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + let mut was_failing = mk_result_from_games("m", games); + was_failing.status = SuiteStatus::Fail; + was_failing.fail_reason = Some("mirror imbalance".into()); + let mut still_failing = mk_result_from_games("m", games); + still_failing.status = SuiteStatus::Fail; + still_failing.fail_reason = Some("mirror imbalance".into()); + let report = compare( + &mk_report(vec![was_failing]), + &mk_report(vec![still_failing]), + &CompareOptions, + ) + .unwrap(); + let row = &report.rows[0]; + + // PREMISE: the status did not shift, and every outcome axis is flat — so a chain that + // only looked at transitions and outcomes would have nothing at all to say here. + assert_eq!(row.suite_status_shift, None); + assert_eq!((row.flipped_w_to_l, row.flipped_l_to_w), (0, 0)); + assert_eq!((row.decisive_to_draw, row.draw_to_decisive), (0, 0)); + + assert_eq!(row.status, CompareStatus::Warn); + assert!( + row.reason + .as_deref() + .is_some_and(|r| r.contains("still failing")), + "reason={:?}", + row.reason + ); + assert!( + !report.any_fail(), + "reported, but not red: the baseline already sanctions this state" + ); + } + + /// The column invariant, enforced instead of asserted in a comment. Round 2 of review measured + /// that both new columns could be deleted with the entire suite still green, because the table + /// was built inline in `println!` and nothing read it. + /// + /// One column per axis the verdict chain can decide on: win/loss, draw, avg-turn, suite status. + #[test] + fn markdown_has_a_column_for_every_verdict_axis() { + for axis in [ + "flips W→L", + "flips L→W", + "sign p", + "dec→draw", + "draw→dec", + "draw sign p", + "Δ avg turns", + "suite status", + ] { + assert!( + COLUMNS.contains(&axis), + "the chain can decide a verdict on {axis} but the table has no column for it" + ); + } + let report = mk_report(vec![mk_result("red-mirror", 5, 10, SuiteStatus::Pass)]); + let rendered = render_markdown(&compare(&report, &report, &CompareOptions).unwrap()); + let header = rendered.lines().next().unwrap(); + // Exact cell equality, not `contains`. The mutation this catches is on the RENDERED header + // line: drop its last cell and a containment loop still passes, because the surviving + // "suite status" cell CONTAINS the string "status". Measured — that variant went from + // killing 1 test to killing 2. + // + // Scope, stated because the two mutations are easy to conflate: against a mutation of + // `COLUMNS` itself this assertion is tautological, since the header is generated from + // `COLUMNS`. That variant is caught anyway, by the `debug_assert_eq!` on cell count in + // `render_markdown` and by the rectangularity test. + let header_cells: Vec<&str> = header + .split('|') + .map(str::trim) + .filter(|c| !c.is_empty()) + .collect(); + assert_eq!(header_cells, COLUMNS, "header cells drifted from COLUMNS"); + + // `status_str`'s `Pass` arm is the last rendered arm of that 5-variant cluster with no + // assertion binding it — WARN, FAIL, NEW and REMOVED are each pinned by a cell assertion + // elsewhere, and mutating `Pass => "PASS"` survived the whole suite. This fixture is an + // identity comparison, so it is the only one that renders a Pass row. + assert_eq!(cell(&rendered, "red-mirror", "status"), "PASS"); + } + + /// Read the cell under a named column, so an assertion binds a column to the field it renders + /// rather than to a position. Splitting `| a | b |` yields a leading empty segment, hence +1. + fn cell<'a>(rendered: &'a str, matchup_id: &str, column: &str) -> &'a str { + let index = COLUMNS + .iter() + .position(|c| *c == column) + .unwrap_or_else(|| panic!("no column named {column}")); + let line = rendered + .lines() + .find(|l| l.starts_with(&format!("| {matchup_id} |"))) + .unwrap_or_else(|| panic!("no row for {matchup_id} in:\n{rendered}")); + line.split('|').nth(index + 1).unwrap().trim() + } + + /// **Every column carries the value it claims to.** Round 3 of review measured that the table + /// was pinned by header *label* only: freezing the `Δ avg turns` cell to a constant, and + /// swapping the `dec→draw` / `draw→dec` cells so the recorded incident would print its counters + /// backwards, both survived the entire suite. A column that renders the wrong number defeats + /// the invariant exactly as thoroughly as a missing one, since columns are the only surface + /// that survives first-match-wins reason suppression. + /// + /// The fixture gives every numeric axis a DISTINCT value (2, 3, 4, 1, and two different + /// p-values), so no pair of cells can be transposed without changing the rendered text. + #[test] + fn markdown_cells_carry_their_own_column_values() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(0), 10), + (3, Some(1), 10), + (4, Some(1), 10), + (5, Some(1), 10), + (6, Some(0), 10), + (7, Some(0), 10), + (8, Some(1), 10), + (9, Some(1), 10), + (10, None, 10), + (11, Some(0), 10), + ]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(1), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, Some(0), 10), + (5, Some(0), 10), + (6, None, 10), + (7, None, 10), + (8, None, 10), + (9, None, 10), + (10, Some(0), 10), + (11, Some(0), 10), + // Present only in the current report. `paired_seed_shift` walks the BASELINE games and + // skips unmatched seeds, so this moves the win rate without touching a flip counter — + // which is what makes the two p0% cells differ while the four counters stay distinct. + (12, Some(0), 10), + ]; + let baseline = mk_report(vec![mk_result_from_games("distinct", before)]); + let mut current_result = mk_result_from_games("distinct", after); + current_result.avg_turns = 16.0; // baseline 10.0 → +6.0 + let report = compare(&baseline, &mk_report(vec![current_result]), &CompareOptions).unwrap(); + let row = &report.rows[0]; + + // PREMISE: the four counters really are pairwise distinct, so a transposition must show. + assert_eq!( + ( + row.flipped_w_to_l, + row.flipped_l_to_w, + row.decisive_to_draw, + row.draw_to_decisive + ), + (2, 3, 4, 1) + ); + // PREMISE: the two win-rate cells DIFFER. They rendered identical `45%` in the first + // version of this test, so swapping them survived — the premise has to be asserted, not + // assumed, or "no pair of cells can be transposed" is false for the pair nobody checked. + assert_ne!( + winrate(row.baseline.as_ref().unwrap()), + winrate(row.current.as_ref().unwrap()) + ); + + let rendered = render_markdown(&report); + assert_eq!(cell(&rendered, "distinct", "exercises"), "AggroPressure"); + assert_eq!(cell(&rendered, "distinct", "baseline p0%"), "45%"); + assert_eq!(cell(&rendered, "distinct", "current p0%"), "50%"); + assert_eq!(cell(&rendered, "distinct", "flips W→L"), "2"); + assert_eq!(cell(&rendered, "distinct", "flips L→W"), "3"); + assert_eq!(cell(&rendered, "distinct", "dec→draw"), "4"); + assert_eq!(cell(&rendered, "distinct", "draw→dec"), "1"); + assert_eq!(cell(&rendered, "distinct", "sign p"), "0.3438"); + assert_eq!(cell(&rendered, "distinct", "draw sign p"), "0.1094"); + assert_eq!(cell(&rendered, "distinct", "Δ avg turns"), "+6.0"); + assert_eq!(cell(&rendered, "distinct", "suite status"), "Pass"); + assert_eq!(cell(&rendered, "distinct", "status"), "WARN"); + } + + /// The `suite status` column's SHIFT branch — the one the status axis exists to surface — was + /// rendered by no test: replacing it with a constant survived the whole suite, because every + /// other rendered fixture has `suite_status_shift == None`. Round 3's finding, one column over. + #[test] + fn markdown_renders_a_suite_status_shift() { + let games: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + let baseline = mk_report(vec![mk_result_from_games("shifted", games)]); + let mut failing = mk_result_from_games("shifted", games); + failing.status = SuiteStatus::Fail; + failing.fail_reason = Some("mirror imbalance".into()); + let report = compare(&baseline, &mk_report(vec![failing]), &CompareOptions).unwrap(); + + // PREMISE: this row really carries a shift, so the fallback branch is not what renders it. + assert_eq!( + report.rows[0].suite_status_shift, + Some((SuiteStatus::Pass, SuiteStatus::Fail)) + ); + let rendered = render_markdown(&report); + assert_eq!(cell(&rendered, "shifted", "suite status"), "Pass→Fail"); + assert_eq!(cell(&rendered, "shifted", "status"), "FAIL"); + } + + /// Every emitted row — header, separator, data, and the reason continuation — must have the + /// same cell count, or the table renders broken in the nightly drift issue that + /// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row + /// that warns (so its reason continuation is emitted), a New row, and a Removed row. + #[test] + fn markdown_rows_are_rectangular() { + let paired_before: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + let paired_after: &[(u64, Option, u32)] = &[(1, Some(1), 10), (2, Some(1), 10)]; + let baseline = mk_report(vec![ + mk_result_from_games("paired", paired_before), + mk_result_from_games("removed", paired_before), + ]); + let current = mk_report(vec![ + mk_result_from_games("paired", paired_after), + mk_result_from_games("brand-new", paired_after), + ]); + let report = compare(&baseline, ¤t, &CompareOptions).unwrap(); + let rendered = render_markdown(&report); + + // PREMISE: all four row shapes really are present, otherwise this measures less than it + // claims to. + assert!( + rendered.contains("| ↳ _"), + "no reason continuation emitted" + ); + assert!(rendered.contains("| NEW |"), "no New row emitted"); + assert!(rendered.contains("| REMOVED |"), "no Removed row emitted"); + + // A New row's verdict is decided FROM its suite status, so that column must carry a value + // even though there is no shift to show. It read `—` until review measured it. + let new_row = rendered.lines().find(|l| l.contains("| NEW |")).unwrap(); + assert!( + new_row.contains("| Pass | NEW |"), + "a status-decided verdict must not print a blank status cell: {new_row}" + ); + + // The `—` fallbacks are what New and Removed rows print in the paired-only columns, and + // every one of them survived a fully green suite until review measured it. A New row has no + // baseline to compare against; a Removed row has no current report at all. + // + // All six sites are listed deliberately. The first pass at this pinned five and missed + // `draw sign p` — which is the fallback the real gate renders on every row today, since a + // matchup with a flat draw axis has no statistic to report. Sweeping a defect by recipe + // means sweeping ALL of its sites; a partial sweep reads as complete. + assert_eq!(cell(&rendered, "brand-new", "baseline p0%"), "—"); + assert_eq!(cell(&rendered, "brand-new", "sign p"), "—"); + assert_eq!(cell(&rendered, "brand-new", "draw sign p"), "—"); + assert_eq!(cell(&rendered, "brand-new", "Δ avg turns"), "—"); + assert_eq!(cell(&rendered, "removed", "current p0%"), "—"); + assert_eq!(cell(&rendered, "removed", "suite status"), "—"); + assert_eq!( + cell(&rendered, "removed", "status"), + "REMOVED", + "and the Removed row still reaches its own verdict" + ); + + // The separator is GENERATED from the header widths, so its validity is not free: an empty + // fill still has the right pipe count but renders the whole table as one paragraph in the + // issue body. Measured: `"-".repeat(0)` survives a pipe-count-only check. + // + // Indexed rather than filtered on purpose. The obvious spelling — + // `.split('|').filter(|s| !s.is_empty())` — is VACUOUS against exactly the mutation this + // is here to catch: with empty fills every segment is empty, the filter drops all of them, + // and the loop body never runs. Measured that too, while fixing this. + let separator = rendered.lines().nth(1).unwrap(); + let segments: Vec<&str> = separator.split('|').collect(); + assert_eq!( + segments.len(), + COLUMNS.len() + 2, + "separator is not bounded by pipes with one segment per column: {separator}" + ); + for segment in &segments[1..=COLUMNS.len()] { + assert!( + segment.len() >= 3 && segment.chars().all(|c| c == '-'), + "separator segment {segment:?} is not a valid markdown rule: {separator}" + ); + } + + let expected = COLUMNS.len() + 1; // n cells ⇒ n+1 pipes + for line in rendered.lines() { + assert_eq!( + line.matches('|').count(), + expected, + "row has the wrong cell count: {line}" + ); + } + } + + /// The mirror avg-turn arm is guarded on `Expected::Mirror`, and every other fixture in this + /// module is a mirror — so deleting that guard survived the whole suite. A `Triangle` matchup + /// has no symmetry expectation, and a turn-count drift is not a finding for it. + /// + /// Pre-existing arm, pinned here because this change reorders around it: the new draw and + /// status Warn arms sit directly above it. + #[test] + fn avg_turn_drift_is_only_a_finding_for_mirrors() { + let games: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + let mut base = mk_result_from_games("triangle", games); + base.expected = Expected::Triangle { + p0_winrate_min: 0.4, + p0_winrate_max: 0.6, + }; + let mut drifted = mk_result_from_games("triangle", games); + drifted.expected = Expected::Triangle { + p0_winrate_min: 0.4, + p0_winrate_max: 0.6, + }; + drifted.avg_turns = 20.0; // +10.0, far past MIRROR_AVG_TURN_WARN_DELTA + + let report = compare( + &mk_report(vec![base]), + &mk_report(vec![drifted]), + &CompareOptions, + ) + .unwrap(); + + // PREMISE: the drift really is past the threshold, so only the Mirror guard suppresses it. + assert_eq!(report.rows[0].avg_turn_delta, Some(10.0)); + assert!(report.rows[0].avg_turn_delta.unwrap() > MIRROR_AVG_TURN_WARN_DELTA); + // PREMISE: nothing else could produce a verdict here. + assert_eq!( + ( + report.rows[0].flipped_w_to_l, + report.rows[0].flipped_l_to_w, + report.rows[0].decisive_to_draw, + report.rows[0].draw_to_decisive + ), + (0, 0, 0, 0) + ); + + assert_eq!( + report.rows[0].status, + CompareStatus::Pass, + "a non-mirror matchup has no symmetry expectation to drift from; reason={:?}", + report.rows[0].reason + ); + } + #[test] fn compare_identity_is_pass() { let report = mk_report(vec![mk_result("red-mirror", 5, 10, SuiteStatus::Pass)]); @@ -443,11 +1652,17 @@ mod tests { let result = compare(&baseline, ¤t, &CompareOptions).unwrap(); assert!(result.any_fail()); assert_eq!(result.rows[0].status, CompareStatus::Fail); - assert!(result.rows[0] - .reason - .as_ref() - .unwrap() - .contains("paired regression")); + // The counters and their ORDER, not just the phrase: transposing the two format args here + // printed `W→L=0 L→W=10` for this very fixture — a regression reported as an improvement — + // and survived the whole suite until review measured it. + assert!( + result.rows[0] + .reason + .as_deref() + .is_some_and(|r| r.contains("paired regression: W→L=10 L→W=0")), + "reason={:?}", + result.rows[0].reason + ); } #[test] @@ -466,6 +1681,15 @@ mod tests { let result = compare(&baseline, ¤t, &CompareOptions).unwrap(); assert!(!result.any_fail()); assert_eq!(result.rows[0].status, CompareStatus::Warn); + // Same in-order pin on the win/loss Warn reason as on its Fail sibling above. + assert!( + result.rows[0] + .reason + .as_deref() + .is_some_and(|r| r.contains("paired shift: W→L=0 L→W=2")), + "reason={:?}", + result.rows[0].reason + ); } #[test] From a87efda5ee77e2c9e06bc8921b6ffff2d9aa125c Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 04:54:22 -0500 Subject: [PATCH 2/8] fix(ai): refuse incomparable reports and stop discarding unpaired samples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the paired-seed gate could still return Pass without comparing the full sample. `compare` validated only `schema_version`, while `paired_seed_shift` built a seed→game map from the current report and `continue`d past any baseline seed missing from it — and, because pairing walks the baseline, never visited extra current games at all. Two reports sharing no seeds scored zero on every counter and passed. This is a false-green in the gate the rest of this PR hardens. Two different defects hide behind one symptom, and they need opposite remedies. A different `base_seed` or `difficulty` makes the same seed NUMBER denote a different game, so pairing by it compares unrelated samples and calls the difference drift. Nothing downstream can rescue that, so `compare` now returns `CompareError::WorkloadMismatch { field, baseline, current }` before classifying any row. That is one parameterized variant rather than a sibling per field, mirroring `PerfCompareError::WorkloadMismatch`, which already solved this exact problem for the perf comparator — the guard is reuse, not invention. A different `games_per_matchup` is not that. It changes how many seeds exist, not what a seed means, so the samples that do pair remain comparable. It is also live: the nightly runs `--games 100` against a `games_per_matchup: 10` baseline, so erroring would convert a working job into a hard failure — and since the report goes to stdout while compare errors go to stderr, the workflow's empty-report branch would fail the job outright rather than opening the drift issue it exists to open. Gating it would break a live workflow to make a point that counting makes better. `PairedSeedShift` and `CompareRow` therefore carry `unpaired_baseline` and `unpaired_current`, each with a column, and a Warn arm fires when either is non-zero. The nightly keeps working and starts saying out loud that it discards 90 of every 100 samples per matchup — true today, and invisible until now. `card_data_hash` is deliberately not gated either: the committed baseline's hash matches no card-data present on a current checkout, so gating it would fail every run immediately. The perf comparator reaches the same conclusion, carrying card-data hashes as informational fields rather than as a guard. The new arm is LAST among the Warns, and the position is argued in both directions. Not higher, because every arm above reports measured drift while this one reports reduced coverage — and since the nightly satisfies this condition on every row every night, ranking it above the drift arms would replace every real headline with a coverage notice. Not absent, because without it the zero-counter Pass survives. `an_unpaired_sample_does_not_shadow_a_real_drift_reason` pins the position; promoting the arm above the W/L arm flips that test and only that test. The tier-order invariant comment is restated rather than patched. Its clause 2 was false the moment this arm landed — it said a row with equal draw counters and no `Fail` takes the pre-change path, which stopped being true for a row with unmatched seeds. That is the THIRD draft of that comment falsified by a later axis, always the same way: a precondition enumerated over the axes that existed when it was written. It now names all three conjuncts and says why one exists per guard, and a new clause 0 records that mismatched workloads no longer produce a verdict at all — including pairs that would previously have reached Fail, which is intended, because a verdict built by pairing seeds across different workloads only looked like one. `markdown_cells_carry_their_own_column_values` is re-derived rather than adjusted. It made its two win-rate cells differ by giving the current report an extra seed absent from the baseline and leaning on the skip to swallow it — the fixture was built ON the defect. It now separates the rates through the seed-8 outcome on matched seed sets, leaving all four counters at (2, 3, 4, 1), and asserts `(unpaired_baseline, unpaired_current) == (0, 0)` so a future edit cannot silently reintroduce the dependency. A census of all 27 test fns found this was the only fixture with asymmetric seed sets. Evidence. Eight mutants, tree restored byte-identical after each. Dropping either workload guard fails only that guard's test. Refusing every pair fails 29 tests including `matching_workloads_are_compared_normally`, the control arm that exists so a reject-everything mutant cannot masquerade as a working guard. Adding the `games_per_matchup` gate fails `a_different_games_per_matchup_is_reported_not_refused`, so the deliberate non-gate is pinned and cannot be "fixed" by a later reader. Reverting to the silent skip, and dropping the current-side count, each fail their own direction's test. Transposing the two counters fails both direction tests, which is why there are two fixtures and not one covering "something was unpaired": this PR has already had to fix a transposition defect that a single combined fixture would have missed. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/duel_suite/compare.rs | 367 ++++++++++++++++++++-- 1 file changed, 345 insertions(+), 22 deletions(-) diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index 1fa7021a88..6b737e48e9 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -54,6 +54,12 @@ pub struct CompareRow { /// `None → Some(_)` — games that started resolving. pub draw_to_decisive: usize, pub unchanged: usize, + /// Baseline samples the comparison could not examine, and current samples it never + /// visited. Carried on the row for the same reason every other axis is: the verdict + /// chain is first-match-wins, so whichever arm fires suppresses every other arm's + /// message, and a column is the only surface that survives that suppression. + pub unpaired_baseline: usize, + pub unpaired_current: usize, pub sign_test_p: Option, pub draw_sign_test_p: Option, pub status: CompareStatus, @@ -79,7 +85,20 @@ impl CompareReport { pub enum CompareError { Io(std::io::Error), Parse(serde_json::Error), - SchemaMismatch { baseline: u32, current: u32 }, + SchemaMismatch { + baseline: u32, + current: u32, + }, + /// A configuration that defines what a seed *means* differs between the reports, so + /// pairing by seed number would compare two unrelated games and call the difference + /// drift. One parameterized variant rather than a sibling per field, mirroring + /// `PerfCompareError::WorkloadMismatch`, which already solves this for the perf + /// comparator. + WorkloadMismatch { + field: &'static str, + baseline: String, + current: String, + }, } impl std::fmt::Display for CompareError { @@ -91,6 +110,15 @@ impl std::fmt::Display for CompareError { f, "schema_version mismatch: baseline={baseline} current={current}" ), + CompareError::WorkloadMismatch { + field, + baseline, + current, + } => write!( + f, + "{field} mismatch: baseline={baseline} current={current} — \ + the reports describe different workloads and cannot be paired by seed" + ), } } } @@ -131,6 +159,36 @@ pub fn compare( }); } + // Everything below pairs games by seed NUMBER. That is only meaningful while both + // reports derive their seeds the same way and play them under the same AI, so the two + // inputs that define what a seed means are checked before any row is classified. + // + // `games_per_matchup` is deliberately NOT here. It changes how MANY seeds exist, not + // what a seed means, so the samples that do pair are still comparable — and the nightly + // job runs `--games 100` against a `games_per_matchup: 10` baseline today, so erroring + // would break a live workflow instead of informing it. That difference is surfaced as + // the unpaired counters below, which is the honest treatment: it was previously + // discarded in silence. + // + // `card_data_hash` is also not here: the committed baseline's hash matches no card-data + // present on any current checkout, so gating it would fail every run immediately. The + // perf comparator reaches the same conclusion — it carries card-data hashes as + // informational report fields rather than as a guard. + if baseline.base_seed != current.base_seed { + return Err(CompareError::WorkloadMismatch { + field: "base_seed", + baseline: baseline.base_seed.to_string(), + current: current.base_seed.to_string(), + }); + } + if baseline.difficulty != current.difficulty { + return Err(CompareError::WorkloadMismatch { + field: "difficulty", + baseline: baseline.difficulty.clone(), + current: current.difficulty.clone(), + }); + } + // BTreeMap for deterministic iteration order. let baseline_by_id: BTreeMap<&str, &MatchupResult> = baseline .results @@ -180,6 +238,8 @@ fn classify_row( decisive_to_draw: 0, draw_to_decisive: 0, unchanged: 0, + unpaired_baseline: 0, + unpaired_current: 0, sign_test_p: None, draw_sign_test_p: None, status: CompareStatus::Removed, @@ -209,6 +269,8 @@ fn classify_row( decisive_to_draw: 0, draw_to_decisive: 0, unchanged: 0, + unpaired_baseline: 0, + unpaired_current: 0, sign_test_p: None, draw_sign_test_p: None, status, @@ -222,21 +284,31 @@ fn classify_row( let paired = paired_seed_shift(b, c); let avg_turn_delta = c.avg_turns - b.avg_turns; - // TIER ORDER IS LOAD-BEARING. Two earlier drafts of this comment overclaimed what it - // buys — the first said "no input that reaches Fail or Warn today can change verdict" - // (false: Warn escalates), the second said "nothing with a flat draw axis changes - // verdict" (false once the status axis landed in the same commit). Both were caught by - // review running the claim through the compiled base. So, by construction and stated - // to its exact edge: + // TIER ORDER IS LOAD-BEARING. THREE earlier drafts of this comment overclaimed what + // it buys — "no input that reaches Fail or Warn today can change verdict" (false: + // Warn escalates), "nothing with a flat draw axis changes verdict" (false once the + // status axis landed in the same commit), and clause 2 as previously written (false + // once the unpaired axis landed, because its precondition named only the axes that + // existed when it was written). Every one was caught by review running the claim + // through the compiled base. The failure mode is always identical: a precondition + // enumerated over today's axes, silently falsified by tomorrow's. So — stated to its + // exact edge, and explicitly scoped to the axes that exist NOW: // - // 1. Nothing that reaches Fail today changes verdict. The W/L Fail arm is still - // first and its counters are byte-identical to before, so it wins every input it - // used to win. - // 2. A row whose draw counters are EQUAL and where NEITHER report's status is - // `Fail` takes precisely the pre-change path. That is the full precondition, not - // a draw-axis-only one: every new guard below needs either unequal draw counters - // or a `Fail` on one side, so with both absent none can fire. This is what keeps - // identity comparisons and every unaffected regression bit-for-bit unchanged. + // 0. Every claim below presupposes two COMPARABLE reports. `compare` rejects a + // mismatched `base_seed` or `difficulty` with `WorkloadMismatch` before any row + // is classified, so such a pair now yields no verdict at all — INCLUDING pairs + // that would previously have produced a Fail. That is intended, not a + // regression: a verdict built by pairing seed numbers across two different + // workloads was never meaningful, it merely looked like one. + // 1. Given comparable reports, nothing that reaches Fail today changes verdict. The + // W/L Fail arm is still first and its counters are byte-identical to before, so + // it wins every input it used to win. + // 2. A row whose draw counters are EQUAL, whose seed sets match EXACTLY, and where + // NEITHER report's status is `Fail` takes precisely the pre-change path. All + // three conjuncts are required, one per guard added since: every new arm below + // needs unequal draw counters, or a `Fail` on one side, or an unmatched seed, so + // with all three absent none can fire. This is what keeps identity comparisons + // and every unaffected regression bit-for-bit unchanged. // 3. A W/L *Warn* DOES escalate to Fail — via the draw Fail arm when the draw axis // is significantly negative, and via the status Fail arm when the matchup newly // fails its own suite check. Both escalations are intended: those are the @@ -246,6 +318,12 @@ fn classify_row( // and `status_regression_escalates_an_insignificant_win_loss_warn` — one per // escalating arm, because a claim about an arm that no test exercises is how the // first two drafts of this comment stayed wrong. + // 4. A row that previously Passed with unmatched seeds now Warns. This is the + // false-green the unpaired arm exists to close: two reports sharing no seeds at + // all scored zero on every counter and passed. Pinned per direction — + // `an_unmatched_baseline_sample_warns_instead_of_passing` and + // `an_extra_current_sample_warns_instead_of_passing` — because one fixture + // covering both directions would let either direction rot unnoticed. // // Do not reorder to group same-axis arms together: moving either Fail arm below the // W/L Warn arm silently demotes its clause-3 escalation back to Warn. Both reorders @@ -357,6 +435,28 @@ fn classify_row( CompareStatus::Warn, Some(format!("mirror avg-turn drift {avg_turn_delta:+.1} turns")), ) + } else if paired.unpaired_baseline > 0 || paired.unpaired_current > 0 { + // LAST of the Warn arms, and the position is deliberate in both directions. + // + // Not higher: every arm above reports measured drift, which is what the gate + // exists to find. This one reports reduced COVERAGE — it says the other + // numbers rest on fewer samples than the reports contain, not that anything + // regressed. The nightly runs `--games 100` against a 10-game baseline, so + // this condition is true on every row every night; ranking it above the drift + // arms would replace every real headline with a coverage notice. + // + // Not absent: without it, two reports sharing no seeds at all produce zero on + // every counter and return Pass, which is the false-green this arm exists to + // close. Both counters share one arm because the remedy is identical — align + // the workloads — and splitting them would be two siblings differing only in + // a direction label, with the direction already carried by its own column. + ( + CompareStatus::Warn, + Some(format!( + "incomplete pairing: {} baseline sample(s) unmatched, {} current sample(s) never compared", + paired.unpaired_baseline, paired.unpaired_current, + )), + ) } else { (CompareStatus::Pass, None) }; @@ -374,6 +474,8 @@ fn classify_row( decisive_to_draw: paired.decisive_to_draw, draw_to_decisive: paired.draw_to_decisive, unchanged: paired.unchanged, + unpaired_baseline: paired.unpaired_baseline, + unpaired_current: paired.unpaired_current, sign_test_p: paired.sign_test_p, draw_sign_test_p: paired.draw_sign_test_p, status, @@ -392,6 +494,15 @@ struct PairedSeedShift { /// `None → Some(_)`: a game that used to stall now resolves. The improvement signal. draw_to_decisive: usize, unchanged: usize, + /// Baseline games whose seed is absent from the current report — samples the comparison + /// could not examine. Previously a bare `continue`: the sample vanished and every + /// counter below stayed silent about it, so a pair of reports sharing no seeds at all + /// scored zero on every axis and returned Pass. + unpaired_baseline: usize, + /// Current games whose seed is absent from the baseline. Pairing walks the baseline, so + /// these were never visited at all — a strictly larger current run could add any number + /// of losses and no counter would move. + unpaired_current: usize, sign_test_p: Option, /// Sign test on the draw axis, computed exactly like `sign_test_p` is on the win/loss axis. draw_sign_test_p: Option, @@ -405,11 +516,23 @@ fn paired_seed_shift(baseline: &MatchupResult, current: &MatchupResult) -> Paire let mut decisive_to_draw = 0; let mut draw_to_decisive = 0; let mut unchanged = 0; + let mut unpaired_baseline = 0; + + // Seeds present on both sides. Counting the intersection lets the current-side leftover + // be derived by subtraction instead of walked a second time, and it stays correct if a + // report ever repeats a seed (`current_by_seed` keeps one entry per seed, so deriving + // from `games.len()` alone would undercount). + let mut paired = 0usize; for baseline_game in &baseline.games { let Some(current_game) = current_by_seed.get(&baseline_game.seed) else { + // NOT a bare `continue` any more. A skipped sample is a sample the comparison + // could not examine, and staying silent about it is how a partial comparison + // reported Pass with every counter at zero. + unpaired_baseline += 1; continue; }; + paired += 1; // EXHAUSTIVE, no `_` fallback. The wildcard this replaces is how the decisive→draw class // became invisible: it swept `Some(_) → None` into `unchanged`, so a matchup could lose // most of its winners and the comparison would report no movement at all. Listing every @@ -435,12 +558,20 @@ fn paired_seed_shift(baseline: &MatchupResult, current: &MatchupResult) -> Paire let draw_sign_test_p = (draw_flips > 0) .then(|| sign_test_mid_p_upper_tail(draw_flips, decisive_to_draw.max(draw_to_decisive))); + // Current games never visited by the loop above, because pairing walks the baseline. + // `current_by_seed` is deduplicated by seed, so this is the count of distinct current + // seeds the baseline does not contain — saturating because a repeated baseline seed can + // push `paired` above the map's length without meaning anything went uncompared. + let unpaired_current = current_by_seed.len().saturating_sub(paired); + PairedSeedShift { flipped_w_to_l, flipped_l_to_w, decisive_to_draw, draw_to_decisive, unchanged, + unpaired_baseline, + unpaired_current, sign_test_p, draw_sign_test_p, } @@ -494,6 +625,8 @@ const COLUMNS: &[&str] = &[ "dec→draw", "draw→dec", "draw sign p", + "unpaired base", + "unpaired cur", "Δ avg turns", "suite status", "status", @@ -565,6 +698,8 @@ fn render_markdown(report: &CompareReport) -> String { row.decisive_to_draw.to_string(), row.draw_to_decisive.to_string(), draw_sign_p_cell, + row.unpaired_baseline.to_string(), + row.unpaired_current.to_string(), avg_turn_cell, suite_status_cell, status_str(row.status).to_string(), @@ -1405,6 +1540,187 @@ mod tests { /// /// The fixture gives every numeric axis a DISTINCT value (2, 3, 4, 1, and two different /// p-values), so no pair of cells can be transposed without changing the rendered text. + /// Condition: only `unpaired_baseline` moves. Every paired seed is UNCHANGED, so every + /// other axis reads zero — before this arm existed the row scored zero on everything and + /// returned Pass while half its samples went unexamined. + #[test] + fn an_unmatched_baseline_sample_warns_instead_of_passing() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(0), 10), + (4, Some(1), 10), + ]; + // Seeds 3 and 4 never ran in current. The two that DID pair are identical. + let after: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + + let report = compare( + &mk_report(vec![mk_result_from_games("partial", before)]), + &mk_report(vec![mk_result_from_games("partial", after)]), + &CompareOptions, + ) + .unwrap(); + let row = &report.rows[0]; + + // PREMISE: every other axis really is flat, so the verdict below can only come from + // the unpaired arm. Without this the test would pass even if some other arm fired. + assert_eq!( + ( + row.flipped_w_to_l, + row.flipped_l_to_w, + row.decisive_to_draw, + row.draw_to_decisive + ), + (0, 0, 0, 0) + ); + assert_eq!((row.unpaired_baseline, row.unpaired_current), (2, 0)); + assert_eq!(row.status, CompareStatus::Warn); + assert!(!report.any_fail()); + + let rendered = render_markdown(&report); + assert_eq!(cell(&rendered, "partial", "unpaired base"), "2"); + assert_eq!(cell(&rendered, "partial", "unpaired cur"), "0"); + assert_eq!(cell(&rendered, "partial", "status"), "WARN"); + } + + /// The other direction, and the one no counter could ever have seen: pairing walks the + /// BASELINE, so extra current samples were not skipped — they were never visited. A + /// current run could add any number of losses here and every counter would stay zero. + #[test] + fn an_extra_current_sample_warns_instead_of_passing() { + let before: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(1), 10), + (3, Some(1), 10), + (4, Some(1), 10), + ]; + + let report = compare( + &mk_report(vec![mk_result_from_games("extra", before)]), + &mk_report(vec![mk_result_from_games("extra", after)]), + &CompareOptions, + ) + .unwrap(); + let row = &report.rows[0]; + + assert_eq!( + ( + row.flipped_w_to_l, + row.flipped_l_to_w, + row.decisive_to_draw, + row.draw_to_decisive + ), + (0, 0, 0, 0) + ); + // Mirrored against the test above: this one must be (0, 2), not (2, 0). A single + // fixture covering "some unpaired sample exists" would pass with the two counters + // swapped, which is the transposition defect this PR already had to fix once. + assert_eq!((row.unpaired_baseline, row.unpaired_current), (0, 2)); + assert_eq!(row.status, CompareStatus::Warn); + assert!(!report.any_fail()); + + let rendered = render_markdown(&report); + assert_eq!(cell(&rendered, "extra", "unpaired base"), "0"); + assert_eq!(cell(&rendered, "extra", "unpaired cur"), "2"); + } + + /// The arm is LAST, so a row with real drift keeps the drift headline and reports the + /// coverage gap through its columns. This pins the tier position: promoting the unpaired + /// arm above the W/L arm would replace this reason string and flip this test. + #[test] + fn an_unpaired_sample_does_not_shadow_a_real_drift_reason() { + let before: &[(u64, Option, u32)] = &[ + (1, Some(0), 10), + (2, Some(0), 10), + (3, Some(0), 10), + (4, Some(0), 10), + ]; + let after: &[(u64, Option, u32)] = &[ + (1, Some(1), 10), + (2, Some(1), 10), + (3, Some(1), 10), + (5, Some(1), 10), + ]; + + let report = compare( + &mk_report(vec![mk_result_from_games("both", before)]), + &mk_report(vec![mk_result_from_games("both", after)]), + &CompareOptions, + ) + .unwrap(); + let row = &report.rows[0]; + + assert_eq!((row.unpaired_baseline, row.unpaired_current), (1, 1)); + assert!( + row.reason.as_deref().unwrap().starts_with("paired"), + "drift reason must survive; got {:?}", + row.reason + ); + // ...and the coverage gap is still visible, because columns outlive suppression. + let rendered = render_markdown(&report); + assert_eq!(cell(&rendered, "both", "unpaired base"), "1"); + assert_eq!(cell(&rendered, "both", "unpaired cur"), "1"); + } + + #[test] + fn a_different_base_seed_is_refused_rather_than_compared() { + let mut baseline = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + let mut current = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + baseline.base_seed = 1; + current.base_seed = 2; + + let err = compare(&baseline, ¤t, &CompareOptions).unwrap_err(); + assert!( + matches!(&err, CompareError::WorkloadMismatch { field, .. } if *field == "base_seed"), + "got {err:?}" + ); + } + + #[test] + fn a_different_difficulty_is_refused_rather_than_compared() { + let baseline = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + let mut current = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + current.difficulty = "cEDH".into(); + + let err = compare(&baseline, ¤t, &CompareOptions).unwrap_err(); + assert!( + matches!(&err, CompareError::WorkloadMismatch { field, .. } if *field == "difficulty"), + "got {err:?}" + ); + } + + /// The control arm for both guards above. Without it, a mutant that made `compare` reject + /// EVERY pair would satisfy the two mismatch tests and nothing would notice. + #[test] + fn matching_workloads_are_compared_normally() { + let baseline = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + let current = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + + assert_eq!(baseline.base_seed, current.base_seed); + assert_eq!(baseline.difficulty, current.difficulty); + let report = compare(&baseline, ¤t, &CompareOptions).expect("must compare"); + assert_eq!(report.rows.len(), 1); + } + + /// `games_per_matchup` differing must NOT error: the nightly runs `--games 100` against a + /// 10-game baseline, so erroring would break a live workflow. The samples that pair are + /// still comparable, and the ones that do not are surfaced as unpaired counts instead. + #[test] + fn a_different_games_per_matchup_is_reported_not_refused() { + let before: &[(u64, Option, u32)] = &[(1, Some(0), 10)]; + let after: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(0), 10)]; + let mut baseline = mk_report(vec![mk_result_from_games("n", before)]); + let mut current = mk_report(vec![mk_result_from_games("n", after)]); + baseline.games_per_matchup = 1; + current.games_per_matchup = 2; + + let report = compare(&baseline, ¤t, &CompareOptions).expect("must not refuse"); + assert_eq!(report.rows[0].unpaired_current, 1); + assert_eq!(report.rows[0].status, CompareStatus::Warn); + assert!(!report.any_fail()); + } + #[test] fn markdown_cells_carry_their_own_column_values() { let before: &[(u64, Option, u32)] = &[ @@ -1415,7 +1731,12 @@ mod tests { (5, Some(1), 10), (6, Some(0), 10), (7, Some(0), 10), - (8, Some(1), 10), + // p0 win, and still `Some(_) → None` in `after`, so this separates the two win-rate + // cells WITHOUT touching any of the four counters. Earlier this test made the rates + // differ by giving `after` an extra seed absent from `before` and leaning on + // `paired_seed_shift` skipping it in silence. That skip is now counted and warned on, + // so the fixture is re-derived to stand on matched seed sets instead of on a hole. + (8, Some(0), 10), (9, Some(1), 10), (10, None, 10), (11, Some(0), 10), @@ -1432,10 +1753,6 @@ mod tests { (9, None, 10), (10, Some(0), 10), (11, Some(0), 10), - // Present only in the current report. `paired_seed_shift` walks the BASELINE games and - // skips unmatched seeds, so this moves the win rate without touching a flip counter — - // which is what makes the two p0% cells differ while the four counters stay distinct. - (12, Some(0), 10), ]; let baseline = mk_report(vec![mk_result_from_games("distinct", before)]); let mut current_result = mk_result_from_games("distinct", after); @@ -1460,11 +1777,17 @@ mod tests { winrate(row.baseline.as_ref().unwrap()), winrate(row.current.as_ref().unwrap()) ); + // PREMISE: and it achieves that on MATCHED seed sets, so this test measures the + // transposition property alone. If a future edit reintroduces an unmatched seed here, the + // row would also start carrying the unpaired axis and this fixture would quietly become + // two tests wearing one name. + assert_eq!((row.unpaired_baseline, row.unpaired_current), (0, 0)); let rendered = render_markdown(&report); assert_eq!(cell(&rendered, "distinct", "exercises"), "AggroPressure"); - assert_eq!(cell(&rendered, "distinct", "baseline p0%"), "45%"); - assert_eq!(cell(&rendered, "distinct", "current p0%"), "50%"); + // 6/11 vs 5/11 — separated by the seed-8 outcome, not by an uncompared sample. + assert_eq!(cell(&rendered, "distinct", "baseline p0%"), "55%"); + assert_eq!(cell(&rendered, "distinct", "current p0%"), "45%"); assert_eq!(cell(&rendered, "distinct", "flips W→L"), "2"); assert_eq!(cell(&rendered, "distinct", "flips L→W"), "3"); assert_eq!(cell(&rendered, "distinct", "dec→draw"), "4"); From 69b2a129c81df4a3414aaf0310b0afe124466822 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 05:01:00 -0500 Subject: [PATCH 3/8] fix(ai): route every report-supplied cell through one markdown encoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comparison table interpolated report-provided text straight into pipe-delimited rows. `fail_reason` is free-form — the suite writes whatever `classify` produced — and `ai_duel compare` will read any report file it is handed, so a `|` in one of those fields adds a column to that row and a newline ends the row early. The table stops being rectangular exactly when a matchup is already failing, which is the moment the diagnostics are actually read, and the nightly `cat`s that table into a GitHub issue body. `md_cell` escapes `|` to `\|` (the documented markdown escape, which renders correctly and keeps raw CI logs readable) and folds newlines to spaces. It is applied to every cell that originates in a report rather than in this file: `matchup_id`, `exercises`, and the reason continuation row. Uniformly, and not only where a pipe looks reachable today — deciding per field means re-deciding each time a field is added, and one of those decisions will eventually be wrong. This one is uncomfortable to write, because the invariant already had a test. `markdown_rows_are_rectangular` asserted precisely the property that was broken, and passed — every one of its fixtures was pipe-free, so it verified the renderer against input that could not trigger the hazard. It was not a weak test of the right thing; it was a confident test of the wrong thing, and it made the table look guarded for as long as it existed. `report_supplied_pipes_cannot_add_columns` is the reachability arm it never had: a matchup id containing a pipe and a `fail_reason` containing two, asserted to leave the header, separator, data and reason rows all the same width. The measurement in that test had to be fixed before it could prove anything. It first counted `line.split('|')`, which cannot distinguish `\|` from `|` — so it reported the escaped output as broken, and would equally have reported a broken encoder as fine had the counts happened to line up. It now counts separators the way a markdown parser does, skipping any pipe preceded by a backslash. A test that cannot tell the fix from the defect is not evidence either way. Evidence. Four mutants, tree restored byte-identical after each, all four killing `report_supplied_pipes_cannot_add_columns`: making `md_cell` the identity; reverting the reason call site alone; reverting the matchup-id call site alone; and keeping newline folding while dropping pipe escaping. The two single-call-site mutants are the sweep check — a partial revert fails, so the fix is not pinned at only one of the places it is applied. Disclosed gap: the `exercises` cell is encoded for uniformity but no test can kill that call site. It renders `format!("{f:?}")` of a fieldless enum, so no fixture can put a pipe in it. Reverting that one site alone leaves the suite green. It is defense against a future `FeatureKind` that carries data, not a defect being fixed, and it is stated here rather than folded into the mutant count. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/duel_suite/compare.rs | 68 ++++++++++++++++++++++- 1 file changed, 65 insertions(+), 3 deletions(-) diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index 6b737e48e9..f459f9170a 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -688,8 +688,8 @@ fn render_markdown(report: &CompareReport) -> String { (None, None) => "—".to_string(), }; let cells = [ - row.matchup_id.clone(), - exercises.join(", "), + md_cell(&row.matchup_id), + md_cell(&exercises.join(", ")), baseline_cell, current_cell, row.flipped_w_to_l.to_string(), @@ -711,7 +711,8 @@ fn render_markdown(report: &CompareReport) -> String { // Reason spans one labelled cell plus blanks for the rest, so the row stays // rectangular no matter how many columns the table has. out.push_str(&format!( - "| ↳ _{reason}_ |{}\n", + "| ↳ _{}_ |{}\n", + md_cell(reason), " |".repeat(COLUMNS.len() - 1) )); } @@ -720,6 +721,21 @@ fn render_markdown(report: &CompareReport) -> String { out } +/// Encode a report-provided string for one markdown table cell. +/// +/// Every cell below that originates in a report rather than in this file goes through here. +/// Report fields are free-form and arrive from JSON — `fail_reason` is whatever the suite +/// wrote, and `ai_duel compare` will read whatever file it is handed — so a `|` silently adds +/// a column to that row and a newline ends the row early. Either way the table stops being +/// rectangular exactly when a matchup is already failing, which is the moment the diagnostics +/// are actually read. +/// +/// Applied uniformly rather than only to the fields that look risky today: deciding per field +/// means re-deciding every time a field is added, and one of those decisions will be wrong. +fn md_cell(text: &str) -> String { + text.replace('|', "\\|").replace(['\n', '\r'], " ") +} + /// Render the comparison table to stdout + emit a summary line. pub fn print_markdown(report: &CompareReport) { println!(); @@ -1825,6 +1841,52 @@ mod tests { /// same cell count, or the table renders broken in the nightly drift issue that /// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row /// that warns (so its reason continuation is emitted), a New row, and a Removed row. + /// Reachability arm for `md_cell`. `markdown_rows_are_rectangular` asserted the property + /// this test is named for, but every one of its fixtures was pipe-free — so it passed for + /// a reason unrelated to the hazard and gave false confidence about exactly the invariant + /// it claimed. A `fail_reason` is free-form text from a report, and `ai_duel compare` + /// reads whatever file it is handed, so a pipe is reachable input, not a hypothetical. + #[test] + fn report_supplied_pipes_cannot_add_columns() { + let mut baseline = mk_report(vec![mk_result("m|id", 5, 10, SuiteStatus::Pass)]); + let mut current = mk_report(vec![mk_result("m|id", 5, 10, SuiteStatus::Fail)]); + baseline.results[0].matchup_id = "m|id".into(); + current.results[0].matchup_id = "m|id".into(); + current.results[0].fail_reason = + Some("imbalance: p0=0.10 | CI [0.02, 0.40] | excludes 0.50".into()); + + let report = compare(&baseline, ¤t, &CompareOptions).unwrap(); + let rendered = render_markdown(&report); + + // PREMISE: the row really did render a reason carrying pipes, so this is not vacuous. + assert!( + rendered.contains("excludes 0.50"), + "reason must be rendered:\n{rendered}" + ); + assert_eq!(report.rows[0].status, CompareStatus::Fail); + + // Every row — header, separator, data, and the reason continuation — must have the + // same cell count. An unescaped pipe shows up here as a longer row. + // Count SEPARATORS the way a markdown parser does — a `|` preceded by a backslash is + // cell content, not a boundary. Splitting on the raw character cannot tell the escape + // from the hazard, so it would report this test green against a broken encoder. + let separators = |line: &str| { + line.char_indices() + .filter(|(i, c)| *c == '|' && (*i == 0 || !line[..*i].ends_with('\\'))) + .count() + }; + let widths: Vec = rendered + .lines() + .filter(|l| l.starts_with('|')) + .map(separators) + .collect(); + assert!(widths.len() >= 4, "expected a reason row too: {widths:?}"); + assert!( + widths.iter().all(|w| *w == widths[0]), + "pipes changed the column count: {widths:?}\n{rendered}" + ); + } + #[test] fn markdown_rows_are_rectangular() { let paired_before: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; From f347c4e146c4e74afd6cc743b7a0aeb26d9e3e0d Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 06:16:42 -0500 Subject: [PATCH 4/8] docs(ai): state the still-failing case's reachability durably MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comments justified the `Fail → Fail` Warn arm by saying `--refresh-baseline` writes the current report verbatim with no `any_fail` check. That was true when written and is about to stop being true: #7029 adds exactly that check. Both comments would then assert, in the tree, a property the tree no longer has — and because they are comments, nothing would fail. They would simply be wrong. The conclusion they support is unaffected, so only the justification changes. `Fail → Fail` stays reachable no matter what guards the write path, because nothing revalidates a committed baseline when it is loaded: a baseline blessed before the guard, or hand-edited, still carries the failure. That reason is true before and after #7029, which is the property a durable comment needs. Found by sweeping this branch's own claims against a branch in flight, rather than against the current tree — an instrument that came from review catching the same shape one layer down, where a doc comment in #7029 asserted the paired arm "never consults `status`", true at that head and false here. Four surfaces carried the claim: both comments, the commit message of the first commit on this branch, and the PR body. The commit message is deliberately left alone — this repo squash-merges, so branch messages do not survive to the tree, and a force-push to reword a doomed message would churn review for no durable effect. The PR body is corrected in place, and the source comments here. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/duel_suite/compare.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index f459f9170a..bf4e36c73c 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -410,9 +410,14 @@ fn classify_row( // wrong as one that hid a regression (the same reason the W/L tier warns on // L→W and the draw tier warns on draw→decisive). // * STILL failing is reported every run rather than passing quietly. Review - // showed this is reachable, not theoretical: `--refresh-baseline` writes the - // current report verbatim with no `any_fail` check, so one refresh from a - // failing run would otherwise make that matchup exit 0 forever. + // showed this is reachable, not theoretical: nothing revalidates a committed + // baseline when it is loaded, so a baseline that already sanctions a failure + // keeps sanctioning it, and that matchup exits 0 forever. (The wording is + // deliberately about the baseline rather than about `--refresh-baseline` + // writing without a verdict check: that WAS the mechanism, and #7029 adds the + // missing check, so naming it here would make this comment false the day that + // lands. Reachability does not depend on it — a baseline blessed before the + // guard, or hand-edited, is unaffected by any guard on the write path.) // // It is a Warn and not a Fail deliberately: the exit code answers "did this change // make things worse", and the baseline — however it got that way — already @@ -1445,9 +1450,12 @@ mod tests { /// A matchup that was ALREADY failing in the baseline and is still failing is reported every /// run, not passed over in silence. /// - /// Reachable, not theoretical: `--refresh-baseline` writes the current report verbatim with no - /// `any_fail` check, so a single refresh from a failing run would otherwise make that matchup - /// exit 0 forever. Warn rather than Fail is deliberate — see the arm's comment. + /// Reachable, not theoretical, and durably so: nothing revalidates a committed baseline on + /// load, so a baseline that already sanctions a failure keeps sanctioning it regardless of + /// what guards the write path. (#7029 adds the missing verdict check to `--refresh-baseline`; + /// this row stays reachable through baselines blessed before it, or hand-edited.) + /// + /// Warn rather than Fail is deliberate — see the arm's comment. #[test] fn a_matchup_still_failing_is_reported_every_run() { let games: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; From 10960665da6b04b6a066ebbd676d4c6e0d1edffe Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 06:51:45 -0500 Subject: [PATCH 5/8] fix(ai): refuse a mismatched sample workload where the refusal can be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A differing `games_per_matchup` was counted and Warned rather than refused, on the reasoning that the samples which do pair are genuinely comparable and that erroring would break a nightly running `--games 100` against a 10-game baseline. Both halves of that are true. The conclusion was still wrong, one layer downstream: a Warn leaves `any_fail` false, so the gate exits 0, so the nightly step SUCCEEDS — and the step that publishes the report is guarded by `if: steps.gate.outcome == 'failure'` (`.github/workflows/ai-gate.yml:90`). The new columns were written to a file nobody reads. A gate whose verdict covers a tenth of its evidence was returning Pass and saying so only where nothing was listening. Producing a diagnostic is not surfacing it. `compare` now returns `WorkloadMismatch` for `games_per_matchup`, and the diagnostics survive the refusal instead of dying with it. `render_error_markdown` puts the field, both values and the remedy on STDOUT, which is what the workflow captures as the issue body (`:87` redirects stdout into `target/ai-gate-report.md`, `:95` aborts when that file is empty). Printing the refusal to stderr alone would have converted a green false-pass into a red job with an empty issue — a different failure, not a fix. `gate_verdict` returns the stdout body and the exit code together because they are one decision: the issue posts only when the exit is non-zero AND the file is non-empty, so a test pinning either alone cannot see the mode this closes. `main` now only prints and exits. `print_markdown` moved onto the same `render_stdout` so there is still exactly one authority for the success body. The unpaired columns stay. Refusing a `games_per_matchup` mismatch does not make two same-workload reports pair perfectly — a crashed matchup or a filter change still leaves a remainder, and it is still counted rather than skipped. Consequence, stated rather than discovered later: the nightly runs `--games 100` against a `games_per_matchup: 10` baseline and will now fail every night until one of the two is aligned. The required PR gate runs `--games 10`, matches the baseline, and is unaffected — verified against the committed baseline header and `ai-gate.yml:50`. Aligning them needs either a baseline refresh or a workflow edit, neither of which belongs in this commit. `md_cell` escaped pipes before backslashes, so a report-supplied `\|` became `\\|` — an escaped backslash followed by a LIVE separator — breaking the row for exactly the input that looked already safe. Backslashes are escaped first now. The test's separator counter had the same bug in mirror image: "the previous character is a backslash" is right for `\|` and wrong for `\\|`, so it would have called the broken encoder green. It is now `md_separator_count`, keyed on run-length parity, shared by both pipe tests. Evidence. Eight mutants, tree restored byte-identical (sha256-checked) after each; kill counts transcribed from the runs, not recalled. Dropping the new guard, emptying the refusal body, forcing every exit to 0, and forcing every exit to 2 each fail a different named test — the last one is why the control arm exists, since a reject-everything implementation satisfies every refusal assertion. Reverting `md_cell` to pipes-first, and reverting the counter to the previous-character rule, each fail the backslash test; the second is the measurement that would otherwise have hidden the first. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 25 +- crates/phase-ai/src/duel_suite/compare.rs | 272 ++++++++++++++++++---- 2 files changed, 245 insertions(+), 52 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index 8099c203fe..0517b881ec 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -11,7 +11,9 @@ 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, gate_verdict, load_report, print_markdown, CompareOptions, +}; use phase_ai::duel_suite::run::{run_suite, SuiteOptions, SuiteReport}; const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/suite-baseline.json"; @@ -99,16 +101,17 @@ fn main() { } }; - let report = match compare(&baseline, ¤t, &CompareOptions) { - Ok(report) => report, - Err(err) => { - eprintln!("compare failed: {err}"); - std::process::exit(2); - } - }; - print_markdown(&report); - if report.any_fail() { - std::process::exit(1); + // stdout carries the report body — the nightly redirects it into the file it posts as a + // drift issue — so a refusal has to be printed there too, not only to stderr. `gate_verdict` + // owns both halves so the pair is testable; `main` prints and exits. + let comparison = compare(&baseline, ¤t, &CompareOptions); + if let Err(err) = &comparison { + eprintln!("compare failed: {err}"); + } + let (body, code) = gate_verdict(&comparison); + print!("{body}"); + if code != 0 { + std::process::exit(code); } } diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index bf4e36c73c..f204ac63d9 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -163,12 +163,26 @@ pub fn compare( // reports derive their seeds the same way and play them under the same AI, so the two // inputs that define what a seed means are checked before any row is classified. // - // `games_per_matchup` is deliberately NOT here. It changes how MANY seeds exist, not - // what a seed means, so the samples that do pair are still comparable — and the nightly - // job runs `--games 100` against a `games_per_matchup: 10` baseline today, so erroring - // would break a live workflow instead of informing it. That difference is surfaced as - // the unpaired counters below, which is the honest treatment: it was previously - // discarded in silence. + // `games_per_matchup` IS here, and the reasoning that kept it out is recorded because it + // was wrong in an instructive way. It changes how MANY seeds exist rather than what a seed + // means, so the samples that do pair really are comparable — from which an earlier draft + // concluded that counting the unpaired remainder (the `unpaired_*` columns below) was the + // honest treatment, and that erroring would break a live nightly that runs `--games 100` + // against a `games_per_matchup: 10` baseline. + // + // The error in that is one layer downstream of the comparator. A `Warn` leaves `any_fail` + // false, so the gate exits 0, so the nightly step SUCCEEDS, and the step that publishes the + // report is guarded by `if: steps.gate.outcome == 'failure'`. The counters were therefore + // written to a file nobody reads: a green run that compared a tenth of its sample and said + // so only where nothing was listening. Producing a diagnostic is not surfacing it. A gate + // whose verdict covers 10% of the evidence must not be able to return Pass, so this is an + // incompatibility, and the diagnostics survive it — `render_error_markdown` puts the two + // values on stdout, which is what the workflow captures as the report body, so the failure + // opens a drift issue that says exactly which knob to turn. + // + // The columns stay. A mismatch of this field is now refused, but two reports built at the + // same `games_per_matchup` can still fail to pair — a crashed matchup, a filter change — + // and that remainder is still counted rather than skipped. // // `card_data_hash` is also not here: the committed baseline's hash matches no card-data // present on any current checkout, so gating it would fail every run immediately. The @@ -188,6 +202,13 @@ pub fn compare( current: current.difficulty.clone(), }); } + if baseline.games_per_matchup != current.games_per_matchup { + return Err(CompareError::WorkloadMismatch { + field: "games_per_matchup", + baseline: baseline.games_per_matchup.to_string(), + current: current.games_per_matchup.to_string(), + }); + } // BTreeMap for deterministic iteration order. let baseline_by_id: BTreeMap<&str, &MatchupResult> = baseline @@ -738,14 +759,19 @@ fn render_markdown(report: &CompareReport) -> String { /// Applied uniformly rather than only to the fields that look risky today: deciding per field /// means re-deciding every time a field is added, and one of those decisions will be wrong. fn md_cell(text: &str) -> String { - text.replace('|', "\\|").replace(['\n', '\r'], " ") + // ORDER IS LOAD-BEARING: backslashes first, then pipes. The reverse — which this function + // did until review caught it — turns the input `\|` into `\\|`, and a markdown parser + // reads that as an escaped backslash followed by a LIVE separator, so the very input that + // looks pre-escaped is the one that breaks the row. Escaping backslashes first makes every + // backslash run even before any `\|` is introduced, so no emitted `|` can ever be preceded + // by an odd run. + text.replace('\\', "\\\\") + .replace('|', "\\|") + .replace(['\n', '\r'], " ") } -/// Render the comparison table to stdout + emit a summary line. -pub fn print_markdown(report: &CompareReport) { - println!(); - print!("{}", render_markdown(report)); - +/// Everything a successful comparison writes to stdout: the table, plus the counts line. +fn render_stdout(report: &CompareReport) -> String { let mut pass = 0usize; let mut warn = 0usize; let mut fail = 0usize; @@ -760,7 +786,56 @@ pub fn print_markdown(report: &CompareReport) { CompareStatus::Removed => removed += 1, } } - println!("\ncompare: {fail} FAIL, {warn} WARN, {pass} PASS, {new} NEW, {removed} REMOVED"); + format!( + "\n{}\ncompare: {fail} FAIL, {warn} WARN, {pass} PASS, {new} NEW, {removed} REMOVED\n", + render_markdown(report) + ) +} + +/// Render the comparison table to stdout + emit a summary line. +pub fn print_markdown(report: &CompareReport) { + print!("{}", render_stdout(report)); +} + +/// The report body for a comparison that could not be made at all. +/// +/// A refused comparison is the case most likely to be read by someone who did not run it: the +/// nightly captures stdout into `target/ai-gate-report.md` and posts it as a drift issue, and it +/// posts nothing when that file is empty. Writing the refusal only to stderr would therefore turn +/// a hard, correct refusal into "AI gate failed without a drift report" — a red job with no +/// statement of what is wrong. The remedy is named per field, because the reader of the issue is +/// the person who has to choose between re-recording the baseline and changing the workload. +pub fn render_error_markdown(err: &CompareError) -> String { + let remedy = match err { + CompareError::WorkloadMismatch { field, .. } => format!( + "The two reports were produced under different `{field}`, so their seeds do not \ + denote the same games and no verdict can be built by pairing them. Either re-record \ + the baseline under the current workload (`cargo ai-gate --refresh-baseline` with the \ + same flags this run used) or run the gate under the baseline's workload. This is not \ + drift: nothing was measured." + ), + CompareError::SchemaMismatch { .. } => "The baseline predates the current report format. \ + Re-record it with `cargo ai-gate --refresh-baseline`." + .to_string(), + CompareError::Io(_) | CompareError::Parse(_) => { + "The baseline could not be read. Check the path and the file's contents.".to_string() + } + }; + format!("## AI gate: comparison refused\n\n**{err}**\n\n{remedy}\n") +} + +/// What the gate prints on stdout and what it exits with, decided together. +/// +/// These two are one decision, not two: the nightly opens its drift issue only when the exit is +/// non-zero, and aborts with "failed without a drift report" when stdout was empty, so a change +/// that satisfies either alone silently disables the other. Returning both from one function is +/// what lets a test bind the pair — `main` only prints and exits. +pub fn gate_verdict(comparison: &Result) -> (String, i32) { + match comparison { + Ok(report) if report.any_fail() => (render_stdout(report), 1), + Ok(report) => (render_stdout(report), 0), + Err(err) => (render_error_markdown(err), 2), + } } #[cfg(test)] @@ -1555,18 +1630,13 @@ mod tests { line.split('|').nth(index + 1).unwrap().trim() } - /// **Every column carries the value it claims to.** Round 3 of review measured that the table - /// was pinned by header *label* only: freezing the `Δ avg turns` cell to a constant, and - /// swapping the `dec→draw` / `draw→dec` cells so the recorded incident would print its counters - /// backwards, both survived the entire suite. A column that renders the wrong number defeats - /// the invariant exactly as thoroughly as a missing one, since columns are the only surface - /// that survives first-match-wins reason suppression. + /// Only `unpaired_baseline` moves: every seed that pairs is UNCHANGED, so every other axis + /// reads zero. Before this arm existed such a row scored zero on everything and returned + /// Pass while half its samples went unexamined. /// - /// The fixture gives every numeric axis a DISTINCT value (2, 3, 4, 1, and two different - /// p-values), so no pair of cells can be transposed without changing the rendered text. - /// Condition: only `unpaired_baseline` moves. Every paired seed is UNCHANGED, so every - /// other axis reads zero — before this arm existed the row scored zero on everything and - /// returned Pass while half its samples went unexamined. + /// (The two paragraphs that stood here were copied from + /// `markdown_cells_carry_their_own_column_values` and described that test's distinct-value + /// fixture, which this one does not have — every counter here is deliberately zero.) #[test] fn an_unmatched_baseline_sample_warns_instead_of_passing() { let before: &[(u64, Option, u32)] = &[ @@ -1727,11 +1797,17 @@ mod tests { assert_eq!(report.rows.len(), 1); } - /// `games_per_matchup` differing must NOT error: the nightly runs `--games 100` against a - /// 10-game baseline, so erroring would break a live workflow. The samples that pair are - /// still comparable, and the ones that do not are surfaced as unpaired counts instead. + /// A differing `games_per_matchup` is refused, and this test is the inversion of one that + /// asserted the opposite. The samples that pair really are comparable, which is why the + /// earlier version counted the remainder and Warned — but a Warn keeps `any_fail` false, the + /// gate exits 0, and the nightly publishes its report only on a non-zero exit. The counters + /// existed and were never read. A verdict drawn from a tenth of the sample must not be able + /// to come back Pass. + /// + /// The two assertions below are the pair that makes this a fix rather than a trade: the row + /// count pins that no verdict is produced, and the body pins that the diagnostics survive. #[test] - fn a_different_games_per_matchup_is_reported_not_refused() { + fn a_different_games_per_matchup_is_refused_with_its_diagnostics_intact() { let before: &[(u64, Option, u32)] = &[(1, Some(0), 10)]; let after: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(0), 10)]; let mut baseline = mk_report(vec![mk_result_from_games("n", before)]); @@ -1739,10 +1815,69 @@ mod tests { baseline.games_per_matchup = 1; current.games_per_matchup = 2; - let report = compare(&baseline, ¤t, &CompareOptions).expect("must not refuse"); - assert_eq!(report.rows[0].unpaired_current, 1); - assert_eq!(report.rows[0].status, CompareStatus::Warn); - assert!(!report.any_fail()); + // PREMISE: the two fields that already gated are equal, so the refusal below can only + // come from the new one. + assert_eq!(baseline.base_seed, current.base_seed); + assert_eq!(baseline.difficulty, current.difficulty); + + let err = compare(&baseline, ¤t, &CompareOptions).expect_err("must refuse"); + assert!( + matches!(&err, CompareError::WorkloadMismatch { field, .. } if *field == "games_per_matchup"), + "unexpected error: {err:?}" + ); + + // Both values reach the reader, so the drift issue says which knob to turn. + let body = render_error_markdown(&err); + assert!(body.contains("games_per_matchup"), "{body}"); + assert!(body.contains("baseline=1"), "{body}"); + assert!(body.contains("current=2"), "{body}"); + assert!(body.contains("--refresh-baseline"), "{body}"); + } + + /// The workflow's two conditions for posting a drift issue, asserted together on the refusal + /// path: a non-zero exit (`if: steps.gate.outcome == 'failure'`) AND a non-empty stdout body + /// (`if [ ! -s target/ai-gate-report.md ]; then ... exit 1`). Pinning one alone cannot see + /// the failure this change exists to prevent — the pre-change behaviour satisfied neither on + /// a mismatched workload, and printing the refusal to stderr alone would satisfy only the + /// first, producing a red job whose issue body is empty. + #[test] + fn a_refused_comparison_exits_nonzero_and_still_writes_a_report_body() { + let mut baseline = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Pass)]); + let current = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Pass)]); + baseline.games_per_matchup = 99; + + let (body, code) = gate_verdict(&compare(&baseline, ¤t, &CompareOptions)); + assert_eq!(code, 2, "a refusal must fail the gate step"); + assert!(!body.trim().is_empty(), "the report body must not be empty"); + assert!(body.contains("games_per_matchup"), "{body}"); + } + + /// Control arm for the test above: `gate_verdict` must not be a constant. An implementation + /// returning `(something, 2)` for everything would satisfy the refusal assertions completely + /// while breaking every green run, so the three outcomes are pinned to three distinct codes + /// — and each carries a body, because the nightly aborts on an empty report file whatever + /// the exit code was. + #[test] + fn gate_verdict_maps_each_outcome_to_its_own_exit_code() { + let clean = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Pass)]); + let (pass_body, pass_code) = gate_verdict(&compare(&clean, &clean, &CompareOptions)); + assert_eq!(pass_code, 0); + assert!(pass_body.contains("compare: 0 FAIL"), "{pass_body}"); + + // A matchup that regressed into Fail: a comparison that succeeded and found drift. + let regressed = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Fail)]); + let comparison = compare(&clean, ®ressed, &CompareOptions); + // PREMISE: this really is the drift path, not another refusal. + assert!(comparison.as_ref().expect("must compare").any_fail()); + let (fail_body, fail_code) = gate_verdict(&comparison); + assert_eq!(fail_code, 1); + assert!(fail_body.contains("| n |"), "{fail_body}"); + + let mut incomparable = clean.clone(); + incomparable.schema_version += 1; + let (err_body, err_code) = gate_verdict(&compare(&incomparable, &clean, &CompareOptions)); + assert_eq!(err_code, 2); + assert!(err_body.contains("schema_version"), "{err_body}"); } #[test] @@ -1849,6 +1984,69 @@ mod tests { /// same cell count, or the table renders broken in the nightly drift issue that /// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row /// that warns (so its reason continuation is emitted), a New row, and a Removed row. + /// Count the `|` characters a markdown parser would treat as cell boundaries. + /// + /// A pipe is a separator iff the run of backslashes immediately before it has EVEN length: + /// each pair is one literal backslash, and an odd leftover escapes the pipe. The obvious + /// "is the previous character a backslash" test is what this replaces — it is right for `\|` + /// and wrong for `\\|`, which is a literal backslash followed by a LIVE separator, and being + /// wrong in exactly that direction it would call a broken encoder green. + fn md_separator_count(line: &str) -> usize { + line.char_indices() + .filter(|(i, c)| { + *c == '|' && line[..*i].chars().rev().take_while(|p| *p == '\\').count() % 2 == 0 + }) + .count() + } + + /// The escape order inside `md_cell`, pinned from the outside. Escaping pipes before + /// backslashes turns the input `\|` into `\\|` — an escaped backslash followed by a live + /// separator — so the one input that already looks escaped is the one that breaks the row. + /// `fail_reason` is free-form text from a JSON report, so a backslash is reachable input. + /// + /// Both parities are exercised, because an encoder can be wrong in either direction: `\|` + /// (odd run, must stay content) and `\\|` (even run, a real separator that must be escaped + /// into the cell). A fixture carrying only one of them cannot distinguish the two. + #[test] + fn a_backslash_before_a_pipe_cannot_smuggle_a_separator() { + let reason = r"odd \| even \\| tail"; + let baseline = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Pass)]); + let mut current = mk_report(vec![mk_result("m", 5, 10, SuiteStatus::Fail)]); + current.results[0].fail_reason = Some(reason.to_string()); + + let report = compare(&baseline, ¤t, &CompareOptions).unwrap(); + let rendered = render_markdown(&report); + + // PREMISE: the reason really was rendered, so the row below exists. + assert!(rendered.contains("tail"), "reason must render:\n{rendered}"); + + let widths: Vec = rendered + .lines() + .filter(|l| l.starts_with('|')) + .map(md_separator_count) + .collect(); + assert!(widths.len() >= 3, "expected a reason row: {widths:?}"); + assert!( + widths.iter().all(|w| *w == widths[0]), + "a backslash run changed the column count: {widths:?}\n{rendered}" + ); + + // CONTROL: the previous implementation, inlined, on this same input. Without this the + // assertion above could be passing for a reason unrelated to the escape order. + let pipes_first = |t: &str| t.replace('|', r"\|").replace(['\n', '\r'], " "); + let old_row = format!("| ↳ _{}_ |", pipes_first(reason)); + let new_row = format!("| ↳ _{}_ |", md_cell(reason)); + assert_eq!( + md_separator_count(&new_row), + 2, + "the fixed encoder must emit exactly the two boundaries this row owns: {new_row}" + ); + assert!( + md_separator_count(&old_row) > 2, + "fixture is not discriminating — the old encoder must leak a separator: {old_row}" + ); + } + /// Reachability arm for `md_cell`. `markdown_rows_are_rectangular` asserted the property /// this test is named for, but every one of its fixtures was pipe-free — so it passed for /// a reason unrelated to the hazard and gave false confidence about exactly the invariant @@ -1875,18 +2073,10 @@ mod tests { // Every row — header, separator, data, and the reason continuation — must have the // same cell count. An unescaped pipe shows up here as a longer row. - // Count SEPARATORS the way a markdown parser does — a `|` preceded by a backslash is - // cell content, not a boundary. Splitting on the raw character cannot tell the escape - // from the hazard, so it would report this test green against a broken encoder. - let separators = |line: &str| { - line.char_indices() - .filter(|(i, c)| *c == '|' && (*i == 0 || !line[..*i].ends_with('\\'))) - .count() - }; let widths: Vec = rendered .lines() .filter(|l| l.starts_with('|')) - .map(separators) + .map(md_separator_count) .collect(); assert!(widths.len() >= 4, "expected a reason row too: {widths:?}"); assert!( From 7ed0b66c3d722da66c26b99f6b3fc1cc4bf73500 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 07:54:33 -0500 Subject: [PATCH 6/8] fix(ai): surface a refused comparison in both gates, and correct why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of the previous commit returned FAIL. The code was right and two of its stated reasons were not, which matters here because the reasons are what the next editor will act on. The claim that a stderr-only refusal would leave `target/ai-gate-report.md` empty is false. `run_suite` prints the suite's own table to stdout before the baseline is ever loaded (`run.rs`, `print_markdown_table`), so on that path the file always has content and the workflow's `[ ! -s ]` abort is unreachable. My own end-to-end log showed it — the refusal appears BELOW a rendered suite table — and I read past it. What a stderr-only refusal actually produces is a red job whose issue body is a table of PASSing matchups and no statement of what failed: worse to read than an empty file, and reached by a different route. Both doc comments now say that. The empty-file failure is real, though — in the other binary. `bin/ai_perf_gate.rs` writes nothing to stdout before `compare`; every diagnostic on the way there is `eprintln!`, and its first stdout write is the success table. So a `PerfCompareError` produced exactly the outcome the previous commit message claimed it had refused to ship: zero-byte report, "Decision-cost perf gate failed without a drift report", no issue. The previous commit cited `PerfCompareError::WorkloadMismatch` as prior art for the guard and did not look at its caller. `perf::render_error_markdown` now exists and both of that binary's bail-outs — compare refusal AND baseline-read failure — print it to stdout. The markdown envelope is shared (`duel_suite::refusal_markdown`) so the two gates cannot drift into describing the same situation differently; the remedy text stays per-error-type, because the knobs differ. The same wiring gap existed on this side. `render_error_markdown`'s I/O arm was unreachable — `compare` takes two reports and does no I/O — while the real baseline-read failure in `bin/ai_gate.rs` was still exiting 2 through stderr alone. That call site is now wired to the renderer, which fixes the live gap and makes the arm reachable rather than deleting it. The remedy text was actively harmful advice and is rewritten. It told the reader to re-record the baseline under the current workload. Both gate jobs read the same default baseline at different workloads — the PR job at `--games 10`, the nightly at `--full-suite --games 100` — so with a symmetric guard no single `games_per_matchup` satisfies both: at 10 the nightly refuses, at 100 the PR job refuses, and that job has no `continue-on-error`. Following remedy 1 would have traded a red nightly for a red PR gate on every pull request. The text now names both directions and states the constraint that makes the choice non-local: a baseline is shared, so whichever workload is chosen, every invocation that compares against it has to use that one. `compare: 0 FAIL` was a degenerate assertion. Four of five counters are zero on that fixture, so the substring survives transposing pass/warn, zeroing every counter, and deleting the tally loop outright. It now pins the whole line, and a second fixture carries two distinct non-zero counts so no pair of counters can be swapped unseen. Also: the refusal heading and the schema remedy are pinned (the existing assertion read `schema_version`, which comes from `Display`, not from either); every refusal variant in both gates is covered by one loop that asserts the body carries the error AND says more than it, since an implementation that forwards the error string loses exactly the remedy; and the doc block an earlier commit stranded above a helper is returned to the test it describes. Evidence. Seven mutants, all three files sha256-restored after each. Dropping the remedy from the shared envelope fails 3 tests; reducing it to the bare error line fails 4; emptying the perf renderer fails its own; deleting remedy 2 from the workload text fails the refusal test; zeroing the tally and transposing pass/new each fail the verdict test that the old substring assertion could not see; reverting `md_cell` to pipes-first now fails on trailing-run parity as well as on the embedded pipe. Not covered, stated rather than implied: no automated test executes either binary's `main`, so the `print!`-then-exit wiring in both gates can be reverted with the whole suite green. The end-to-end runs are logs, not gates. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_gate.rs | 8 +- crates/phase-ai/src/bin/ai_perf_gate.rs | 11 +- crates/phase-ai/src/duel_suite/compare.rs | 181 +++++++++++++++++++--- crates/phase-ai/src/duel_suite/mod.rs | 12 ++ crates/phase-ai/src/duel_suite/perf.rs | 88 +++++++++++ 5 files changed, 278 insertions(+), 22 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index 0517b881ec..1fd770da62 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -12,7 +12,7 @@ use std::process::Command; use engine::database::CardDatabase; use phase_ai::config::AiDifficulty; use phase_ai::duel_suite::compare::{ - compare, gate_verdict, load_report, print_markdown, CompareOptions, + compare, gate_verdict, load_report, print_markdown, render_error_markdown, CompareOptions, }; use phase_ai::duel_suite::run::{run_suite, SuiteOptions, SuiteReport}; @@ -96,7 +96,13 @@ fn main() { let baseline = match load_report(&args.baseline) { Ok(report) => report, Err(err) => { + // Same reasoning as the compare refusal below: the nightly posts stdout, so a + // read failure that spoke only to stderr produced a red job whose issue body was + // the suite table and no statement of what went wrong. This is also the only + // caller that can reach `render_error_markdown`'s I/O arm — `compare` does no + // I/O, so before this the arm existed and was unreachable. eprintln!("failed to load baseline {}: {err}", args.baseline.display()); + print!("{}", render_error_markdown(&err)); std::process::exit(2); } }; diff --git a/crates/phase-ai/src/bin/ai_perf_gate.rs b/crates/phase-ai/src/bin/ai_perf_gate.rs index 9311c8161a..b594e5208c 100644 --- a/crates/phase-ai/src/bin/ai_perf_gate.rs +++ b/crates/phase-ai/src/bin/ai_perf_gate.rs @@ -34,8 +34,8 @@ use std::process::{Command, Stdio}; use engine::database::CardDatabase; use phase_ai::duel_suite::perf::{ compare, default_scenarios, load_report, median_report, print_markdown, print_repro_margin, - repro_margin_report, run_perf_suite, PerfReport, PERF_ACTION_CAP, PERF_BASE_SEED, - PERF_SAMPLE_COUNT, + render_error_markdown, repro_margin_report, run_perf_suite, PerfReport, PERF_ACTION_CAP, + PERF_BASE_SEED, PERF_SAMPLE_COUNT, }; use phase_ai::duel_suite::{find_matchup, resolve_deck_ref}; @@ -256,10 +256,16 @@ fn run_parent_gate(args: &Args) { return; } + // Both bail-outs print the refusal to STDOUT as well as stderr. The workflow redirects + // stdout into `target/ai-perf-gate-report.md` and posts it as a drift issue, and nothing + // on the path above this point writes to stdout — so a stderr-only refusal left that file + // at zero bytes and the workflow answered it with "failed without a drift report" and no + // issue. The exit code and a non-empty body are needed TOGETHER; either alone posts nothing. let baseline = match load_report(&args.baseline) { Ok(report) => report, Err(err) => { eprintln!("failed to load baseline {}: {err}", args.baseline.display()); + print!("{}", render_error_markdown(&err)); cleanup_temps(&temp_paths); std::process::exit(2); } @@ -269,6 +275,7 @@ fn run_parent_gate(args: &Args) { Ok(report) => report, Err(err) => { eprintln!("compare failed: {err}"); + print!("{}", render_error_markdown(&err)); cleanup_temps(&temp_paths); std::process::exit(2); } diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index f204ac63d9..da37a87d26 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -9,7 +9,7 @@ use std::collections::{BTreeMap, HashSet}; use std::path::Path; use super::run::{GameResult, MatchupResult, SuiteReport, SuiteStatus}; -use super::{Expected, FeatureKind}; +use super::{refusal_markdown, Expected, FeatureKind}; const MIRROR_AVG_TURN_WARN_DELTA: f64 = 3.0; @@ -800,28 +800,49 @@ pub fn print_markdown(report: &CompareReport) { /// The report body for a comparison that could not be made at all. /// /// A refused comparison is the case most likely to be read by someone who did not run it: the -/// nightly captures stdout into `target/ai-gate-report.md` and posts it as a drift issue, and it -/// posts nothing when that file is empty. Writing the refusal only to stderr would therefore turn -/// a hard, correct refusal into "AI gate failed without a drift report" — a red job with no -/// statement of what is wrong. The remedy is named per field, because the reader of the issue is -/// the person who has to choose between re-recording the baseline and changing the workload. +/// nightly captures stdout into `target/ai-gate-report.md` and posts it as a drift issue. +/// +/// The reason is NOT that stderr-only would leave that file empty. Review measured the opposite: +/// `run_suite` prints the suite's own table to stdout before the baseline is ever loaded +/// (`run.rs`, `print_markdown_table`), so on this path the file always has content and the +/// workflow's empty-file abort is unreachable. What a stderr-only refusal actually produces is +/// worse to read than an empty file: a red job whose issue body is a table of PASSing matchups +/// and no statement of what failed. (An earlier draft of this comment asserted the empty-file +/// story, which was false here — though it is true of `ai-perf-gate`, whose refusal path this +/// commit fixes for exactly that reason.) pub fn render_error_markdown(err: &CompareError) -> String { let remedy = match err { - CompareError::WorkloadMismatch { field, .. } => format!( + CompareError::WorkloadMismatch { + field, + baseline, + current, + } => format!( "The two reports were produced under different `{field}`, so their seeds do not \ - denote the same games and no verdict can be built by pairing them. Either re-record \ - the baseline under the current workload (`cargo ai-gate --refresh-baseline` with the \ - same flags this run used) or run the gate under the baseline's workload. This is not \ - drift: nothing was measured." + denote the same games and no verdict can be built by pairing them. This is not \ + drift: nothing was measured.\n\n\ + Two remedies. Pick the workload this gate should measure at, then make EVERY \ + invocation that reads this baseline use it — the baseline file is shared, so \ + re-recording it to suit one job starts failing this same check in every other job \ + that compares against it at a different workload.\n\n\ + 1. **Move the baseline to `{current}`** — re-record it under the workload this run \ + used (`cargo ai-gate --refresh-baseline` with this run's flags), AND align every \ + other invocation that compares against it.\n\ + 2. **Move this run to `{baseline}`** — invoke the gate under the workload the \ + baseline was recorded at (for `games_per_matchup`, the `--games` flag). Touches \ + nothing else.\n\n\ + Until one of them is done this gate fails every run, by design: a verdict built \ + from a fraction of the sample is not a verdict." ), CompareError::SchemaMismatch { .. } => "The baseline predates the current report format. \ Re-record it with `cargo ai-gate --refresh-baseline`." .to_string(), CompareError::Io(_) | CompareError::Parse(_) => { - "The baseline could not be read. Check the path and the file's contents.".to_string() + "The baseline could not be read. Check the path, and that the file is the JSON a \ + previous `--refresh-baseline` wrote." + .to_string() } }; - format!("## AI gate: comparison refused\n\n**{err}**\n\n{remedy}\n") + refusal_markdown(err, &remedy) } /// What the gate prints on stdout and what it exits with, decided together. @@ -1831,7 +1852,68 @@ mod tests { assert!(body.contains("games_per_matchup"), "{body}"); assert!(body.contains("baseline=1"), "{body}"); assert!(body.contains("current=2"), "{body}"); - assert!(body.contains("--refresh-baseline"), "{body}"); + + // BOTH remedies, named with their concrete target values. One-remedy text reads as + // "you must re-record", which is the more expensive of the two and not always the + // one the reader wants — and a reader who believes it is the only option will take + // it. Each direction is asserted through the value it moves TO, so a text that names + // both remedies but transposes their targets fails here. + assert!( + body.contains("--refresh-baseline"), + "refresh remedy missing: {body}" + ); + assert!(body.contains("--games"), "re-run remedy missing: {body}"); + assert!( + body.contains("Move the baseline to `2`"), + "remedy 1 must target the CURRENT workload: {body}" + ); + assert!( + body.contains("Move this run to `1`"), + "remedy 2 must target the BASELINE workload: {body}" + ); + } + + /// Every refusal variant renders a body, including the two `compare` itself cannot produce. + /// + /// `Io` and `Parse` are reachable only through `load_report`, which `bin/ai_gate.rs` calls + /// before `compare` — review found that arm was written and unreachable, because the caller + /// that could hit it was still failing to stderr alone. That caller is now wired to this + /// function, and this test covers the arm regardless of which caller reaches it. + /// + /// The `assert_ne!` against the bare `Display` is the discriminating half: an implementation + /// that forwarded the error string would satisfy every other assertion here while losing the + /// remedy, and losing the remedy is the whole failure mode. + #[test] + fn every_refusal_renders_a_body_that_says_more_than_the_error_line() { + let io = CompareError::Io(std::io::Error::other("disk")); + let parse = CompareError::Parse(serde_json::from_str::("{").unwrap_err()); + let schema = CompareError::SchemaMismatch { + baseline: 1, + current: 2, + }; + let workload = CompareError::WorkloadMismatch { + field: "games_per_matchup", + baseline: "10".to_string(), + current: "100".to_string(), + }; + + for err in [&io, &parse, &schema, &workload] { + let body = render_error_markdown(err); + assert!(!body.trim().is_empty(), "empty body for {err:?}"); + assert!( + body.contains("## Gate: comparison refused"), + "missing heading for {err:?}: {body}" + ); + assert!( + body.contains(&err.to_string()), + "body must carry the error itself for {err:?}: {body}" + ); + assert_ne!( + body.trim(), + err.to_string().trim(), + "body must add a remedy, not echo the error, for {err:?}" + ); + } } /// The workflow's two conditions for posting a drift issue, asserted together on the refusal @@ -1862,7 +1944,14 @@ mod tests { let clean = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Pass)]); let (pass_body, pass_code) = gate_verdict(&compare(&clean, &clean, &CompareOptions)); assert_eq!(pass_code, 0); - assert!(pass_body.contains("compare: 0 FAIL"), "{pass_body}"); + // The WHOLE line, not `contains("compare: 0 FAIL")`. Four of five counters are zero on + // this fixture, so the substring form is invariant under transposing pass/warn, under + // zeroing every counter, and under deleting the tally loop outright — measured, all + // three still contain it. A count that is only ever asserted at zero is not asserted. + assert!( + pass_body.contains("compare: 0 FAIL, 0 WARN, 1 PASS, 0 NEW, 0 REMOVED"), + "{pass_body}" + ); // A matchup that regressed into Fail: a comparison that succeeded and found drift. let regressed = mk_report(vec![mk_result("n", 5, 10, SuiteStatus::Fail)]); @@ -1873,11 +1962,35 @@ mod tests { assert_eq!(fail_code, 1); assert!(fail_body.contains("| n |"), "{fail_body}"); + // A tally with two DISTINCT non-zero counters, so no pair of counters can be + // transposed without changing the rendered line. `1 PASS` and `2 NEW` are different + // numbers in different slots; the single-row fixture above cannot see that. + let mut widened = clean.clone(); + widened + .results + .push(mk_result("fresh-a", 5, 10, SuiteStatus::Pass)); + widened + .results + .push(mk_result("fresh-b", 5, 10, SuiteStatus::Pass)); + let (mixed_body, mixed_code) = gate_verdict(&compare(&clean, &widened, &CompareOptions)); + assert_eq!(mixed_code, 0); + assert!( + mixed_body.contains("compare: 0 FAIL, 0 WARN, 1 PASS, 2 NEW, 0 REMOVED"), + "{mixed_body}" + ); + let mut incomparable = clean.clone(); incomparable.schema_version += 1; let (err_body, err_code) = gate_verdict(&compare(&incomparable, &clean, &CompareOptions)); assert_eq!(err_code, 2); + // `schema_version` comes from the Display impl, so it does NOT pin the remedy text or + // the heading. Both are asserted separately or they can be emptied in silence. assert!(err_body.contains("schema_version"), "{err_body}"); + assert!( + err_body.contains("## Gate: comparison refused"), + "{err_body}" + ); + assert!(err_body.contains("--refresh-baseline"), "{err_body}"); } #[test] @@ -1980,10 +2093,6 @@ mod tests { assert_eq!(cell(&rendered, "shifted", "status"), "FAIL"); } - /// Every emitted row — header, separator, data, and the reason continuation — must have the - /// same cell count, or the table renders broken in the nightly drift issue that - /// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row - /// that warns (so its reason continuation is emitted), a New row, and a Removed row. /// Count the `|` characters a markdown parser would treat as cell boundaries. /// /// A pipe is a separator iff the run of backslashes immediately before it has EVEN length: @@ -2045,6 +2154,36 @@ mod tests { md_separator_count(&old_row) > 2, "fixture is not discriminating — the old encoder must leak a separator: {old_row}" ); + + // Trailing backslashes, asserted on the ENCODER rather than on row width, and the + // distinction is the point. Cells are joined with `" | "`, so a trailing backslash is + // separated from the boundary by a space and the row stays rectangular under BOTH + // encoders — a width assertion here would pass for a reason unrelated to the bug. + // The property that does separate them is that an emitted cell never ends in an ODD + // backslash run, since the very next character the renderer writes is a separator's + // neighbourhood. Runs 0..=4 cover both parities and the boundary case of none. + for run in 0..=4usize { + let input = format!("tail{}", "\\".repeat(run)); + let encoded = md_cell(&input); + let trailing = encoded.chars().rev().take_while(|c| *c == '\\').count(); + assert_eq!( + trailing % 2, + 0, + "run of {run} must encode to an even trailing run, got {trailing}: {encoded:?}" + ); + // CONTROL: the previous encoder leaves odd runs odd, so this loop is discriminating + // for every odd `run` rather than trivially true. + let old_trailing = pipes_first(&input) + .chars() + .rev() + .take_while(|c| *c == '\\') + .count(); + assert_eq!( + old_trailing % 2, + run % 2, + "control drifted — the old encoder must preserve run parity: {input:?}" + ); + } } /// Reachability arm for `md_cell`. `markdown_rows_are_rectangular` asserted the property @@ -2085,6 +2224,10 @@ mod tests { ); } + /// Every emitted row — header, separator, data, and the reason continuation — must have the + /// same cell count, or the table renders broken in the nightly drift issue that + /// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row + /// that warns (so its reason continuation is emitted), a New row, and a Removed row. #[test] fn markdown_rows_are_rectangular() { let paired_before: &[(u64, Option, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)]; diff --git a/crates/phase-ai/src/duel_suite/mod.rs b/crates/phase-ai/src/duel_suite/mod.rs index 3ffed86daa..8fb3d7fd05 100644 --- a/crates/phase-ai/src/duel_suite/mod.rs +++ b/crates/phase-ai/src/duel_suite/mod.rs @@ -20,6 +20,18 @@ mod tests; use serde::{Deserialize, Serialize}; +/// Wrap a refused comparison in the markdown envelope both gates publish. +/// +/// Both `ai-gate` and `ai-perf-gate` redirect stdout into a file that +/// `.github/workflows/ai-gate.yml` posts as a drift issue, and both abort when that file +/// is empty. A refusal that reaches only stderr is therefore invisible exactly where it +/// matters most — the reader of a red job sees no statement of what failed. One envelope +/// rather than two so the two gates cannot drift into describing the same situation +/// differently; the remedy text stays per-error-type, because the knobs differ. +pub(crate) fn refusal_markdown(err: &dyn std::fmt::Display, remedy: &str) -> String { + format!("## Gate: comparison refused\n\n**{err}**\n\n{remedy}\n") +} + pub use run::{run_suite, MatchupResult, SuiteReport, SuiteStatus}; pub use snapshots::{load_snapshot, resolve_deck_ref, SnapshotError}; pub use spec::{all_matchups, find_matchup, MATCHUPS}; diff --git a/crates/phase-ai/src/duel_suite/perf.rs b/crates/phase-ai/src/duel_suite/perf.rs index 10a3d62610..d463529731 100644 --- a/crates/phase-ai/src/duel_suite/perf.rs +++ b/crates/phase-ai/src/duel_suite/perf.rs @@ -74,6 +74,7 @@ use engine::game::perf_counters::{self, PerfCounterSnapshot}; use serde::{Deserialize, Serialize}; use crate::config::AiDifficulty; +use crate::duel_suite::refusal_markdown; use super::find_matchup; use super::run::{drive_game, resolve_matchup}; @@ -711,6 +712,40 @@ fn verdict_str(v: CounterVerdict) -> &'static str { } } +/// The report body for a perf comparison that could not be made at all. +/// +/// This gate has the failure the duel-suite gate only looked like it had. Nothing in +/// `bin/ai_perf_gate.rs` writes to stdout before `compare` — every diagnostic on the way +/// there is `eprintln!` — so a refusal that reached only stderr left +/// `target/ai-perf-gate-report.md` at zero bytes, and `.github/workflows/ai-gate.yml` +/// answers an empty report by aborting with "Decision-cost perf gate failed without a +/// drift report" and posting no issue at all. The refusal was produced and then thrown +/// away. Sharing `refusal_markdown` with the duel-suite gate so the two cannot drift. +pub fn render_error_markdown(err: &PerfCompareError) -> String { + let remedy = match err { + PerfCompareError::WorkloadMismatch { + field, + baseline, + current, + } => format!( + "The samples were taken under different `{field}` (`{baseline}` vs `{current}`), so \ + their counters describe different runs and any comparison would be a false verdict. \ + Either re-record the baseline under the current workload \ + (`cargo ai-perf-gate --refresh-baseline`) — checking first that every other \ + invocation reading this baseline uses that workload too — or run the gate under the \ + baseline's workload. Nothing was measured, so this is not a perf regression." + ), + PerfCompareError::SchemaMismatch { .. } => "The baseline predates the current report \ + format. Bump `schema_version` and re-record it with \ + `cargo ai-perf-gate --refresh-baseline`." + .to_string(), + PerfCompareError::Io(_) | PerfCompareError::Parse(_) => "The baseline could not be read. \ + Check the path, and that the file is the JSON a previous `--refresh-baseline` wrote." + .to_string(), + }; + refusal_markdown(err, &remedy) +} + /// Render the comparison as a markdown table to stdout; diagnostics (hash-delta /// annotation, removed-field warning) go to stderr so a redirected stdout report /// stays a clean table. @@ -1044,6 +1079,59 @@ mod tests { )); } + /// Every refusal this gate can produce must carry a non-empty report body naming what + /// happened, because the workflow posts stdout and aborts on an empty file — so a refusal + /// that reaches only stderr posts nothing at all. Asserted over EVERY variant by + /// construction rather than over the one that is easiest to build: a variant added later + /// with no remedy would otherwise ship silently. + /// + /// The `assert_ne!` against the bare `Display` is the discriminating half. Without it a + /// `render_error_markdown` that just forwarded the error string would pass every other + /// assertion here, and that implementation is precisely the one that loses the remedy. + #[test] + fn every_refusal_renders_a_body_that_says_more_than_the_error_line() { + let io = PerfCompareError::Io(std::io::Error::other("disk")); + let parse = PerfCompareError::Parse(serde_json::from_str::("{").unwrap_err()); + let schema = PerfCompareError::SchemaMismatch { + baseline: 1, + current: 2, + }; + let workload = PerfCompareError::WorkloadMismatch { + field: "action_cap", + baseline: "10".to_string(), + current: "20".to_string(), + }; + + for err in [&io, &parse, &schema, &workload] { + let body = render_error_markdown(err); + assert!(!body.trim().is_empty(), "empty body for {err:?}"); + assert!( + body.contains("## Gate: comparison refused"), + "missing heading for {err:?}: {body}" + ); + assert!( + body.contains(&err.to_string()), + "body must carry the error itself for {err:?}: {body}" + ); + assert_ne!( + body.trim(), + err.to_string().trim(), + "body must add a remedy, not echo the error, for {err:?}" + ); + } + + // The workload arm is the reachable one in CI, so its two values and both directions + // of remedy are pinned rather than left to the loop's generic assertions. + let body = render_error_markdown(&workload); + assert!(body.contains("action_cap"), "{body}"); + assert!(body.contains("`10`") && body.contains("`20`"), "{body}"); + assert!(body.contains("--refresh-baseline"), "{body}"); + assert!( + body.contains("run the gate under the baseline's workload"), + "{body}" + ); + } + // Matrix 7: adapter totality — a distinct non-zero value per field yields one // map entry per field, values round-trip, WITHOUT hardcoding the field count. // Assigning 1..=N in the struct literal is self-flagging: adding/removing a From 73e3fe5c08d20aa8873224d8a75e779826d31382 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 08:14:21 -0500 Subject: [PATCH 7/8] fix(ai): bind the gate's exit/report contract at the process boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review's remaining point was that the new coverage stopped one layer short of the thing CI actually runs: `gate_verdict` was tested, but the two statements that use it — print the body, exit with the code — lived in binaries no test executed. A `main` that printed the refusal to stderr, or exited 0 on it, reverted the whole change with every unit test green. That is the same shape as the defect this branch started with, one level up. `emit_gate_verdict` now owns both statements, and all three binaries that compare two suite reports end in it: `ai-gate`, `ai-duel compare`, and — through its own renderer — `ai-perf-gate`. `ai-duel compare` had the identical defect and is fixed by being routed through it rather than repaired in place: its refusal spoke only to stderr, so anything redirecting that command's stdout got an empty file and no statement of what failed. `tests/gate_cli.rs` drives that contract through a real process. `ai-duel compare` is the binary under test because it is the only one of the three that needs no card database and plays no games — it reads two report files and prints a verdict — so the test costs milliseconds instead of a suite run, and it does not add a card-data load that `scripts/check-test-card-data-load.sh` exists to keep out of the test suite. Both halves are asserted on the SAME invocation, because the workflow needs both: it posts the drift issue only when the step failed, and aborts when the report file is empty, so a test pinning either alone cannot see the mode this closes. A control arm asserts a comparable pair still exits 0 with a table, since a binary that refused everything would satisfy every refusal assertion. The fixture is built from the real structs and serialised, not hand-written. The first draft hand-wrote the JSON, got `Expected`'s internally-tagged encoding wrong, and both arms failed on a parse error rather than on the contract — a fixture the binary rejects proves nothing about the binary. Constructing `SuiteReport` means a schema change breaks compilation here instead of silently producing invalid input. Evidence. Three mutants at the boundary, files restored byte-identical. Printing to stderr instead of stdout fails BOTH tests, which is what pins stdout specifically rather than "some output happened". Returning 0 unconditionally fails only the refusal test's exit assertion, and reverting `ai-duel` to its stderr-only refusal fails only the refusal test — so the exit half and the body half are each independently load-bearing at the process boundary, not just in the library. Assisted-by: ClaudeCode:claude-opus-5 --- crates/phase-ai/src/bin/ai_duel.rs | 28 ++-- crates/phase-ai/src/bin/ai_gate.rs | 5 +- crates/phase-ai/src/duel_suite/compare.rs | 14 ++ crates/phase-ai/tests/gate_cli.rs | 149 ++++++++++++++++++++++ 4 files changed, 178 insertions(+), 18 deletions(-) create mode 100644 crates/phase-ai/tests/gate_cli.rs diff --git a/crates/phase-ai/src/bin/ai_duel.rs b/crates/phase-ai/src/bin/ai_duel.rs index 49a6409fbd..88d9e7c3dd 100644 --- a/crates/phase-ai/src/bin/ai_duel.rs +++ b/crates/phase-ai/src/bin/ai_duel.rs @@ -20,8 +20,7 @@ use engine::types::player::PlayerId; use phase_ai::auto_play::run_ai_actions; use phase_ai::config::{create_config_for_players, AiDifficulty, Platform}; use phase_ai::duel_suite::compare::{ - compare as compare_reports, load_report, print_markdown as print_compare_markdown, - CompareOptions, + compare as compare_reports, emit_gate_verdict, load_report, CompareOptions, }; use phase_ai::duel_suite::run::{resolve_matchup, run_suite, AttributionMode, SuiteOptions}; use phase_ai::duel_suite::{all_matchups, find_matchup}; @@ -707,7 +706,8 @@ fn print_usage() { eprintln!("Compare mode (CI regression gate):"); eprintln!(" compare BASELINE CURRENT Diff two suite reports"); eprintln!(" reports paired-seed flips and a binomial sign-test p-value"); - eprintln!(" Exit code 0 if no regressions; 1 if any matchup FAILs."); + eprintln!(" Exit code 0 if no regressions; 1 if any matchup FAILs; 2 if the two"); + eprintln!(" reports cannot be compared at all (the refusal is printed to stdout)."); } /// Parse `compare` subcommand arguments and run the comparison. Returns the @@ -743,19 +743,17 @@ fn run_compare(args: &[String]) -> i32 { } }; - let report = match compare_reports(&baseline, ¤t, &CompareOptions) { - Ok(r) => r, - Err(e) => { - eprintln!("Compare failed: {e}"); - return 2; - } - }; - print_compare_markdown(&report); - if report.any_fail() { - 1 - } else { - 0 + // Third caller of the same two statements, and it had the same defect: a refusal spoke + // only to stderr, so anything redirecting this command's stdout got an empty file and no + // statement of what failed. Routed through the shared emitter rather than repaired in + // place — `tests/gate_cli.rs` drives THIS binary, because it is the only one of the three + // that needs no card database, so the contract is bound at a real process boundary for + // milliseconds instead of a full suite run. + let comparison = compare_reports(&baseline, ¤t, &CompareOptions); + if let Err(e) = &comparison { + eprintln!("Compare failed: {e}"); } + emit_gate_verdict(&comparison) } fn list_matchups() { diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index 1fd770da62..ec60177cc7 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -12,7 +12,7 @@ use std::process::Command; use engine::database::CardDatabase; use phase_ai::config::AiDifficulty; use phase_ai::duel_suite::compare::{ - compare, gate_verdict, load_report, print_markdown, render_error_markdown, CompareOptions, + compare, emit_gate_verdict, load_report, print_markdown, render_error_markdown, CompareOptions, }; use phase_ai::duel_suite::run::{run_suite, SuiteOptions, SuiteReport}; @@ -114,8 +114,7 @@ fn main() { if let Err(err) = &comparison { eprintln!("compare failed: {err}"); } - let (body, code) = gate_verdict(&comparison); - print!("{body}"); + let code = emit_gate_verdict(&comparison); if code != 0 { std::process::exit(code); } diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index da37a87d26..dd31326346 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -859,6 +859,20 @@ pub fn gate_verdict(comparison: &Result) -> (String } } +/// Write the gate's stdout body and return the process exit code. +/// +/// Every binary that compares two suite reports ends in these same two statements, and review +/// pointed out that a unit test on `gate_verdict` cannot see them: a `main` that printed to +/// stderr, or exited 0 on a refusal, would revert the whole fix with the suite green. Both +/// halves live here so that surface is one shared function instead of one copy per binary — +/// and `tests/gate_cli.rs` drives it through a real process, so the pairing is bound at the +/// boundary CI actually redirects, not just at the library call below it. +pub fn emit_gate_verdict(comparison: &Result) -> i32 { + let (body, code) = gate_verdict(comparison); + print!("{body}"); + code +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/phase-ai/tests/gate_cli.rs b/crates/phase-ai/tests/gate_cli.rs new file mode 100644 index 0000000000..8e6ce5ecb9 --- /dev/null +++ b/crates/phase-ai/tests/gate_cli.rs @@ -0,0 +1,149 @@ +//! The gate's process-level contract: exit status and stdout body, together. +//! +//! Review found that every test for this pairing stopped at the library call, while the two +//! statements that actually matter — print the body, exit with the code — lived in a binary no +//! test executed. A `main` that printed the refusal to stderr, or exited 0 on it, would revert +//! the fix with the whole unit suite green. +//! +//! `.github/workflows/ai-gate.yml` redirects the gate's **stdout** into a file, posts that file +//! as a drift issue only when the step **failed**, and aborts when the file is empty. Those two +//! conditions are one contract: satisfying either alone posts nothing. So these tests assert +//! both on the same invocation rather than in separate cases. +//! +//! `ai-duel compare` is the binary under test because it is the only one of the three sharing +//! `emit_gate_verdict` that needs no card database and plays no games — it reads two report +//! files and prints a verdict. That makes this a millisecond test instead of a full suite run, +//! and it exercises the same shared emitter `ai-gate` and `ai-perf-gate` end in. + +use std::process::Command; + +use phase_ai::duel_suite::run::{GameResult, MatchupResult, SuiteReport, SuiteStatus}; +use phase_ai::duel_suite::Expected; + +/// Build the fixture from the real types rather than hand-written JSON. +/// +/// The first draft of this test hand-wrote the report and got `Expected`'s encoding wrong — it +/// is an internally tagged enum — so both arms failed on a parse error instead of on the +/// contract under test. Serialising the actual structs cannot drift from the schema: a field +/// added to `SuiteReport` breaks compilation here rather than silently producing a fixture the +/// binary rejects, and the parse is exercised by the binary, not asserted by the test. +/// +/// `games_per_matchup` is the workload knob; everything else is held equal so the refusal in +/// the first test can only come from that field. +fn report_json(games_per_matchup: usize) -> String { + let report = SuiteReport { + schema_version: 2, + git_sha: None, + card_data_hash: None, + unix_timestamp_secs: 0, + difficulty: "Medium".to_string(), + games_per_matchup, + base_seed: 7, + results: vec![MatchupResult { + matchup_id: "red-mirror".to_string(), + exercises: Vec::new(), + p0_label: "a".to_string(), + p1_label: "b".to_string(), + expected: Expected::Mirror { tolerance: 0.4 }, + p0_wins: 1, + p1_wins: 0, + draws: 0, + games: vec![GameResult { + seed: 1, + winner: Some(0), + turns: 10, + }], + total_turns: 10, + total_duration_ms: 1, + avg_turns: 10.0, + avg_duration_ms: 1.0, + status: SuiteStatus::Pass, + fail_reason: None, + attribution: None, + }], + }; + serde_json::to_string_pretty(&report).expect("serialize fixture") +} + +fn write(dir: &std::path::Path, name: &str, body: &str) -> std::path::PathBuf { + let path = dir.join(name); + std::fs::write(&path, body).expect("write fixture"); + path +} + +fn run(baseline: &std::path::Path, current: &std::path::Path) -> (i32, String, String) { + let out = Command::new(env!("CARGO_BIN_EXE_ai-duel")) + .args([ + "compare", + &baseline.display().to_string(), + ¤t.display().to_string(), + ]) + .output() + .expect("spawn ai-duel"); + ( + out.status.code().expect("exit code"), + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) +} + +fn tempdir(tag: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("phase-gate-cli-{tag}-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create tempdir"); + dir +} + +/// The refusal route, asserted as the PAIR the workflow needs. A non-zero exit with an empty +/// stdout aborts the publishing step ("failed without a drift report"); a populated stdout with +/// a zero exit never reaches it. Both, or the drift issue does not exist. +#[test] +fn a_refused_comparison_exits_nonzero_and_writes_its_reason_to_stdout() { + let dir = tempdir("refuse"); + let baseline = write(&dir, "baseline.json", &report_json(10)); + let current = write(&dir, "current.json", &report_json(100)); + + let (code, stdout, stderr) = run(&baseline, ¤t); + + assert_ne!( + code, 0, + "a refusal must fail the step; stdout was:\n{stdout}" + ); + assert!( + !stdout.trim().is_empty(), + "an empty report body aborts the publishing step; stderr was:\n{stderr}" + ); + // The body must be the refusal, not merely non-empty — a table of PASSing rows with no + // statement of what failed is the outcome this whole change exists to prevent. + assert!(stdout.contains("comparison refused"), "stdout:\n{stdout}"); + assert!(stdout.contains("games_per_matchup"), "stdout:\n{stdout}"); + assert!( + stdout.contains("10") && stdout.contains("100"), + "stdout:\n{stdout}" + ); + + std::fs::remove_dir_all(&dir).ok(); +} + +/// Control arm. Without it every assertion above is satisfied by a binary that refuses +/// everything, which would be a worse regression than the one being fixed. +#[test] +fn a_comparable_pair_exits_zero_and_writes_a_table() { + let dir = tempdir("accept"); + let baseline = write(&dir, "baseline.json", &report_json(10)); + let current = write(&dir, "current.json", &report_json(10)); + + let (code, stdout, stderr) = run(&baseline, ¤t); + + assert_eq!( + code, 0, + "identical reports must compare clean; stderr:\n{stderr}" + ); + assert!(stdout.contains("| red-mirror |"), "stdout:\n{stdout}"); + assert!(stdout.contains("compare: 0 FAIL"), "stdout:\n{stdout}"); + assert!( + !stdout.contains("comparison refused"), + "control arm must not refuse; stdout:\n{stdout}" + ); + + std::fs::remove_dir_all(&dir).ok(); +} From 7d9a53ed10338a501c516b72058b1586e8a17398 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 10:58:53 -0500 Subject: [PATCH 8/8] fix(ai): publish a stdout refusal when a report cannot be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the last two instances of a defect this PR has now fixed three times: `ai-duel compare` returned 2 after an `eprintln!` alone when either `load_report` failed, so a caller redirecting stdout got an empty file and no statement of what went wrong. That is the only way the command is used in CI — `.github/workflows/ai-gate.yml` redirects the gate's stdout, posts it as a drift issue when the step failed, and aborts when the file is empty. An empty body turns a diagnosable refusal into "failed without a drift report". Both arms now render through `render_error_markdown`, the same emitter `ai-gate` and `ai-perf-gate` already reach on this path. The path stays on stderr: `CompareError` carries the cause but not the file, and a refusal reading "I/O error" without naming which of two inputs it was reading is not actionable. Fixing this made one remedy string reachable from a new place, and it was wrong there. `CompareError::Io | Parse` said "The baseline could not be read", which is false half the time now that the current-report arm renders it — so it says "report" and points at the stderr line for the side. Caught by writing the test for the current-side arm, not by review. Scope of the sweep, stated because "the same recipe" has a boundary. `run_repro_report` in `ai-perf-gate` also exits 2 from a stderr-only load failure and is deliberately NOT changed: its stdout is `print_repro_margin`, the margin gate's own report, not the drift-issue body this contract is about. Changing it would put a comparison refusal into an output nothing parses as one. The two usage errors in `run_compare` are likewise left alone — a mistyped argument has no report to publish. Evidence. Four mutants, both files restored byte-identical after each and verified with `diff -q`. | mutant | result | |---|---| | revert the baseline-side fix only | killed | | revert the current-side fix only | killed | | publish the body to stderr (the original defect's shape) | killed | | empty the remedy, leaving the envelope | killed | The first two are why the test covers both inputs rather than one: they are separate arms in the source, and a fix applied to one and not the other is precisely the shape of defect this file exists to catch. They fail the same test function but different sub-cases, and the assertion message names which. Missing and malformed are both exercised because they carry different `CompareError` variants (`Io` vs `Parse`) into the same renderer; a remedy keyed on only one would leave the other with an envelope and no content, which is the fourth mutant. Four sub-cases in total: {missing, malformed} x {baseline, current}. The "missing" fixture asserts the file really is absent, so it cannot silently degrade into a second malformed case. Measured at this tree: 3 CLI tests green, 2049 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_duel.rs | 15 ++++++- crates/phase-ai/src/duel_suite/compare.rs | 7 ++- crates/phase-ai/tests/gate_cli.rs | 53 +++++++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_duel.rs b/crates/phase-ai/src/bin/ai_duel.rs index 88d9e7c3dd..b53de5c85e 100644 --- a/crates/phase-ai/src/bin/ai_duel.rs +++ b/crates/phase-ai/src/bin/ai_duel.rs @@ -20,7 +20,8 @@ use engine::types::player::PlayerId; use phase_ai::auto_play::run_ai_actions; use phase_ai::config::{create_config_for_players, AiDifficulty, Platform}; use phase_ai::duel_suite::compare::{ - compare as compare_reports, emit_gate_verdict, load_report, CompareOptions, + compare as compare_reports, emit_gate_verdict, load_report, render_error_markdown, + CompareOptions, }; use phase_ai::duel_suite::run::{resolve_matchup, run_suite, AttributionMode, SuiteOptions}; use phase_ai::duel_suite::{all_matchups, find_matchup}; @@ -728,10 +729,21 @@ fn run_compare(args: &[String]) -> i32 { } } + // A report that cannot be READ is refused on the same terms as one that cannot be COMPARED. + // Review found these two arms spoke only to stderr while every other refusal on this path + // publishes a stdout body, so a caller redirecting stdout — which is the only way this + // command is used in CI — got an empty file and no statement of what failed. That is the + // same defect this PR fixed twice already, at `compare`'s error arm and in `ai-perf-gate`; + // these were the last two instances of it on the gate's report contract. + // + // The path stays on stderr because `CompareError` carries the cause but not the file, and a + // refusal that says "I/O error" without naming which of two inputs it was reading is not + // actionable. let baseline = match load_report(&baseline_path) { Ok(r) => r, Err(e) => { eprintln!("Failed to load baseline {}: {e}", baseline_path.display()); + print!("{}", render_error_markdown(&e)); return 2; } }; @@ -739,6 +751,7 @@ fn run_compare(args: &[String]) -> i32 { Ok(r) => r, Err(e) => { eprintln!("Failed to load current {}: {e}", current_path.display()); + print!("{}", render_error_markdown(&e)); return 2; } }; diff --git a/crates/phase-ai/src/duel_suite/compare.rs b/crates/phase-ai/src/duel_suite/compare.rs index dd31326346..06c6982be2 100644 --- a/crates/phase-ai/src/duel_suite/compare.rs +++ b/crates/phase-ai/src/duel_suite/compare.rs @@ -836,9 +836,12 @@ pub fn render_error_markdown(err: &CompareError) -> String { CompareError::SchemaMismatch { .. } => "The baseline predates the current report format. \ Re-record it with `cargo ai-gate --refresh-baseline`." .to_string(), + // "report", not "baseline": `ai-duel compare` now renders this arm for a failure on + // EITHER input, so naming the baseline would send the reader to the wrong file half the + // time. The side and the exact path are on stderr, where they are known. CompareError::Io(_) | CompareError::Parse(_) => { - "The baseline could not be read. Check the path, and that the file is the JSON a \ - previous `--refresh-baseline` wrote." + "The report could not be read. Check the path named on stderr, and that the file is \ + the JSON a previous `--refresh-baseline` wrote." .to_string() } }; diff --git a/crates/phase-ai/tests/gate_cli.rs b/crates/phase-ai/tests/gate_cli.rs index 8e6ce5ecb9..073a08df71 100644 --- a/crates/phase-ai/tests/gate_cli.rs +++ b/crates/phase-ai/tests/gate_cli.rs @@ -124,6 +124,59 @@ fn a_refused_comparison_exits_nonzero_and_writes_its_reason_to_stdout() { std::fs::remove_dir_all(&dir).ok(); } +/// A report that cannot be READ must refuse on the same terms as one that cannot be COMPARED. +/// +/// Review found these two arms returned 2 after an `eprintln!` alone, so the workflow's redirected +/// stdout stayed empty and its "failed without a drift report" abort fired instead of the refusal +/// being posted. Both inputs are covered because they are separate arms in the source — a fix +/// applied to one and not the other is exactly the shape of defect this file exists to catch. +/// +/// Missing and malformed are both exercised because they take different `CompareError` variants +/// (`Io` vs `Parse`) to the same renderer, and a remedy keyed on only one of them would leave the +/// other with an empty body. +#[test] +fn an_unreadable_report_still_publishes_a_refusal_body() { + for (case, make_bad) in [("missing", false), ("malformed", true)] { + for bad_side in ["baseline", "current"] { + let dir = tempdir(&format!("unreadable-{case}-{bad_side}")); + let good = write(&dir, "good.json", &report_json(10)); + let bad = dir.join(format!("{bad_side}-bad.json")); + if make_bad { + std::fs::write(&bad, "{ this is not a suite report").expect("write malformed"); + } + // PREMISE: the "missing" case really is missing, or it would be testing nothing. + assert_eq!(bad.exists(), make_bad, "fixture for {case}/{bad_side}"); + + let (baseline, current) = if bad_side == "baseline" { + (bad.clone(), good.clone()) + } else { + (good.clone(), bad.clone()) + }; + let (code, stdout, stderr) = run(&baseline, ¤t); + + assert_eq!(code, 2, "{case}/{bad_side} must exit 2; stderr:\n{stderr}"); + assert!( + stdout.contains("Gate: comparison refused"), + "{case}/{bad_side} must publish a refusal body on STDOUT, not stderr; \ + stdout was {} bytes:\n{stdout}", + stdout.len() + ); + // The body must say more than the header — an envelope with no remedy is the same + // empty-file problem wearing a title. + assert!( + stdout.contains("could not be read"), + "{case}/{bad_side} body must carry the remedy; stdout:\n{stdout}" + ); + // The side is what makes it actionable, and it lives on stderr by design. + assert!( + stderr.contains(bad_side), + "{case}/{bad_side} stderr must name which report failed; stderr:\n{stderr}" + ); + std::fs::remove_dir_all(&dir).ok(); + } + } +} + /// Control arm. Without it every assertion above is satisfied by a binary that refuses /// everything, which would be a worse regression than the one being fixed. #[test]