refactor(parser): Plan 05b Class-B bring-up, tranches T0-T3 - #6699
Merged
Conversation
The PLAN-05 DEBT note said this variant was constructed only by the Class-B bring-up and that the allow should be retired there. That landed in #6325: static_ir_at (oracle.rs) constructs OracleNodeIr::Static from 26 call sites, so the allow is stale and was masking nothing. OracleNodeIr::Trigger keeps its allow deliberately -- it still has no live producer until the trigger seam converts (Plan 05b T4/T5). Plan 05b T0.
"Is Plan 05b done?" was answerable only by archaeology. That is how the #[allow(dead_code)] on OracleNodeIr::Static survived nine days past the commit that discharged its debt. Adds a per-file ceiling ledger over textual PreLowered occurrences in crates/engine/src/parser (generated snapshots excluded). Reducing a count passes; raising one fails; a parser file carrying PreLowered with no ledger entry fails, so a new producer cannot be added in a third file and escape the burn-down. Slack prints the tightening edit rather than failing, per plan 6.1, so each tranche lowers its own number and the burn-down shows up in git log on one file. The metric is deliberately a grep, not a type-level check: the point is to catch a producer being written, not a variant being reachable. A type check would go green only when the last variant is deleted -- the end state, not the tracking mechanism. Both failure branches were verified red before wiring: over-ceiling, and present-but-unlisted. Plan 05b 6.1.
No Class card was in the two-layer IR corpus, which made Plan 05b T1's
byte-identity gate vacuous: oracle_class.rs's level-section arms could be
converted from PreLowered* to IR nodes with zero snapshot churn and zero
proof the converted path was reached.
Five pool-verified cards, chosen so each arm T1 converts has a witness and
so both the wrapped and unwrapped halves of the ClassLevelGE gate are
covered:
granted quoted static (193) wrapped: Sorcerer Class L2
plain static (204) unwrapped: Wizard Class L1
wrapped: Barbarian L3, Innkeeper L2, Bard L2
replacement (221) unwrapped: Bard L1
wrapped: Innkeeper's Talent L3
Arm attribution was read off the generated baseline (node variant + source
fragment per item), not predicted -- predicting it got three of the seven
wrong. is_granted_static_line needs a prefix AND a quoted verb (has " /
have " / gains " / gain "), so an unquoted grant like "Creatures you
control have haste" falls through to is_static_pattern.
Two gaps recorded in the test file rather than papered over:
* Row 260 (ability-word-prefixed static) has NO witness in the pool --
all three such Class level bodies are "Landfall -- Whenever ..." and
take the trigger arm. Its conversion rests on the class argument alone.
* Barbarian Class L1 does not reach the replacement arm; it lands as
PreLoweredSpell/Unimplemented. Pre-existing parser gap, baselined so
T1 churn against it would be visible.
This commit must precede the conversion or it proves nothing.
Plan 05b T1 (baseline).
Convert the four `class-section-prelowered` producers in
`parse_class_oracle_text` from `OracleNodeIr::PreLowered{Static,Replacement}`
to the real IR nodes `Static(StaticIr)` / `Replacement(ReplacementIr)`, so the
Class level sections lower through the same seam as every other static.
Each site passes the text the recognizer above it actually consumed as
`source_text` — `&static_line` at the two static arms, `line` at the
replacement arm, `&effect_static` at the ability-word arm — rather than
`lines[line]`, which would fabricate provenance for the post-strip body.
Because `parse_static_line` already lowers internally, routing its result
through `StaticIr` makes document lowering run `lower_static_ir` a second
time, with `wrap_static_with_class_level` interposed between the passes. That
is a no-op for the whole class, not just today's faces:
`populate_active_zones_from_condition` self-guards on `active_zones.is_empty()`
and its collector reads only `SourceInZone` (so `ClassLevelGE` contributes
nothing), and `bind_counter_anaphor_to_recipient` rewrites only
`ObjectScope::Anaphoric`, of which none survive the first pass. Record that
invariant as a standing obligation on `lower_static_ir`'s doc comment.
`lower_replacement_ir` is pure identity, so the replacement arm is safe by
construction. CR 607.2d relation discovery and CR 707.9a slot indexing are
untouched: `item_static` and `finish()` already group the converted variants
with their `PreLowered` counterparts in the same match arms.
Snapshot churn is confined to the five `_ir` fixtures and is a pure
re-nesting — verified structurally, not by eye: unwrapping `Static.definition`
reproduces the previous `PreLoweredStatic` payload byte-for-byte across all
seven converted sites, with `active_zones` still empty and no rebound counter
scope. The `_lowered` fixtures are unchanged, which is the byte-identity claim.
Tighten the burn-down ledger 10 -> 6 to match.
… own provenance
Convert the two `preprocessor` static producers — `parse_level_blocks`
(CR 711 LEVEL blocks) and `parse_spacecraft_threshold_lines`
(CR 702.184a / CR 721.2 `N+ | body` lines) — to yield `StaticIr` rather than
bare `StaticDefinition`, so each preprocessor is the single authority for the
source text of what it produced.
That authority is the point of the change. The real recognizer input is the
post-`|` threshold body or the self-ref-normalized LEVEL sub-line, never the
printed line the span anchors to; only the preprocessor knows which. Passing
`lines[line]` at the call site would compile and would fabricate provenance.
The leveler's block summary is synthesized from the block's modification
lines with no printed line behind it at all, so it carries those joined lines
— the same string as its description — rather than claiming the `LEVEL N-M`
header its span happens to anchor to.
The leveler keeps `emit_span` instead of routing through `static_ir_at`: its
`first..=last` range is load-bearing, and `static_ir_at` also writes the
`last_static` peek mirror, which the leveler deliberately does not update.
Spacecraft moves `static_at` -> `static_ir_at`, which does update that mirror
— as `static_at` already did.
Byte-identity rests on T1's argument: both definitions arrive already lowered
from `parse_static_line`, so document lowering runs `lower_static_ir` a second
time, and both of its transforms are idempotent. CR 711.2a level ranges and
CR 702.184a station thresholds are encoded as `StaticCondition::HasCounters`,
which `collect_source_in_zones` does not read, so the second pass cannot
rewrite `active_zones`.
Two fixtures, because the existing corpus could not witness either row:
* `lumen_class_frigate` — the only two-layer card reaching a Spacecraft
threshold line at all, and it reaches both static arms (`parse_static_line`
at :256 and keyword-only at :178). Chalice of the Void matched a grep for
`charge` but prints no threshold line, so without this fixture the
Spacecraft conversion would have been snapshot-invisible.
* `kabira_vindicator` — `student_of_warfare` reaches only the block-SUMMARY
arm (:194); Kabira prints a sentence in each LEVEL block, witnessing the
body arm (:154) twice while still carrying two summaries.
The Spacecraft fixture also pins a pre-existing wart rather than fixing it: the
keyword-only arm's description keeps the `12+ | ` prefix while its source text
does not. Left alone — the description feeds card data, and this is incidental
evidence for the change's own premise, that the description was never a
trustworthy provenance carrier.
Recorded gap: the multi arm (`oracle_level.rs:146`, `parse_static_line_multi`)
has no pool witness — no printed LEVEL body line lowers to more than one
static. Its conversion rests on the class argument alone.
Note the ledger moves only 21 -> 20: the Spacecraft site emitted through the
`static_at` helper, so it never had a textual `PreLowered` hit of its own. The
ratchet is a ceiling, not a progress meter — helper-mediated producers stay
invisible until the helper itself dies.
…s IR
Convert the two replacement producers that Plan 05b's T3 can reach today:
* `lower_as_enters_or_face_up_counters` (CR 614.1c + CR 708.11, the dual
"As ~ enters or is turned face up, put X counters on it" sentence) now
returns `Option<Vec<ReplacementIr>>` instead of pushing into a shared
`ParsedAbilities` scratch and returning `bool`. Both arms derive from the
same printed sentence — the sentence never splits, which is the whole
reason this recognizer exists — so both carry it as their source text.
* `parse_saga_chapters`' ETB lore-counter replacement (CR 714.3a) now yields
a `ReplacementIr`.
Dropping the mutator's `drain_result_vectors` call is safe, and provably so
rather than by inspection: the loop hands `&mut result` to exactly three
mutators, each immediately drain-followed, and the only other `&mut result`
uses in the loop touch the `additional_cost` singleton, which the drain does
not clear. The scratch's vectors are therefore empty on entry, and replacements
were the only vector this recognizer ever wrote.
CR 714.3a forces a provenance decision, so state it as a convention: a
SYNTHESIZED rule item's source text is its own description. The Saga ETB
replacement prints no line — the rule applies from the subtype alone. The
anchor tempts a lie here, since `etb_line` points at the first chapter's line
for ordering, but the chapter-I trigger owns that text. A synthetic label
cannot be located in the Oracle text, which is the honest answer for an item
that was never printed.
Both conversions are byte-identical by construction: `lower_replacement_ir` is
`ir.definition.clone()`, and `emit_at` derives the span and fragment from
`self.lines[line]`, not from `ir.source_text`. Emission line and ordering are
unchanged.
Scoped out of this commit, and out of T3 as chartered: U0-05 / U0-06, the
`drain_result_vectors` static and replacement loops. Converting them requires
`ParsedAbilities.statics` / `.replacements` to carry IR, which is 93 call sites
across the parser on a `pub` type, and — the reason it is design work rather
than mechanical — the modal-block feeder passes only the block HEADER line, so
every drained per-mode static would have to source its own text from inside
`lower_oracle_block`. Until that lands, `static_at` and `replacement_at` each
retain exactly one caller (those two loops), so T3's headline payoff of
deleting both emitter methods is still pending.
matthewevans
enabled auto-merge
July 27, 2026 18:15
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOracle parser paths now construct ChangesOracle IR migration and enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 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_static/mod.rs (1)
221-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdempotency-requirement doc understates which producers actually double-lower.
The new comment on
lower_static_irexplains that a recognizer callingparse_static_line(which already lowers) and then wrapping the result inStaticIr::from_definitioncauses a secondlower_static_irpass, and names "the Class level-section arms inoracle_class.rs" as "the current example." But this same PR introduces the identical pattern in two other files that the comment doesn't mention, so a future contributor auditing "which producers double-lower" before adding a non-idempotent transform, using this comment as the checklist, would miss two real sites.
crates/engine/src/parser/oracle_static/mod.rs#L221-L234: broaden the comment to nameoracle_level.rs(body-arm statics) andoracle_spacecraft.rs(static-body arm) as additional double-lowering sites, not just the Class arms.crates/engine/src/parser/oracle_level.rs#L153-L161: body statics fromparse_static_line/parse_static_line_multi(already lowered) are re-wrapped viaStaticIr::from_definitionand will be lowered a second time at the document level.crates/engine/src/parser/oracle_spacecraft.rs#L258-L268: same pattern for the static-body arm (multi and single).This isn't a live bug today — both existing transforms are self-guarded/idempotent per the same comment — but the whole point of this doc addition is to give future maintainers an accurate map before they add a transform that isn't.
🤖 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_static/mod.rs` around lines 221 - 234, Broaden the idempotency documentation in lower_static_ir to identify oracle_level.rs body-arm statics and oracle_spacecraft.rs static-body arms alongside the existing oracle_class.rs Class level-section arms as double-lowering producers. At oracle_level.rs lines 153-161 and oracle_spacecraft.rs lines 258-268, no direct code change is required; these sites provide the parse_static_line/parse_static_line_multi to StaticIr::from_definition pattern that the updated documentation must accurately cover.
🤖 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_static/mod.rs`:
- Around line 221-234: Broaden the idempotency documentation in lower_static_ir
to identify oracle_level.rs body-arm statics and oracle_spacecraft.rs
static-body arms alongside the existing oracle_class.rs Class level-section arms
as double-lowering producers. At oracle_level.rs lines 153-161 and
oracle_spacecraft.rs lines 258-268, no direct code change is required; these
sites provide the parse_static_line/parse_static_line_multi to
StaticIr::from_definition pattern that the updated documentation must accurately
cover.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd35e039-aa63-40f7-b96f-03edd92264f3
⛔ Files ignored due to path filters (15)
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__barbarian_class_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bard_class_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bard_class_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__innkeepers_talent_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__innkeepers_talent_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__kabira_vindicator_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__kabira_vindicator_lowered.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__lumen_class_frigate_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__sorcerer_class_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__sorcerer_class_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__student_of_warfare_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__wizard_class_two_layer_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__wizard_class_two_layer_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (13)
.githooks/pre-commit.github/workflows/ci.ymlcrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_class.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_level.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_saga.rscrates/engine/src/parser/oracle_spacecraft.rscrates/engine/src/parser/oracle_static/mod.rsscripts/check-prelowered-ratchet.shscripts/prelowered-ratchet.txt
💤 Files with no reviewable changes (1)
- crates/engine/src/parser/oracle_ir/doc.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
`clippy::question_mark`, surfaced by the T3 signature change rather than by
new code: `lower_as_enters_or_face_up_counters` returned `bool` until this
tranche, so its `let ... else { return false }` guards were invisible to the
lint. Returning `Option` makes the `checked_sub` guard a textbook `?`.
The sibling guard above it stays a `let ... else` — it destructures a `Result`
from `tag(", ")`, where `?` would try to convert the nom error into the
function's `Option` and does not apply.
`check-skill-doc.sh` harvests §3 priority-table labels with `grep -oE '// Priority [^:]+:'`. The comment this tranche left where slot 13e used to live kept that exact shape, so the gate read the gravestone as a declaration and demanded a §3 row for a slot the same commit had just deleted — it asked the table to document something that no longer exists. The count invariant did not catch it: deleting the slot and leaving a colon-terminated comment held the total at 41, so only the per-label check fired. That is the gate working correctly; two of its three invariants are count-based and this is the one shaped to catch renames. Reword the gravestone out of the harvested shape (41 -> 40 slots) and drop the now-dead `13e` row (41 -> 40 rows). A retired slot is documented by its absence from the table, not by a row saying it is gone — a row would re-assert the slot to every reader who greps the table for live dispatch order. The comment says so inline, because the first fix attempt reintroduced the failure: prose citing the harvested shape verbatim is itself harvested. The gate is textual and cannot tell a declaration from a quotation of one.
This was referenced Jul 27, 2026
Merged
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 Class-B bring-up, tranches T0–T3 (
.planning/architecture-remediation/05b-class-b-bringup.md,row table in
05b-U0-recensus-2026-07-26.md). Six commits, four tranches, seven converted producer sites.2b230ba0fadead_codeallow onOracleNodeIr::Static8c224078edPreLoweredratchet early (§6.1), seeded as a ceiling2e1973d48a22a58386c7class-section-preloweredrows →StaticIr/ReplacementIr163a47bee5ddf26b32a9Gate half one — empirical
Byte-identity: PASS, by attribution rather than by
cmp.The pinned baseline was voided mid-tranche by exactly the hazard the charter names
("MTGJSON refresh rolls Monday — a mid-tranche refresh voids the baseline"):
3cf2930a39bumpedcrates/engine/data/mtgjson-vintage. A rawcmptherefore shows red for reasons that are not this PR.Full-pool regeneration (card-data build 2427) diffed per card key against the pinned baseline:
35,516 entries both sides; 0 added, 0 removed.
13 entries changed — all 13 are Licids, attributable to
6b29e0d720"fix(engine): Licids attach themselves, not their enchant target (fix(engine): Licids attach themselves, not their enchant target (#605) #6668)", which landed between the
baseline pin and the regeneration.
0 changed entries in any class these conversions touch, and the negative is non-vacuous:
LEVELblock)N+ | body)Snapshots.
student_of_warfare_iris the only genuinely pre-conversion baseline among the churnedfixtures; its churn was verified to be pure re-nesting by normalizing
Static{definition,..}back toPreLoweredStatic{..}and comparing==— not by reading the re-indented diff.active_zonesstayed[]and noCountersOn.scoperebound, which is the charter's STOP condition for the double-lowering.student_of_warfare_lowered.snapdid not change at all.Two fixtures were added because the corpus could not witness T2's rows at all
(
lumen_class_frigatereaches both Spacecraft static arms;kabira_vindicatorreaches the levelerbody arm twice). Six
Staticnodes, zeroPreLoweredStatic, everysource_textthe real recognizerinput rather than
lines[line].test-enginegreen at build 2420 — observed red first at 2413 on exactly these two fixtures, thengreen after accepting, per the seen-red rule.
cargo fmt --allclean. Ratchet gate PASS.Gate half two — CR-faithfulness for the class
lower_static_irapplies two transforms and both are idempotent and blind to theinterposed condition:
collect_source_in_zonesreads onlySourceInZoneandpopulate_active_zones_from_conditionself-guards onactive_zones.is_empty();bind_anaphoric_countersrewrites onlyObjectScope::Anaphoricand derives its bound value fromdef.affected, which neither theClassLevelGEwrap nor CR 711.2a / CR 702.184aHasCounterstouches. So the double-lowering is a no-op for the class, not merely for today's corpus.
lower_replacement_irisir.definition.clone()— identity by construction.ETB replacement (CR 714.3a) prints no line — the rule applies from the subtype alone — and the anchor
tempts a lie, since
etb_linepoints at chapter I's line, which the chapter-I trigger owns.Standing obligation created:
lower_static_ir's doc comment now records that any future transformadded there must be idempotent, or all 29 sites silently double-apply it.
Harvest (§5.4)
"X can't be 0.") was structurally unreachable:strip_x_cant_be_zero_suffixreturns""for exactly that input andloweris bound once from thepost-strip line, so the empty-line guard always claims it first. Its own comment called it a
"defensive fallback". Removing it also drops
mutate_last_spellfrom two callers to one, halving thesurface of item 2.
doc.rs:804pushes bothSpell(_)andPreLoweredSpell(_)onto
spells_emitted, andtake_last_spellpops that stack with no variant filter — whileoracle.rs:3683and:5827destructure withunreachable!("… returns only PreLoweredSpell items").The first native
Spellproducer makes both reachable: a panic during card parsing, not a silentskip. This is a hard prerequisite for T8, confirming §7.4.
description: "12+ | Flying, lifelink"butsource_text: "Flying, lifelink". Left as-is (thedescription feeds card data), and it is incidental evidence for why the preprocessor had to own
source_textexplicitly.(
oracle_level.rs:146,parse_static_line_multi— no printed LEVEL body lowers to more than onestatic) and T1's row U0-60 (ability-word static — all three ability-word-prefixed Class level bodies
are
Landfall — Whenever …and take the trigger arm). Their conversions rest on the class argument.21 → 20, because the Spacecraft site emitted through the
static_athelper and never had a textualPreLoweredhit of its own. Helper-mediated producers stay invisible until the helper dies.instawrites.snap.newinto the Tilt-watched source tree, so a failing snapshot testself-perpetuates a rebuild loop that starves every other resource on the cargo lock —
card-datasatpendingfor ~7h behind it. Same failure classgen-card-data.sh's.tmp.infix already guardsagainst for
known-tokens.toml.Scoped out, deliberately
U0-05 / U0-06 (the
drain_result_vectorsstatic and replacement arms). Converting them needsParsedAbilities.statics/.replacementsto carry IR — 93 call sites on apubtype — and themodal-block feeder passes only the block HEADER line, so every drained per-mode static would have to
source its own text from inside
lower_oracle_block. Until that lands,static_atandreplacement_ateach retain exactly one caller, so T3's headline payoff of deleting both emitter methods is still pending.
Summary by CodeRabbit