refactor(parser): retain nom dispatch results as IR - #6776
Merged
Conversation
matthewevans
enabled auto-merge
July 29, 2026 04:36
Contributor
📝 WalkthroughWalkthroughUnsupported spell and ability residuals now use structured ChangesStructured unsupported residual flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant dispatch_line_nom
participant DocEmitter
participant lower_unsupported_node
OracleParser->>dispatch_line_nom: dispatch line
dispatch_line_nom-->>OracleParser: NomDispatchIr::Spell or Unsupported
OracleParser->>DocEmitter: emit spell or unsupported IR
DocEmitter-->>OracleParser: OracleNodeIr
OracleParser->>lower_unsupported_node: lower UnsupportedAbilityIr
lower_unsupported_node-->>OracleParser: AbilityDefinition with Effect::Unimplemented
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.
Actionable comments posted: 2
🤖 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_ir/doc.rs`:
- Around line 42-68: Replace UnsupportedAbilityIr.name’s String with a closed
enum covering Unknown and the known trigger/static/replacement/effect structure
categories, and update its constructors to accept or produce those variants.
Propagate the typed category through all UnsupportedAbilityIr producers and
convert it to the legacy string only at Effect::unimplemented, preserving
existing output names.
In `@crates/engine/src/parser/oracle.rs`:
- Around line 6604-6610: Update the fallback dispatch match around
dispatch_line_nom so the previously computed min_x_value is threaded into both
NomDispatchIr arms: apply max with the spell IR shell’s existing floor before
emitting Spell, and pass the same floor into the Unsupported residual node
emitter instead of allowing it to seed zero. Preserve the CR 601.2b announced-X
floor for both fallback paths.
🪄 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: 7104e1f6-6987-49d9-ae21-2e52192e83b7
⛔ Files ignored due to path filters (21)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__arni_brokenbrow_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__baneslayer_angel_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__barbarian_class_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__birds_of_paradise_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bomat_courier_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__brazen_borrower_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__changeling_outcast_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__experiment_one_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__goblin_guide_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__leonin_arbiter_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__lumen_class_frigate_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__monastery_swiftspear_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__murderous_rider_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__questing_beast_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__reckless_bushwhacker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__serra_angel_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__slippery_bogle_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__smugglers_copter_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__snapcaster_mage_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__thalia_guardian_of_thraben_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__wolfir_silverheart_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (6)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_class.rscrates/engine/src/parser/oracle_dispatch.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_tests.rs
Comment on lines
+42
to
+68
| pub(crate) struct UnsupportedAbilityIr { | ||
| pub(crate) name: String, | ||
| pub(crate) fragment: String, | ||
| pub(crate) description: String, | ||
| } | ||
|
|
||
| impl UnsupportedAbilityIr { | ||
| pub(crate) fn unknown(text: impl Into<String>) -> Self { | ||
| let text = text.into(); | ||
| Self { | ||
| name: "unknown".to_string(), | ||
| fragment: text.clone(), | ||
| description: text, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn new( | ||
| name: impl Into<String>, | ||
| fragment: impl Into<String>, | ||
| description: impl Into<String>, | ||
| ) -> Self { | ||
| Self { | ||
| name: name.into(), | ||
| fragment: fragment.into(), | ||
| description: description.into(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a closed type for the residual category.
name: String makes coverage keys typo-prone and lets future producers create unrecognized categories without compiler enforcement. Model the known categories (Unknown, trigger/static/replacement/effect structure) as an enum, converting to the legacy string only at Effect::unimplemented.
As per coding guidelines, “prefer enums over stringly typed data”; as per path instructions, CLAUDE.md requires unsupported residuals to remain typed.
🤖 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/doc.rs` around lines 42 - 68, Replace
UnsupportedAbilityIr.name’s String with a closed enum covering Unknown and the
known trigger/static/replacement/effect structure categories, and update its
constructors to accept or produce those variants. Propagate the typed category
through all UnsupportedAbilityIr producers and convert it to the legacy string
only at Effect::unimplemented, preserving existing output names.
Sources: Coding guidelines, Path instructions
Comment on lines
+6604
to
+6610
| // Priority 14a: the dispatcher parses once and retains successful spell IR. | ||
| // Priority 15: its exact unsupported payload reaches final lowering unchanged. | ||
| match dispatch_line_nom(&line, card_name, ctx.host_self_reference.clone()) { | ||
| NomDispatchIr::Spell(ir) => emitter.ability_ir_at(item_line, ir), | ||
| NomDispatchIr::Unsupported(unsupported) => { | ||
| emitter.unsupported_ir_at(item_line, unsupported) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the announced-X floor through fallback dispatch.
line arrives after its X can't be 0 suffix was stripped, but neither arm carries the already-computed min_x_value: Spell(ir) retains the default shell floor and Unsupported calls an emitter that seeds zero. A fallback-dispatched spell can therefore be cast with X = 0. Thread the floor into both arms, using max for the IR shell and the residual node.
As per path instructions, engine rules must preserve the CR 601.2b announced-X floor.
🤖 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 6604 - 6610, Update the
fallback dispatch match around dispatch_line_nom so the previously computed
min_x_value is threaded into both NomDispatchIr arms: apply max with the spell
IR shell’s existing floor before emitting Spell, and pass the same floor into
the Unsupported residual node emitter instead of allowing it to seed zero.
Preserve the CR 601.2b announced-X floor for both fallback paths.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
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.
Summary
AbilityIrthrough the Oracle document lowering seamVerification
cargo clippy -p engine --lib -- -D warningscargo nextest run -p engine --lib(17,922 passed; 6 skipped)cargo nextest run -p engine --test integration(4,164 passed; 2 skipped)a788a27b43682f98c595ea7ae0a0de7ff997923cf90104d33a782fa464968ad8Summary by CodeRabbit
Bug Fixes
Tests