diff --git a/crates/phase-ai/src/bin/ai_perf_gate.rs b/crates/phase-ai/src/bin/ai_perf_gate.rs index 0c3458b748..9311c8161a 100644 --- a/crates/phase-ai/src/bin/ai_perf_gate.rs +++ b/crates/phase-ai/src/bin/ai_perf_gate.rs @@ -25,8 +25,9 @@ #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; +use std::collections::{BTreeMap, BTreeSet}; use std::fs::File; -use std::io::BufWriter; +use std::io::{BufWriter, Write}; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; @@ -36,6 +37,7 @@ use phase_ai::duel_suite::perf::{ 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}; const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/perf-baseline.json"; const DEFAULT_CURRENT: &str = "target/ai-perf-gate-current.json"; @@ -214,16 +216,7 @@ fn run_parent_gate(args: &Args) { let mut current = median_report(&samples); // Stamp provenance the parent can compute without loading the DB. current.git_sha = command_output("git", &["rev-parse", "--short=12", "HEAD"]); - let db_path = args.data_root.join("card-data.json"); - current.card_data_hash = command_output( - "git", - &[ - "hash-object", - db_path - .to_str() - .expect("card-data path must be valid UTF-8"), - ], - ); + current.card_data_hash = gate_card_data_hash(&args.data_root); eprintln!( "perf suite: seed={} action_cap={} sample_count={} scenarios={:?} wall_clock={}ms", @@ -349,6 +342,119 @@ fn command_output(program: &str, args: &[&str]) -> Option { .filter(|s| !s.is_empty()) } +/// Provenance hash over ONLY the `card-data.json` entries this gate's scenarios +/// actually consume. +/// +/// This was previously `git hash-object data/card-data.json` — the whole file, +/// ~35.6k cards. The three scenarios in [`default_scenarios`] draw from +/// committed, frozen decks (inline builders plus pinned snapshots) naming ~46. +/// `card-data.json` is a DERIVED artifact of both MTGJSON and the Oracle parser, +/// so every unrelated set release *and every parser change* moved the stamp. +/// `PerfCompareReport::card_data_changed` was therefore true on nearly every +/// run, which made it useless for the one judgement it exists to support: +/// telling a genuine cost-per-node regression (hashes equal) apart from a +/// card-data-driven trajectory shift (hashes differ). +/// +/// Measured over five local card-data vintages spanning 2026-07-25..2026-08-04, +/// one pair differing only by an Oracle-parser change and not by MTGJSON: five +/// distinct whole-file hashes, exactly ONE distinct gate-subset hash. +/// +/// Still a `git hash-object` blob SHA, so the field's format and meaning are +/// unchanged and no hashing dependency enters this crate. Re-serializing is +/// canonical: this workspace leaves serde_json's `preserve_order` off, so object +/// keys serialize sorted (documented at `engine/src/bin/set_check.rs`), and the +/// `BTreeMap`s below fix the order of everything above them. A card a deck names +/// but `card-data.json` lacks is simply absent from the subset — which still +/// moves the hash, since the key set shrinks. +/// +/// Returns `None` on any failure, matching [`command_output`]'s convention: +/// `card_data_changed()` then reports false rather than inventing a delta. Every +/// failure path announces itself on stderr — an unstamped run must not read as a +/// clean one. +fn gate_card_data_hash(data_root: &Path) -> Option { + // DB-free by construction: `resolve_deck_ref` expands inline builders and + // pinned snapshots without a `CardDatabase`, preserving this branch's + // documented never-loads-the-DB property. + let mut names = BTreeSet::new(); + for id in default_scenarios() { + let Some(matchup) = find_matchup(id) else { + eprintln!("provenance: scenario {id:?} does not resolve — card-data hash unstamped"); + return None; + }; + for deck in [&matchup.p0, &matchup.p1] { + match resolve_deck_ref(deck) { + // The engine keys `card-data.json` by Rust `to_lowercase()`. + Ok(cards) => names.extend(cards.into_iter().map(|c| c.to_lowercase())), + Err(err) => { + eprintln!( + "provenance: deck {deck:?} failed to resolve ({err}) — card-data hash unstamped" + ); + return None; + } + } + } + } + + let db_path = data_root.join("card-data.json"); + let db: BTreeMap = match std::fs::read_to_string(&db_path) + .map_err(|e| e.to_string()) + .and_then(|raw| serde_json::from_str(&raw).map_err(|e| e.to_string())) + { + Ok(db) => db, + Err(err) => { + eprintln!( + "provenance: could not read {} ({err}) — card-data hash unstamped", + db_path.display() + ); + return None; + } + }; + let subset: BTreeMap<&str, &serde_json::Value> = names + .iter() + .filter_map(|name| db.get(name).map(|entry| (name.as_str(), entry))) + .collect(); + + // `git hash-object` needs a path, so the canonical subset goes through a + // temp file. Removed on every exit path, including the failures below. + let tmp = std::env::temp_dir().join(format!("ai-perf-gate-cards-{}.json", std::process::id())); + let hash = match File::create(&tmp) + .map_err(|e| e.to_string()) + .and_then(|file| { + let mut writer = BufWriter::new(file); + serde_json::to_writer(&mut writer, &subset).map_err(|e| e.to_string())?; + // `BufWriter` discards errors from its drop-time flush, so a failed + // final write would leave a TRUNCATED subset that `git hash-object` + // still hashes — yielding a well-formed provenance stamp for content + // that was never written. Flush explicitly and fail closed instead. + writer.flush().map_err(|e| e.to_string()) + }) { + Ok(()) => match tmp.to_str() { + Some(path) => command_output("git", &["hash-object", path]), + None => { + eprintln!("provenance: temp path is not valid UTF-8 — card-data hash unstamped"); + None + } + }, + Err(err) => { + eprintln!( + "provenance: could not write the card subset ({err}) — card-data hash unstamped" + ); + None + } + }; + let _ = std::fs::remove_file(&tmp); + + if hash.is_some() { + eprintln!( + "provenance: card-data hash covers {} of {} deck-named cards from scenarios {:?}", + subset.len(), + names.len(), + default_scenarios() + ); + } + hash +} + fn write_report(report: &PerfReport, path: &Path) -> Result<(), std::io::Error> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; diff --git a/crates/phase-ai/src/duel_suite/perf.rs b/crates/phase-ai/src/duel_suite/perf.rs index d6622473a8..a6de550d52 100644 --- a/crates/phase-ai/src/duel_suite/perf.rs +++ b/crates/phase-ai/src/duel_suite/perf.rs @@ -52,6 +52,15 @@ //! (`--refresh-baseline`), which prints the baseline-vs-current diff before //! overwriting — never a blind widen. //! +//! That stamp covers **only the card-data entries [`default_scenarios`]'s decks +//! actually name**, not the whole file (`ai_perf_gate::gate_card_data_hash`). +//! `card-data.json` is derived from both MTGJSON and the Oracle parser, so a +//! whole-file hash moved on every set release and every parser change while this +//! gate's frozen decks saw none of it — leaving the diagnostic true on nearly +//! every run, and therefore unable to make the distinction above. The workload is +//! fixed by construction (inline builders + pinned snapshots), so the narrow +//! stamp is the one that tracks this gate's real input. +//! //! Wall-clock is recorded (`wall_clock_ms`) for human triage only; it is never //! compared. @@ -783,8 +792,68 @@ pub fn print_repro_margin(report: &ReproMarginReport) { #[cfg(test)] mod tests { use super::*; + use crate::duel_suite::DeckRef; use std::collections::BTreeSet; + /// The narrowed `card_data_hash` (`ai_perf_gate::gate_card_data_hash`) is only + /// meaningful because every gate scenario draws from a deck fixed at compile + /// time — an inline builder or a pinned snapshot. A scenario whose deck were + /// derived from the card pool would make the narrow stamp silently wrong: the + /// workload would move with the pool while the stamp reported "unchanged", + /// which is strictly worse than the whole-file hash it replaced. + /// + /// This guards that premise at the point it can break — adding a scenario — + /// rather than at the hash, which cannot tell where its names came from. + #[test] + fn gate_scenarios_draw_only_from_decks_fixed_at_compile_time() { + let mut names = BTreeSet::new(); + for id in default_scenarios() { + let matchup = crate::duel_suite::find_matchup(id) + .unwrap_or_else(|| panic!("gate scenario {id:?} must resolve to a matchup")); + for deck in [&matchup.p0, &matchup.p1] { + // Exhaustive and wildcard-free ON PURPOSE: the compiler is the + // census here, not the card count below. Both current variants + // are fixed at compile time — `Inline` is a Rust builder, + // `Snapshot` a committed, `frozen_date`-stamped file — and a + // future pool-derived variant would resolve perfectly well and + // land inside the band, so no runtime assertion can catch it. + // Adding a `DeckRef` variant must break THIS match, because + // `gate_card_data_hash`'s narrowing is unsound for any deck the + // card pool can move. + match deck { + DeckRef::Inline { .. } | DeckRef::Snapshot { .. } => {} + } + let cards = crate::duel_suite::resolve_deck_ref(deck).unwrap_or_else(|err| { + panic!("gate scenario {id:?} deck {deck:?} must resolve without a card pool: {err}") + }); + assert!( + !cards.is_empty(), + "gate scenario {id:?} deck {deck:?} resolved to an EMPTY deck — the perf \ + workload would be vacuous and the narrowed provenance stamp would cover \ + nothing" + ); + names.extend(cards.into_iter().map(|c| c.to_lowercase())); + } + } + + // Non-vacuity: the stamp must actually cover cards. The upper bound is the + // load-bearing half — it is what fails if a scenario starts pulling a + // pool-sized deck, at which point narrowing the hash stops being sound. + // Measured at 46 distinct names for the three mirrors on 2026-08-04. + // + // The band is a const so the panic message cannot drift from the + // assertion — printing a hardcoded range here was wrong once already. + const BAND: std::ops::RangeInclusive = 20..=400; + assert!( + BAND.contains(&names.len()), + "gate scenarios name {} distinct cards, outside the {BAND:?} band this narrowing \ + assumes. Too few means a scenario stopped resolving; too many means a deck is no \ + longer a fixed list, and `gate_card_data_hash` must be re-justified before the \ + band is widened", + names.len() + ); + } + fn mk_report(counters: &[(&str, u64)]) -> PerfReport { PerfReport { schema_version: PERF_SCHEMA_VERSION,