refactor(parser): Plan 05b T9b — the payload swap, all nine spell producers at once - #6724
Conversation
…ducers at once
`OracleNodeIr::Spell` carries an `AbilityIr` instead of a bare `EffectChainIr`,
and `ability_ir_at` emits it directly rather than lowering eagerly into the
pre-lowered variant. Because T8 had already routed every phase-A producer
through that one method, flipping its body is what converts all nine — the whole
payoff of the A0–A4 staging.
A chain alone cannot carry the CR 602.1 activation envelope a recognizer stamps
around it (cost, activation restrictions, ability tag, the CR 601.2b announced-X
floor), which is why a chain-payloaded node forced eager lowering in the first
place.
CR 707.9a stamping moves from `OracleDocBuilder::finish` to `lower_oracle_ir`'s
bucketing loop. It has to: the stamp rewrites the placeholder printed slot inside
a definition, and an IR-native spell has no definition until `lower_ability_ir`
builds one, so a finish()-time walk could only stamp the pre-lowered shapes and
would silently skip every converted producer. The two walks are order-equivalent
— verified, not assumed:
* both iterate the same `BTreeMap` keyed by `(first_line, start_byte, ordinal)`,
so both visit the identical source-ordered sequence and count each category
separately; the k-th spell item is at ability slot k either way
* the slot is now `result.<category>.len()` at the moment of the push, so
"consumes a slot" and "is pushed" are the same operation — the old walk's
separate counter could drift from the vector it was predicting, and this
cannot
* stamped inside the loop, before the relation passes that insert into, remove
from, and move ids between the category tracks — the same pre-relation state
finish() saw
* nothing between the two seams reads a printed slot: relation discovery and
the swallow audit inspect effect trees for shape, never
`RetainPrinted{Trigger,Ability}FromSource` indices (grep-verified)
The exhaustiveness obligation moves with it. `lower_oracle_ir`'s match is already
exhaustive over `OracleNodeIr` with no `_` arm, so a new node variant still cannot
be added without deciding its slot behavior.
`mutate_last_spell` is retired for a typed `raise_last_spell_min_x`, and
`reemit_spell` generalized to `reemit_node`. The general closure mutator's
`impl FnOnce(&mut AbilityDefinition)` could only be honored by lowering the popped
node, so it could not preserve an IR-native spell — it silently re-emitted
pre-lowered. `OracleNodeIr::spell_min_x_mut` handles both shapes at the layer each
stores the floor (`AbilityShellIr::min_x_value` vs the lowered root), which agree
because `apply_ability_shell_envelope` applies the shell's value with `max`.
It has exactly ONE caller (the empty-line guard); T7 already deleted the other.
Measured over the real pool (data/mtgjson/AtomicCards.json symlinked into the
worktree, generated from the repo root): card-data.json is BYTE-IDENTICAL before
and after, sha256
c3f8431c494c1a9710e08fc15161eab75203a2dc98a5aa35e3f7a993ec43c245,
99,201,978 bytes, 35,516 face-keyed entries. Generator rebuilt fresh for both
runs.
Zero delta is a semantic no-op, not an unapplied probe:
* the compiler witnesses the conversion — `lower_effect_chain_ir` became an
unused import in `oracle.rs`; no spell path there lowers a bare chain any more
* 10 `*_ir.snap` files churn, every one of them exactly `PreLoweredSpell` ->
`Spell` (total re-serialization: flat def -> `{source_text, body:{clauses},
shell}`). Zero `*_lowered.snap` files change, re-verified after accepting the
IR snapshots so a failing `_ir` assertion could not mask a later `_lowered`
one — that pairing is the non-vacuity probe, and it is what proves the
re-serialization lowers to the same definitions
* both stamping directions watched go red: dropping the relocated stamp makes
Thespian's Stage read `RetainPrintedAbilityFromSource(0)` instead of `(1)`;
restoring the closure mutator makes the new min_x test's node assertion fail
while its floor assertion still passes
The CR 707.9a placeholder churn the plan predicted did NOT materialize, and that
is a finding rather than a discrepancy: no `*_ir.snap` fixture carries a
copy-except clause, because `parse_activated_ability_definition` is the only
writer of `ParseContext::current_ability_index` and therefore the only source of
`RetainPrintedAbilityFromSource` — and it is deliberately still pre-lowered
(U0-28/U0-31, T10b). So no IR-native spell body can carry a printed slot today;
the stamp's IR-native ability arm is structurally unreachable, which is why the
move is byte-neutral. It becomes load-bearing the moment that router converts.
New test `a_standalone_x_floor_annotation_raises_an_ir_native_spells_floor` pins
the newly reachable path: before this commit the only IR-native producer could not
precede a "X can't be 0." line, and now nine can.
Harvest, corrected in place because each is prose that now contradicts the code:
* `PrintedAbilityIndex::placeholder`'s doc block read as forbidding this very
relocation ("deriving the slot from a category-vector length … the
source-ordered document builder makes that equality false"). It does not: that
sentence is about the retired PARSE-TIME `from_category_vector_len`, which read
the length in emission order while a preprocessor could emit out of order. The
lowering walk reads the same expression in SOURCE order, where it genuinely
coincides with the printed slot. Same expression, different quantity;
disambiguated so the next reader does not conclude this commit is unsound.
* `lower_oracle_ir`'s "PreLowered variants are identity-lowered (pushed directly
to the result)" was true until this commit and is not any more — both spell and
trigger arms now stamp.
* The `OracleNodeIr` block comment claiming all four IR-native siblings are
"dead-coded pending Plan 05 U2's document-seam hoist" — all four have live
producers now.
* The deleted priority-13e gravestone still named `mutate_last_spell` as having
"two callers"; T7 deleted one and this commit retires the symbol.
* `ir_spell_node_readers.rs` claimed the IR-native `item_ability` arm has no
witness and that `Spell` producers would "rise from 1 to ~14" — the real number
is 9, and the arm now has producers (though still no relation witness).
Harvest, reported not fixed:
* The `*_ir.snap` corpus contains ZERO fixtures carrying a copy-except clause, so
it supplies no evidence about CR 707.9a stamping in either direction. The
coverage lives entirely in `thespians_stage_emits_retain_printed_ability_from_source`
and `an_ir_native_spell_consumes_the_printed_ability_slot_it_occupies`. A
two-layer fixture belongs with T10b, when the router that produces the carrier
converts.
* `finish()` no longer needs `mut self` — it took it only for the stamping walk.
Ratchet: `oracle.rs` 16 -> 15. The token count and the semantic producer count move
independently (T9a's observation): `ability_ir_at` never contained the literal
token, so converting nine producers moves the ledger by one incidental prose
mention, not by nine. `doc.rs` stays at its ceiling because
`OracleNodeIr::spell_min_x_mut`'s match is exhaustive over the enum and four of its
arms name a pre-lowered variant. That match feeds an `.expect()`, so a `_` arm
would convert a future compile error into a runtime panic — worth four tokens.
📝 WalkthroughWalkthroughOracle spell nodes now carry ChangesOracle IR spell lowering
Sequence Diagram(s)sequenceDiagram
participant DocEmitter
participant OracleNodeIr
participant lower_oracle_ir
participant lower_ability_ir
participant stamp_printed_ability_slot
DocEmitter->>OracleNodeIr: emit Spell(AbilityIr)
OracleNodeIr->>lower_oracle_ir: provide source-ordered item
lower_oracle_ir->>lower_ability_ir: lower AbilityIr
lower_ability_ir-->>lower_oracle_ir: AbilityDefinition
lower_oracle_ir->>stamp_printed_ability_slot: stamp printed slot
stamp_printed_ability_slot-->>lower_oracle_ir: store stamped 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 |
There was a problem hiding this comment.
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_ir/snapshot_tests.rs`:
- Around line 235-244: Update the test around the OracleNodeIr::Spell assertion
to destructure its payload and verify the embedded AbilityShellIr min_x_value is
1, preserving the existing IR-native shape check as needed. Keep the current
lowered.abilities[0].min_x_value assertion to validate both the IR stamp and its
lowered result.
🪄 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: ae1013e4-03c9-4bf9-9990-96f30528d191
⛔ Files ignored due to path filters (10)
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__blunt_the_assault_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.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__ghost_lit_stalker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__govern_the_guildless_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__guul_draz_assassin_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__joraga_treespeaker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__liliana_the_repentant_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (5)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/tests/integration/ir_spell_node_readers.rsscripts/prelowered-ratchet.txt
| assert!( | ||
| matches!(ir.items[0].node, OracleNodeIr::Spell(_)), | ||
| "the re-emitted node must stay IR-native, got {:?}", | ||
| ir.items[0].node | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| lowered.abilities[0].min_x_value, 1, | ||
| "the \"X can't be 0.\" annotation must raise the lowered root's floor to 1" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the IR-side min-X stamp, not only the lowered result.
The test claims to pin AbilityShellIr::min_x_value, but it only checks the Spell variant and the final lowered value. A regression that leaves the shell floor at 0 while applying 1 through another lowering path would still pass. Destructure the OracleNodeIr::Spell payload and assert its shell min_x_value == 1 before retaining the existing lowered assertion.
As per path instructions, regression tests must prove the specific failure path rather than rely on a partial shape/result check.
🤖 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 235 - 244,
Update the test around the OracleNodeIr::Spell assertion to destructure its
payload and verify the embedded AbilityShellIr min_x_value is 1, preserving the
existing IR-native shape check as needed. Keep the current
lowered.abilities[0].min_x_value assertion to validate both the IR stamp and its
lowered result.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
…ed (phase-rs#6726) * refactor(parser): Plan 05b U0-61 — hoist the Class effect-sentence recognizer to IR `oracle_class.rs`'s effect/spell-like line arm ("You may play an additional land...") emitted a pre-lowered `AbilityDefinition`; it now emits `OracleNodeIr::Spell(AbilityIr)` and lets `lower_oracle_ir` do the lowering. Byte-identical BY CONSTRUCTION, not by corpus. `parse_effect_chain(t, k)` **is** `lower_ability_ir(&parse_ability_ir_standalone(t, k))` — that is the function's literal body at `oracle_effect/mod.rs`, not a claim about it: pub fn parse_effect_chain(text: &str, kind: AbilityKind) -> AbilityDefinition { lower_ability_ir(&parse_ability_ir_standalone(text, kind)) } So splitting the call into its two halves moves WHERE the lowering happens, not WHAT it produces. This is the same algebraic identity every T8 R1 row used. The `has_unimplemented` gate still reads a LOWERED def — `has_unimplemented(&lower_ability_ir(&ir))` — the shape A3 used at U0-12 and T9a used at U0-39. Whether to emit at all is control flow, and `has_unimplemented` is defined over an `AbilityDefinition`, so the predicate must see the definition the site actually emits. An IR-level `has_unimplemented` over `EffectChainIr` would be a rival authority free to diverge from the one the rest of the parser uses. Only safe on top of T9b: before the `Spell` payload carried `AbilityIr`, this hoist would have dropped `finalize_effect_chain` and the owner-library reveal anchor. Based on T9b's tip (phase-rs#6724, not yet merged). Scope: line 334 only. `oracle_class.rs:398` (the class-level trigger's `execute`) is a different site and is untouched; the `make_unimplemented` residual beneath this block is U0-62, which is blocked on a user ruling. Burn-down: `oracle_class.rs` drops 5 -> 4 in scripts/prelowered-ratchet.txt. * refactor(parser): Plan 05b U0-40 — the heterogeneous IR node mechanism `lower_as_enters_becomes_choice_modal` (CR 208.2b modal "As ~ enters, it becomes your choice of <P/T profile>, ..." — Aquamorph Entity, Primal Clay, Primal Plasma) stops pushing pre-lowered definitions into a scratch `ParsedAbilities` and returns typed IR instead: N `StaticIr` + 1 `ReplacementIr`. This introduces the heterogeneous `Vec<OracleNodeIr>` producer the recensus says to introduce ONCE, deliberately (no such producer existed anywhere in the parser). It is built as a general mechanism — `DocEmitter::emit_ir_nodes_at` emits any ordered node sequence at one line — not as a special case for this site. Which drain caller this is: `drain_result_vectors` has two callers. oracle.rs 4304 is the Priority-1 modal block feeder (`parse_oracle_block` -> `lower_oracle_block`), which is U0-18 and belongs to T10h; it is untouched. This row is the OTHER one, the Priority-8 replacement-adjacent site, identified by content: it is guarded by `is_as_enters_becomes_choice_pattern` and calls `lower_as_enters_becomes_choice_modal`, and its in-code CR annotations (208.2b / 614.1c / 614.12a) are exactly the census row's citations for U0-40. Byte-safe by construction. Converting these definitions to IR newly subjects them to `lower_static_ir`'s two transforms and to `lower_replacement_ir`; all three are provably inert here: * `lower_replacement_ir` is `ir.definition.clone()` — identity, by definition. * `populate_active_zones_from_condition` collects only `SourceInZone` (through And/Or/Not); `StaticCondition::ChosenLabelIs` hits the `_ => {}` arm, so `zones` stays empty and the `!zones.is_empty()` guard never fires. * `bind_counter_anaphor_to_recipient` rewrites only `QuantityRef::CountersOn { scope: Anaphoric }`. Anaphoric counter scopes are produced by exactly one combinator, `parse_deferred_counter_pronoun` (via `parse_for_each_counters_on_source`), and survive only through the two `_deferred` entry points — every other path runs `settle_deferred_counter_anaphor_ref`, which collapses Anaphoric -> Source at parse time. Both non-test callers of the deferred entry are in `oracle_static/anthem.rs`. These modifications come from `animation_modifications_with_replacement` <- `parse_animation_spec`, whose dynamic-P/T routes are `parse_quantity_ref_complete` and `parse_dynamic_pt_clause` -> `nom_quantity::parse_quantity_ref`; neither reaches a deferred entry point. The condition axis is inert for the same reason as above: `bind_anaphoric_counters_in_condition` matches only `UnlessPay`/And/Or/Not. Emission order is preserved deliberately. `drain_result_vectors` emitted by CATEGORY (abilities, triggers, statics, replacements) regardless of push order, and `emit_at` stamps `ordinal_within_span` in emission order, so the caller emits the CR 614.1e face-up residual first, then the statics, then the replacement — which is NOT the order the recognizer constructs them in. `emit_ir_nodes_at` dispatches through the typed `*_ir_at` helpers rather than straight to `emit_at`, so the `last_static` peek mirror that `static_at` maintains today keeps being maintained; `parsed_result_recently_granted_flashback` reads it mid-loop. The CR 614.1e residual stays a bare `AbilityDefinition` the caller emits via `ability_at`: it is a fixed strict-failure marker with no parsed body, and constructing a pre-lowered node inside `oracle_replacement.rs` would move a producer into a file the burn-down ledger does not track. Giving it a real IR node is U0-62's question, which is out of scope. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Plan 05b T9b — Unit 3b phase B: the payload swap.
OracleNodeIr::Spell's payload becomesAbilityIr, converting all nine spell producers at once. Follows #6722 (T9a).One compile-coupled commit — two exhaustive
OracleNodeIrmatches indoc.rsforce every producer tomove together.
This is the payoff of T8's five-tranche staging. Every phase-A producer already routed through
ability_ir_at, so the conversion itself is one line: that method stops lowering eagerly and emits theIR-native node. The alternative was a single cutover where a hundred snapshots and a dozen recognizers
moved at once and no diff was readable.
Headline: card-data is BYTE-IDENTICAL
sha256 c3f8431c494c1a9710e08fc15161eab75203a2dc98a5aa35e3f7a993ec43c245— identical before andafter,
cmpclean.read off a log line
AtomicCards.json+SetList/Meta/CardTypes/sets/deckssymlinked from the maincheckout, generated from the worktree repo root (§5.1.3 — a worktree carries only
test_fixture.json, so generating against that measures a fixture population and is void, not green)T9a already absorbed the campaign's one intentional behavioural delta, so byte-identity here is the
expected result and the proof that the swap is semantically neutral.
Snapshots
10 changed, all
*_ir, zero*_lowered. 10 M / 0 A / 0 D;.snap.newcount 0.All ten are mechanism 1 (total re-serialization), machine-checked: for every file the only node-key
change is
PreLoweredSpell→Spell, andgrep -c 'source_(ability|trigger)_index'on each diff is0. Nothing fell outside the expected mechanisms.
Read in full:
fog_ir(single clause — flat def becomes{source_text, body:{clauses:[1]}, shell}with the
PreventDamagepayload preserved verbatim) andblunt_the_assault_ir(multi-clause —the old flat root +
sub_ability+sub_link: SequentialSiblingbecomes two chain clauses withboundary: Sentence, thesub_linknow derived at lowering).*_loweredbeing unchanged is proven, not assumed._iris asserted before_loweredin the sametest, so a failing
_irmasks the_loweredassertion entirely; the ten were accepted and the suitere-run — 311/311 pass, zero new
.snap.new.Mechanism 2 did not materialize, and that is a finding rather than a discrepancy: no
*_ir.snapfixture carries a copy-except clause, so the corpus supplies zero evidence about the relocated stamp in
either direction. The two seen-red probes below are what cover it.
The CR 707.9a stamp move: the conclusion holds, the plan's stated reason did not
The stamp moves from
finish()tolower_oracle_ir's bucketing loop, whereslot = result.<category>.len()at the push.doc.rs's own doc block argued "the load-bearing invariant is per-category counting, not sourceorder … the walk never depended on that." That disclaimer is wrong. Per-category counting alone is
not sufficient — the two walks must also visit the same sequence. They do, because both iterate the
same
BTreeMapkeyed by(first_line, start_byte, ordinal). So the conclusion survives while thestated reason was incomplete; the comment has been rewritten.
Three conditions the plan's argument never mentioned, each verified:
RetainPrinted/source_*_indexinoracle.rsoutside one comment;swallow_checknever touches.node), somoving downstream of relation discovery is safe.
move ids between the category tracks. It does.
PrintedAbilityIndex::placeholder's doc block reads as forbidding exactly this move. It doesnot — that sentence is about the retired parse-time
from_category_vector_len, which read the lengthin emission order. Same expression, different quantity. Disambiguated in place, because otherwise
the next reader concludes this commit is unsound.
Structural bonus: the slot is now the vector length itself, so "consumes a slot" and "is pushed" are a
single operation. The old walk had two counters predicting a vector they did not own; that drift is
now unrepresentable.
Non-vacuity — both directions watched go red
RetainPrintedAbilityFromSource(0)instead of(1). Red, then reverted.still passes. Red, then reverted.
Both probes removed (
grep -c PROBE= 0). New testa_standalone_x_floor_annotation_raises_an_ir_native_spells_floorpins the path that only becamereachable in this commit.
Compiler witness:
lower_effect_chain_irbecame an unused import inoracle.rs— no spell paththere lowers a bare chain any more. That is the invariant the swap exists to establish, asserted
mechanically rather than by grep.
Gates (all on the final commit)
Parser string-dispatch diff gate: no output (PASS). CR diff gate: 601.2b, 602.1, 603.1, 608.2, 707.9a
— zero unverified, each rule's text read rather than its existence checked.
Ratchet caveat, stated plainly. The burn-down moved by one incidental prose mention, not by
nine.
ability_ir_atnever contained the literal token, so converting nine semantic producers isinvisible to a textual gate.
doc.rsstays at ceiling becausespell_min_x_mut's match is exhaustiveand four arms name a pre-lowered variant — it feeds an
.expect(), so a_arm would convert a futurecompile error into a runtime panic. The match was kept and the cost paid in prose instead. The ledger
was never edited upward.
Deferred: U0-40
Confirmed not done by T3 (
oracle.rs:5595still callsdrain_result_vectors), and notcompile-coupled — the swap compiles and both suites pass without it.
The analysis was done and it is byte-safe by construction:
collect_source_in_zonesignoresStaticCondition::ChosenLabelIssoactive_zonesstays empty, andbind_counter_anaphor_to_recipientonly rewrites
ObjectScope::Anaphoricwhile these modifications are fixed P/T (the recognizer returnsfalse unless power and toughness are both
Some). Bothlower_static_irtransforms provably no-op andlower_replacement_iris identity. Emission order must stay abilities → statics → replacements topreserve
ordinal_within_span.It was nonetheless left out, deliberately: it introduces the new heterogeneous
Vec<OracleNodeIr>mechanism the recensus says to introduce once, and it needs its own isolated full-pool measurement.
Bundling a new mechanism into the one commit whose entire value is an isolated byte-identity
measurement would defeat that measurement. The analysis above is the brief for the follow-up.
For reviewers
parse_activated_ability_definitionis the only writer ofParseContext::current_ability_indexandtherefore the only source of
RetainPrintedAbilityFromSource, and it is still pre-lowered(U0-28/31, T10b). So the move is byte-neutral because of that, not by luck, and it becomes
load-bearing the moment that router converts. Do not read the green byte gate as evidence the
IR-native stamping path works — it is covered by construction plus the two seen-red probes.
reemit_node's instead-fold caller must stay pre-lowered. The fold builds a genuinely newAbilityDefinitionandlower_ability_irhas no inverse. Do not "fix" it to emitSpell.⇒
take_last_spell()isNone), unchanged here.Summary by CodeRabbit
New Features
Bug Fixes
Tests