fix(ai): scope the perf gate's card-data stamp to the cards it actually runs - #7013
Merged
Conversation
…ly runs `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.
matthewevans
enabled auto-merge
August 5, 2026 00:21
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe performance gate now hashes only cards referenced by the default scenarios. It resolves decks without a database, canonicalizes the selected card data, reports failures as an unstamped result, and removes temporary files. A regression test validates scenario resolution and card-count bounds. ChangesScenario-scoped card-data hashing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DefaultScenarios
participant PerfGate
participant CardDataJson
participant GitHashObject
DefaultScenarios->>PerfGate: resolve default scenario decks
PerfGate->>CardDataJson: select referenced card entries
PerfGate->>GitHashObject: hash canonical serialized subset
GitHashObject-->>PerfGate: return card-data hash
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/bin/ai_perf_gate.rs`:
- Around line 420-424: Update the temporary subset-writing flow around the
`File::create`/`serde_json::to_writer` chain to retain the `BufWriter`,
explicitly call `flush()` after serialization, and propagate either
serialization or flush errors as `None` before invoking `git hash-object`.
Ensure hashing only occurs after a successful flush.
In `@crates/phase-ai/src/duel_suite/perf.rs`:
- Around line 813-819: Update the gate scenario deck handling around
resolve_deck_ref to explicitly match only DeckRef::Inline and DeckRef::Snapshot
before resolving the deck; reject or assert on any other DeckRef variant without
using a wildcard arm, then preserve the existing resolution error assertion for
the fixed variants.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2155e2c7-a00e-49bb-b5cb-0621c4d39aae
📒 Files selected for processing (2)
crates/phase-ai/src/bin/ai_perf_gate.rscrates/phase-ai/src/duel_suite/perf.rs
… DeckRef 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.
lgray
pushed a commit
to lgray/phase
that referenced
this pull request
Aug 5, 2026
…provenance (phase-rs#7027) phase-rs#7013 changed `card_data_hash` from a whole-file digest of card-data.json to a digest of only the cards `default_scenarios()` actually plays (46 of 46, as CI reports). The committed baseline still carries the OLD whole-file value, so the two sides are produced by different functions and can never agree — every perf gate run since the merge prints note: card-data hash changed (e2db8a6d…→d11502ce…) — likely a card-data-driven trajectory shift, not a cost-per-node regression on an unchanged workload. A diagnostic that fires unconditionally is worse than none: the next reader sees "card-data changed" and stops investigating a real regression. The new value is the one the merged binary computes on the CI runner, observed identically on two separate runs against different weekly card pools (30963718102 pre-merge, 30996886019 post-merge) — consistent with the scoped subset being stable across MTGJSON vintages, which is what motivated phase-rs#7013. Provenance only. Counters, seed, action_cap, sample_count and scenarios are untouched, so this changes no PASS/FAIL verdict — `card_data_hash` is diagnostic and never feeds `any_fail()`. NOT fixed here, and worth its own change: `git_sha` reads 64b65e5, a pull/6777 BRANCH head that was never on main. Measuring at that commit yields layers_full_eval=4045 against the 3495 recorded here, so the baseline's counters do not reproduce at the commit they name. Restamping the SHA without re-measuring would only move the inconsistency. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ai-perf-gatestamped provenance asgit 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, naming46 distinct cards between them.
card-data.jsonis derived from BOTH MTGJSON and the Oracle parser, so thestamp 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:
PERF_SCHEMA_VERSIONbump.card_data_hashnever enters the comparedpayload, 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.
git hash-objectblob SHA, so the field's format and meaning areunchanged and no hashing dependency enters this crate.
DefaultHasherwasrejected: std does not guarantee it across Rust versions, and this value is
committed to a baseline that outlives toolchain bumps.
CardDatabase.resolve_deck_refexpandsboth
DeckRefvariants without one, preserving that documented property.ai_gate.rs's win-rate stamp is left alone. The full suite consumes a muchlarger 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 legitimaterefresh 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_timeguards that premisewhere 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..=400while the assertion was20..=30.The band is now a const the message interpolates.
Verified in an isolated CARGO_TARGET_DIR:
ai-perf-gatebuilds, the lib testpasses and was watched fail,
clippy -D warningsclean.Summary by CodeRabbit
Bug Fixes
Tests