fix(oracle-gen): stop the card export rewriting the parser's own subtype vocabulary#5745
Merged
Conversation
…ype vocabulary `cargo export-cards` rewrote the tracked, committed parser input crates/engine/data/oracle-subtypes.json on every run, deleting 26 creature subtypes (Army, Servo, Pentavite, Sculpture, Tentacle, ...) and corrupting the working tree of anyone who ran an export. Root cause: the vocabulary is CardTypes.json (canonical) union the AtomicCards harvest (type-line corroborated). Token-only subtypes are printed on no card face, so they live in CardTypes.json alone -- the harvest can never corroborate what a type line never carries. `load_card_types(..).ok()` silently swallowed a missing sidecar and regenerated from the harvest alone, dropping exactly those 26. No CI workflow, and no bare `cargo export-cards`, ever downloads CardTypes.json -- only scripts/gen-card-data.sh does -- so the degraded write was the norm, not the exception. The file is `include_str!`d by the parser, so the rewrite bumped its mtime and rebuilt the engine against the degraded vocabulary. That gave the export a feedback loop: run 1 parsed with the committed 392-entry vocabulary, run 2 rebuilt and parsed with the 366-entry one. Twenty token-subtype faces changed between consecutive exports at identical source (Servo Exhibition's token name flipping "~" <-> "Servo", Pentavus losing Subtype: Pentavite, Doomed Artisan SelfRef <-> Typed(Sculpture)), moving the pool-wide Unimplemented count by 3. This was previously read as nondeterminism in the token/subtype synthesis; it is not. Three runs of a fixed binary are byte-identical. The export is deterministic -- it was rebuilding itself between runs. The same rewrite fires in CI, where it silently degraded the parser that card-data-validate, coverage-report, and semantic-audit then ran against. Fix: - Gate the vocabulary refresh behind an explicit `--write-subtypes`. A plain export is now a pure read: it never mutates the tree it is measuring. - Pass the flag from scripts/gen-card-data.sh, the sole caller that fetches the MTGJSON sidecars. - Under the flag, CardTypes.json is REQUIRED: `build_creature_subtype_vocabulary` takes it by reference rather than `Option`, so a partial source set cannot reach the writer, and a missing sidecar is a hard, actionable failure instead of a quiet 26-subtype deletion. - Guard it in CI: `git diff --exit-code` on oracle-subtypes.json after the export, scoped to run exactly when an export ran so it is never vacuous. - Pin the invariant in subtype_vocab: the harvest recovers card-printed subtypes (Mammoth) but never token-only ones (Army, Germ). Evidence (full pool, 35,396 faces, base 8aee471): - Before: one export deleted the 26 subtypes; three engine tests went red with no source change (is_subtype_word_recognizes_token_only_creature_subtypes, counter_plus_one_replacement_mauhur_scope, mauhur_does_not_add_counters_to_ unlisted_creature_types). A second export diverged from the first on exactly 20 faces, +3 Unimplemented. - After: three consecutive exports are byte-identical to each other AND to the pre-fix first-run baseline (sha 857d3c68) -- the 20 faces are stabilised at their correct parse, and nothing else moved. oracle-subtypes.json is untouched throughout. With the sidecar present, --write-subtypes regenerates the file byte-identically ("Skipped write ... unchanged"): the refresh is lossless. - The 3 tests pass with no checkout-revert.
matthewevans
enabled auto-merge
July 13, 2026 16:37
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new --write-subtypes flag to make the regeneration of the committed creature-subtype vocabulary a deliberate action. It refactors build_creature_subtype_vocabulary to require CardTypesFile by reference instead of as an Option, ensuring that token-only subtypes are not silently dropped due to a missing source file. The review feedback correctly points out a minor documentation error in a comment that incorrectly describes the parameter as being passed 'by value' instead of 'by reference'.
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.
| // can NEVER recover a token-only subtype — no card face prints "Army". | ||
| // CardTypes.json is therefore the sole source of that half of the | ||
| // vocabulary, which is why `build_creature_subtype_vocabulary` takes it | ||
| // by value rather than as an `Option`: regenerating the committed, |
Contributor
There was a problem hiding this comment.
[MED] Incorrect comment description of parameter passing. Evidence:
crates/engine/src/database/subtype_vocab.rs:289.\n> Why it matters: The comment states thatbuild_creature_subtype_vocabularytakescard_types"by value", but the function signature actually takes it by reference (&CardTypesFile).\n> Suggested fix: Change "by value" to "by reference" in the comment.
Suggested change
| // by value rather than as an `Option`: regenerating the committed, | |
| // by reference rather than as an Option: regenerating the committed, |
References
- Accurate documentation and comments are essential for maintainability and alignment with the repository's standards. (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 14, 2026
* fix(parser): a cross-LINE "instead" override must BIND or FAIL — never publish as a sibling
CR 614.15 says a self-replacement effect replaces "part or all of that spell or
ability's own effect(s)", and that its text "can be a separate ability,
particularly when preceded by an ability word". CR 614.6 says a replaced event
never happens. So an ability-word override line ("Corrupted — Counter that spell
instead if …") must be BOUND into the ability it replaces. Publishing it as a
second, independent top-level ability makes the engine perform the base effect
AND the replacement — the exact CR 614.6 violation phase-rs#44 killed intra-chain.
The cross-line binder already existed and built the right shape. Two defects fed
it a condition-less override, and it silently fell through to a sibling emission:
1. VOCABULARY ASYMMETRY. The cross-line path lowered its condition through the
nom `StaticCondition` grammar alone, which cannot express a target-relative
predicate. The intra-chain path (`build_instead_def`) ran a three-parser
ladder that can. A condition the chain lowers fine — "its controller has three
or more poison counters" — was therefore dropped at the line level. Both paths
now lower through ONE authority, `conditions::lower_instead_condition`, so the
two vocabularies cannot drift apart again.
2. The poison predicate was not parameterized. CR 122.1f DEFINES "poisoned" as
one or more poison counters, so "its controller is poisoned" (Corrupted
Resolve) and "its controller has three or more poison counters" (the Corrupted
ability word) are the same predicate at two thresholds — same subject, same
`QuantityRef::TargetControllerCounter`, same `GE`. They are now one combinator
parameterized on its threshold, not two.
And the floor: when a cross-line override cannot be conditioned at all, it is now
reported as `Effect::unimplemented` rather than published unconditioned. An
honest red, never a silent double-execution (mirrors the intra-chain
`InsteadLowering::ConditionUnlowerable` floor).
Runtime witness (Anoint with Affliction), watched RED before this commit:
"Exile target creature if it has mana value 3 or less. / Corrupted — Exile that
creature instead if its controller has three or more poison counters." Its
override published a second, condition-less `ChangeZone -> Exile { ParentTarget }`.
Targeting a mana-value-5 creature whose controller has ZERO poison counters —
both printed gates false — the engine EXILED it anyway (left: Exile, right:
Battlefield). It now stays on the battlefield, and the SHAPE test pins that the
override is bound as a `ConditionInstead` branch on a single top-level ability
rather than merely neutered into an inert sibling.
8122 parser tests pass; parser combinator gate A PASS.
* fix(parser): bind the CR 614.15 PARTIAL cross-line override ("… instead of <N>")
CR 614.15: a self-replacement effect replaces "part or all" of the ability's own
effect, and the printed grammar says WHICH. A bare trailing "instead" replaces the
whole clause. A trailing "instead of <N>" replaces only a PART — the antecedent's
quantity — and names the OLD value it displaces ("put up to two creature cards …
into your hand instead of one").
The cross-line gate recognized only the whole-clause printings, so every partial
override fell through it and was published as an INDEPENDENT top-level ability —
CR 614.6 double-execution. Two things were needed:
1. The trailing "of <N>" is a MARKER, not an operand. It is redundant with the
antecedent, which still holds the old value, so it carries nothing the branch
needs. `is_replacement_marker_tail` now accepts a bare "instead" and an
"instead of <N>" as the same replacement marker, via one combinator. Anything
else after "instead" ("instead of putting it into your hand") names a
NON-quantity part and is deliberately NOT accepted: treating it as a
whole-clause marker would silently drop the part it names.
2. The override's body cannot be lowered in isolation. Gather the Pack's
"put up to two creature cards from among the revealed cards into your hand"
parses alone to a bare `ChangeZone` — losing the reveal, the library source and
the rest-to-graveyard rider the printed Dig carries. Binding THAT as the
replacement would trade the double-execution for an effect LOSS.
`try_parse_dig_instead_alternative` is the existing antecedent-parameterized
handler for exactly this shape (it already serves Follow the Lumarets
intra-chain): it rebuilds the alternative as a full `Dig` that reuses the
preceding Dig's source and reveal-mode and swaps only what the override
changes. It is now also reached across a line boundary, handed the previous
LINE's def as its antecedent — the same relationship, one line further out.
The rebuilt alternative carries its own condition, so it flows into the existing
ability-word merge and the existing cross-line binder untouched: the binder wraps
it in `ConditionInstead` and parks the printed Dig as the `else_ability` fallback.
No new binding mechanism.
Runtime witness (Gather the Pack), watched RED before this commit — TWO top-level
abilities (left: 2, right: 1). The stray second ability was
`ChangeZone { destination: Hand, target: Creature, up_to, multi_target: 0..2 }`:
with spell mastery on, the card both dug AND offered to BOUNCE up to two creatures
off the battlefield, an effect it does not have. It is now one ability whose Dig
carries a ConditionInstead alternative Dig — keep_count 1 -> 2, with the top-five
source, the reveal and the rest-to-graveyard rider all preserved (asserted).
8122 parser tests pass; parser combinator gate A PASS.
* fix(parser): the residual cross-line overrides fail honestly — no face double-executes
Closes the class. The two commits before this one BIND the cross-line
self-replacement printings that can be bound faithfully. This one makes the
remainder fail honestly, so that after this commit NO face in the class publishes
a self-replacement override as an independent ability (CR 614.6).
The residuals are the CR 614.15 PARTIAL printings whose antecedent is not a Dig:
Nissa's Pilgrimage "search your library for up to three basic Forest cards
instead of two" -- replaces the FIRST clause's count
Talent of the Telepath "... instead of one" -- replaces a NON-FIRST clause
See the Unwritten "put two creature cards onto the battlefield instead of
one" -- Dig antecedent, but the body omits
"from among them", so the alternative
cannot be rebuilt from it
Caravan Vigil "put that card onto the battlefield instead of putting it
into your hand" -- replaces a NON-FIRST clause's
destination
These all carry a perfectly good condition, so it is tempting to hand them to the
binder. That would be WRONG, and silently so. The binder binds the FIRST emitted
clause and parks the base's tail in `else_ability`, which the runtime walks ONLY
when the swap does NOT fire: Nissa's Pilgrimage would search for three basic
Forests and then never reveal them, never put one onto the battlefield, never
shuffle. That trades a double-execution for an effect LOSS — a different silent
wrong, and the one this unit was explicitly chartered not to introduce.
A faithful bind needs clause-level antecedent selection (the override replaces the
base clause it PARALLELS, not necessarily the first) plus a tail that survives in
BOTH branches. That is a distinct mechanism; it is filed, not guessed at. Until it
exists these fail honestly: the base ability stands exactly as printed, and the
override is reported `Effect::unimplemented`.
The "would" exclusion is CR 614.1 — a replacement effect watches for an event that
WOULD happen. A "would" clause names an EVENT (CR 614.1a) and is owned by the
replacement machinery, so claiming it here would replace a WORKING rider encoding
with an honest red (this is the boundary phase-rs#44 established; 68 faces regressed when
it was missing).
Full-pool ledger (35,396 faces, base origin/main efe7409 vs after, ONE export
each, zero noise floor post-phase-rs#5745) -- MEASURED:
8 faces changed, 4 reds gained, 0 reds lost.
3 BOUND (2 -> 1 top-level abilities): Anoint with Affliction, Bring the Ending,
Gather the Pack.
4 silent double-execution -> HONEST RED: Caravan Vigil, Nissa's Pilgrimage,
See the Unwritten, Talent of the Telepath.
1 shape change, behaviour verified unchanged and pinned: Precognitive Perception.
Nothing else in the pool moved: the residual predicate did not over-fire.
Tomb of Horrors Adventurer (the phase-rs#80 regression face) is NOT in the changed set —
its "instead" is INTRA-line inside a trigger and never reaches this path.
Precognitive Perception is the face the ledger surfaced that this unit did not
predict. Widening the condition vocabulary made its Addendum line take the
strip-the-body path, moving the override's second clause ("then draw three cards")
out from under the condition the chain had been stamping on it. The engine runs an
unswapped `ConditionInstead` sub's tail when it has no `else_ability`, so an
unguarded tail would have drawn three MORE cards outside your main phase — six off
a card that draws three. Driven through the real cast pipeline in the upkeep it
draws exactly three; the assertion is pinned, and proven able to fail (asserting 6
reports left: 3, right: 6).
8122 parser tests pass; parser combinator gate A PASS.
* fix(lint): blank line before a doc-list continuation paragraph (clippy::doc_lazy_continuation)
---------
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.
cargo export-cardsrewrote the tracked, committed parser inputcrates/engine/data/oracle-subtypes.json on every run, deleting 26 creature
subtypes (Army, Servo, Pentavite, Sculpture, Tentacle, ...) and corrupting the
working tree of anyone who ran an export.
Root cause: the vocabulary is CardTypes.json (canonical) union the AtomicCards
harvest (type-line corroborated). Token-only subtypes are printed on no card
face, so they live in CardTypes.json alone -- the harvest can never corroborate
what a type line never carries.
load_card_types(..).ok()silently swallowed amissing sidecar and regenerated from the harvest alone, dropping exactly those
26. No CI workflow, and no bare
cargo export-cards, ever downloadsCardTypes.json -- only scripts/gen-card-data.sh does -- so the degraded write
was the norm, not the exception.
The file is
include_str!d by the parser, so the rewrite bumped its mtime andrebuilt the engine against the degraded vocabulary. That gave the export a
feedback loop: run 1 parsed with the committed 392-entry vocabulary, run 2
rebuilt and parsed with the 366-entry one. Twenty token-subtype faces changed
between consecutive exports at identical source (Servo Exhibition's token name
flipping "~" <-> "Servo", Pentavus losing Subtype: Pentavite, Doomed Artisan
SelfRef <-> Typed(Sculpture)), moving the pool-wide Unimplemented count by 3.
This was previously read as nondeterminism in the token/subtype synthesis; it is
not. Three runs of a fixed binary are byte-identical. The export is
deterministic -- it was rebuilding itself between runs.
The same rewrite fires in CI, where it silently degraded the parser that
card-data-validate, coverage-report, and semantic-audit then ran against.
Fix:
--write-subtypes. A plainexport is now a pure read: it never mutates the tree it is measuring.
MTGJSON sidecars.
build_creature_subtype_vocabularytakes it by reference rather than
Option, so a partial source set cannotreach the writer, and a missing sidecar is a hard, actionable failure instead
of a quiet 26-subtype deletion.
git diff --exit-codeon oracle-subtypes.json after theexport, scoped to run exactly when an export ran so it is never vacuous.
(Mammoth) but never token-only ones (Army, Germ).
Evidence (full pool, 35,396 faces, base 8aee471):
no source change (is_subtype_word_recognizes_token_only_creature_subtypes,
counter_plus_one_replacement_mauhur_scope, mauhur_does_not_add_counters_to_
unlisted_creature_types). A second export diverged from the first on exactly
20 faces, +3 Unimplemented.
pre-fix first-run baseline (sha 857d3c68) -- the 20 faces are stabilised at
their correct parse, and nothing else moved. oracle-subtypes.json is untouched
throughout. With the sidecar present, --write-subtypes regenerates the file
byte-identically ("Skipped write ... unchanged"): the refresh is lossless.