fix(parser): Plan 05b T9a — route U0-39 through lower_ability_ir - #6722
Merged
Conversation
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. #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.
…gnizer 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 #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.
matthewevans
enabled auto-merge
July 28, 2026 08:28
Contributor
📝 WalkthroughWalkthroughThe prevention spell parsing branch now uses full ability IR parsing and emission. Snapshot tests cover Fog and Blunt the Assault, and documentation records the retained ChangesPrevention spell parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant AbilityIRParser
participant AbilityLowerer
participant AbilityEmitter
OracleParser->>AbilityIRParser: parse_ability_ir_with_context
AbilityIRParser-->>OracleParser: AbilityIr
OracleParser->>AbilityLowerer: lower_ability_ir
AbilityLowerer-->>OracleParser: lowered ability
OracleParser->>AbilityEmitter: ability_ir_at
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_ir/snapshot_tests.rs (1)
140-193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the CR 614.15 exclusion in the new tests.
These fixtures only exercise the positive prevention route; none proves that ability-word self-replacement text still bypasses it. Add an Arrow Storm/Lightning Surge-shaped case and assert that the existing
ConditionInsteadstructure is preserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs` around lines 140 - 193, Extend the prevention recognizer snapshot tests alongside fog_prevention_spell and blunt_the_assault_prevention_spell with an Arrow Storm/Lightning Surge-shaped instant or sorcery fixture whose text uses ability-word self-replacement. Assert its lowered IR retains the existing ConditionInstead structure, confirming the CR 614.15 exclusion bypasses the positive prevention route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 5559-5578: Preserve the precomputed min_x_value when this early
spell branch emits through parse_ability_ir_with_context and
emitter.ability_ir_at. Thread the annotation into the emitted AbilityIr using
the existing metadata mechanism, ensure X-based prevention spells retain the X
minimum constraint, and add a fixture covering “X can't be 0.” with X = 0
rejected.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs`:
- Around line 140-193: Extend the prevention recognizer snapshot tests alongside
fog_prevention_spell and blunt_the_assault_prevention_spell with an Arrow
Storm/Lightning Surge-shaped instant or sorcery fixture whose text uses
ability-word self-replacement. Assert its lowered IR retains the existing
ConditionInstead structure, confirming the CR 614.15 exclusion bypasses the
positive prevention route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c3cfdc2-8cdf-4f46-bbb0-e83efcb0518f
⛔ Files ignored due to path filters (4)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__blunt_the_assault_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__blunt_the_assault_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fog_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__fog_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (3)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rs
Comment on lines
+5559
to
+5578
| // Routed 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. #6123 converted this site to the raw pair | ||
| // `parse_effect_chain_ir` + `lower_effect_chain_ir` while hoisting the | ||
| // Class-H replacement producers, which silently dropped three things the | ||
| // entry point had been supplying: `finalize_effect_chain`, the | ||
| // owner-library reveal anchor, and the `WithContext` whole-body | ||
| // recognizer set. That made this the only spell path in the parser | ||
| // lowering a whole ability body without them. Restored here. | ||
| // | ||
| // The guard runs on `lower_ability_ir(&ir)` for the same reason the | ||
| // effect fallback below does: whether to emit at all is control flow, | ||
| // and `has_unimplemented` reads a lowered root, so the predicate must | ||
| // see the definition this site will actually emit. `lower_ability_ir` | ||
| // is a pure `&AbilityIr -> AbilityDefinition`, so lowering here and | ||
| // again in `ability_ir_at` repeats one computation rather than | ||
| // performing two different ones. | ||
| let ir = parse_ability_ir_with_context(&line, AbilityKind::Spell, &mut ctx); | ||
| if !has_unimplemented(&lower_ability_ir(&ir)) { | ||
| emitter.ability_ir_at(item_line, ir); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve minimum-X metadata in the new IR route.
min_x_value is computed before strip_x_cant_be_zero_suffix at Line 4185 and stamped by the generic spell path at Lines 5891-6022. This early branch bypasses that path without transferring the value into the emitted AbilityIr, so prevention spells containing X can't be 0. can lose their legality constraint and accept X = 0. Thread this annotation through the IR/emitter path and add an X-based prevention fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/parser/oracle.rs` around lines 5559 - 5578, Preserve the
precomputed min_x_value when this early spell branch emits through
parse_ability_ir_with_context and emitter.ability_ir_at. Thread the annotation
into the emitted AbilityIr using the existing metadata mechanism, ensure X-based
prevention spells retain the X minimum constraint, and add a fixture covering “X
can't be 0.” with X = 0 rejected.
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 28, 2026
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.
Plan 05b T9a — the U0-39 lowering fix, deliberately isolated ahead of T9b's payload swap so its
behavioural delta is reviewable on its own rather than buried in ~51 files of re-serialization.
Follows #6720, which completed T8.
The measured full-pool delta is ZERO — and that is a proven semantic no-op, not an unapplied probe.
That distinction is the entire point of the unit.
What changed
U0-39 (the instant/sorcery prevention-text recognizer) was the only spell path in the parser that
lowered a whole ability body without
finalize_effect_chain, the owner-library reveal anchor, theWithContextwhole-body recognizer set, or CR 707.9a printed-slot stamping. It now routes throughability_ir_at, phase A's designated seam:No new machinery:
ability_ir_atalready exists, and the effect fallback atoracle.rs:6353alreadyuses this exact
has_unimplemented(&lower_ability_ir(&ir))idiom — its comment even reads "same shapethe prevention-text site above already uses." U0-39 simply stops being the odd one out.
OracleNodeIr::Spell's payload is untouched. T9b is intact.#6123's story: incidental, and the recensus understates it
The commit is
refactor(parser): hoist Class-H replacement producers to the oracle document IR seam.Its body describes only the replacement hoist and never mentions a spell path. The U0-39 change is
one unremarked hunk among twenty in
oracle.rs:It dropped three mechanisms, not the two §T9 records — the
WithContextwhole-body bypasses(conditional protection, for-each-attacker-copy-blocker, face-down pile, Balance equalization) went
too, because
parse_effect_chain_with_contextrantry_parse_chain_bypassbefore lowering.What let it through review is the interesting part: the commit's byte-identity claim was TRUE.
Byte-identity cannot see a semantic narrowing whose reach is empty. This is the canonical case for
§5.4's "byte-identity with no harvest is a signal".
The measurement
Population, stated and evidenced. The real
AtomicCards.jsonwas symlinked into the worktree(§5.1.3's void-evidence trap: a worktree carries only
test_fixture.json, so generating there measuresa fixture population). Both runs: 34,739 cards / 35,516 faces, 450 set files, 92.1% implemented,
output 99,201,978 bytes. The fixture population is ~519 KB and cannot produce those numbers. All
symlinks removed; tree clean.
Result:
sha256 c3f8431c…, byte-identical before and after.cmp→ IDENTICAL.Liveness — zero was treated as broken until proven otherwise (§5.3), three independent ways:
warning: variant Spell is never constructed—oracle.rs:5561wasthe sole construction site pool-wide, so that warning is only possible if the edit took effect.
population does reach this code and it does matter.
Comeuppance, Deflecting Palm, Blunt the Assault…). Two cards have two qualifying lines.
Mechanism-by-mechanism attribution, computed per reach on cloned
ParseContexts:finalize_effect_chain(6 passes)def_same=true×155[]WithContextwhole-body bypassesbypass_fired=false×155RetainPrintedAbilityFromSourcecarriers are permanentshas_unimplementedgateThat last row was a genuine risk worth measuring:
fold_additional_combat_attacker_restrictionandfold_speed_floor_sentencesconsumeEffect::Unimplementedchildren, so finalize can remove anunimplemented node and flip the gate — changing which lines the site claims. It never fires here.
Nothing is made worse — vacuously, but provably. The 7 declined reaches fall through to Priority 8+
exactly as before: unchanged honest reds, not new ones.
So what is the value?
A latent-defect fix plus a conformance change. Any future prevention card touching finalize, the
anchor, a bypass, or stamping would have silently misparsed. It also makes U0-39 satisfy phase A's
precondition for T9b —
ability_ir_at's own doc block says phase B flips its body "and every produceris already correct."
The honest caveat, stated plainly: the value is entirely prospective. There is no
revert-flipping runtime test, because reverting changes no output. No such test was fabricated.
Snapshot guards — closing the hole that let #6123 through
None of the 153 cards reaching U0-39 was snapshotted. The one spell path with a silent semantic
narrowing was also the one with no two-layer guard. T9b lands on this exact recognizer, so two
fixtures were added, both verbatim from the pool:
Prevent all combat damage that would be dealt this turn.Canonical single clause.You gain 1 life for each creature on the battlefield. Prevent all combat damage that would be dealt this turn.The non-degenerate fixture: the prevention marker is inthe second sentence, which is verbatim what the site's own comment cites as its reason for
existing. Lowers to
GainLife{Ref(ObjectCount(Creature))}withPreventDamagechained assub_link: SequentialSibling— two instructions in the order written, CR 608.2c. Both clausespreserved.
A one-clause-only fixture would have taken the same path either way and proven nothing about chain
assembly, hence the pair. All four snapshots were read before acceptance.
Gates
Zero existing snapshots changed; zero
*_ir.snapmodified.git diff --name-statusis 4 A / 3 M,all three modified files source.
.snap.newcount 0. No pre-existing test failed — nothing in thesuite pinned the post-#6123 behavior. Ledger never edited.
Parser string-dispatch diff gate: clean (the change removes a call and adds none). CR-annotation diff
gate: zero CR numbers added or changed, so nothing to verify — though every rule cited in the
commit bodies was grep-verified with its text read: CR 615.1/615.1a, 608.2c, 707.9a, 614.15,
108.3/400.3, 602.1/602.1a, 601.2b.
One judgement call worth reviewing
Routing through
lower_ability_irwhile keeping the node asSpell(EffectChainIr)genuinely requiresT9b, because the anchor needs
source_textandEffectChainIrdoes not carry it. Rather than stop,the producer was sent through
ability_ir_at, which already builds theAbilityIrand callslower_ability_ir. Payload untouched, goal achieved.Consequence:
OracleNodeIr::Spellloses its only producer and carries a dated#[allow(dead_code)]with a PLAN-05 DEBT block. T9b deletes that allow when it flips
ability_ir_at's body andre-constructs the variant for every producer at once. The variant is kept rather than removed because
the two exhaustive
doc.rsmatches andfinish()'s slot accounting are the surface T9b lands on.Ratchet honesty: Gate P passes, but note why.
ability_ir_atdeliberately avoids the literalPreLoweredtoken (its doc block says so, to keep the ratchet monotone), so the token count isunchanged while the semantic count of pre-lowered spell producers went up by one. By design, not
evasion — but the gate did not measure what its name suggests here. This is another instance of the
already-known property that the ratchet tracks helper deletion, not producer conversion.
Harvest (§5.4)
WithContextbypass set as well asfinalize + anchor. §T9 and the census row should say three mechanisms. Reach today: zero.
pool-wide carry
RetainPrintedAbilityFromSourceand all 7 are permanents, so the CR 707.9amis-index could never fire on an
is_spellsite.Worth asking which other converted sites have reaching populations and no two-layer guard — this
is the failure mode that let refactor(parser): hoist Class-H replacement producers to the oracle document IR seam #6123 through, and the same probe measures it.
fold_additional_combat_attacker_restriction/fold_speed_floor_sentencesconsumeEffect::Unimplemented, sofinalize_effect_chaincan changehas_unimplemented's answer. Anysite whose emit gate reads it pre-finalize can claim a different set of lines than one reading
post-finalize. Zero flips here, but the pattern deserves a grep across the remaining gates.
Summary by CodeRabbit
Bug Fixes
Tests