refactor(parser): Plan 05b T8-A3 — the R1 remainder (U0-50, U0-12) - #6719
Merged
Conversation
…ys default Priority 14 ability-word "try as effect" (census row 170). The only site in the family that stamps **nothing** — not `cost`, not `activation_restrictions`, not even `description` — so `ir.shell` stays `AbilityShellIr::default()` and the conversion is the bare entry-point swap with no envelope to carry and no `ShellStage` to list. No fields were invented for it. **The conditional emit stays at the call site, on purpose.** "Should this be emitted at all" is control flow over the enclosing dispatch loop, not a property of the definition being built; folding it into the shell would make `AbilityShellIr` a place where a recognizer can decline, which is not what the CR 602.1 envelope describes. The guard is therefore untouched. **How the lowered-def predicate survives the conversion.** `has_unimplemented` reads an `AbilityDefinition`, and an `AbilityDefinition` cannot be un-lowered into an `AbilityIr` (there is no `AbilityIr::from_definition`, by design), so the predicate cannot move onto the IR. It instead runs on `lower_ability_ir(&ir)` while the *retained* artifact stays the IR. This is not a new idiom: the instant/sorcery prevention-text site a few hundred lines above already routes `has_unimplemented(&lower_effect_chain_ir(&effect_ir))` around a retained `OracleNodeIr::Spell`. Lowering twice (once for the predicate, once inside `ability_ir_at`) is a repeat of the same computation, never a different one: `lower_ability_ir` is a pure `&AbilityIr -> AbilityDefinition` — it takes no `ParseContext`, and `rg 'thread_local!|static mut|AtomicU|RefCell<|Cell<' crates/engine/src/parser/oracle_effect/` returns zero hits, so nothing under the whole of the lowering path carries interior mutability. H4's non-idempotence is about `def -> IR -> def` round-tripping, which is exactly what does not exist; it says nothing about applying a pure function twice to the same input. Mode inherited by NAME: the original called `parse_effect_chain_with_context`, so this takes `parse_ability_ir_with_context`. `AbilityKind::Spell` is preserved — this site is the one in A3 that is not `Activated`. CR-faithfulness: **by construction**, for the whole class of text the recognizer handles; no property of any card's text participates. `parse_effect_chain_with_context(t,k,cx)` IS `lower_ability_ir(&parse_ability_ir_with_context(t,k,cx))` (`oracle_effect/mod.rs`), and phase-A `ability_ir_at(l, ir)` IS `ability_at(l, lower_ability_ir(&ir))` (`oracle.rs`). With a `default()` shell and no stages the envelope applier is a no-op, so the converted site computes exactly the old site's value. Reachability (probe-attributed, never by test name): `panic!` at the site fires in `parser::swallow_check::tests::duration_until_eot_drop_tower` (--lib) and `printed_ability_order::memory_test_triggers_are_in_printed_order` (--test integration). Note neither name mentions this recognizer — attribution by probe message was load-bearing here, exactly as A2 warned. Gates: fmt clean, `Gate P PASS` (oracle.rs unmoved at 16), skill-doc `✓`, Gate G + Gate A PASS, `cargo clippy -p engine --lib -D warnings` clean, and zero snapshot churn (no `*.snap.new`; `git status` shows only this file).
…y runs on the lowered def
CR 711.2a + CR 711.2b activated line inside a LEVEL block (census row 132), and
the only site in phase A whose *retry decision* is made on a LOWERED definition
while the *retained artifact* must stay an IR.
**The retry, and why this shape is behavior-identical.** `AbilityIr` has no
`from_definition` and `lower_ability_ir` is not idempotent, so an
`AbilityDefinition` cannot be un-lowered — the retry cannot be expressed by
lowering, mutating and re-wrapping. Both candidates are therefore parsed as IR
and each is lowered purely to *ask* `has_unimplemented`, while whichever IR won
is what reaches `ability_ir_at`. No IR-level `has_unimplemented` was invented: a
second predicate over `EffectChainIr` would be a rival authority free to diverge
from the real one, so this lowers and asks the real one.
Three properties make it the same computation as the original:
1. `parse_effect_chain_with_context(t,k,cx)` IS
`lower_ability_ir(&parse_ability_ir_with_context(t,k,cx))`, so each
`has_unimplemented` argument is bit-for-bit the definition the original tested.
2. **The `ctx` sequencing is preserved exactly.** The retry's second parse
receives the SAME, already-mutated `ctx` as the first — not a fresh one — and
the `ctx` reset above it is not reordered. Interposing the lowering between
the two parses cannot perturb this: `lower_ability_ir` takes no
`ParseContext`, and `rg 'thread_local!|static mut|AtomicU|RefCell<|Cell<'
crates/engine/src/parser/oracle_effect/` returns zero hits, so nothing on the
lowering path carries interior mutability. `lower_ability_ir` is a pure
`&AbilityIr -> AbilityDefinition` and calling it twice is safe — verified by
reading it, not assumed.
3. The predicate is invariant under the envelope: `has_unimplemented` reads only
`effect` and `sub_ability`, both CR 608.2 resolution-tree fields, and the
shell stamps neither. So keeping the predicate pre-stamp (where it already
was) is safe rather than merely convenient.
Cost: one extra lowering per LEVEL-block activated line (two or three rather than
one or two). It is intrinsic, not laziness — the predicate's lowered value is
*pre*-shell and the emitted one is *post*-shell, so they are genuinely different
values and neither can be cached as the other. The path runs only on LEVEL
blocks: nine cards in today's pool.
Composed order — restrictions: parsed `constraints.restrictions` LEAD, then the
implicit `LevelCounterRange{minimum,maximum}` trails. Same polarity as A2's
Boast/Exhaust/Forecast, opposite of A1's U0-22. Stages:
`[ExtractCostReduction, ExtractManaSpendTrigger]`, the order the site ran the two
folds in; `lower_ability_ir` runs stages after the envelope stamps, which is the
pinned order the site had by hand.
The original's `=` is equivalent to the shell's `extend`, verified not assumed:
`rg activation_restrictions crates/engine/src/parser/oracle_effect/` returns 3
hits, all inside `apply_ability_shell_envelope` itself, so nothing reachable from
`lower_ability_ir` writes the root's restrictions and the field is empty when the
shell runs.
`cost` still comes from `normalize_self_refs_for_static(cost_text, card_name)`
before the colon split — that normalization is untouched. `description` is the
full `ability_text`, not `effect_text`. The triggered-ability branch below, whose
`TriggerCondition::And` graft must stay post-lowering to keep `And[gate,x,y]`
flat, is deliberately NOT touched.
Mode inherited by NAME: the original called `parse_effect_chain_with_context`
twice, so both calls take `parse_ability_ir_with_context`.
CR-faithfulness: **by construction**, for the whole class of text the recognizer
handles; no property of any card's text participates. CR numbers grep-verified
against docs/MagicCompRules.txt with the rule text read, not just the number
matched: 711.2 (:5798), 711.2a (:5800), 711.2b (:5802) — the level striation
gating the printed abilities; 602.1a (:2516) "everything before the colon";
602.1b (:2519) activation instructions restricting when a player may activate.
Gates: fmt clean, `Gate P PASS` (oracle.rs unmoved at 16), skill-doc OK, Gate G +
Gate A PASS, `cargo clippy -p engine --lib -D warnings` clean, `cargo nextest run
-p engine --lib` 17849 passed / 6 skipped, `--test integration` 4123 passed / 2
skipped, zero snapshot churn (no `*.snap.new`; only this file modified).
…e (§5.3 remediation)
Additive two-layer snapshots. No production code and no pre-existing snapshot is
touched: `git diff --name-status` is one `M` on `snapshot_tests.rs` and four `A`s.
**What was unwitnessed at U0-12.** The only pre-existing test over the site
(`oracle_tests::leveler_activated_abilities_get_level_counter_range`) asserts
`LevelCounterRange` presence via `.contains(…)` — order-insensitive — and asserts
nothing about the `cost` or `description` the site also stamps. Of the
perturbations run against it, only *dropping* `LevelCounterRange` went red.
* **Guul Draz Assassin** — the richest fixture in the nine-card leveler
population: two striations, a two-component `{B}, {T}` cost (so the CR 602.1a
stamp is pinned to more than a bare `{T}`), a targeted effect, and two
different range shapes (bounded `2-3`, unbounded `4+`) so a range collapsing to
a constant would show.
* **Joraga Treespeaker** — pins the first LIVE `ExtractManaSpendTrigger` guard in
phase A. A2 established its four keyword sites can never run that fold (no pool
card with those keywords lowers to a root `Effect::Mana`); Joraga's `LEVEL 1-4`
body `{T}: Add {G}{G}.` does, so the guard passes here for the first time. The
fold's *body* is still inert — it additionally needs a trailing "when you spend
this mana …" sub-ability and no leveler prints one — so this fixture is the one
that starts discriminating the moment a striation prints a spend trigger.
Both fixtures are pool-verified, not synthetic: Oracle text, types and subtypes
are verbatim from `data/card-data.json`. Both were **watched go red** — dropping
the `LevelCounterRange` push fails both, and reverting it restores green.
**U0-50 is deliberately NOT remediated here, and that is a finding.** Inverting
its emit guard left both full suites green. Exactly two tests reach it and
neither constrains it: `duration_until_eot_drop_tower` asserts only
`!has_swallowed_detector(…)`, a negative that holds vacuously when nothing is
emitted, and `memory_test_triggers_are_in_printed_order` constrains only
triggers. A snapshot fixture was built for Drop Tower and then DISCARDED after
measurement: modelling the card correctly — `Artifact` with its verbatim
`Attraction` subtype — routes the Visit line to the CR 717 preprocessor and the
site is never reached. The two in-suite reachers hit U0-50 only because they
supply no subtype, i.e. only because they model the card wrongly. So no
correctly-modelled fixture can currently witness that guard, and identifying
U0-50's true production population is a separate question, raised to the lead
rather than papered over with a fixture that passes for the wrong reason.
Gates: fmt clean, Gate P PASS, skill-doc OK, Gate G + Gate A PASS, `cargo clippy
-p engine --lib -D warnings` clean, `cargo nextest run -p engine --lib` 17851
passed / 6 skipped (two new), zero `*.snap.new`, zero pre-existing snapshots
modified.
matthewevans
enabled auto-merge
July 28, 2026 04:17
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 ignored due to path filters (4)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughActivated ability parsing now uses ChangesActivated ability IR parsing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant AbilityIrLowerer
participant AbilityEmitter
OracleParser->>OracleParser: Parse activated ability into AbilityIr
OracleParser->>AbilityIrLowerer: Check lowered IR for unimplemented content
OracleParser->>AbilityEmitter: Emit with ability_ir_at
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR✓ No card-parse changes detected. |
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
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 tranche T8-A3 — the R1 remainder: U0-50 (Priority 14 ability-word "try as effect")
and U0-12 (CR 711.2a/711.2b LEVEL-block activated line). Follows #6716 (A2).
Three commits, one concern each. The two sites are asymmetric to an unusual degree: U0-50 stamps
nothing at all, and U0-12 has the only retry in phase A whose predicate runs on the lowered
definition.
U0-50 — the site whose shell stays
default()Entry point
parse_effect_chain_with_context(&effect_text, AbilityKind::Spell, &mut ctx)— noteSpell, the one A3 site that is notActivated— so it takesparse_ability_ir_with_context, chosenby name.
ir.shellstaysAbilityShellIr::default(); no fields were invented for it. The conditional emitguard stays at the call site: "should this be emitted at all" is control flow over the dispatch
loop, not a property of a definition, and folding it into the shell would make
AbilityShellIraplace where a recognizer can decline.
U0-12 — the retry whose predicate runs on the lowered def
The site parses, tests
has_unimplemented(&def), and on failure re-parses a self-ref-normalizedvariant through the same already-mutated
ctx, keeping the alternative only if it comes backclean. A0's H4 forbids an
AbilityIr::from_definition, so this cannot lower-mutate-rewrap.The shipped shape lowers purely to ask the predicate, while whichever IR won is what reaches
ability_ir_at. No IR-levelhas_unimplementedwas invented — a second predicate overEffectChainIrwould be a rival authority free to diverge from the real one.Three properties make it the same computation, each verified rather than assumed:
parse_effect_chain_with_context(t,k,cx)islower_ability_ir(&parse_ability_ir_with_context(t,k,cx)),so every
has_unimplementedargument is bit-for-bit the definition the original tested.ctxsequencing preserved. The retry's second parse receives the same already-mutatedctx,not a fresh one, and the reset above it is not reordered. Interposing the lowering cannot perturb
this:
lower_ability_irtakes noParseContext, and a sweep forthread_local!/static mut/AtomicU/RefCell</Cell<acrossparser/oracle_effect/returns zero hits, so nothing on thelowering path carries interior mutability. H4's non-idempotence concerns
def → IR → defround-trips, which do not exist here; applying a pure function twice is safe.
has_unimplementedreads onlyeffectandsub_ability, both CR 608.2 resolution-tree fields, andthe shell stamps neither. So keeping the predicate pre-stamp is provably safe rather than merely
faithful.
Cost is one extra lowering per LEVEL-block activated line. That is intrinsic rather than laziness: the
predicate's lowered value is pre-shell and the emitted one is post-shell, so they are genuinely
different values and neither can be cached as the other. The path runs on nine cards.
The shape is also already precedented in the tree — the instant/sorcery prevention-text site does
has_unimplemented(&lower_effect_chain_ir(&effect_ir))around a retainedOracleNodeIr::Spell.Restrictions: parsed
constraints.restrictionslead, thenLevelCounterRange{minimum,maximum}—same polarity as A2. Stages
[ExtractCostReduction, ExtractManaSpendTrigger]. The triggered branchbelow, whose
Andgraft is deliberately post-lowering, is untouched.§5.3 — reachability first, and the silences are distinguished
Reachability probes attributed by message, never by name — which mattered, since neither U0-50
reacher's name mentions the recognizer:
--libswallow_check::tests::duration_until_eot_drop_towerprinted_ability_order::memory_test_triggers_are_in_printed_orderoracle::tests::leveler_activated_abilities_get_level_counter_rangeprinted_ability_order::under_construction_skyscraper_abilities_are_in_printed_orderExactly one test per binary per site. Because a
panic!at block entry fails every test that enters,that enumeration is complete, which licenses scoping perturbations to those tests by argument
rather than by hope.
Perturbations ran through a harness carrying a mandatory-red liveness arm, so a dead harness could
not read as "all silent". Liveness → red, control → green, in both binaries.
LevelCounterRangeExtractCostReduction/ dropExtractManaSpendTrigger/ reorder stagesChainLoweringMode→StandaloneThe silences were then diagnosed rather than lumped together, over the full nine-card leveler
population:
constraints.restrictionsis empty forevery leveler card, so the composed vec is always one element and a reversal is a no-op. There is
no non-degenerate pool card, so the A1/A2 remediation recipe is impossible here rather than
skipped.
Under-Construction Skyscraper, whose
Add {W}, {B}, {G}, or {C}lowers toUnimplemented— but thattext has no self-reference, so
normalized_effect != effect_textis false and the alt parse neverruns. Every card that does self-reference parses cleanly and never opens the gate. The most
intricate logic in the tranche is unwitnessed and currently unreachable.
ExtractManaSpendTrigger's guard is LIVE here — the first time in phase A. A2 proved zero poolcards at its four sites lower to a root
Effect::Mana; Joraga Treespeaker's{T}: Add {G}{G}does.The fold body remains inert (it additionally needs a trailing "when you spend this mana" node, and
no leveler prints one), so dropping the fold is correctly silent. Stated as guard-live /
body-dead rather than as a blanket "first live witness".
Remediation
Additive two-layer snapshots for Guul Draz Assassin (two striations, two-component
{B}, {T}cost, bounded
2-3plus unbounded4+) and Joraga Treespeaker (pins the live mana root), Oracletext verbatim from the card pool. Both watched go red under a dropped
LevelCounterRange, thenrestored.
Gates
Zero existing snapshots changed:
git diff --name-statusis 4 A / 2 M, both modified filessource.
.snap.newcount 0.oracle.rsunmoved at 16 on the ratchet; ledger unedited.Parser string-dispatch diff gate clean, including the
rfind/split/split_once/rsplitblind-spot arms. CR diff gate: 8 numbers, 0 unverified, text read for each — 106.6, 601.2f, 602.1a,
602.1b, 603.3, 608.2, 711.2a, 711.2b.
Harvest (§5.4)
bug.
parse_level_blocks(oracle_level.rs) attempts static classification beforeactivated-colon detection, and
{R}: this creature gets +1/+0 until end of turn.matches a staticline, which consumes it with a
continue. The card yields zero abilities where all eightsibling levelers yield gated ones — and an unconsumed line would instead fall through and yield
an ungated ability, so the observed zero pins the static branch as the consumer. Net effect: a
repeatable
{R}-per-activation pump becomes a permanent static bonus (CR 711.2 + CR 602.1). Anylevel-block activated ability whose post-colon text parses as a static is affected.
Add {W}, {B}, {G}, or {C}isUnimplementedat bothstriations — a real parser gap for "add one of N colors" mana abilities.
duration_until_eot_drop_toweris a vacuous negative. Its only assertion is!has_swallowed_detector(…), which holds when the ability is never emitted at all — which isprecisely why inverting U0-50's emit guard is silent.
One limitation stated rather than papered over
U0-50's emit guard polarity is unwitnessed. A Drop Tower fixture was built, measured, and
discarded: modelling the card correctly —
Artifactplus its verbatimAttractionsubtype —routes the Visit line to the CR 717 preprocessor, and a re-inserted probe confirmed the site is never
reached. The two in-suite reachers hit U0-50 only because they supply no subtype, i.e. only because
they model the card wrongly. Shipping that fixture would have "closed" the gap with a test that passes
for the wrong reason.
For the record, this does not put the site's production reachability in doubt: U0-50 is the general
ability-word handler (its own comment cites "Void Shields —"), not an Attraction path, and 1,027
cards in the pool carry an ability-word prefix line. The unwitnessed axis is the guard's polarity,
not the site.
Summary by CodeRabbit
Bug Fixes
Tests