From 15ed53ed2b8e5dc138026141f71195a22eefcdc4 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 17:20:35 -0700 Subject: [PATCH 1/2] fix(ai): scope the perf gate's card-data stamp to the cards it actually runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ai-perf-gate` stamped provenance as `git hash-object data/card-data.json` — the whole ~35.6k-card file — while its three scenarios (red-mirror, affinity-mirror, enchantress-mirror) draw from decks fixed at compile time: one inline Rust builder and two committed, `frozen_date`-stamped snapshots, naming 46 distinct cards between them. `card-data.json` is derived from BOTH MTGJSON and the Oracle parser, so the stamp moved on every set release AND every parser change. `card_data_changed()` was therefore true on nearly every run, which made it useless for the single 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 across five local card-data vintages spanning 2026-07-25..08-04, one pair differing only by an Oracle-parser change and not by MTGJSON at all: whole-file gate-subset vintage e2930a52ffc9 d9d92bd40c408570 main, 08-04 1cbd383a93e0 d9d92bd40c408570 timecap-fix, 08-04 c967d6be63c8 d9d92bd40c408570 i6941 projection, 08-03 6d391b797d65 d9d92bd40c408570 wt-6965 base, 08-04 482ebc9e0887 d9d92bd40c408570 wt-6965 final, 08-04 Five distinct whole-file hashes; one distinct gate-subset hash. Deliberately NOT changed: - No `PERF_SCHEMA_VERSION` bump. `card_data_hash` never enters the compared payload, so changing how it is computed cannot invalidate a comparison. A bump would have forced a full counter refresh — precisely the act that would entrench the currently-unexplained FAILs. - Still a `git hash-object` blob SHA, so the field's format and meaning are unchanged and no hashing dependency enters this crate. `DefaultHasher` was rejected: std does not guarantee it across Rust versions, and this value is committed to a baseline that outlives toolchain bumps. - The parent still never loads a `CardDatabase`. `resolve_deck_ref` expands both `DeckRef` variants without one, preserving that documented property. - `ai_gate.rs`'s win-rate stamp is left alone. The full suite consumes a much larger deck set and deserves its own measurement rather than an assumption carried over from here. First run after this lands: the committed baseline still carries a whole-file hash, so `card_data_changed()` reports true once more until the next legitimate refresh stamps a narrow one. That is not a loss of signal — it is the same "true" it already reported on essentially every run. The narrowing is sound only while every scenario's deck is fixed at compile time. A pool-derived scenario would make the stamp report "unchanged" while the workload moved, which is strictly WORSE than the whole-file hash it replaces. `gate_scenarios_draw_only_from_decks_fixed_at_compile_time` guards that premise where it can actually break — at the scenario list — asserting each deck resolves without a card pool, is non-empty, and stays within a 20..=400 band. That test was watched go red at 46 cards before being accepted, which is how a defect in it was found: the first version hardcoded the band in its panic message, so the message reported `20..=400` while the assertion was `20..=30`. The band is now a const the message interpolates. Verified in an isolated CARGO_TARGET_DIR: `ai-perf-gate` builds, the lib test passes and was watched fail, `clippy -D warnings` clean. --- crates/phase-ai/src/bin/ai_perf_gate.rs | 120 ++++++++++++++++++++++-- crates/phase-ai/src/duel_suite/perf.rs | 60 ++++++++++++ 2 files changed, 170 insertions(+), 10 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_perf_gate.rs b/crates/phase-ai/src/bin/ai_perf_gate.rs index 0c3458b748..17c6a43491 100644 --- a/crates/phase-ai/src/bin/ai_perf_gate.rs +++ b/crates/phase-ai/src/bin/ai_perf_gate.rs @@ -25,6 +25,7 @@ #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; +use std::collections::{BTreeMap, BTreeSet}; use std::fs::File; use std::io::BufWriter; use std::path::{Path, PathBuf}; @@ -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,113 @@ 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| { + serde_json::to_writer(BufWriter::new(file), &subset).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..9d2572cd41 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. @@ -785,6 +794,57 @@ mod tests { use super::*; 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] { + // Both `DeckRef` variants are compile-time fixed: `Inline` is a + // Rust builder, `Snapshot` a committed, `frozen_date`-stamped + // file. `resolve_deck_ref` never consults the card pool, so if a + // pool-derived variant is ever added this call is where it lands. + 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, From a75c01b25e36d6157476c912c48293ce0b947722 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 17:34:24 -0700 Subject: [PATCH 2/2] fix(ai): flush the subset before hashing, and let the compiler police DeckRef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from the CodeRabbit review of #7013. **Data integrity.** `serde_json::to_writer(BufWriter::new(file), ..)` dropped the writer without checking its final flush, and `BufWriter`'s destructor discards errors by design. A failed last write would have left a TRUNCATED subset that `git hash-object` still hashes into a well-formed provenance stamp — a confident value for content that was never written, in the one field whose whole job is to be trustworthy. The writer is now bound, `flush()` is explicit, and either failure returns `None`. **The guard could not see what it was guarding.** The test asserted that each scenario's deck resolves and that the card count sits in a band. A future pool-derived `DeckRef` variant would resolve perfectly well and land inside that band, so the guard would stay green while its premise — every deck is fixed at compile time — had become false. That is the exact condition under which `gate_card_data_hash`'s narrowing is unsound, and no runtime assertion can detect it. Replaced with an exhaustive, wildcard-free `match` on `DeckRef`, per the codebase rule that a known enum gets no fallback arm: the compiler is the census, not the count. Verified by adding a probe variant to `DeckRef` and confirming E0004 fires at `perf.rs:823` (alongside `snapshots.rs:100`/`:109` and the `Debug` impl), then reverting it — the same watch-it-go-red discipline the band itself got. Verified: bin builds, lib test passes, `clippy -D warnings` clean, `cargo check --lib --tests` clean after the probe revert, all in an isolated CARGO_TARGET_DIR. --- crates/phase-ai/src/bin/ai_perf_gate.rs | 10 ++++++++-- crates/phase-ai/src/duel_suite/perf.rs | 17 +++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/crates/phase-ai/src/bin/ai_perf_gate.rs b/crates/phase-ai/src/bin/ai_perf_gate.rs index 17c6a43491..9311c8161a 100644 --- a/crates/phase-ai/src/bin/ai_perf_gate.rs +++ b/crates/phase-ai/src/bin/ai_perf_gate.rs @@ -27,7 +27,7 @@ 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}; @@ -420,7 +420,13 @@ fn gate_card_data_hash(data_root: &Path) -> Option { let hash = match File::create(&tmp) .map_err(|e| e.to_string()) .and_then(|file| { - serde_json::to_writer(BufWriter::new(file), &subset).map_err(|e| e.to_string()) + 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]), diff --git a/crates/phase-ai/src/duel_suite/perf.rs b/crates/phase-ai/src/duel_suite/perf.rs index 9d2572cd41..a6de550d52 100644 --- a/crates/phase-ai/src/duel_suite/perf.rs +++ b/crates/phase-ai/src/duel_suite/perf.rs @@ -792,6 +792,7 @@ 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 @@ -810,10 +811,18 @@ mod tests { 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] { - // Both `DeckRef` variants are compile-time fixed: `Inline` is a - // Rust builder, `Snapshot` a committed, `frozen_date`-stamped - // file. `resolve_deck_ref` never consults the card pool, so if a - // pool-derived variant is ever added this call is where it lands. + // 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}") });