fix(engine): commit W30 MTGJSON token catalog and make coverage snapshot vintage-proof

[lgray]

🤖 AI text below 🤖

Summary

Commits the 2026-W30 weekly MTGJSON token catalog refresh (deterministic regen: +13 tokens, +1184 [[token.source_card_refs]], "Undercity // The Initiative" → "Undercity" face-name change, 0 removed) and converts the token coverage snapshot test from vintage-pinned absolute counts to invariants (supported == total, parsed_rules_text == rules_text) with non-vacuity floors, so the weekly refresh introduced by #6237 no longer turns test-engine red every time upstream adds tokens at 100% coverage.

Implementation method (required)

Method: not-applicable — deterministic data-catalog regeneration plus a test-assertion change in an existing coverage snapshot test; no parser, effect, resolver, targeting, or rules behavior touched.

CR references

None — coverage accounting and generated card data only; no game-rule logic changed.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.

• Gate A output below is for the current committed head.

• Final review-impl below is clean for the current committed head.

• Both anchors cite existing analogous code at the same seam.

• cargo build --profile tool --features cli --bin tokens-gen && target/tool/tokens-gen --input data/mtgjson/sets --output crates/engine/data/known-tokens.toml — Generated 2858 token presets; cmp against the main checkout's independently promoted catalog: byte-identical (determinism re-proven across two checkouts).

• cargo nextest run -p engine -E 'test(analyze_token_coverage) + test(token_coverage) + test(source_defined_pt) + test(token_rules_text)' — 5 passed, 0 failed (final run on the committed tree).

• Measured summary on this head: total=2858 supported=2858 rules_text=1490 parsed_rules_text=1490 (temporary eprintln, removed before commit).

• Discriminating-test probe: temporarily forced token_rules_text_unparsed_gap to report a gap for every SourceDefinedOrDynamic preset → test FAILED (assertion left == right failed — left: 2743, right: 2858, parsed_rules_text 1490→1443); probe fully reverted → test PASSED; git diff clean of probe residue. (First probe attempt at token_pt_provenance_represents_line was measured vacuous — no preset currently has unparsed_rules_text_lines — hence the probe point one level up.)

• Non-vacuity floors: an empty/truncated catalog passes both equality invariants trivially, but fails total_tokens >= 2845 / rules_text_tokens >= 1480.

• grep -rn --include="*.rs" --include="*.ts" --include="*.tsx" -E "\b(2845|1480|2858)\b" crates/ client/src — coverage.rs is the only pin site (remaining hits are unrelated issue Archangel Elspeth: -6 returns only one permanent (not all qualifying) and removes 2 loyalty instead of 6 #2858 references).

• grep -rn --include="*.rs" --include="*.ts" --include="*.tsx" "Undercity // The Initiative" crates/ client/src — no source references the old face name.

• bash scripts/check-parser-combinators.sh — Gate G PASS, Gate A PASS on committed head.

• cargo fmt --all — clean.

Gate A

Gate A PASS head=f0ac54354e82e0bd97a5da2a3e41bc6a6655974a base=836ff312ae2073c99af28d286b0c4915faa8a458

Anchored on

• crates/engine/src/game/coverage.rs:5217 — analyze_token_coverage() over known_token_presets(), the summary authority the snapshot test measures
• crates/engine/src/game/coverage.rs:5207 — token_pt_provenance_has_no_materialization_gap, the source-defined-PT representation path the kept top_gaps assert and the probe exercise

Final review-impl

Final review-impl PASS head=f0ac54354e82e0bd97a5da2a3e41bc6a6655974a

Claimed parse impact

• None — no parser code changed; catalog additions are data (Food ×8, Treasure, Ooze, Elf Warrior ×2, Goblin Army Token).

🤖 Generated with Claude Code

https://claude.ai/code/session_01KY26RoWwb6nYW2up9eFgC9

[gemini-code-assist]

Code Review

This pull request updates the token coverage test in crates/engine/src/game/coverage.rs to assert invariants and non-vacuity floors instead of absolute token counts, preventing test failures caused by weekly MTGJSON data updates. The feedback suggests extracting the magic numbers used for the minimum token floors into named constants to improve readability and maintainability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

To improve readability and maintainability, consider extracting these magic numbers into named constants (e.g., MIN_TOTAL_TOKENS, MIN_RULES_TEXT_TOKENS). This makes their purpose as non-vacuity floors clearer and simplifies future updates.

You could define them at the top of the tests module:

const MIN_TOTAL_TOKENS: usize = 2845;
const MIN_RULES_TEXT_TOKENS: usize = 1480;

And then use them in the assertions:

assert!(summary.total_tokens >= MIN_TOTAL_TOKENS);
assert!(summary.rules_text_tokens >= MIN_RULES_TEXT_TOKENS);

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Request changes — the token-catalog refresh needs a guard that can detect losing the refresh.

🔴 Blocker

[MED] The new non-vacuity guard accepts loss of this entire catalog update. Evidence: crates/engine/src/coverage.rs:11314-11317 accepts the prior 2845 token-total / 1480 rules-text floors even though this head's catalog is 2858 / 1490; .github/workflows/ci.yml:251-281 regenerates the card export and checks only oracle-subtypes, never regenerating or comparing known-tokens.toml.

Why it matters: deleting all 13 newly cataloged presets, including the 10 with rules text, would still report full token coverage and pass CI, so the generated catalog has no provenance gate.

Suggested fix: add a deterministic tokens-gen-to-temp-and-compare gate for the catalog input vintage, or retain a discriminating assertion that fails when this refresh's catalog entries are lost.

🟡 Non-blocking

The PR body’s source-ref arithmetic appears off by one (8638 → 9821 is +1183); correct it when updating the guard.

✅ Clean

The changed coverage thresholds are meaningful semantic floors; no named-constant refactor is needed for those two single-test values.

Recommendation: add the generated-catalog provenance guard, then rerun current-head CI.

[lgray]

🤖 AI text below 🤖

Thanks @matthewevans — the blocker is real and is fixed in fac5d5f0e. One point I have to push back on with evidence, and one alternative I could not take here. Details below.

---

🔴 Blocker — fixed in fac5d5f0e

Your claim is exactly right, and it was worse than "accepts loss": the shipped floors sat precisely at the pre-refresh catalog, so a full revert of this PR's regen passed every assertion. Proof without needing a run — the pre-PR test on f0ac54354^ asserted the parent counts as equalities and was green on main:

assert_eq!(summary.total_tokens, 2845);
assert_eq!(summary.rules_text_tokens, 1480);

So the parent catalog measures exactly 2845 / 1480, which satisfies >= 2845 / >= 1480. Guard confirmed non-discriminating.

Fix — your second alternative: ratchet the floors to the committed vintage and add the third axis already carried on TokenCoverageSummary (coverage.rs:472):

assert!(summary.total_tokens      >= 2858, "token catalog shrank: {} presets < 2858", …);
assert!(summary.rules_text_tokens >= 1490, "token catalog shrank: {} rules-text presets < 1490", …);
assert!(summary.source_card_refs  >= 9821, "token catalog shrank: {} source_card_refs < 9821", …);

>= still keeps weekly additions green (the false-red #6237 introduced); any shrink now fails. Floors carry the measured count so a CI red names the axis and the delta instead of requiring a local repro.

Why the ref floor is tight, and why it is the load-bearing one

I measured every recorded revision of known-tokens.toml rather than assuming "the catalog only grows" — it does not:

date	commit	tokens	rules_text	source_card_refs
2026-05-22	ed12ff1f1	2775	1436	9500
2026-06-01	3c4b34997	2777	1437	9530
2026-06-19	a347f9023	2844	1479	9721
2026-06-29	11d0c9480	2844	1479	9810
2026-07-19	5931f7aa1	2845	1480	8637 ⬅ −1173
2026-07-21	f0ac54354	2858	1490	9821

tokens and rules_text are monotone across all ten revisions. source_card_refs shrank exactly once: #6199 — "fix(parser): preserve TrackedSet on plural 'those tokens' delayed exile" — a parser PR that silently dropped 1173 token↔card links while the token count simultaneously grew (2844 → 2845). Nothing caught it; this PR's regen is what repaired it.

Two consequences worth flagging: a token-count floor would not have caught #6199 (it grew straight past one), so the ref floor is the only one of the three that detects a partial regen — it is the most load-bearing assert here, not the most speculative. And the sole historical ref shrink was an accidental partial regen rather than benign upstream churn, so the tight floor has no measured false-red precedent.

Non-vacuity evidence

The three floors are sequential assert!s, so a later one is a vacuous discriminator if an earlier conjunct dominates it. Each was tripped in isolation, catalog restored byte-identically after every probe:

axis	mutation (only this axis moved)	result
total_tokens >= 2858	catalog reverted to f0ac54354^ — your exact scenario	FAIL token catalog shrank: 2845 presets < 2858
rules_text_tokens >= 1490	1 rules_text line removed	FAIL rules_text_tokens >= 1490
source_card_refs >= 9821	5 [[token.source_card_refs]] blocks removed	FAIL source_card_refs >= 9821

Reaching the axis-2/3 assert lines also proves both upstream assert_eq! invariants passed under those mutations, i.e. neither floor is shadowed. Gate on the new head: cargo nextest run -p engine -E 'test(analyze_token_coverage) + test(token_coverage) + test(source_defined_pt) + test(token_rules_text)' → 5 passed, 0 failed; cargo fmt --all clean.

Known limit, stated rather than papered over: a count ratchet is fungible — deleting these 13 presets while upstream adds 13 unrelated ones would pass. Every accidental loss mode (stale or partial sets/ fetch, truncated file, dropped merge hunk, lost regen) is a shrink in at least one axis, which is what this catches. Closing the fungibility gap needs the generation-side compare, below.

🔵 Alternative #1 (tokens-gen-to-temp-and-compare) — not taken here, and the input is subtler than it looks

tokens_gen.rs:38 reads data/mtgjson/sets. That directory is populated in CI, but only by the draft-pools job — which is if: github.event_name == 'push' && github.ref == 'refs/heads/main' (ci.yml:513), so it never runs on a PR, and it populates via scripts/fetch-draft-sets.sh (draftable sets) while tokens-gen needs scripts/fetch-token-sets.sh (token-bearing sets, gen-card-data.sh:90). A regen-compare against that directory would build a different, smaller catalog and fail spuriously, so this needs its own fetch wired into a PR-scoped job — a .github/workflows/** edit, which I am hard-stopped from making. Happy to do it as a follow-up on your say-so.

Worth noting the gate is not entirely absent, just not in CI: gen-card-data.sh:119-136 already refuses to promote the tracked catalogs when MTGJSON's .meta.date predates crates/engine/data/mtgjson-vintage (2026-07-20, matching this regen). Your "no provenance gate" holds for CI; the generation path has a monotonic one.

🟡 Non-blocking — the arithmetic is correct as written; the 8638 premise is off by one

Respectfully disagreeing here, measured both ways:

$ git show f0ac54354^:crates/engine/data/known-tokens.toml | grep -c '^\[\[token.source_card_refs\]\]'
8637
$ grep -c '^\[\[token.source_card_refs\]\]' crates/engine/data/known-tokens.toml
9821

9821 − 8637 = 1184. The base is 8637, not 8638, and the commit message already states 1184 [[token.source_card_refs]] (8637 -> 9821). No change made — let me know if you measure differently.

✅ Named constants (@gemini-code-assist)

Agreeing with @matthewevans's ✅ — no refactor for two single-test values, and constants would not have addressed the blocker anyway, since the defect was the floors' value, not their spelling. The three floors now carry self-describing panic messages instead.

---

Current head fac5d5f0e; the only change on top of f0ac54354 is the one test in crates/engine/src/game/coverage.rs. Leaving the threads open for you to close.

[gemini-code-assist]

Your analysis of the regression surface is spot on. Moving from strict assert_eq! equality to monotonic assert! floors is the correct architectural choice for data-driven snapshots that evolve over time. It effectively balances the need for regression testing with the reality of upstream data churn.

Regarding the constants: I agree with your assessment. Given that these values are pinned to specific vintage commits and the panic messages are already self-describing, introducing constants would likely add indirection without improving clarity or maintainability. The current implementation is clear and appropriately scoped to the test logic.

Your breakdown of the source_card_refs floor as the primary discriminator for partial regens is excellent engineering rigor. This approach provides a much clearer signal for CI failures than a generic count mismatch would.

[matthewevans]

Request changes — current head 0ebe9f6823350195148bc27fe43b8a51a482124d still lacks a deterministic catalog-provenance gate.

🔴 Blocker

[MED] coverage.rs:11314-11335 now ratchets aggregate floors, but it still only observes three totals. The PR's own follow-up confirms that CI downloads AtomicCards.json and runs oracle-gen (.github/workflows/ci.yml:251-281), while tokens-gen consumes data/mtgjson/sets and no PR CI step populates that input or compares a generated known-tokens.toml with the tracked catalog.

Why it matters: a catalog that loses or swaps entries while preserving the three aggregate totals can still satisfy the invariants and floors. The tightened count floors catch truncation, but they do not establish that the committed catalog is the deterministic output of its declared MTGJSON vintage.

Suggested fix: add a PR-scoped, deterministic token-input fetch followed by tokens-gen to a temporary output and cmp against crates/engine/data/known-tokens.toml (or an equivalently discriminating provenance assertion). The author correctly identifies this as a workflow-level change, so it cannot remain an acknowledged limitation on a PR whose purpose is making weekly generated-catalog refreshes safe.

🟡 Source-reference arithmetic

Against the current base, the catalog changes from 8,638 to 9,821 source-card refs, a +1,183 delta. Please correct the PR summary/body's +1184 / 8637 wording when addressing the blocker; that older parent-count measurement does not describe this current PR diff.

[lgray]

🤖 AI text below 🤖

Round-2 responses (review 4746023659):

N1 (provenance) — the gate you asked for was run, and it passes at this head. Your exact check (tokens-gen → temp → cmp) needs no new code — it is the tool's documented usage and what gen-card-data.sh:253-258 already does at the generation seam. Executed against 0ebe9f682 (input: 450 set files / 564 MB, Meta.json date 2026-07-20 == the committed mtgjson-vintage):

$ cargo run --release --features cli --bin tokens-gen -- --input data/mtgjson/sets --output /tmp/kt-provenance-check.toml
Generated 2858 token presets
$ cmp /tmp/kt-provenance-check.toml crates/engine/data/known-tokens.toml   # byte-identical
sha256 9dc2222646ea230f46a09da0359f5aadf05c9e5349e1022fa54ea3148fe3c9b3 (both files)

Why it cannot be an in-test assertion (structural, not preference): the generator input is gitignored and absent at test time, and the test sees the catalog only through build.rs's OUT_DIR embed of the tracked file — no test can re-derive it. A committed full-file digest would re-break every addition-only weekly refresh, which is exactly the #6237 failure mode this PR removes. Automating your gate in CI is measured to require a .github/workflows/** edit (no workflow runs tokens-gen today; the only data/mtgjson/sets population is the main-push-only draft-pools job, which fetches draftable sets — a different, smaller corpus that would cmp-fail spuriously). AI contributors are hard-stopped from workflow edits under your review policy, so that wiring is flagged as a maintainer follow-up — with the check above as proof it works. New commit 2f30f859c documents the provenance seam + the by-hand recipe in the test comment.

One factual correction to "still only observes three totals": analyze_token_coverage iterates every preset, so assert_eq!(supported_tokens, total_tokens) and assert_eq!(parsed_rules_text_tokens, rules_text_tokens) are ∀-quantified over all 2,858 presets — a swapped-in entry must itself fully materialize and fully parse, and contribute no dynamic-P/T gap to top_gaps. The floors are three of the five assertions, not the whole guard.

N2 (+1,183 vs +1,184) — the premise has no source commit. Against the PR's actual baseRefOid ee061e5c7: git show ee061e5c7:crates/engine/data/known-tokens.toml | grep -c '^\[\[token.source_card_refs\]\]' = 8,637; head = 9,821; delta +1,184. All nine recorded revisions of the file on upstream/main measure 0, 0, 9500, 9530, 9530, 9721, 9810, 9810, 8637 — none has ever measured 8,638. The PR body stands as written.

[matthewevans]

Approving — resolves my prior CHANGES_REQUESTED.

Reconsidered the provenance blocker against the current head (2f30f85) and I'm satisfied this is the proportionate resolution:

• The three ratchet floors — including source_card_refs >= 9821 — now catch the realistic failure mode (partial-regen / truncation, the actual #6199 class where token count grew while refs silently dropped). The purely adversarial 'swap entries keeping all three aggregate totals byte-identical' is not a realistic outcome for a deterministic generator reading upstream data.
• Your point that no unit test can establish provenance is correct — the tokens-gen input (data/mtgjson/sets) is gitignored and absent from the test binary. A true provenance gate belongs in CI, and the documented fetch-token-sets → tokens-gen → cmp seam is the right recipe for it.
• A per-PR CI cmp gate is worthwhile defense-in-depth, but it must be vintage-aware (only hard-fail the cmp when the fetched MTGJSON .meta.date equals the committed mtgjson-vintage stamp; when newer, that's expected staleness, not tampering) or it re-introduces the exact weekly red this PR removes. That's follow-up-sized, not a blocker on a data+test PR.

Tracking the CI provenance gate separately; #6278 (weekly auto-refresh) is the natural home for it and also makes the automated regen the catalog's sole update path, giving provenance-by-construction going forward.

Non-blocking nit for a future edit: the body's source-ref arithmetic reads +1184/8637; the actual delta is 8638 → 9821 (+1183). Cosmetic, not gating.

[lgray]

🤖 AI text below 🤖

Correcting my own previous comment before you spend review time on it — an independent check of that reply found one claim overstated. Three additions, all measured.

1. Correction: the generation seam does not verify provenance. I wrote that your check "is what gen-card-data.sh:253-258 already does". That overstates it, and the script says so itself:

# tokens-gen output is deterministic, so only overwrite when content actually
# changed — an unconditional copy bumps the file's mtime and forces a full
# (40-65s) engine recompile via build.rs's rerun-if-changed for nothing.
if cmp -s "$TOKENS_TMP" "$TOKENS_FILE"; then

That cmp is an mtime-churn optimization, not a gate: its mismatch branch (:258-268) promotes — it overwrites the tracked catalog and never fails. The only refusal (:269-274) is the vintage gate (ALLOW_TRACKED_WRITES, set at :134-137), which fires on a backward .meta.date, not on content divergence. So the seam produces the catalog and prevents it regressing to older inputs; it does not verify an already-committed one. Your "no provenance gate" reading is correct as stated for both CI and the seam — the narrower true claim is that the catalog cannot silently regress to a stale vintage. The comment landed in 2f30f859c says this correctly; my summary of it did not.

2. The canonical command (mine carried a needless flag). crates/engine/Cargo.toml:76-78 declares tokens-gen with no required-features (contrast oracle-gen, :54-56), and grep -rn 'feature = "cli"' crates/engine/src → 0 hits, so --features cli is inert for this bin and merely re-fingerprints the engine (gen-card-data.sh:221-224 documents that cost). The form in tokens_gen.rs:4 is the right one, and it reproduces identically:

$ cargo run --bin tokens-gen -- --input data/mtgjson/sets --output /tmp/kt-nofeat.toml
Generated 2858 token presets at /tmp/kt-nofeat.toml
$ sha256sum /tmp/kt-nofeat.toml
9dc2222646ea230f46a09da0359f5aadf05c9e5349e1022fa54ea3148fe3c9b3

Determinism now has three independent confirmations, all 9dc22226…: the original W30 regen (byte-identical across two checkouts, per 34912e03a), the --release run in my previous comment, and this dev-profile run — i.e. stable across cargo profiles, not an artifact of one build.

3. Conceding the limit of my "∀-quantified" point. You're right that it doesn't rescue the guard. Both assert_eq!s range over whatever the catalog currently contains, so substituting one legitimately-generated entry for another satisfies them. It shrinks the surviving-mutation space; it is not a provenance check, and I shouldn't have offered it as a rebuttal.

What I considered and did not add, so you can see it was priced rather than skipped:

option	why not
committed full-file digest asserted in the test	re-breaks every addition-only weekly refresh — the #6237 false-red this PR exists to remove
tokens-gen --check flag	redundant: --output <tmp> + cmp already is the check
sorted id-manifest (known-token-ids.txt), asserted as committed ⊆ current	the only test-time option that actually catches loss/swap/rename, and it is monotone under additions — but ~106 KB tracked, append-only upkeep, and a hand-added entry still passes, so it does not prove tokens-gen output either. Zero observed instances of the failure mode; the one real regression (#6199) was a count drop, which source_card_refs >= 9821 already catches
free structural invariants (ids strictly ascending; token_image_ref.preset_id == id) — both measured true for all 2,858	genuine hardening, but they do not close your stated failure mode, and adding them under a blocker would be responsiveness theatre

Say the word on any of these and I'll add it. Otherwise my position is unchanged: the discriminating gate is the CI one you described, it needs a .github/workflows/** edit I'm barred from making, and the run above is evidence it works when wired.

One caveat for whoever does wire it: output order is pinned by tokens_gen.rs:106 (presets.sort_by(|a, b| a.id.cmp(&b.id))) plus BTreeMap/BTreeSet for the source index, but :97 dedupes token uuids in fs::read_dir order (:79, unsorted) — so if a uuid ever appeared in two set files, the winning entry's set_code/set_name/released_at would depend on directory iteration order. Measured across all 450 set files: 9,057 token uuid entries, 9,057 distinct, 0 duplicates — deterministic today, but that is where a CI flake would come from on a differently-ordered filesystem.