Skip to content
39 changes: 25 additions & 14 deletions crates/phase-ai/src/bin/ai_duel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +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,
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};
Expand Down Expand Up @@ -707,7 +707,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
Expand All @@ -728,34 +729,44 @@ 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;
}
};
let current = match load_report(&current_path) {
Ok(r) => r,
Err(e) => {
eprintln!("Failed to load current {}: {e}", current_path.display());
print!("{}", render_error_markdown(&e));
return 2;
}
};

let report = match compare_reports(&baseline, &current, &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, &current, &CompareOptions);
if let Err(e) = &comparison {
eprintln!("Compare failed: {e}");
}
emit_gate_verdict(&comparison)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

fn list_matchups() {
Expand Down
30 changes: 19 additions & 11 deletions crates/phase-ai/src/bin/ai_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, emit_gate_verdict, load_report, print_markdown, render_error_markdown, CompareOptions,
};
use phase_ai::duel_suite::run::{run_suite, SuiteOptions, SuiteReport};

const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/suite-baseline.json";
Expand Down Expand Up @@ -94,21 +96,27 @@ 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);
}
};

let report = match compare(&baseline, &current, &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, &current, &CompareOptions);
if let Err(err) = &comparison {
eprintln!("compare failed: {err}");
}
let code = emit_gate_verdict(&comparison);
if code != 0 {
std::process::exit(code);
}
}

Expand Down
11 changes: 9 additions & 2 deletions crates/phase-ai/src/bin/ai_perf_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
Loading
Loading