refactor(parser): Plan 05b T8-A4 — U0-53, the one R2 row under the cap - #6720
Conversation
…e row under the R2 cap
Census row 183: the "{cost}: Level N" level-up ability, **76 of 79 Class
abilities** and the highest-volume row in A4. The hand-built `AbilityDefinition`
becomes a one-clause `EffectChainIr` body plus a CR 602.1 activation shell,
lowered through the single authority `lower_ability_ir`.
`oracle_class.rs` has no `DocEmitter`, so there is no `ability_ir_at` to call:
the preprocessor returns `(usize, OracleNodeIr)` pairs that `parse_oracle_ir`
emits with `emit_at`. The site therefore states phase A's identity inline —
`OracleNodeIr::PreLoweredSpell(lower_ability_ir(&ir))` is exactly what
`ability_ir_at(l, ir) == ability_at(l, lower_ability_ir(&ir))` does one file
over. The `PreLowered` token count is unchanged at 5 (ceiling 5), for the same
reason A0 wrote `ability_ir_at` as delegation: the ratchet counts a bare token
textually and this file sits at its ceiling. T9 turns this into the IR payload
and the token goes away then.
**Restriction ORDER is preserved, and it is the reverse of the rule's.**
CR 716.2a states the two instructions as "Activate only if this Class is level
N-1 and only as a sorcery"; this site has always pushed `AsSorcery` first. The
shell vec is applied verbatim, so it is built in the site's order. Same class of
finding as A2's Boast/CR 702.142a case — preserved here rather than quietly
fixed inside a byte-identity conversion.
**A pre-existing CR miscitation is corrected, not carried forward.** The block
cited "CR 602.5d + CR 716.4". CR 716.4 (`MagicCompRules.txt:6030`) is *leveler
cards*, and says in terms that they "are not the same as class level abilities.
Level counters do not interact with Class cards". The claim being annotated is
CR 716.2a's (:6020). CR 602.5d (:2549) was correct and is kept for the
sorcery-speed half. The U0-53 census row carries the same wrong number and
should be corrected there too.
CR-faithfulness: **correct-by-corpus** (recipe R2), stated explicitly. R1's
argument — "no property of any card's text participates" — is NOT available
here and is not borrowed. Routing a hand-built body through
`assemble_effect_chain` + `finalize_effect_chain` +
`apply_owner_library_reveal_anchor_from_text` newly subjects it to clause
assembly it never had. Why that is inert for this body:
* `Effect::SetClassLevel` occurs **exactly once** in the whole of
`crates/engine/src/parser/oracle_effect/` — `sequence.rs:6175`, inside
`clause_is_dig_lookback_transparent`, a predicate that only runs when an
antecedent search walks BACK over intervening clauses. A one-clause chain
has no intervening clause, so it is never invoked. Every other pass in the
assembly tail and in `finalize_effect_chain`'s six passes dispatches on a
different `Effect` variant or on `sub_ability` / `else_ability` /
`mode_abilities` presence, and this body has none of them.
* `PlayerScopeRewrite::Preserve` (T8's mandated R2 mitigation) removes
`apply_player_scope_rewrites` by construction — the hand-built path never
ran it. Measured, it is a **null**: see below.
* The owner-library anchor is doubly inert. Its guard needs both "shuffles it
into their library" and "reveals the top card of their library" in the
source text, and the walk beneath the guard looks for `Effect::Shuffle` /
`Effect::RevealTop`, neither of which this body can contain.
**What a future card or future commit could violate.** The corpus properties
relied on: (1) no Class level bar prints "Level 0" — `section.level - 1` on a
`u8` would underflow, a pre-existing hazard this conversion preserves rather
than introduces; (2) no Class level-bar cost text contains the anchor's two
trigger phrases (belt-and-braces — the walk is inert regardless). The larger
residual dependence is on **code**, not cards: unlike R1's algebraic identity,
this argument is a survey of today's pass list. A pass added later to
`finalize_effect_chain` or to the assembly tail that keys on
`AbilityKind::Activated`, on a single-clause chain, or on `SetClassLevel` would
newly apply here with nothing in the type system to catch it.
§5.3, reachability — attributed by probe MESSAGE, never by test name. A
`panic!("A4-U0-53-REACHABILITY-PROBE")` at the site fires in **9** `--lib`
tests and **12** `--test integration` tests (21 total). Attribution by message
was load-bearing again: `parser::oracle_effect::tests::bandits_talent_level3_
draw_counts_hellbent_opponents` and all ten Scavengers-Talent / Party-Dude
integration tests name a card, never the recognizer. The site needs no
remediation snapshot — the five Class two-layer snapshots landed at T1 already
witness it twice each (Level 2 and Level 3), across ten distinct cost shapes.
§5.3, perturbations — every one quoted, every null reported as a null:
* swap the restriction order → **RED**, 6 tests (liveness arm)
* drop the shell `description` → **RED**, 6 tests
* drop the shell `cost` → **RED**, 6 tests
* `PlayerScopeRewrite::Apply` → **NULL**, 137/137 pass either way
The `Apply`/`Preserve` null is the honest headline of the R2 mitigation at this
site: it is extensionally inert on today's corpus and therefore supplies **no**
evidence that the conversion is safe. `Preserve` is still the right value —
it is what the hand-built path did, by construction — but the safety argument
rests on the shape analysis above, not on the mitigation. `ChainLoweringMode` is
not applicable here and is reported as such: this recognizer parses no text, so
there is no entry point whose mode could be crossed.
CR numbers grep-verified against docs/MagicCompRules.txt, text read and not
merely number-matched: 602.1 (:2514), 602.1a (:2516, "the activation cost is
everything before the colon"), 602.1b (:2519, activation instructions),
602.5d (:2549, "Activate only as a sorcery"), 716.2a (:6020, the level-bar
expansion). CR-annotation diff gate: zero UNVERIFIED.
Gates: `cargo fmt --all` clean, `Gate P PASS` (oracle_class.rs unmoved at 5,
ledger unedited), skill-doc `✓`, Gate G + Gate A PASS, `cargo clippy -p engine
--lib -D warnings` clean, `cargo nextest run -p engine --lib` 17851 passed /
6 skipped, `--test integration` 4123 passed / 2 skipped, and **zero snapshot
churn** (no `*.snap.new`; `git status` shows only this file).
📝 WalkthroughWalkthroughThe class parser now constructs level-up abilities as effect-chain IR, preserves player scope, and lowers the resulting ChangesOracle class level-up parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes 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_class.rs`:
- Around line 161-163: Update parse_class_level_line when constructing
ActivationRestriction::ClassLevelIs to reject or handle section.level == 0
before evaluating section.level - 1. Preserve the existing level subtraction
only for valid positive levels, preventing underflow on malformed class text.
🪄 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: 8d5acc5b-be16-4c95-99d7-543a5035e001
📒 Files selected for processing (1)
crates/engine/src/parser/oracle_class.rs
| ActivationRestriction::ClassLevelIs { | ||
| level: section.level - 1, | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm parse_class_level_line's return range for `level`
ast-grep run --pattern 'fn parse_class_level_line($$$) { $$$ }' --lang rust crates/engine/src/parser/oracle_class.rsRepository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find the parsing helper/uses.
ast-grep outline crates/engine/src/parser/oracle_class.rs --view expanded || true
rg -n "parse_class_level_line|section\.level|ClassLevelIs|level - 1" crates/engine/src/parser/oracle_class.rs crates/engine/src/parser -SRepository: phase-rs/phase
Length of output: 8507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section with line numbers.
sed -n '1,260p' crates/engine/src/parser/oracle_class.rs | cat -nRepository: phase-rs/phase
Length of output: 13999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/engine/src/parser/oracle_class.rs')
text = p.read_text()
for needle in ['parse_class_level_line', 'section.level', 'ClassLevelIs', 'level - 1']:
print(f"\n## {needle}")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f"{i}: {line}")
PYRepository: phase-rs/phase
Length of output: 1669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### oracle_class.rs matches"
rg -n "parse_class_level_line|section\.level|ClassLevelIs|Level [0-9]+" crates/engine/src/parser/oracle_class.rs -S || true
echo
echo "### oracle_class.rs excerpt"
nl -ba crates/engine/src/parser/oracle_class.rs | sed -n '1,260p'Repository: phase-rs/phase
Length of output: 1873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba crates/engine/src/parser/oracle_class.rs | sed -n '120,210p'Repository: phase-rs/phase
Length of output: 192
🌐 Web query:
CR 716.2a level up first level level 2 class cards level 1 class abilities
💡 Result:
In Magic: The Gathering, rule 716.2a defines how the activated abilities on Class enchantments function [1][2]. Specifically, the rule states that an ability formatted as "[Cost]: Level N — [Abilities]" means "Activate only if this Class is level N-1 and only as a sorcery" [1][2]. Class cards operate according to the following mechanics: 1. Starting State: When a Class enchantment enters the battlefield, it is at level 1 and has access to its first (level 1) class ability [3][4]. 2. Leveling Up: To advance a Class to the next level, you must activate the corresponding level-up ability by paying its cost [4][5]. This is an activated ability that uses the stack and can be interacted with (e.g., countered) [5]. 3. Timing and Requirements: Per rule 716.2a, you can only activate a level-up ability at sorcery speed, and only if the Class is currently at the level immediately preceding the level you are attempting to reach (e.g., you must be at level 1 to activate the ability to reach level 2) [3][6][4]. 4. Cumulative Abilities: Gaining a level does not remove abilities the Class had at previous levels [7]. When a Class reaches a new level, it retains all its previous class abilities and gains the new one associated with the new level [3][4][7]. Each Class card typically contains five abilities: three "class abilities" (which represent the effects gained at levels 1, 2, and 3) and two "level abilities" (the activated abilities used to advance from level 1 to 2 and from level 2 to 3) [3][6].
Citations:
- 1: https://www.reddit.com/r/mtgrules/comments/1gohi4m/question_about_bello_marvin_and_alchemists_talent/
- 2: https://www.reddit.com/r/mtgrules/comments/16d6sko/agathas_soul_cauldron_and_level_up_cards/
- 3: https://gatherer.wizards.com/Pages/Card/Details.aspx?name=Monk%20Class
- 4: https://cardgamebase.com/mtg-class/
- 5: https://draftsim.com/mtg-class-cards/
- 6: https://scryfall.com/card/pafr/202a/ranger-class
- 7: https://scryfall.com/card/pafr/228p/monk-class
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba crates/engine/src/parser/oracle_class.rs | sed -n '140,190p'Repository: phase-rs/phase
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn parse_number|parse_number\\(" crates/engine/src/parser -SRepository: phase-rs/phase
Length of output: 29357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### parse_number implementation"
sed -n '1,140p' crates/engine/src/parser/oracle_nom/primitives.rs
echo
echo "### parse_number tests around zero / digits"
sed -n '1400,1675p' crates/engine/src/parser/oracle_nom/primitives.rs
echo
echo "### oracle_class tests around level lines"
sed -n '460,580p' crates/engine/src/parser/oracle_class.rsRepository: phase-rs/phase
Length of output: 21537
Reject Level 0 before subtracting 1. parse_class_level_line accepts Level 0, so section.level - 1 can underflow on malformed class text; gate it explicitly when building ClassLevelIs.
🤖 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_class.rs` around lines 161 - 163, Update
parse_class_level_line when constructing ActivationRestriction::ClassLevelIs to
reject or handle section.level == 0 before evaluating section.level - 1.
Preserve the existing level subtraction only for valid positive levels,
preventing underflow on malformed class text.
Parse changes introduced by this PR✓ No card-parse changes detected. |
Plan 05b tranche T8-A4 — the R2 rows, and the last sub-tranche of T8. Follows #6719 (A3).
One row converted, four excluded with evidence. The exclusion table is this tranche's headline
deliverable, not the conversion count.
Why the scope is 1 of 5
§T8 lists five R2 rows (U0-53, U0-62, U0-02, U0-52, U0-38) but its own R2 paragraph caps R2 at
single-clause bodies with no
sub_ability, "anything larger is a recognizer rewrite, not an R2conversion". Neither statement wins outright, and the four exclusions have three different
reasons — for two of them the cap is not even the operative axis.
oracle_class.rs:118-172sub_abilityoracle.rs:5511-5531oracle.rs:6392oracle.rs:2164oracle_class.rs:341-344U0-38 exceeds the cap textually.
oracle.rs:5519is.sub_ability(AbilityDefinition::new(AbilityKind::Spell, Effect::PreventDamage{..}))— a two-nodeChoose→PreventDamagebody. Corroborating from a second direction: the recensus's own tranchetable at line 601 assigns U0-38 (and U0-02) to T10e, not T8. Two of the document's three
statements agree; the §T8 row list is the outlier.
U0-52 is not a hand-built-body site.
oracle.rs:6382bindsnom_defonce; U0-51 emits it whenthe dispatcher produced a real effect, and U0-52 emits the same binding otherwise. The five
Effect::unimplementedshapes the census attributes to this site are produced insidedispatch_line_nom. Converting it needs anAbilityIrfor the fallback branch, and there are exactlythree ways to get one: an IR-returning dispatcher (precisely T10a's stated blocker), re-running the
dispatcher (the census forbids it), or un-lowering the definition (impossible —
AbilityIr::from_definitiondoes not exist, by A0's H4 design).U0-02 and U0-62: verdict confirmed, reasoning corrected. Both bodies are
make_unimplemented(...)— single clause, no
sub_ability— so they pass the structural cap, and the implied byte-identityblocker is not real:
Effect::unimplemented("unknown", line)expands to exactly that literal, soGate F is satisfiable and byte-identity is mechanically reachable. Gate F is not the blocker.
The verdict holds on firmer ground. §5.2 makes the written CR-faithfulness argument a mandatory half
of the gate, and requires it "for the CLASS of text the recognizer handles". Neither site has a
recognizer. Both are terminal residuals whose membership is defined negatively — whatever every
preceding arm declined. A negatively-defined set has no text class, so the required argument cannot be
written for either site under any recipe. That is a structural property, not a judgement call, and
it survives even if the shared residual node is later built. Wrapping the sentinel ad hoc at two sites
would also be the exact per-site special case that node exists to replace.
The shared residual node was not built — architecture work, out of scope.
The converted row: U0-53, the Class level bar
76 of 79 Class abilities.
kind = Activated,effect = SetClassLevel{level}, pluscost,description, andactivation_restrictions = [AsSorcery, ClassLevelIs{level-1}]in that order.Labelled correct-by-corpus, and R1's "no property of any card's text participates" is deliberately
not borrowed. What makes it byte-identical:
Effect::SetClassLeveloccurs exactly once in the whole ofparser/oracle_effect/—sequence.rs:6175, insideclause_is_dig_lookback_transparent, a predicate that runs only when anantecedent search walks back over intervening clauses. A one-clause chain has none, so it is never
invoked. Every other pass in the assembly tail and in
finalize_effect_chain's six passes keys on adifferent
Effectvariant, or onsub_ability/else_ability/mode_abilitiespresence — none ofwhich this body has.
PlayerScopeRewrite::Preserveremovesapply_player_scope_rewritesby construction.and the walk beneath it looks for
Effect::Shuffle/Effect::RevealTop, neither expressible here.What a future card could violate: a Class card printing "Level 0" —
section.level - 1on au8underflows. Pre-existing, preserved, not introduced.
What a future commit could violate — the honest limit of R2. Unlike R1's algebraic identity, this
argument is a survey of today's pass list. A pass added later to
finalize_effect_chainorassemble_effect_chain's tail that keys onAbilityKind::Activated, on chain length, or onSetClassLevelwould newly apply here, with nothing in the type system to catch it. This is why R2is not correct-by-construction, stated rather than glossed.
§5.3
Reachability, attributed by probe message and never by test name: 21 reachers — 9 in
--lib, 12 inthe integration binary. Attribution mattered a third consecutive time:
bandits_talent_level3_…andall ten Scavengers-Talent / Party-Dude integration tests name a card and never the recognizer.
No remediation snapshot was needed — the first tranche in A1–A4 where that is true. The five Class
two-layer snapshots landed at T1 already witness this site twice each (Level 2 and Level 3), across
ten distinct cost shapes. Their
*_ir.snappayloads were checked to confirm they serialize the fulllowered def including the ordered
activation_restrictionsarray — that is why the perturbationsdiscriminate, verified rather than assumed.
ClassLevelIsbeforeAsSorcery)descriptioncostPlayerScopeRewrite::Applyinstead ofPreserveThe
Apply/Preservenull deserves emphasis rather than burial.Preserveis one of §T8's twomandated R2 mitigations, and measured, it is extensionally inert on today's corpus — so it supplies
no evidence that this conversion is safe.
Preserveremains the correct value (it is what thehand-built path did, by construction), but the safety argument rests entirely on the shape analysis
above. This row should not be read as "mitigation applied, therefore safe".
ChainLoweringModeis inapplicable here rather than null: the recognizer parses no text, ithand-builds an
Effectfrom a parsed level number, so there is no entry point whose mode could becrossed.
Gates
Zero snapshot churn: the entire diff is one line —
M crates/engine/src/parser/oracle_class.rs.No files added, no snapshots changed,
.snap.newcount 0.Gate P did not lower for
oracle_class.rs, deliberately. It stays at 5/5, ledger unedited. Thatfile has no
DocEmitter, so there is noability_ir_atseam to hide the token behind; the site nowstates phase A's identity inline as
PreLoweredSpell(lower_ability_ir(&ir))— the same valueability_ir_atcomputes one file over. The token retires at T9 with the payload swap.Harvest (§5.4)
oracle_class.rscited CR 716.4 for the Class level gate — the rule that explicitly disclaims theconnection. CR 716.4 (
:6030) is about leveler cards and states they "are not the same asclass level abilities. Level counters do not interact with Class cards, and class levels do not
interact with leveler cards." The annotated claim is CR 716.2a's (
:6020). CR 602.5d wascorrect and is kept. Fixed in this commit. The U0-53 census row carries the same wrong
number — the recensus needs correcting, which is a maintainer call; no planning doc was edited.
and only as a sorcery"). Directly analogous to A2's Boast/CR 702.142a finding. Semantically
harmless — the restrictions are a conjunction — and preserved, not fixed.
section.level - 1is an unguardedu8subtraction; a "Level 0" line would underflow.Unreachable today only as a corpus property. Pre-existing.
SetClassLevelsingle-occurrence grep is reusable as the mechanical core of any futuresingle-leaf-
EffectR2 argument.oracle_class.rs:332-338:is_effect_sentence_candidate→parse_effect_chain→ ifhas_unimplemented, the parsed def is thrown away and the line falls through tomake_unimplemented(line). Whoever designs the shared residual node must decide whether to preservethat discarded partial chain — and doing so is a behaviour change, not a conversion.
AbilityDefinitionbody inoracle_class.rs— itsAbilityDefinitionimport is now unused and was removed.Deviation flagged rather than buried
A pre-existing CR citation was corrected inside a byte-identity commit. Justified by CLAUDE.md
("if you are modifying existing game logic, verify existing CR annotations are present and still
accurate"); it changes no code, and leaving a known-wrong CR number in a block being rewritten was the
worse option. Flagged because byte-identity tranches are otherwise strictly no-extras.
Summary by CodeRabbit