refactor(parser): Plan 05b T10f — cross-line self-replacement as a document relation (U0-46) - #6748
Merged
Conversation
`pop_last_spell` existed solely for the cross-line "instead" fold's pop-and-rebuild. T10f replaced that fold with `DocumentRelationIr::SelfReplacementOverride` (CR 614.15), leaving the wrapper with no callers — `cargo clippy -p engine --lib -- -D warnings` fails on it. Delete it rather than `#[expect(dead_code)]` it for a later unit: it is a two-line private wrapper over `take_last_spell`, which stays live for `raise_last_spell_min_x`, and git history restores it if U0-47 wants it back. `reemit_node`'s doc argued its `OracleNodeIr` parameter from "the two re-emitting callers legitimately differ" — there is one caller now, so the doc is restated on that caller's own terms and records where the other one went. Tighten the burn-down ledger 13 -> 12 to match. The deleted expression carried one pre-lowered spell token, and the ledger's own contract is that a tranche converting a producer lowers its ceiling in the same commit — otherwise the burn-down is invisible in `git log` on that file and a later change may silently drift back up to the stale ceiling.
matthewevans
enabled auto-merge
July 28, 2026 22:36
Contributor
📝 WalkthroughWalkthroughThe Oracle parser now supports expanded “begin the game with” clauses and represents cross-line CR 614.15 self-replacement overrides as document relations that are folded during lowering. Related emission helpers, audits, regression coverage, and the prelowered ratchet were updated. ChangesOracle parser updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant DocEmitter
participant DocumentRelations
participant OracleLowering
OracleParser->>DocEmitter: Emit override paragraph
DocEmitter-->>OracleParser: Return override item ID
OracleParser->>DocumentRelations: Record base and override item IDs
OracleLowering->>DocumentRelations: Apply SelfReplacementOverride
DocumentRelations-->>OracleLowering: Fold override into base ability
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle.rs (1)
3859-3869: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
emit_ir_nodes_atuses a wildcard arm overOracleNodeIrinstead of an exhaustive match.Every other dispatcher over
OracleNodeIrin this file (emit()'s printed-slot match,spell_payload,spell_min_x_mut,lower_oracle_ir's bucketing loop) is deliberately exhaustive with no_/catch-all arm specifically so the compiler forces a decision when a new variant is added.emit_ir_nodes_at'sother => { self.emit_at(item_line, other); }breaks that invariant for this one dispatcher — a futureOracleNodeIrvariant needing special peek-mirror handling (likeStatic/Triggerdo here) would silently fall through to the genericemit_atpath with no compiler nudge.♻️ Proposed exhaustive rewrite
fn emit_ir_nodes_at(&mut self, item_line: usize, nodes: Vec<OracleNodeIr>) { for node in nodes { match node { OracleNodeIr::Static(ir) => self.static_ir_at(item_line, ir), OracleNodeIr::Trigger(ir) => self.trigger_ir_at(item_line, ir), - other => { - self.emit_at(item_line, other); - } + other @ (OracleNodeIr::Spell(_) + | OracleNodeIr::Replacement(_) + | OracleNodeIr::Keyword(_) + | OracleNodeIr::Modal(_) + | OracleNodeIr::AdditionalCost(_) + | OracleNodeIr::CastingRestriction(_) + | OracleNodeIr::CastingOption(_) + | OracleNodeIr::SolveCondition(_) + | OracleNodeIr::StriveCost(_) + | OracleNodeIr::Unsupported { .. } + | OracleNodeIr::PreLoweredTrigger(_) + | OracleNodeIr::PreLoweredStatic(_) + | OracleNodeIr::PreLoweredReplacement(_) + | OracleNodeIr::PreLoweredSpell(_)) => { + self.emit_at(item_line, other); + } } } }As per coding guidelines, "Use exhaustive matches without wildcard fallbacks for known enums so the compiler detects missing variants."
🤖 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 3859 - 3869, Update emit_ir_nodes_at to match every OracleNodeIr variant explicitly, preserving the existing static_ir_at handling for Static and trigger_ir_at handling for Trigger while routing each remaining known variant through emit_at. Remove the wildcard arm so adding a future OracleNodeIr variant produces a compiler error requiring this dispatcher to be updated.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 3859-3869: Update emit_ir_nodes_at to match every OracleNodeIr
variant explicitly, preserving the existing static_ir_at handling for Static and
trigger_ir_at handling for Trigger while routing each remaining known variant
through emit_at. Remove the wildcard arm so adding a future OracleNodeIr variant
produces a compiler error requiring this dispatcher to be updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1d2ec8a-8a13-4c5a-accb-a88b424941fd
📒 Files selected for processing (6)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/effect_chain.rscrates/engine/src/parser/oracle_ir/relation.rscrates/engine/tests/integration/cross_line_instead_override_branch.rsscripts/prelowered-ratchet.txt
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 30, 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 T10f — converts U0-46, the cross-line CR 614.15 self-replacement fold, to a document relation. Follows #6742.
A separate ability-word-prefixed paragraph that replaces the preceding ability's effect ("Raid — If you attacked this turn, instead Arrow Storm deals 5 damage…"). Today the parser pops the previously-emitted spell item, nests the override into it, and re-emits at the base's original span. That pop-and-rebuild is the last thing in
oracle.rsthat reaches backwards into already-emitted document state.DocumentRelationIr::SelfReplacementOverride+ the apply passCR 614.15 is the rules warrant, and it names this population verbatim
The rules describe the separate ability-word-prefixed paragraph as one of the two printed forms. The relation exists to bind that printed form to the ability it replaces.
The design's stated cost was a misattributed rule
The recensus framed design (b)'s cost as "momentarily puts a standalone override ability in the doc IR — which CR 614.6 forbids publishing." CR 614.6 says nothing of the kind. It reads, in full:
That is the semantics of a replaced event; it makes no statement about document representation. So no comment in this PR claims it does — writing one would plant a false CR annotation, the exact defect the CR gate exists to prevent.
The two existing CR 614.6 annotations at
oracle.rs:6258and:6282are untouched, deliberately. They invoke 614.6 about runtime semantics — publishing an unbindable override as an independent ability makes the engine perform the base effect and the replacement, so the replaced event did happen. Different claim, correctly cited. A finding that a citation was misapplied in one place is not a licence to sweep every instance of the number.CR 614.15 (
:3122), 614.6 (:3076), 614.1a (:3056), 608.2c (:2793) and 707.9a (:5646) were each grep-verified and their text read.The boundary between two "instead" mechanisms, stated at both types
ReplaceMeaningKind::Instead— within one chain: a clause replaces a prior clause's def inside a singleparse_effect_chain. Unchanged here.DocumentRelationIr::SelfReplacementOverride— across document items: a paragraph replaces a paragraph, preserving the base's span and printed slot.Without that written down, the next agent duplicates one into the other.
Full-pool byte identity
Fresh generator build each side, distinct target dir,
MTGJSON_SKIP_REFRESH=1, realAtomicCards.json(158,014,511 bytes — resolved and size-checked, since the fixture population would be void evidence rather than a green).The risk byte identity could NOT have caught, and how it was actually settled
This is the part worth reviewing.
Today the fold pops the base and re-emits ONE item, so the override paragraph never becomes a document item. Under this design both are emitted — and
stamp_printed_ability_slotruns at push time, before the relation passes (the PLACEMENT PIN deliberately fixes that order). So every ability emitted after the override would take a printed slot one higher, and a later removal does not restamp.A clean hash would not have detected this.
stamp_printed_ability_slotonly rewrites the placeholder insideContinuousModification::RetainPrintedAbilityFromSource; an ability carrying no such modification is unaffected. The defect bites only a card with both a cross-line "instead" fold and a laterRetainPrintedAbilityFromSource. A green hash proves that intersection is empty, not that the slot handling is correct.Settled two ways instead:
ability_idsentry at the same index, then restamps every surviving ability by its post-fold index. The offset cannot survive, whatever the corpus contains.AtomicCards.jsoncensus (reminder text stripped, printed lines split, non-first lines matching the parser's short ability-word grammar and containing "instead") finds 95 unique cards, of which 64 are in the spell-only dispatcher subset. Of all 95, zero carry a later line containingexcept it has this ability— the sole parser surface forRetainPrintedAbilityFromSource.The charter said 90 cards. The measured figure on current input is 95/64; the method is stated above rather than the old number repeated.
The swallow audit — a real red, and why the fix is a correction rather than a silencer
Commit 2's first full run went red: 2 failures, Arrow Storm and Lightning Surge. That was a genuine regression, not noise, and it is worth being precise about what caused it.
The swallow audit ("the parser must never silently discard Oracle text") is per item and resolves each item's id through the parallel
_idstracks. The relation pass consumes the override's id — it is removed fromability_idsentirely, not moved to another track asapply_linked_choice_copy_chosen_hostdoes. So the audit found an item with no reachable lowered evidence and reported its whole fragment as swallowed. The text was in fact represented, nested under the base'ssub_ability; it was simply no longer addressable.The fix omits relation-consumed items from the audit while retaining them in the document IR. That restores the pre-existing audit shape exactly: before this PR the override paragraph was never an item, so its fragment was never audited either. The audit's coverage is unchanged, not narrowed.
And that claim is measured, not argued.
parse_warningsis serialized intocard-data.json— 914 non-empty instances across the pool — so the byte-identical full-pool hash is a whole-corpus comparison of swallow-audit output, including every card that carries a warning.Worth naming for a reviewer: the dangerous failure mode on this row is not a crash but a relation under-fire — the fold silently stops happening and the override republishes as an independent ability, which is the CR 614.6 semantic defect
oracle.rs:6258exists to prevent (Anoint with Affliction exiled a creature with zero poison counters). The existingcross_line_instead_override_branchintegration test is the guard: with the inline fold deleted, its assertion of one top-level ability plus aConditionInsteadbranch can only be satisfied through the relation path.finalize_document_relationsassigned where it needed to extenddoc.relations = detect_document_relations(…)would have clobbered any relation recorded during dispatch. It now extends. Dispatch is the right place to detect this one:is_cross_line_dig_altis a parse-time fact — whethertry_parse_dig_instead_alternativesucceeded against the previous line's def — and is not recoverable from item text after assembly, so a post-assembly detector would silently under-fire. The Class route contributes no dispatch producers and reaches the extend with an empty vector.The closed enum — the class was selector-shaped, not wildcard-shaped
DocumentRelationIris documented closed. Grep found zero_ =>/_ ifarms over it or overLinkedChoiceKind. The class a wildcard grep alone would miss is the sixlet DocumentRelationIr::… else { continue }appliers; the new pass is the seventh. Enumerated rather than sampled — on #6733 a reviewer's list of two turned out to be a class of five.Commit 3 — dead code, and a doc that had gone stale with it
Deleting the fold left
pop_last_spellwith no callers, soclippy -D warningsfailed on it. Deleted rather than#[expect(dead_code)]-ed for a later unit: it is a two-line private wrapper overtake_last_spell, which stays live forraise_last_spell_min_x, and git history restores it if U0-47 wants it back. Keeping dead code alive for a hypothetical future requirement is the thing CLAUDE.md prohibits.reemit_node's doc argued itsOracleNodeIrparameter from "the two re-emitting callers legitimately differ in what they hand back" — there is one caller now. Restated on that caller's own terms, with a note on where the other went.Gate P's ledger is tightened 13 → 12, a genuine burn-down: the deleted re-emission expression carried one pre-lowered spell token. The ledger's own contract is that a tranche converting a producer lowers its ceiling in the same commit, otherwise the burn-down is invisible in
git logand a later change drifts back up to a stale ceiling.Gates
No fixture exercised the new IR shape, so the ruled
*_ir.snapdev-artifact concession was never spent. Had one, the churn would have been justified on dev-artifact grounds alone — not as a trade against a CR 614.6 representation cost, which was found not to exist.Scope held
U0-47 (T10g) is untouched:
reemit_nodeand itsraise_last_spell_min_xcaller stay, as does theunreachable!asserting the three-lowerable-spell-shape invariant.ReplaceMeaningKind::Insteadstill embeds aBox<AbilityDefinition>— latent pre-lowered debt of the family §6 wants gone, recorded rather than touched.Summary by CodeRabbit
New Features
Bug Fixes
Tests