refactor(parser): Plan 05b — the shared residual node and `optional` on the ability shell by matthewevans · Pull Request #6733 · phase-rs/phase

matthewevans · 2026-07-28T17:09:53Z

Plan 05b — the two designs the maintainer ruled on: a shared residual node and optional on the ability shell. Follows #6726.

These were the last two rows blocked on a decision rather than on work. With them landed, §6's closing criterion — deleting the pre-lowered spell variants — becomes reachable for the first time.

Five commits, one concern each. Commits 1 and 3 convert nothing: infrastructure lands byte-identical by construction, then producers move under their own reviewable diff. That staging is what made T8-A0 reviewable and is deliberate.

#	commit	producers
1	add `OracleNodeIr::Unsupported`	zero
2	convert U0-62 (`oracle_class.rs`) + U0-02 (`oracle.rs`)	2
3	add `optional` to `AbilityShellIr`	zero
4	convert U0-43, the CR 103.5b mulligan recognizer	1
5	name the third spell shape in the prose that says two	—
6	centralize spell payload classification (review feedback)	—

The residual node, and why the discard is load-bearing

oracle_class.rs reaches its residual by two routes, and on one of them the chain parser runs, has_unimplemented is true, and the parsed definition is thrown away before make_unimplemented(line) is built fresh. Preserving that partial chain would be a behavior change — it would name the failed clause rather than the fixed "unknown" sentinel, moving the coverage key. Discarding it is what keeps the conversion byte-identical.

The node therefore takes text and nothing from the discarded IR, and ir is scoped inside the if block, so the discard is structural rather than a convention a later edit could quietly drop.

`Unsupported` carries an X floor — soundness, not corpus

Shape landed: Unsupported { text: String, min_x_value: u32 }.

raise_last_spell_min_x does node.spell_min_x_mut().expect(..), and the residual is emitted onto spells_emitted — so it is a spell for slot accounting and can be handed to that assertion. A text-only variant would have to answer None there, converting a compile-time-guaranteed Some into a runtime panic on a structurally reachable path. The pre-lowered shape already carried the floor as an AbilityDefinition root field, so this preserves a floor rather than inventing one.

The reachability probe says raise_last_spell_min_x is never reached with a residual node in either test binary. Reported as a null: the field is soundness-preserving, not corpus-required, and the argument stands independently of today's corpus.

The two `_ => None` readers, and the layer that was actually missing

lower_spell_node and item_ability both ended in a wildcard, so the new variant compiled clean and
silently became a non-spell. The dangerous one is lower_spell_node: it is the and_then target
behind last_ability_definition(), read mid-loop by parsed_result_recently_granted_flashback and by
the cross-line "instead" fold. A None there is neither a compile error nor a panic — it is a
behavior change on a different card from the converted one, which per-card byte-identity cannot
catch.

Commit 6 removes both, but not by enumerating => None arms. The file had already diagnosed
itself. item_trigger's doc:

The exhaustiveness obligation lives on TriggerNodeIr::definition(), not on this match, and that
is the correct layer.

and item_ability's, explaining why the spell side could not do the same:

There is no equivalent layer to push this obligation onto: a spell node's IR payload is an
AbilityIr, not an enum of definition-owning representations, so all three shapes are named here.

So the wildcard was a symptom; the missing layer was the defect. OracleNodeIr::spell_payload() -> Option<SpellPayloadIr<'_>> now supplies it — exhaustive over all 16 variants with no wildcard,
sitting beside its sibling spell_min_x_mut, which already carries the same "which variants are
spell-shaped" obligation for the X floor. Both readers then match a closed three-variant
representation, so a fourth spell shape breaks both at compile time.

Enumerating thirteen => None arms in each reader would have stated the classification a third and
fourth time. This states it once and deletes two copies.

It moves Gate P the right way

The naive fix collides with the ratchet: oracle.rs sat at exactly its ceiling of 15, and the arms
name three pre-lowered variants twice each — 21, gate fails. The ratchet is a textual grep and
cannot tell a producer (debt) from a => None exhaustiveness arm (the opposite of debt).

Pushing the obligation down instead means oracle.rs stops naming PreLoweredSpell in both readers:

crates/engine/src/parser/oracle.rs        15 -> 13     (a burn-down improvement)
crates/engine/src/parser/oracle_ir/doc.rs 16 -> 20

doc.rs is the ledger's own "infrastructure: NOT burn-down targets" section — where the variant
declarations and their consumers are explicitly expected to live until the variants are deleted. No
gate was modified and no burn-down ceiling was raised.

Three sibling readers are deliberately left alone

item_replacement, item_trigger, and item_static have the identical _ => None shape.
CodeRabbit flagged two of the five; the class has five. They are untouched by this PR and their
hazard is unchanged by it, so generalising the layer to the trigger/static/replacement sides is its
own reviewable diff rather than a rider here.

Worth recording for whoever takes it: item_trigger's doc argues its wildcard is safe because "a
new trigger representation is a new TriggerNodeIr variant, so it breaks that match at compile time
before it can reach here." That is precisely the assumption this plan falsified on the spell side —
Unsupported landed on OracleNodeIr, not inside AbilityIr. The premise should be re-tested, not
inherited.

`optional` on the shell — a monotone OR, and why that is load-bearing

def.optional |= shell.optional, placed beside cant_be_copied. Never an assignment: lower_effect_chain_ir legitimately sets optional from a printed "you may", so an assignment would let any producer building a default() shell clear a flag the chain had already established. The OR keeps AbilityShellIr::default() a no-op, which is what makes the widening byte-identical by construction — no existing producer was touched.

The field doc states plainly that this is the one field that is not the CR 602.1 activation envelope. CR 608.2d places the choice "while applying the effect" — CR 608.2 resolution, the half EffectChainIr owns. It lives on the shell specifically so it can be stamped unconditionally after lowering, matching what the game-start recognizers do by hand, and thereby bypassing assemble_effect_chain's conditional clause→root mapping — four suppressions plus an arm that forces optional = false for SearchOutsideGame. That was read in the source, not taken from the brief.

U0-43 is by construction, and the identity is the function body

parse_effect_chain(t, k) is lower_ability_ir(&parse_ability_ir_standalone(t, k)) — the entire body in oracle_effect/mod.rs, not a claim about it. Splitting the call moves where lowering happens, not what it produces. Both stamps ride the shell: description as an Option override, optional as a monotone OR against true, value-identical to the def.optional = true it replaces on every input.

clauses[0].parsed.optional is deliberately NOT set. Routing it through clause 0 would subject it to the conditional mapping above, and would additionally assume clause 0 becomes the emitted root — which ClauseDisposition does not guarantee.

Measurement

The claim is that this PR changes no generated card data. It is measured against the PR's own
base, not against the commits' original authoring base:

FULL-POOL BYTE-IDENTICAL
2737d0783ece3443cb0041323114ee06eaa06f8e30431f817784e4dd5312f289   55c5d8dc11  (PR base)
2737d0783ece3443cb0041323114ee06eaa06f8e30431f817784e4dd5312f289   68f8ea665f  (tip)

Generator rebuilt fresh on each side, run from the worktree repo root against the real
AtomicCards.json (158,014,511 bytes — readlink -f resolved and size-checked, since a run against
the 507 KiB test_fixture.json would be void evidence rather than a green), MTGJSON_SKIP_REFRESH=1,
identical MTGJSON sidecars symlinked on both sides.

Why this was re-measured rather than inherited. Two earlier measurements existed and neither
covered this PR against its actual base: commits 1–4 were measured identical on fd59ea48f8, their
authoring base, and commit 6 on 77caacb159. Commits 1–4 were cherry-picked onto 55c5d8dc11 and
never re-measured there. The gap was covered only by the argument that the three intervening commits
never reference OracleNodeIr, the pre-lowered variants, or AbilityShellIr — which is weaker than
it reads, because those commits are parser fixes and commit 2 changed which lines fall through to
the residual path. A new recognizer that stops a line reaching make_unimplemented would interact
without naming any of those types. So it was measured instead of argued.

Commit 6's own base/tip pair was additionally recorded before its implementation began, so that
identity claim is not circular.

Probe verified applied, not assumed: the filtered output for commit 2 (38 Class faces ∪ 33 "same
is true" cards → 76 exported faces) contains exactly the three residuals the census predicted —
Barbarian Class, Gourmand's Talent, and Cairn Wanderer's the same is true for landwalk, protection.
Commit 4's population (No-Regrets Egret, Serum Powder) still exports kind: Mulligan,
optional: true, full printed line as description.

Stated rather than buried: D13's original run lacked SetList.json and emitted "legality inference
will be degraded". The measurements above supply it, so they are production-shaped as well as
mutually comparable.

Reachability (§5.3) — every null reported as a null

probe	`--lib`	`--test integration`
Class residual (U0-62)	REACHED	REACHED
"same is true" residual (U0-02)	REACHED	NULL
mulligan (U0-43)	REACHED	NULL
`raise_last_spell_min_x`, any node	REACHED	NULL
`raise_last_spell_min_x`, residual node	NULL	NULL

Attributed by probe message, never by test name. Probes run before any perturbation and fully reverted.

Gates

cargo fmt --all                                 clean
clippy -p engine --lib -- -D warnings           zero warnings
cargo nextest run -p engine --lib               17909 passed, 6 skipped
cargo nextest run -p engine --test integration   4159 passed, 2 skipped
Gate P (PreLowered ratchet)                     PASS   oracle_class.rs 4 -> 3, oracle.rs 15 -> 13
check-skill-doc.sh                              PASS
check-parser-combinators.sh                     Gate G PASS / Gate A PASS
*.snap.new pending                              0

CR-annotation diff gate: zero UNVERIFIED. CR 103.5b, 601.2b, 602.1, 608.2, 608.2d, 707.9a — text read, not existence checked.

Snapshots: exactly one *_ir.snap moved (barbarian_class_ir, the fixture carrying a residual) — same text, same floor, now the typed node. Zero *_lowered.snap changes, asserted rather than assumed: the _ir/_lowered masking trap was hit deliberately, the _ir accepted, and the test re-run to confirm _lowered passes unchanged.

Two findings worth a reviewer's attention

The textual ratchet is blind to producers routing through a shared emitter helper. Two producers were converted; the ratchet moved once. U0-02's producer was emitter.ability_at(line, make_unimplemented(..)) — the counted token lives once inside ability_at's body, not at the producer, so converting it reduces the textual count by zero. oracle.rs legitimately stays at 15 and the ledger was not touched upward. The discriminating measure is the ability_at call-site count: 18 → 17 at commit 2, 17 → 16 at commit 4.

make_unimplemented was hand-constructing an Effect::Unimplemented literal, the pattern CLAUDE.md gates and check-parser-combinators.sh enforces elsewhere. Replaced with the sanctioned Effect::unimplemented constructor (value-identical) and made private, so the residual's single-authority property is real rather than conventional.

Also harvested, not fixed

oracle_dispatch.rs builds the same Effect::unimplemented("unknown", line) pair via the sanctioned constructor — a third producer of this coverage key, and the natural next member of the Unsupported family. Not a census row; out of scope here.
SKILL.md claims 26 pre-lowered references in oracle.rs; the actual count is 15, and was 15 before this PR. Drift, not gated by check-skill-doc.sh (which does not check prose counts).
The shell's doc block said it mirrored 10 of AbilityDefinition's root fields; counted from source it was already off by one before this PR, having omitted the pre-existing sub_link. Corrected with the count derived from source.

Base note

D13's four commits were authored on fd59ea48f8; commit 6 answers review feedback on top. Rather
than reason about whether the cherry-pick preserved byte-identity, the whole PR was re-measured
against its actual base 55c5d8dc11 — see Measurement above. The queue re-validates.

…structure only) Decision 1, half one. Adds the shared honest-failure residual node and wires it through every consumer. Converts ZERO producers, so the commit is byte-identical BY CONSTRUCTION and shows zero snapshot churn — the T8-A0 staging, repeated. What lands: * `OracleNodeIr::Unsupported { text, min_x_value }`. Two sites in the parser build the residual today (`oracle_class.rs`'s terminal Class fallback and `oracle.rs`'s CR 611.3a "the same is true for …" unqualified-keyword tail) and both produce the same value: `Effect::Unimplemented` with the fixed sentinel `name: "unknown"` and `description` = the residual text, with that text also on the definition's `description`. That pair is load-bearing for coverage, which is why the node carries raw text instead of seeding a chain parse — a chain-parsed residual names whatever clause the chain failed inside, and would break byte-identity on the coverage key. * `oracle::lower_unsupported_node`, which delegates to `make_unimplemented` rather than rebuilding the definition. One construction authority, so the node and the two hand-built sites provably cannot drift. * `min_x_value` on the variant, and this is a soundness requirement rather than a convenience. The residual is emitted through the ability channel, so it lands on `spells_emitted` and occupies a CR 707.9a printed ability slot — which makes it reachable by `raise_last_spell_min_x`, whose `spell_min_x_mut().expect("both spell shapes carry an X floor")` is sound only while every node on that stack can name where its floor lives. A text-only variant would have to answer `None` there and would convert a compile-time-guaranteed `Some` into a runtime panic on a structurally reachable path. The shape it replaces already carried the field as an `AbilityDefinition` root field (CR 601.2b, MagicCompRules.txt:2459), so this preserves a floor rather than widening one. Consumers wired, no `_` arm added anywhere: * `doc.rs::OracleNodeIr::spell_min_x_mut` (exhaustive) — returns the new field. * `doc.rs::OracleDocBuilder::emit` slot accounting (exhaustive) — joins the `spells_emitted` arm. Omitting it would stamp every ability printed after an unrecognized line one CR 707.9a slot low. * `oracle.rs::lower_oracle_ir` bucketing loop (exhaustive) — lower, stamp, push. * `oracle_ir/feature.rs` unit-attribution fold (exhaustive) — explicit no-op arm; the residual is already attributed through the ability id track. * `oracle.rs::lower_spell_node` and `oracle.rs::item_ability` — BOTH carry a `_ => None` arm, so the compiler could not have found them. Found by grepping wildcard arms instead. `lower_spell_node` is the quiet one: it backs `last_ability_definition()`, read mid-loop by `parsed_result_recently_granted_flashback` and by the cross-line "instead" fold's `previous_spell`, so a silent `None` there is a behavior change on a DIFFERENT card from the converted one — which per-card byte-identity cannot catch. `item_trigger` / `item_static` / `item_replacement` keep `_ => None`: the residual is genuinely not a trigger, static, or replacement. `emit_ir_nodes_at`'s `other =>` arm routes it to `emit_at`, which is correct. CR numbers grep-verified against docs/MagicCompRules.txt, text read: CR 601.2b (:2459, announced value of a variable cost), CR 707.9a (:5646, copy gains an ability — the printed-slot rule this file already cites). Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS with every per-file count UNCHANGED (15/4/16/9/2 — no ledger edit, and the doc prose above deliberately avoids the counted token); check-skill-doc PASS.

…ual node Decision 1, half two. Both hand-built honest-failure residual producers now emit `OracleNodeIr::Unsupported` instead of an eagerly-lowered definition. The residual text is unchanged at both sites, so the coverage key (`name: "unknown"` / `description` = that text) is unchanged; only WHEN the definition is built moves, from the producer to `lower_oracle_ir`. * **U0-62** — `oracle_class.rs`, the terminal Class fallback. **The discard on its second route is deliberate and is preserved exactly.** Two routes reach the fallback: a line no recognizer claimed, and an effect-sentence candidate whose chain DID parse but whose lowered root contains an `Unimplemented`. On the second route the parsed `ir` is thrown away and the residual is rebuilt from `line`. Keeping that partial chain would be a BEHAVIOR change, not a conversion — it would name the failed clause instead of the fixed `"unknown"` sentinel and move the coverage key. `ir` is scoped inside the `if` block, so the discard is structural rather than a convention a later edit could drop. * **U0-02** — `oracle.rs`, the CR 611.3a "the same is true for …" unqualified-keyword tail, via a new `DocEmitter::unsupported_at` that mirrors `ability_at` (no peek mirror to maintain — the ability peek is pop-aware, read from the builder's `spells_emitted` stack). `make_unimplemented` is now private and has exactly one caller, `lower_unsupported_node`. The residual therefore has a single construction authority reachable only through the node; a new residual producer must go through the node rather than minting a definition of its own. Its body also stops hand-constructing an `Effect::Unimplemented` literal and calls `Effect::unimplemented`, the constructor CLAUDE.md mandates — a value identity (the helper expands to exactly that literal), not a behavior change. **Ratchet: which occurrence moved.** The count alone cannot distinguish a converted producer from a prose mention, so both files are named explicitly. * `oracle_class.rs` 4 -> 3, ledger tightened. The occurrence that moved is the terminal fallback push, formerly `OracleNodeIr::PreLoweredSpell(...)` — the U0-62 producer itself. * `oracle.rs` stays at **15, and this is not an oversight.** U0-02's producer called the shared `DocEmitter::ability_at` helper, and that helper — not the producer — is where the counted token textually lives. Converting the producer therefore reduces the count by zero while genuinely retiring a producer: `ability_at` call sites in `oracle.rs` go **18 -> 17**, verified against the parent commit. The textual ratchet is blind to any producer that routes through a shared emitter helper; `ability_at` call-site count is the discriminating measure for this file, and the ledger stays at 15 because the helper legitimately survives for its 17 remaining callers. **Snapshot: exactly one `*_ir.snap` moves, and it is the fixture that carries a residual.** `barbarian_class_ir` had serialized the eagerly-lowered residual definition; it now serializes `Unsupported { text, min_x_value: 0 }`. Same text, same floor, IR-native representation. **No `*_lowered.snap` changes** — and that is asserted, not assumed: `parse_two_layer` asserts `_ir` BEFORE `_lowered` in the same test, so a failing `_ir` masks the `_lowered`. The `_ir` snapshot was accepted and the test RE-RUN, and `_lowered` passes unchanged. **Byte gate, non-vacuously verified.** Filtered regeneration over the exact population that can reach either converted site (38 Class faces + 33 cards whose Oracle text contains "same is true" -> 76 exported faces) is byte-identical to `origin/main` (sha256 8d3b3f87…). The probe is verifiably applied: that output contains exactly the three residuals the census predicted — Barbarian Class and Gourmand's Talent (U0-62, "2 abilities") and Cairn Wanderer's "the same is true for landwalk, protection" (U0-02) — all present and unchanged. Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS (oracle_class.rs lowered, no ledger entry raised); Gate G + Gate A PASS; check-skill-doc PASS.

…frastructure only) Decision 3, half one. An ELEVENTH field on A0's shell. A0 chose ten deliberately, so this is an explicit amendment rather than a fill-in, and the field doc says why the amendment is warranted. Converts ZERO producers → byte-identical by construction, zero snapshot churn. **This is the one shell field that is NOT the CR 602.1 activation envelope, and the doc block says so rather than pretending otherwise.** Every other field partitions along the seam CR 602.1 (MagicCompRules.txt:2514) draws — cost before the colon, activation instructions after it. `optional` does not: CR 608.2d (:2795) places the choice *"while applying the effect"*, which is CR 608.2 resolution, the half this type deliberately leaves to `EffectChainIr`. Its presence here is a named exception, paid knowingly. The exception is correct because the mechanical requirement is that `optional` be stamped **unconditionally, after lowering** — exactly what the game-start recognizers (`AbilityKind::Mulligan`, `BeginGame`) do by hand today. The alternative, expressing it on clause 0 and letting assembly carry it up, is NOT equivalent: `assemble_effect_chain` maps `clause_ir.parsed.optional` to the root through four suppressions — `Effect::GrantCastingPermission`, `is_lingering_cast_from_zone`, `is_join_forces_pay_any_amount_mana_cost`, `is_pay_to_end_effect_termination` — and a following arm sets `def.optional = false` outright for `Effect::SearchOutsideGame`. It additionally assumes clause 0 becomes the emitted root, which `ClauseDisposition` does not guarantee. All five guards read from the source, not from memory. The shell is the only place the stamp can be unconditional AND survive the IR conversion. **Applied as a monotone OR, never an assignment:** `def.optional |= shell.optional`, mirroring `cant_be_copied`. Here the choice is load-bearing rather than merely tidy — `lower_effect_chain_ir` legitimately sets `optional` from the printed "you may", so an assignment would let every one of the many producers that build a `default()` shell CLEAR a flag the chain had already established. The OR is what preserves A0's defer-on-default property, which is precisely what makes this widening byte-identical by construction: `AbilityShellIr::default()` remains a no-op, so no existing producer changes behavior and no existing producer had to be touched. `serde` skips the field when `false`, so an unset shell serializes exactly as it did before the widening and `*_ir.snap` fixtures cannot churn. Also corrects the struct's own field-count claim, which this field would otherwise have made staler. Counted from the source rather than from the previous sentence: the struct has thirteen fields, twelve of which mirror an `AbilityDefinition` root field (`stages` is a transform list, not a root field). A0's "10 of 38" counted the fields that tranche ADDED and omitted the pre-existing `sub_link`, so it already read one low before `optional` arrived. CR numbers grep-verified against docs/MagicCompRules.txt, text read: CR 608.2d (:2795, choices announced while applying the effect), CR 602.1 (:2514, the envelope this field is the exception to), CR 707.10 (the neighbouring field's citation, unchanged). Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS with every per-file count unchanged; check-skill-doc PASS.

…n recognizer Decision 3, half two. `try_parse_mulligan_time_ability` (Serum Powder, No-Regrets Egret) returns an `AbilityIr` and emits through `ability_ir_at` instead of building a definition and emitting through `ability_at`. **By construction, stated as the identity rather than as a corpus argument.** `parse_effect_chain(t, k)` **is** `lower_ability_ir(&parse_ability_ir_standalone( t, k))` — that is the entire body of `parse_effect_chain` in `oracle_effect/mod.rs`, not a claim about it. Splitting it into its two halves moves WHERE the lowering happens without changing WHAT it produces. Both root stamps ride the shell and are applied after lowering, exactly as the two lines they replace applied them: * `description` — an `Option` override, so `Some(line)` reproduces `.description(line)`. * `optional` — a monotone OR against the shell's `true`, which for a `true` stamp is value-identical to the `def.optional = true` it replaces, on every input, whatever lowering produced. `apply_ability_shell_envelope` runs before the stage loop, and this site lists no stages, so the stamps are the last thing that happens either way. No property of any card's text participates in the argument, so a future printing reaching this recognizer is covered too. `parse_ability_ir_standalone` is the mode-pinned wrapper for a site whose original called `parse_effect_chain`; the argument list is unchanged, so the `ChainLoweringMode` is inherited mechanically rather than by reviewer judgment. No `has_unimplemented` gate exists at this site, so none was added — the recognizer emits whatever the chain produced, as before (No-Regrets Egret's inner effect is an honest `Unimplemented` today and stays one). **`clauses[0].parsed.optional` is NOT set, deliberately.** CR 103.5b (MagicCompRules.txt:300) says the player *"may perform that action"* — a property of the whole printed ability. Routing it through clause 0 would subject it to `assemble_effect_chain`'s conditional clause→root mapping (four suppressions plus a `SearchOutsideGame` arm that forces `optional = false`) and would additionally assume clause 0 becomes the emitted root, which `ClauseDisposition` does not guarantee. `ability_at` call sites in `oracle.rs`: 17 -> 16. The textual `PreLowered` count is unchanged at 15 for the same reason commit 2 explained — this producer routed through the shared helper, not through a token of its own. Byte gate, non-vacuously verified over the recognizer's exact population (`--filter "No-Regrets Egret|Serum Powder"`): both cards still export `kind: Mulligan`, `optional: true`, and the full printed line as `description`. Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS; check-skill-doc PASS.

…ys two Follows the four D13 commits, which add `OracleNodeIr::Unsupported` as a third spell-shaped node. Five comments encode "there are exactly two spell shapes" and became false the moment that variant landed. Two of them are the argument text of live assertions, which is what the next reader takes as the invariant. Prose only — no behavior, no signature, no control flow. * `oracle.rs` `raise_last_spell_min_x` — `.expect("… both spell shapes carry an X floor")`. `spell_min_x_mut` now has three `Some` arms. * `oracle.rs` cross-line "instead" fold — `unreachable!("… both spell shapes lower")`. `lower_spell_node` now lowers three. * `oracle.rs` `item_ability` doc — "both shapes are named here". Three are. Also records why its surviving `_ => None` is a hazard for the *next* spell-shaped variant rather than for this one: it is an `and_then` target, so a swallowed variant is a behavior change with no compile error. Deleting it is deferred to the pre-lowered variants' removal, which rewrites these arms anyway. * `doc.rs` `spell_min_x_mut` doc — "Both spell shapes store the floor … at different layers". There are three layers now: the residual holds text rather than a definition, so its floor lives on the node itself until `lower_unsupported_node` builds a definition to put it on. The interchange argument still holds because every path applies with `max`. * `doc.rs` `stamp_printed_ability_slot` doc — "both spell node shapes converge on one here". Three converge; the residual arrives via `lower_unsupported_node`. Found by a case-insensitive sweep, not by the exact-phrase grep that produced the original list — `both spell NODE shapes` and the capitalized doc line both escape a naive `both spell shapes` match. Two of the five would have been missed. Gates: cargo fmt; Gate P PASS; check-skill-doc PASS; Gate G/A PASS. Both gates are textual and count comment prose, so this commit's own wording avoids the literal ratchet token — the first draft raised oracle.rs 15 -> 16 on a single word in a comment.

coderabbitai · 2026-07-28T17:10:28Z

📝 Walkthrough

Walkthrough

Oracle parsing now preserves unsupported residuals as spell-shaped IR nodes, routes them through shared lowering and printed-slot accounting, and avoids duplicate scoped evidence. Ability shells also carry and monotonically propagate optionality through effect composition.

Changes

Oracle residual handling

Layer / File(s)	Summary
Residual representation and emission `crates/engine/src/parser/oracle_class.rs`, `crates/engine/src/parser/oracle.rs`, `crates/engine/src/parser/oracle_ir/doc.rs`	Parser fallbacks and keyword-tail failures emit `Unsupported` residuals with retained text and X-floor metadata, while document emission treats them as spell-shaped nodes.
Residual lowering and accounting `crates/engine/src/parser/oracle.rs`, `crates/engine/src/parser/oracle_ir/doc.rs`, `crates/engine/src/parser/oracle_ir/feature.rs`, `scripts/prelowered-ratchet.txt`	Unsupported nodes lower through a shared unimplemented-definition helper, preserve minimum X values, populate parsed abilities and printed slots, and avoid duplicate scoped evidence.
Optional ability shell propagation `crates/engine/src/parser/oracle.rs`, `crates/engine/src/parser/oracle_ir/effect_chain.rs`, `crates/engine/src/parser/oracle_effect/mod.rs`	Mulligan-time abilities use `AbilityIr`, and the new optional shell field is propagated through effect composition using monotone OR semantics.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OracleParser
  participant DocEmitter
  participant OracleDocBuilder
  participant AbilityLowerer
  OracleParser->>DocEmitter: emit Unsupported residual
  DocEmitter->>OracleDocBuilder: record spell-shaped node
  OracleDocBuilder->>AbilityLowerer: lower residual with min-X floor
  AbilityLowerer->>OracleDocBuilder: add ability and printed slot

Possibly related PRs

phase-rs/phase#6699: Updates the Plan 05b pre-lowered producer ratchet that this change also adjusts.
phase-rs/phase#6708: Updates related spell-node lowering and printed-ability-slot handling.
phase-rs/phase#6714: Provides related AbilityIr parsing and emission infrastructure.

Suggested labels: quality

Suggested reviewers: andriypolanski

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the two main changes: the shared residual node and the new optional flag on the ability shell.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch ship/plan-05b-d13

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 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 667-677: Remove the wildcard fallback from both OracleNodeIr
matches: lower_spell_node at crates/engine/src/parser/oracle.rs:667-677 and
item_ability at crates/engine/src/parser/oracle.rs:1233-1242. Explicitly list
every current non-spell OracleNodeIr variant with a None result, while
preserving the existing spell-shaped handling, so future enum variants cause
compiler errors instead of being silently ignored.

🪄 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: 95c9b2a2-8fa6-48bf-b995-51ab01b66d1b

📥 Commits

Reviewing files that changed from the base of the PR and between 55c5d8d and 77caacb.

⛔ Files ignored due to path filters (1)

crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__barbarian_class_ir.snap is excluded by !**/*.snap, !**/snapshots/**

📒 Files selected for processing (7)

crates/engine/src/parser/oracle.rs
crates/engine/src/parser/oracle_class.rs
crates/engine/src/parser/oracle_effect/mod.rs
crates/engine/src/parser/oracle_ir/doc.rs
crates/engine/src/parser/oracle_ir/effect_chain.rs
crates/engine/src/parser/oracle_ir/feature.rs
scripts/prelowered-ratchet.txt

github-actions · 2026-07-28T17:26:33Z

Parse changes introduced by this PR

✓ No card-parse changes detected.

matthewevans added 5 commits July 28, 2026 10:06

matthewevans enabled auto-merge July 28, 2026 17:09

coderabbitai Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread crates/engine/src/parser/oracle.rs Outdated

matthewevans disabled auto-merge July 28, 2026 17:25

refactor(parser): centralize spell payload classification

68f8ea6

matthewevans added this pull request to the merge queue Jul 28, 2026

Merged via the queue into main with commit 6cedb28 Jul 28, 2026
16 checks passed

matthewevans deleted the ship/plan-05b-d13 branch July 28, 2026 18:43

matthewevans mentioned this pull request Jul 28, 2026

refactor(parser): Plan 05b T10c — the die-result branch table as IR (U0-45) #6742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(parser): Plan 05b — the shared residual node and `optional` on the ability shell - #6733

refactor(parser): Plan 05b — the shared residual node and `optional` on the ability shell#6733
matthewevans merged 6 commits into
mainfrom
ship/plan-05b-d13

matthewevans commented Jul 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

matthewevans commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The residual node, and why the discard is load-bearing

Unsupported carries an X floor — soundness, not corpus

The two _ => None readers, and the layer that was actually missing

It moves Gate P the right way

Three sibling readers are deliberately left alone

optional on the shell — a monotone OR, and why that is load-bearing

U0-43 is by construction, and the identity is the function body

Measurement

Reachability (§5.3) — every null reported as a null

Gates

Two findings worth a reviewer's attention

Also harvested, not fixed

Base note

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026

Parse changes introduced by this PR

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

matthewevans commented Jul 28, 2026 •

edited

Loading

`Unsupported` carries an X floor — soundness, not corpus

The two `_ => None` readers, and the layer that was actually missing

`optional` on the shell — a monotone OR, and why that is load-bearing

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading