refactor(parser): hoist Class-H replacement producers to the oracle document IR seam - #6123
Merged
Conversation
…ocument IR seam Replacement producers now emit typed OracleNodeIr::Replacement document nodes instead of PreLoweredReplacement, giving the document seam a single authority for replacement lowering. Relation discovery (item_replacement) accepts both representations so ID-keyed LinkedChoice relations survive the hoist: choose + chosen-dependent ETB counters still compose into one replacement and chosen-type cost reducers keep their linkage. Six not-yet-constructed IR/index APIs are re-homed to the Class-B bring-up under dated PLAN-05 DEBT item-level allows. The four oracle_ir snapshots move representation-only (PreLoweredReplacement -> Replacement). Full-corpus card-data export verified byte-identical to the pre-hoist baseline (sha256 18fe47ae07...aac7e8b5, 35424 faces).
matthewevans
enabled auto-merge
July 18, 2026 00:32
![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 refactors the Oracle parser to transition from pre-lowered definitions to typed intermediate representations (IR), specifically introducing ReplacementIr and OracleNodeIr::Spell(EffectChainIr) to represent parsed elements before lowering. It updates several parsing functions, the DocEmitter helper, and the item resolution logic to process these IR nodes uniformly. The single review comment regarding a missing #[allow(dead_code)] attribute has been removed, as the repository style guide explicitly instructs reviewers not to duplicate Clippy or compiler warnings that are already covered by the CI gate. Consequently, there is no additional feedback to provide.
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.
Parse changes introduced by this PR✓ No card-parse changes detected. |
andriypolanski
pushed a commit
to andriypolanski/phase
that referenced
this pull request
Jul 28, 2026
…se-rs#6722) * fix(parser): Plan 05b T9a — route U0-39 through lower_ability_ir The instant/sorcery prevention recognizer (CR 615.1a: "Effects that use the word 'prevent' are prevention effects") was the only spell path in the parser that lowered a whole ability body without `finalize_effect_chain`, the owner-library reveal anchor, and the `WithContext` whole-body recognizer set. It now goes through `parse_ability_ir_with_context` + `ability_ir_at`, i.e. `lower_ability_ir` — which is what `parse_effect_chain_with_context` has always been. phase-rs#6123 introduced the skip. Its hunk at oracle.rs:5108 replaced let def = parse_effect_chain_with_context(&line, AbilityKind::Spell, &mut ctx); with the raw pair `parse_effect_chain_ir` + `lower_effect_chain_ir` while hoisting the Class-H replacement producers. At 61e597a^, `parse_effect_chain_with_context` was `try_parse_chain_bypass(..)` followed by `lower_ability_ir(..)`, so the conversion dropped THREE things, not the two the recensus records: finalize, the anchor, and the bypass set. The skip was incidental — the commit body describes only the replacement hoist and never mentions a spell path, and its byte-identity claim is why it passed review. Measured over the real pool (data/mtgjson/AtomicCards.json, 34,739 cards / 35,516 faces, generated from the repo root): card-data.json is byte-identical before and after, sha256 c3f8431c494c1a9710e08fc15161eab75203a2dc98a5aa35e3f7a993ec43c245, 99,201,978 bytes. Zero delta is a semantic no-op, not an unapplied probe. The site is reached 155 times across 153 cards; disabling it moves the export (probe went red), and the compiler independently witnesses the conversion — `OracleNodeIr::Spell` lost its only producer. Per-mechanism reach over those 155 reaches: * finalize_effect_chain (6 passes): no-op — lower_ability_ir(&ir) == lower_effect_chain_ir(&ir.body) on all 155 * owner-library reveal anchor: 0 cards — no instant/sorcery prevention line carries both "shuffles it into their library" and "reveals the top card of their library" * WithContext whole-body bypasses: 0 cards — no bypass fires on any of the 155 * CR 707.9a printed-slot stamping: 0 cards — all 7 pool-wide carriers of RetainPrintedAbilityFromSource are permanents, so none reaches an is_spell site * has_unimplemented gate: 0 flips — 148 emit and 7 decline, identically both ways So this is a latent-defect fix with no present-day behavioral change: it closes the CR 707.9a mis-index window recorded as recensus harvest item 4 (now provably unreachable rather than merely "plausible/latent"), and makes U0-39 conform to phase A, which is the precondition `ability_ir_at`'s doc block names for T9b's payload swap. The `OracleNodeIr::Spell` payload is untouched — that swap is T9b. * test(parser): Plan 05b T9a — two-layer guards for the prevention recognizer 153 cards in the pool reach the instant/sorcery prevention recognizer (U0-39) and, before this commit, NOT ONE of them was snapshotted. The only spell path that lowered a whole ability body without finalize, the owner-library anchor and the WithContext bypass set was also the one with no two-layer guard — which is how phase-rs#6123 narrowed it without any test noticing, and why T9a's full-pool zero delta had nothing local to pin it to. These are the §5.3 remediation for that: T9b's payload swap lands on this exact recognizer, so it must not be able to move it silently. * Fog — the canonical single-clause case. The whole card is the prevention sentence, so the chain is one clause with no sub_ability. * Blunt the Assault — the multi-clause case the recognizer exists for. The site's own comment cites this shape verbatim ("preserve any preceding clauses ('You gain 1 life for each ...')") because the prevention marker sits in the SECOND sentence, so a replacement classifier reaching the line first would drop the life gain. It lowers to GainLife{ObjectCount(Creature)} with the PreventDamage chained as sub_link: SequentialSibling — two independent instructions in the order written, CR 608.2c. This is the non-degenerate fixture: a one-clause body would take the same path either way and prove nothing about chain assembly. Both texts are verbatim MTGJSON, not paraphrases — a paraphrase can take a different parser branch and go green while the real card stays broken. Snapshots read and verified correct before acceptance, not merely accepted as current: both nodes are PreLoweredSpell (the visible witness of T9a's routing), Fog is PreventDamage{All, Any, CombatDamage} with UntilEndOfTurn, and Blunt the Assault retains BOTH clauses. --------- 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.
Replacement producers now emit typed OracleNodeIr::Replacement document
nodes instead of PreLoweredReplacement, giving the document seam a single
authority for replacement lowering. Relation discovery (item_replacement)
accepts both representations so ID-keyed LinkedChoice relations survive
the hoist: choose + chosen-dependent ETB counters still compose into one
replacement and chosen-type cost reducers keep their linkage.
Six not-yet-constructed IR/index APIs are re-homed to the Class-B bring-up
under dated PLAN-05 DEBT item-level allows. The four oracle_ir snapshots
move representation-only (PreLoweredReplacement -> Replacement).
Full-corpus card-data export verified byte-identical to the pre-hoist
baseline (sha256 18fe47ae07...aac7e8b5, 35424 faces).