fix(scripts): index token printings Scryfall does not label by layout - #7335
Conversation
Role tokens (Wilds of Eldraine: Wicked, Cursed, Royal, Young Hero, Monster, Virtuous, Sorcerer) rendered with no art. Scryfall prints the two Roles on one card with the second upside down, so it classifies all 6 printings as `layout: "flip"` — not "token" or "double_faced_token". The generator selected on `layout` alone, so `scryfall-token-images.json` carried zero Role entries and all 12 Role presets in known-tokens.toml had a `token_image_ref` that resolved to nothing in `fetchTokenImageByRef`. `is_token_printing` keeps the two layout values as a fast path and adds a layout-agnostic clause: a printing whose type line marks it as a token is a token, whatever Scryfall calls its layout. Roles are not alone out there — the Secret Lair `Mechtitan // Mechtitan` is `layout: "reversible_card"` with a null top-level type line and token-typed faces, and the two presets pointing at it were silently degrading to the `tneo` printing's art through the `oracle:` fallback. Gating the clause on "flip" would have fixed the Role instance and left the class. `type_line` is the discriminator rather than `set_type` because two of the twelve Role presets point at the Wicked // Cursed printing in The List (`set_type: "masters"`), and rather than `layout` because that would pull the genuine flip cards (21 cards, 39 printings — Budoka Gardener, Erayo, ...) into a table of tokens. Both the top-level and per-face type lines are tested: 81 of 120 `double_faced_token` entries have no "Token "-prefixed top-level `type_line` (62 read "Card // Card") and the Mechtitan printing has none at all. `validate_token_images` is the single authority for "is this table healthy", and all three gates now use it: * **The skip-guard.** Skipping on existence alone is what let this table stay wrong — any checkout, cache or dev machine already holding a Role-less file would never regenerate it, so the fix would reach nobody who had run the script before. It now skips only a table that still passes the check; a stale or truncated one costs 4s to rebuild. * **A pre-install gate**, in the shape gen-scryfall-locale-images.sh uses for its own empty classes, with a message separating the benign cause from the real one. Nothing ever noticed the class was empty; the table stayed plausible and the art simply never appeared. * **The recovery branch** inside `scryfall_finalize_download`, where the generic "is this JSON" would accept an empty table that beat us into place. The check is arithmetic, not a string compare against "0": jq exits 0 with EMPTY stdout on a 0-byte or whitespace-only input, so `|| echo 0` never fires and `[ "" = 0 ]` reads as healthy. Both gates in this file got that wrong once during review. Generation also builds into a `.tmp` staging file (the suffix `.gitignore` already covers here) under a trap and installs it with the shared `scryfall_finalize_download`, instead of truncating the output before jq runs. Also gitignores the finalizer's rename diagnostic: this script is its first caller to stage into a tracked directory. Measured against the shipped table: 17 keys added, 0 removed, 0 changed. Of the 2869 presets carrying a `token_image_ref`, all 2869 now resolve on their exact `scryfall:` key — previously 12 missed entirely and 2 fell back to another printing's art. Both faces of a flip pair share one `image_uris` — the printed card is a single image — so both Roles resolve to it, unrotated, exactly as this codebase already renders every other flip card. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 token image generator now validates existing and newly generated data, detects nonstandard token printings, and installs validated output atomically. Git ignores diagnostic files created during Scryfall downloads. ChangesToken image generation
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This localized script change improves token image generation and validation, with normal checks reported clean; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Generator as gen-scryfall-token-images.sh
participant Scryfall as Scryfall download
participant Temporary as Temporary output file
participant Destination as Final token image output
Generator->>Scryfall: Download token data
Scryfall-->>Temporary: Write generated data
Generator->>Temporary: Validate generated data
Generator->>Destination: Atomically install validated data
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The new staging-file template is invalid on macOS/Darwin, so this generator never reaches the token-indexing change on that supported platform. Evidence: scripts/gen-scryfall-token-images.sh:104 calls mktemp "$OUTPUT.XXXXXX.tmp"; Darwin mktemp requires the X run to be the template suffix, while this path has .tmp after it. Why it matters: the command exits during staging-file creation, so builds and local recovery cannot generate any token-image map, including the Role entries this PR is intended to restore. Suggested fix: use a Darwin-valid suffix such as "$OUTPUT.tmp.XXXXXX" and ignore the corresponding interrupted staging-file pattern (or use an API that explicitly supports a suffix), then rerun the script on macOS.
mkstemp(3) is EINVAL unless the template ends in XXXXXX; accepting a suffix after the X run is a GNU extension, documented as such under GNU mktemp(1) --suffix. `$OUTPUT.XXXXXX.tmp` therefore aborts this generator under `set -e` on BSD and macOS, before it can index anything at all — including the Role printings this branch exists to restore. Use `$OUTPUT.tmp.XXXXXX`, the form gen-card-data.sh:248 and lib/mtgjson-fetch.sh:70 already stage with, and add the `client/public/*.tmp.*` ignore rule that name needs: the existing `client/public/*.tmp` covers fixed-name staging files and does not match a randomised tail, so without it an interrupted run would leave an untracked 2 MB file in a tracked tree. No tracked path in client/public matches the new rule. Reported by @matthewevans in review of phase-rs#7335. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on 7ab197b156e218a78b7792433060ceda35048345: the portable trailing-XXXXXX mktemp template resolves the prior Darwin blocker, and the current two-file diff has no remaining review finding. The cancelled lint job is a workflow cancellation; the merge queue will rerun required checks.
|
Maintainer hold — current head The rerun replaced the earlier cancelled lint job, but its current card-data coverage check has failed while the Rust jobs are still settling. Approval, the No contributor action is requested solely for this hold. |
|
🤖 AI text below 🤖 Thanks for the approval — and on the hold, here is the failure evidence, since it is cheaper for me to hand over than for you to dig out. It is branch staleness, not this PR. The complete diff against this PR's base No The baseline is main's last-good The likely origin is #7327 (Forage player-action completion timing) — the only one of the seven that reworks trigger parsing ( The I am happy to rebase onto current main so the comparison is apples-to-apples; I have confirmed it is conflict-free. Flagging the cost so the choice is yours rather than mine: that moves the head off |
|
Maintainer update hold — current head I brought this branch through current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on 12a8bc41c69f2cd1f9cb73ec0560de1510e1d362: the current-main merge preserves the reviewed portable staging and token-printing classification change. Current required CI is green and this current-head recheck found no remaining implementation finding.
2 participants
🤖 AI text below 🤖
Summary
Role tokens (Wilds of Eldraine — Wicked, Cursed, Royal, Young Hero, Monster, Virtuous, Sorcerer) render with no art. Scryfall prints the two Roles on one card with the second upside down, so it classifies all 6 Role printings as
layout: "flip"— not"token"or"double_faced_token".scripts/gen-scryfall-token-images.shselects onlayoutalone, so every Role printing fell through the filter andclient/public/scryfall-token-images.jsoncarried zero Role entries.The chain is fully determined, not inferred:
crates/engine/data/known-tokens.toml,[token.token_image_ref]scryfall:<id>, thenoracle:<oracle_id>:<face_name>client/src/services/scryfall.ts:887fetchTokenImageByRefscryfall:key#5232(merged) fixed preset↔token matching one layer up and claimed "resolve face art"; that claim died at the empty table. This is the next link in the same chain, not a second attempt at the same seam.The fix is for the class, not the Roles
is_token_printingkeeps the two tokenlayoutvalues as a fast path and adds a layout-agnostic clause: a printing whose type line marks it as a token is a token, whatever Scryfall calls its layout.The first draft of this PR conjoined that clause with
layout == "flip". Review caught it as instance-shaped, and the corpus proved it: Roles are not alone out there. The Secret LairMechtitan // Mechtitan(5d02dee4-…) islayout: "reversible_card"with a null top-leveltype_lineand token-typed faces. Two real presets point at it, and both were silently degrading through theoracle:fallback to thetneoprinting's art instead of the Secret Lair art they name. Gating on"flip"would have fixed the Roles and left the class — and would have left the one card already broken by it still broken.Today the clause admits exactly 7 printings the layout values do not already carry: the 6 flip Roles and that Mechtitan. (The type-line test itself is true for 2797 printings; the other 2790 arrive on
layoutanyway.) Nothing else in the corpus carries aToken-prefixed type line outside the two token layouts.Three details are load-bearing, all measured:
type_line, notset_type. Two of the twelve Role presets point at theWicked // Cursedprinting in The List, whoseset_typeis"masters". Aset_type == "token"filter strands exactly those two.type_line, notlayout. Barelayout == "flip"would pull the genuine flip cards — 21 cards, 39 printings (Budoka Gardener, Erayo, Akki Lavarunner, …) — into a table of tokens.double_faced_tokenentries have noToken-prefixed top-leveltype_line(62 read"Card // Card"), and the Mechtitan printing has none at all. Those survive onlayouttoday, but a top-level-only test would silently drop the next multi-face token that has to enter through this clause.Deliberately not copying #2682's mechanism. That PR fixed the same
reversible_cardblind spot in the two sibling generators by falling back tocard_faces[0].oracle_id. Doing that here would emit anoracle:a4fecf0a-…:mechtitankey from thesldprinting, which sits at corpus index 42383 againsttneo's 1160 —from_entriesis last-wins, so it would overwrite an existing entry and silently repoint every other Mechtitan consumer at Secret Lair art. The layout-agnostic clause reaches the same printing through itsscryfall:key and changes nothing that already worked.One health check, three gates
validate_token_imagesis the single authority for "is this table healthy" — the count of printings the tokenlayoutvalues do not carry. Three gates now use it:preview-inputs, and not covered by thedata/scryfallcache), so CI behaviour is byte-identical to base.gen-scryfall-locale-images.shuses for its own empty classes. The message separates the benign cause (Scryfall reclassified these into a tokenlayout— confirm they still arrive, then drop the clause) from the real one (the type-line prefix changed and the class is about to go artless). It is a presence check, not a completeness one; the comment says so and says why.scryfall_finalize_download, where the generic "is this JSON" would accept an empty table that beat us into place.The check is arithmetic, not a string compare against
"0". jq exits 0 with empty stdout on a 0-byte or whitespace-only input, so|| echo 0never fires and[ "" = 0 ]reads as healthy — both gates in this file were written that way once and caught in review.Atomic install. Generation builds into a
.tmpstaging file (the suffix.gitignorealready covers in this directory) under atrap, and installs it with the sharedscryfall_finalize_download. Writing straight to$OUTPUTtruncated it before jq ran, so an interrupt or an OOM on the 600 MB+ input left a 0-byte file for the skip-guard to certify.Files changed
scripts/gen-scryfall-token-images.sh— theis_token_printingpredicate and its rationale, the empty-class guard, the temp+rename install and its validator..gitignore— coverscryfall_finalize_download's rename diagnostic; this script is its first caller to stage into a tracked directory.Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — no change under
crates/engine/. This is a data-generation script plus a.gitignoreline, pathsCONTRIBUTING.mdnames as directly editable. No game logic, parser, effect, trigger, targeting or state-machine behavior is touched; everyTokenImageRefthe engine emits is unchanged, and only the lookup table they resolve against gains rows. Six/review-implpasses ran regardless, per CONTRIBUTING; rounds 1–5 each produced findings addressed with code — two factual errors in the comments, an instance-shaped predicate, a non-atomic write, a file-mode regression, and the same empty-stdout hole in both health checks. Round 6 returned no findings.CR references
None — no rules logic.
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.
bash -n scripts/gen-scryfall-token-images.sh— clean./scripts/gen-scryfall-token-images.sh—4785 entries, 7 via type line, output mode 644, no staging file left behind; 4.3s cold, 0.02s when skippingDiff against the shipped table (same
default-cards.jsoninput) — 17 keys added, 0 removed, 0 values changed. 7scryfall:keys (6 Roles + Mechtitan) and 10oracle:keys; Mechtitan's nulloracle_idsuppresses its oracle keys, which is what keeps the existingoracle:a4fecf0a-…:mechtitanvalue intact.End-to-end, over the whole catalog — of the 2869 presets in
known-tokens.tomlcarrying atoken_image_ref, all 2869 now resolve on their exactscryfall:key. Before: 2855 exact, 12 missed entirely (the Roles), 2 fell back to another printing's art (Mechtitan SLD).Two-way control — with the type-line clause deleted from
is_token_printing, the script prints its ERROR block, exits1, writes no output file and leaves no staging file. With it present, 7 printings.Validator against its real inputs — accepts the generated table; rejects 0-byte, whitespace-only, malformed, a bare scalar,
[],{}, and a layout-only table.Skip-guard, four ways — absent → builds; healthy → skips; 0-byte → rebuilds; Role-less shipped table installed as the output → rebuilds to 4785.
./scripts/check-parser-combinators.sh— Gate G PASS, Gate A PASSpre-commit — Gate P PASS
Tilt is not installed in this environment;
scripts/tilt-wait.shwould return3(cannot answer), never a build result. No Rust or TypeScript source changed, so no Tilt resource is implicated.Gate A
Gate A PASS head=a4a8f22a5bd045602a16c75b8ebd397ccc60302c base=1f612b55d8897f3734efef381a150a03392b589b
Anchored on
scripts/gen-scryfall-images.sh+scripts/gen-scryfall-printings.shvia fix missing art for reversible cards #2682 (Fixes #2031) — the existing authority that Scryfall's non-tokenlayouts need explicit handling in these generators. That PR fixedreversible_cardin the two sibling scripts and left this one out; the same class of printing is why the clause here is layout-agnostic rather than flip-specific.scripts/lib/scryfall-fetch.sh:41scryfall_finalize_download— the existing temp+rename authority, already used by both bulk download paths in the same library. The generator now installs its output the same way instead of truncating in place.scripts/gen-scryfall-locale-images.sh:67,:105— the house guard idiom the empty-class check copies: an expected class coming back empty is an upstream shape change to look at, so itexit 1s naming the suspected cause rather than shipping a table quietly missing a class.Final review-impl
Final review-impl PASS head=a4a8f22a5bd045602a16c75b8ebd397ccc60302c
Claimed parse impact
None. No engine input changes;
known-tokens.toml,card-data.jsonand everyAbilityDefinitionare untouched. The only artifact that changes is the gitignoredclient/public/scryfall-token-images.json, regenerated from scratch bydeploy.yml:339/release.yml:319— it is never cached across runs, onlydata/scryfallis. Local checkouts need no manual step: the skip-guard now rejects the Role-less table and rebuilds it.Scope Expansion
None. Two adjacent defects were found while measuring this one. Both are deliberately not in this PR — recorded here so they are not mistaken for oversights, and offered as follow-ups.
isFlipreaches the token path fromisCardImageFlipLayoutSync(resolvedOracleId, cardName)(client/src/hooks/useCardImage.ts:570,:592,:618), which resolves throughscryfall-data.json. That file is keyed by$card.nameandcard_faces[0].nameonly, sowicked,royalandmonsterare present butcursed,young hero,virtuousandsorcererare not — verified by direct lookup. The four Roles whose printed image reads the other Role's name upright are exactly the four with no Ctrl-spin. The clean fix is atokenImagesResolvedmodule global besideprintingsDataResolved(client/src/services/scryfall.ts:74), derivingisFlipin theisTokenbranch from the token entry's ownlayout— which this table already carries. Note this is a display affordance, not the art:isSidewaysLayoutissplit-only, so this codebase never auto-orients any flip card, and every Role now shows the printed token exactly as a Budoka Gardener does.gen-scryfall-images.sh:70NON_PLAYABLEomits"flip", so the 5 Role oracle entries are emitted intoscryfall-data.jsonas playable cards. That leak is the only reason three Roles have a spin today. Adding"flip"toNON_PLAYABLE— shared withgen-scryfall-printings.sh— silently removes the spin from those three unless (1) lands first or together. Hoisting a shared token predicate intoscripts/lib/belongs with that change, where it would have more than one consumer; adefwith a single consumer belongs in its own script.Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Improvements
Chores