refactor(parser): Plan 05b — U0-61 and U0-40, the two rows T9 unblocked by matthewevans · Pull Request #6726 · phase-rs/phase

matthewevans · 2026-07-28T11:07:22Z

Plan 05b — the two rows T9 unblocked: U0-61 (the Class effect-sentence recognizer) and U0-40
(the heterogeneous IR node mechanism). Follows #6724, which completed Unit 3b.

These were the only remaining rows needing no design decision. Everything else left in the plan —
U0-62 and all nine T10 sub-units — is blocked on rulings and is untouched here.

One commit per row.

U0-61 — byte-identical by construction, and the identity is the function body

parse_effect_chain(t, k) is lower_ability_ir(&parse_ability_ir_standalone(t, k)). That is not a
doc claim; it is the entire body at oracle_effect/mod.rs:26761-26763:

pub fn parse_effect_chain(text: &str, kind: AbilityKind) -> AbilityDefinition {
    lower_ability_ir(&parse_ability_ir_standalone(text, kind))
}

Splitting the call into its two halves moves where lowering happens, not what it produces. No
corpus argument is used or needed — this is the same algebraic identity every T8 R1 row relied on.

The recensus's warning was precisely calibrated: this could not land before the Spell payload carried
AbilityIr, because emitting Spell(EffectChainIr) would have lowered via lower_effect_chain_ir and
dropped finalize_effect_chain + the owner-library anchor — the exact defect #6722 spent a PR fixing
at U0-39. #6724 removed the hazard.

has_unimplemented keeps reading a lowered def (has_unimplemented(&lower_ability_ir(&ir))), the
U0-12/U0-39 shape. No IR-level predicate was invented — a second authority over EffectChainIr would
be free to diverge from the real one.

Scope held to line 334. oracle_class.rs:398 (the class-level trigger execute) and the
make_unimplemented fallback (U0-62, blocked on a ruling) are untouched.

Ratchet LOWERED: oracle_class.rs 5 → 4, count now equal to ceiling.

U0-40 — which `drain_result_vectors` caller, and how we know

drain_result_vectors has two callers and one of them is U0-18, which belongs to T10h and is
blocked. Since line numbers in oracle.rs have drifted ~+300 across this campaign, proximity to the
census's cited line could not settle it. Four independent lines of evidence identify oracle.rs:5630:

Content — :4304 sits inside if let Some((block, next_i)) = parse_oracle_block(..) and calls
lower_oracle_block, which is verbatim the U0-18 census row. :5630 is guarded by
is_as_enters_becomes_choice_pattern.
CR citations — the in-code annotation at :5642 is "CR 208.2b + CR 614.1c + CR 614.12a", exactly
the U0-40 census row's citations.
Drift — :5630 is +304 from the cited ~5326; :4304 is +161 from U0-18's cited 4094, the
census's own recorded drift for that row.
Population — the census names aquamorph entity / primal clay / primal plasma, exactly the cards
this recognizer's frame matches.

:4304 is unmodified, and drain_result_vectors itself is unmodified — U0-18 still uses it unchanged.

The inherited safety argument held, but one of its REASONS was wrong

U0-40's byte-safety analysis was carried over from T9b's executor and verified rather than
inherited. Three claims confirmed as stated; one had a wrong reason and was not shipped as written:

lower_replacement_ir is identity — confirmed, literally ir.definition.clone().
collect_source_in_zones ignores ChosenLabelIs — confirmed; it matches only
SourceInZone/And/Or/Not, so zones stays empty and the !zones.is_empty() guard never fires.
Corroborated in the artifact: every static of all four cards has "active_zones":[].
bind_counter_anaphor_to_recipient is inert — conclusion confirmed, reason corrected. The
inherited reason was "these modifications are fixed P/T (the recognizer returns false unless power
and toughness are both Some)". That guard does not exclude dynamic P/T: parse_animation_spec's
"where X is …" path sets spec.dynamic_power before the fixed-P/T parse and is not gated on it,
and split_animation_dynamic_pt_clause can fire after split_animation_base_pt_clause has already
set fixed P/T, so both can coexist — and SetPowerDynamic/SetToughnessDynamic are in the
Some-arm. The real reason: bind_anaphoric_counters rewrites only CountersOn { scope: Anaphoric }; that scope is produced by exactly one combinator (parse_deferred_counter_pronoun) and
survives only through the two _deferred entry points, whose only non-test callers are both in
oracle_static/anthem.rs. The animation path reaches neither. Corroborated: all 43 modifications
across the four cards are in the None arm, with zero Dynamic|CountersOn|Anaphoric occurrences.
Emission order must stay abilities → statics → replacements — confirmed and preserved by
construction, matching drain_result_vectors deliberately. See the caveat below.

Full-pool measurement

Baseline re-measured on this branch's own base, not inherited.

base: sha256 c3f8431c494c1a9710e08fc15161eab75203a2dc98a5aa35e3f7a993ec43c245, 99,201,978 bytes
tip (both rows): identical hash, identical size; cmp byte-identical

Population is real: 35,516 face-keyed entries, generated from the worktree repo root against the
main checkout's AtomicCards.json (158 MB) symlinked in, MTGJSON_SKIP_REFRESH=1, generator rebuilt
fresh at each commit. The fixture population is 519 KB and cannot produce those numbers. Coverage
percentages and all parse-warning counts are identical base vs tip, including the span-sensitive
swallowed-clause audit (963 warnings / 868 cards).

Liveness — the probe was watched go red, not assumed (§5.3: a probe that shows nothing is broken
until proven otherwise):

Compiler witness — the signature change (&str, &mut ParsedAbilities) -> bool →
(&str) -> Option<AsEntersChoiceModalIr> forced the call site.
Reach, read from the artifact — U0-40's four cards all carry the recognizer's output. Notably
aquamorph entity has 2 statics / 1 replacement / 1 ability (the CR 614.1e face-up residual), so
the residual-first ordering branch is live, not dead. U0-61's single pool ability is
rogue class → CastFromZone with a sub_ability — i.e. it exercises finalize_effect_chain,
the exact machinery refactor(parser): Plan 05b T9b — the payload swap, all nine spell producers at once #6724 was the prerequisite for.
Disable probe watched red — forcing lower_as_enters_becomes_choice_modal to return None
moved the filtered output hash ec05c400… → 2781bafd…; aquamorph entity went 2 statics/1 rep →
0/0, primal plasma 3/1 → 0/0. The gate destroys exactly what the conversion produces. Probe
fully reverted.

Gates (all on the final commit, re-run after commit)

cargo fmt --all                                      clean
cargo clippy -p engine --lib -- -D warnings          zero warnings
cargo nextest run -p engine --lib                    17858 passed, 6 skipped
cargo nextest run -p engine --test integration       4132 passed, 2 skipped
./scripts/check-prelowered-ratchet.sh                Gate P PASS  (oracle_class.rs 5 → 4)
./scripts/check-skill-doc.sh                         ✓ oracle-parser skill references valid
./scripts/check-parser-combinators.sh                Gate G PASS / Gate A PASS

Zero snapshots changed — the diff is four source/ledger files only, .snap.new count 0. Stated
honestly: that is vacuous for these rows, since neither row's cards are in the parse_two_layer
corpus, so the snapshot gate supplied no signal here. The byte measurement and the disable probe are
the evidence.

CR gate: exactly two numbers added — CR 208.2b and CR 614.1e — zero unverified, text read rather
than existence checked.

Judgement call worth a look

The charter said U0-40 keeps "one residual PreLoweredSpell … which the caller can emit directly."
Putting it in the node vector made Gate P fail immediately — the gate counts the token textually
and oracle_replacement.rs has no ledger entry, so a new pre-lowered producer in an untracked file is
exactly what it exists to catch. The residual is therefore a bare Option<AbilityDefinition> field on
the returned struct, emitted via the existing ability_at. That follows the charter's own wording and
keeps the burn-down honest.

emit_ir_nodes_at dispatches through the typed *_ir_at helpers rather than raw emit_at, because
static_at maintains the last_static peek mirror that parsed_result_recently_granted_flashback
reads mid-loop — raw emission would have silently stopped updating it.

Harvest (§5.4)

Stale doc in oracle_static/shared.rs — bind_counter_anaphor_to_recipient's doc says
parse_counter_object_scope "parks those pronouns on ObjectScope::Anaphoric". False: that
combinator has no Anaphoric arm at all and yields only Source/Target. The real producer is
parse_deferred_counter_pronoun. The doc names the wrong combinator.
The inherited U0-40 safety reason was wrong (above), though the conclusion held.
The result.modal check at the U0-40 site is incidental.
lower_as_enters_becomes_choice_modal never wrote result.modal, and result is shared across the
dispatch loop, so the check can only ever observe a modal set by an earlier line. Preserved
verbatim for byte-identity, but it is dead-or-cross-line-leaky as written.
Census undercount — the U0-40 row says "3 cards"; it is 4. Corrupted Shapeshifter also
reaches it. Mercurial Transformation carries "becomes your choice of" but lacks the "As ~ enters"
frame and correctly does not.
Ordering caveat for review: card-data byte-identity is weak evidence for the
abilities → statics → replacements ordering specifically — a cross-category reorder is largely
unobservable there, since within-category order is untouched and the replacement is a singleton. The
order is preserved by construction; the corroboration is that the span-sensitive swallowed-clause
counts are also unchanged.
Cheap liveness instrument: oracle-gen --output X --filter "A|B" runs in seconds against the
real pool and diffs cleanly. Given a full-pool regen costs ~5 min plus ~7 GB of tool-profile
artifacts, future tranches should reach for it first.

Summary by CodeRabbit

New Features
- Improved parsing of modal “as enters, it becomes your choice” abilities, including ordered effects and face-up variants.
- Improved recognition and lowering of spell-like effect text.
Bug Fixes
- Fixed ordering and state tracking when processing mixed ability results.
- Improved handling of unsupported or partially parsed modal effects.

…cognizer 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 (#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.

`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.

coderabbitai · 2026-07-28T11:08:14Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 20f1a586-eb15-49c9-8e0c-eb81db5a3b17

📥 Commits

Reviewing files that changed from the base of the PR and between 8746d32 and 9d4359c.

📒 Files selected for processing (4)

crates/engine/src/parser/oracle.rs
crates/engine/src/parser/oracle_class.rs
crates/engine/src/parser/oracle_replacement.rs
scripts/prelowered-ratchet.txt

📝 Walkthrough

Walkthrough

The PR refactors as-enters choice modal lowering to return typed IR, adds ordered heterogeneous IR emission, and changes effect-like sentence parsing to emit OracleNodeIr::Spell. The prelowered emission ceiling for oracle_class.rs is reduced from 5 to 4.

Changes

Typed Oracle IR migration

Layer / File(s)	Summary
Modal replacement IR lowering `crates/engine/src/parser/oracle_replacement.rs`	The modal parser returns an optional residual ability and ordered `OracleNodeIr` values, using early exits for parse failures and validating modes and labels before constructing statics and replacements.
Typed modal emission `crates/engine/src/parser/oracle.rs`	`DocEmitter` emits heterogeneous IR nodes through typed helpers, and the modal replacement path emits the returned residual and nodes without draining shared result vectors.
Standalone spell IR parsing `crates/engine/src/parser/oracle_class.rs`, `scripts/prelowered-ratchet.txt`	Effect sentence candidates are parsed as standalone ability IR and emitted as `OracleNodeIr::Spell`; the prelowered ceiling is reduced to 4.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant oracle_replacement
  participant DocEmitter
  participant ParsedAbilities
  oracle_replacement->>DocEmitter: return AsEntersChoiceModalIr
  DocEmitter->>ParsedAbilities: emit residual ability and ordered IR nodes
  DocEmitter->>DocEmitter: update last_static and last_trigger peeks

Possibly related PRs

phase-rs/phase#6325: Both changes use typed IR emission helpers while preserving last_static handling.
phase-rs/phase#6699: Both refactor as-enters replacement parsing to return typed IR instead of mutating shared result vectors.
phase-rs/phase#6708: Both update parser handling of IR-native OracleNodeIr::Spell nodes.

Suggested labels: quality

Suggested reviewers: galuis116, nghetienhiep, lgray

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title correctly identifies the parser refactor and the two affected Plan 05b rows.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ 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/plan05b-t6r

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

github-actions · 2026-07-28T11:21:12Z

Parse changes introduced by this PR

✓ No card-parse changes detected.

matthewevans added 2 commits July 28, 2026 04:05

matthewevans enabled auto-merge July 28, 2026 11:07

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

Merged via the queue into main with commit c7dfdff Jul 28, 2026
15 checks passed

matthewevans deleted the ship/plan05b-t6r branch July 28, 2026 11:44

matthewevans mentioned this pull request Jul 28, 2026

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(parser): Plan 05b — U0-61 and U0-40, the two rows T9 unblocked - #6726

refactor(parser): Plan 05b — U0-61 and U0-40, the two rows T9 unblocked#6726
matthewevans merged 2 commits into
mainfrom
ship/plan05b-t6r

matthewevans commented Jul 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

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 by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

U0-61 — byte-identical by construction, and the identity is the function body

U0-40 — which drain_result_vectors caller, and how we know

The inherited safety argument held, but one of its REASONS was wrong

Full-pool measurement

Gates (all on the final commit, re-run after commit)

Judgement call worth a look

Harvest (§5.4)

Summary by CodeRabbit

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!

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 by coderabbitai Bot

Loading

U0-40 — which `drain_result_vectors` caller, and how we know

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading