fix(tools): stop keying the parse-diff comparison on a non-unique card name - #7118
Conversation
…d name
`coverage-parse-diff` is the gate that decides whether an engine change altered
parser output. It built its base and head maps with
`BTreeMap<String, &CardCoverageResult>` keyed on `card_name.to_ascii_lowercase()`,
so every row after the first for a duplicated name was silently dropped — last
wins, no signal. Card names are not unique in the coverage snapshot.
The consequence is the part that matters: the gate could certify "no parse
impact" for a change that had one. Demonstrated end-to-end rather than argued.
On the real pinned 35,657-row snapshot, planting a single parse change on a
duplicate-name row that last-wins provably discards ("Lightning Bolt", the
discarded index) and running both binaries on the same pair gives:
before: 0 clusters, 0 added, 0 removed, 0 oracle_changed — the change is INVISIBLE
after: 1 cluster, DealDamage | field | amount | '3' -> '3_MUTATED'
A first attempt at this fixture compared the unmodified base-vs-live pair and
produced 0 clusters on *both* sides. That proves nothing, so it was discarded in
favour of the planted-defect pair above. A fixture that cannot fail is not
evidence.
Second defect, same function: the
`if b.oracle_text != h.oracle_text { oracle_changed += 1; continue; }` arm
returned before comparing `diff_level`, so a row whose Oracle text was errata'd
*and* whose parse level regressed reported only the errata. The carve-out is
still honoured — genuine Oracle changes must not be reported as parse
regressions — but it no longer swallows the parse signal on the same row.
The comparison is now a multi-pass pairing within each name group: an exact
identity pass first, then canonical `row_key` ordering for the remainder, with
`changed_cards` counted per row rather than per name and the global
`cards.dedup()` removed so a duplicate group's second row is no longer erased at
report time.
Scope: one file, +666/-86 — 261 production lines and 405 test lines across 9 new
tests. Nothing else changes; the four `game/coverage.rs` types, `card_db.rs`,
`Cargo.toml`, CI workflows and the receipt contract are untouched. Tests are an
inline `#[cfg(test)] mod tests` because `Cluster`/`diff_level`/`compare` are
private to the bin and unreachable from `tests/integration/`, so no new test
binary is created.
Discrimination was measured with seven ablations, not one revert, and run in an
isolated harness rather than in the shared checkout — reverting in place would
have turned Tilt red for every other agent working in this tree. Harness fidelity
was proven programmatically (byte-identical to the real file except the one
`use engine::game::coverage::{...}` line, replaced by those four type
definitions verbatim; equal line counts asserted).
full revert of `compare` to last-wins -> 8 of 9 new tests red
group_by_name -> last-wins only -> 8 red
changed_cards counted per name again -> 6 red
global cards.dedup() restored -> exactly 1 red
canonical row_key sort deleted -> exactly 1 red
exact-identity pass deleted -> exactly 1 red
group_names' names.sort() deleted -> exactly 1 red
The last four landing on exactly one test each is the point: every constraint is
*uniquely* pinned, so a later simplification cannot remove one invisibly. The
errata carve-out test stays green through the full revert, which is required — it
is the control that must hold in both states.
Verified in the real tree, not only the harness: all 21 tests in
`phase-engine::bin/coverage-parse-diff` pass on a `test-engine` build starting
01:01:33Z, and clippy is clean on a build starting 01:04:14Z. Both post-date the
final edit, with no touched file carrying a later mtime. The one unrelated
repo-wide failure in that run
(`battlefield_entry_authority_census::every_token_created_construction_lives_in_the_single_emitter`,
23365/23366 passed) belongs to another agent's in-flight token work: disjoint
file set, failing identically on builds predating this change, and already dirty
before this work began.
Formatted with `rustfmt` on the single file rather than `cargo fmt`, which would
have rewritten other agents' dirty files; it made zero changes, confirmed
non-vacuous because rustfmt did rewrite a deliberately misformatted copy.
No CR annotations added: this is a build tool and implements no game rule. The
same probe saw all 667 added lines, so the zero is a measurement.
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Parse changes introduced by this PRBaseline pending for |
1 participant
coverage-parse-diffis the gate that decides whether an engine change alteredparser output. It built its base and head maps with
BTreeMap<String, &CardCoverageResult>keyed oncard_name.to_ascii_lowercase(),so every row after the first for a duplicated name was silently dropped — last
wins, no signal. Card names are not unique in the coverage snapshot.
The consequence is the part that matters: the gate could certify "no parse
impact" for a change that had one. Demonstrated end-to-end rather than argued.
On the real pinned 35,657-row snapshot, planting a single parse change on a
duplicate-name row that last-wins provably discards ("Lightning Bolt", the
discarded index) and running both binaries on the same pair gives:
before: 0 clusters, 0 added, 0 removed, 0 oracle_changed — the change is INVISIBLE
after: 1 cluster, DealDamage | field | amount | '3' -> '3_MUTATED'
A first attempt at this fixture compared the unmodified base-vs-live pair and
produced 0 clusters on both sides. That proves nothing, so it was discarded in
favour of the planted-defect pair above. A fixture that cannot fail is not
evidence.
Second defect, same function: the
if b.oracle_text != h.oracle_text { oracle_changed += 1; continue; }armreturned before comparing
diff_level, so a row whose Oracle text was errata'dand whose parse level regressed reported only the errata. The carve-out is
still honoured — genuine Oracle changes must not be reported as parse
regressions — but it no longer swallows the parse signal on the same row.
The comparison is now a multi-pass pairing within each name group: an exact
identity pass first, then canonical
row_keyordering for the remainder, withchanged_cardscounted per row rather than per name and the globalcards.dedup()removed so a duplicate group's second row is no longer erased atreport time.
Scope: one file, +666/-86 — 261 production lines and 405 test lines across 9 new
tests. Nothing else changes; the four
game/coverage.rstypes,card_db.rs,Cargo.toml, CI workflows and the receipt contract are untouched. Tests are aninline
#[cfg(test)] mod testsbecauseCluster/diff_level/compareareprivate to the bin and unreachable from
tests/integration/, so no new testbinary is created.
Discrimination was measured with seven ablations, not one revert, and run in an
isolated harness rather than in the shared checkout — reverting in place would
have turned Tilt red for every other agent working in this tree. Harness fidelity
was proven programmatically (byte-identical to the real file except the one
use engine::game::coverage::{...}line, replaced by those four typedefinitions verbatim; equal line counts asserted).
full revert of
compareto last-wins -> 8 of 9 new tests redgroup_by_name -> last-wins only -> 8 red
changed_cards counted per name again -> 6 red
global cards.dedup() restored -> exactly 1 red
canonical row_key sort deleted -> exactly 1 red
exact-identity pass deleted -> exactly 1 red
group_names' names.sort() deleted -> exactly 1 red
The last four landing on exactly one test each is the point: every constraint is
uniquely pinned, so a later simplification cannot remove one invisibly. The
errata carve-out test stays green through the full revert, which is required — it
is the control that must hold in both states.
Verified in the real tree, not only the harness: all 21 tests in
phase-engine::bin/coverage-parse-diffpass on atest-enginebuild starting01:01:33Z, and clippy is clean on a build starting 01:04:14Z. Both post-date the
final edit, with no touched file carrying a later mtime. The one unrelated
repo-wide failure in that run
(
battlefield_entry_authority_census::every_token_created_construction_lives_in_the_single_emitter,23365/23366 passed) belongs to another agent's in-flight token work: disjoint
file set, failing identically on builds predating this change, and already dirty
before this work began.
Formatted with
rustfmton the single file rather thancargo fmt, which wouldhave rewritten other agents' dirty files; it made zero changes, confirmed
non-vacuous because rustfmt did rewrite a deliberately misformatted copy.
No CR annotations added: this is a build tool and implements no game rule. The
same probe saw all 667 added lines, so the zero is a measurement.
Two notes for a reviewer — both are readings that look like defects and are not
1. This PR's own
/engine-implementerreceipt will show an EMPTY parse-diff. That is not evidence the fix does nothing.The receipt harness invokes the base-built comparator (
"$BASE_TARGET/tool/coverage-parse-diff"), so this PR's receipt is still produced by the OLD last-wins binary — the very binary being fixed. The fix becomes effective for receipts only from the next base commit onward.CI is different and gets the fix immediately:
.github/workflows/ci.yml:443runscargo run --profile tool --features cli --bin coverage-parse-diff, i.e. a head-built binary, against a downloaded base data file (:441-442) rather than a base-built binary.No gate can surface this, because it is a property of which binary the receipt harness invokes. The empty receipt is the natural reading and it is the wrong one.
2. A cluster's card list can now legitimately repeat a name, e.g.
"Fast", "Fast".Two rows sharing one card name, both correctly counted. Before this change a global
cards.dedup()collapsed them into one — that collapse was the defect. The repetition is the correct behaviour; it reads like a rendering bug.Related and also new: when a duplicate-name group shrinks,
_Cards only in baseline: N._can name a card still present in head. The count is right, the noun is imprecise. Renaming itRowswas verified safe againstscripts/pr_review.py(which matchesPARSE_DIFF_MARKERat :218/:3043 and classifies on the substringsignature(s)at :3047) and deliberately declined on scope grounds. No test asserts the noun, so nothing will flag it either way.3. Heads-up, not a finding. Every CI run now emits one new stderr line:
coverage-parse-diff: 30 card name(s) carry more than one row in one or both snapshots; every row was compared (none dropped).One stable line, not per-card noise, and it doubles as receipt-visible proof the group path executed. The30is a fact about today's corpus, not a bound — the code handles arbitrary group cardinality, so it is not an invariant if it moves.