refactor(parser): Plan 05b T8-A2 — the four R1 sites that run both extract_* stages - #6716
Merged
Conversation
…tract_* stages (U0-23/24/25/27)
`ShellStage`'s first consumers. Four recipe-R1 activated-ability recognizers
converted from "lower, then stamp the lowered def" to "stamp the shell, then
lower", each carrying a **two-element ordered `stages` vec**.
* **U0-23** — Channel (CR 207.2c ability word + CR 602.1). Stamps
`activation_zone = Hand`; its restriction vec is the parsed constraints alone.
* **U0-24** — Boast (CR 702.142a-b). Parsed constraints LEAD, then
`OnlyOnceEachTurn`, then `RequiresCondition{SourceAttackedThisTurn}`.
* **U0-25** — Exhaust (CR 702.177a). Parsed constraints LEAD, then `OnlyOnce`.
* **U0-27** — Forecast (CR 702.57a-b). Stamps `activation_zone = Hand`; parsed
constraints LEAD, then `DuringYourUpkeep`, then `OnlyOnceEachTurn`.
Every vec is composed in its own site's original sequence and applied verbatim.
The four are deliberately NOT unified behind a shared builder: they differ in
zone, tag, and restriction set *and order*, and a shared builder would normalize
away exactly the asymmetry A0's ordered-`Vec` design exists to protect. Note
U0-24 pushes its two implicit restrictions in the REVERSE of the order CR
702.142a states them ("attacked this turn" then "only once each turn"); that is
a pre-existing property of the site, preserved here rather than quietly fixed.
**U0-23's `=` is equivalent to the shell's `extend`, verified not assumed.** Its
original wrote `if !constraints.restrictions.is_empty() { def.activation_restrictions
= constraints.restrictions }`. Nothing reachable from `lower_ability_ir` writes the
root's `activation_restrictions` — `rg activation_restrictions
crates/engine/src/parser/oracle_effect/` returns 3 hits, all inside
`apply_ability_shell_envelope` itself — so the field is empty when the shell runs
and `extend` onto empty reproduces the assignment exactly. The is-empty guard was
already redundant: assigning an empty vec and skipping the assignment are the
same state.
All four called `parse_effect_chain_with_context`, so all four take the
`parse_ability_ir_with_context` wrapper. The mode is inherited by NAME, not by
judgment — crossing the wrappers silently moves ~113 die-roll cards (§7.1).
Verified per site, not assumed uniform.
All four list `[ExtractCostReduction, ExtractManaSpendTrigger]`, matching the
call order each site ran the two folds in. `lower_ability_ir` runs stages last,
after the envelope stamps, which is the pinned order the sites had by hand.
CR-faithfulness: **by construction**, for the whole class of text each 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`). The old site computed
`stamp_then_fold(lower_ability_ir(ir))`; the converted site computes
`lower_ability_ir(ir_with_shell)`, and the shell reproduces the stamps and then
the folds in the same order.
CR numbers grep-verified against docs/MagicCompRules.txt, text read not just
number matched: 207.2c (:1497, ability-word list — "channel" is in it; boast,
exhaust and forecast are NOT, consistent with them being keywords), 602.1
(:2514), 602.1a (:2516), 602.1b (:2519), 702.142a (:5044), 702.142b (:5046),
702.177a (:5321), 702.57a (:4442), 702.57b (:4444), 601.2f (:2468), 106.6
(:425), 603.3 (:2582).
`ShellStage`'s `#[allow(dead_code)]` is DELETED, not moved — A0 assigned it to
whichever tranche added the first producer, and both variants now have four.
Gates: `Gate P PASS` (oracle.rs unmoved at 16 — the marker text lives in consumer
arms and emitter bodies, never at producer call sites; ledger unedited),
skill-doc `✓`, Gate G + Gate A PASS, `cargo clippy -p engine --lib -D warnings`
clean, `cargo nextest run -p engine --lib` 17845 passed / 6 skipped, integration
4123 passed / 2 skipped, and **zero snapshot churn** (no `*.snap.new`;
`git status` shows only the two source files).
…t the four keyword recognizers (§5.3 remediation)
The §5.3 non-vacuity probe on T8-A2 measured a real gap, so this lands the
missing guards rather than reporting a clean run.
**What the probe measured.** Unlike A1's site, all four A2 recognizers ARE
reached by `--lib` tests (34 tests total across `--lib` and the integration
binary, mapped by temporary `panic!`). But their restriction *order* was
unwitnessed. With the conversion in place:
* swapping parsed constraints against implicit ones at Boast/Exhaust/Forecast —
**green** on all 34;
* swapping the two implicit restrictions against each other at Boast and
Forecast — **green** on all 34.
Cause: every pre-existing fixture is degenerate on this axis. The reminder text
that states the restrictions ("(Activate only if this creature attacked this
turn and only once each turn.)") is stripped before `strip_activated_constraints`
runs, so `constraints.restrictions` is EMPTY, and the `boast_*` / `exhaust_*` /
`forecast_*` parser tests assert with order-insensitive `.contains(..)`.
**The fixtures.** Four real pool cards, verbatim Oracle text from
`data/card-data.json`, each closing a specific measured hole:
* **Ghost-Lit Stalker** (Channel) — its "Activate only as a sorcery."
(CR 602.5d) is outside the reminder parens, so `constraints.restrictions` is
non-empty. Channel is the site whose vector is the parsed constraints ALONE
and whose original wrote `=`; no existing fixture could observe that vector
arriving at the lowered def at all.
* **Liliana the Repentant** (Exhaust) — the only Exhaust card in the pool whose
activation instruction sits outside the reminder parens. Pins parsed
`AsSorcery` first, implicit `OnlyOnce` (CR 702.177a) second.
* **Arni Brokenbrow** (Boast) — pins `OnlyOnceEachTurn` before
`RequiresCondition{SourceAttackedThisTurn}`, which is the REVERSE of the order
CR 702.142a states them in. Pre-existing, preserved, now pinned.
* **Govern the Guildless** (Forecast) — pins `DuringYourUpkeep` before
`OnlyOnceEachTurn` (CR 702.57b's own order). Also Forecast's first two-layer
snapshot coverage of any kind.
**Watched red, not assumed.** Each fixture was watched fail under its own
perturbation before being committed: Liliana relocates `OnlyOnce` ahead of
`AsSorcery` under an implicit-first swap; Arni and Govern transpose their two
implicit restrictions; Ghost-Lit Stalker loses `AsSorcery` entirely when the
`activation_restrictions` assignment is deleted. In each case the other three
fixtures stayed green, so each red is attributable to its own site.
**Two axes remain honestly unwitnessed and are reported, not papered over:**
1. No Boast or Forecast card in the pool states its activation instruction
outside reminder text, so the parsed-vs-implicit axis at those two sites is
unwitnessable by any real card. Inventing a card to cover it would violate
the verbatim-Oracle-text rule, so it stays a known gap.
2. `ShellStage::ExtractManaSpendTrigger` is dead at all four sites on today's
pool — dropping it entirely is silent across all 34 reaching tests, and a
pool scan finds no A2-site line whose root effect is `Effect::Mana`. It is
retained because it reproduces the pre-conversion call sequence exactly; the
conversion must not quietly drop a fold. Consequently the stage *reorder* is
inert too, since only one of the two stages ever fires.
`ShellStage::ExtractCostReduction` IS witnessed, at Channel only, by the
pre-existing `boseiju_who_endures` snapshot: dropping it leaves the
"costs {1} less to activate" node unfolded in `sub_ability` as
`Effect::Unimplemented` and `cost_reduction` absent.
Additive only — eight NEW `*.snap` files, **no existing snapshot changed**, so
the tranche's zero-churn property is preserved.
matthewevans
enabled auto-merge
July 28, 2026 02:21
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 (8)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChannel, Boast, Exhaust, and Forecast handlers now use shell-based ability IR parsing with staged extraction. New snapshots cover activation-restriction ordering for four card fixtures. ChangesActivated keyword parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OracleHandlers
participant AbilityParser
participant AbilityIrShell
participant SnapshotTests
OracleHandlers->>AbilityParser: parse activated keyword with context
AbilityParser->>AbilityIrShell: populate cost, description, restrictions, zone, and tag
AbilityIrShell->>AbilityIrShell: apply staged cost and mana-trigger extraction
SnapshotTests->>AbilityParser: parse four card fixtures
AbilityParser-->>SnapshotTests: return IR and lowered snapshots
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
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 tranche T8-A2 — the four R1 sites that run both
extract_*chain-structure folds:U0-23 Channel · U0-24 Boast · U0-25 Exhaust · U0-27 Forecast.
Follows #6715 (A1). This is the first tranche to consume
ShellStage, so it deletes that type's#[allow(dead_code)]— A0 assigned the allow to whichever tranche added the first producer.Two commits: the conversion, and a §5.3 remediation that pins restriction order at all four sites.
The conversion
All four originals were read individually rather than assumed uniform, and all four turned out
verbatim identical in entry point —
parse_effect_chain_with_context(&effect_text, AbilityKind::Activated, &mut ctx)— so all four takeparse_ability_ir_with_context, chosen byname per §7.1.
activation_zone = Hand[parsed]— no implicit at all[ExtractCostReduction, ExtractManaSpendTrigger]ability_tag = Boast[parsed…, OnlyOnceEachTurn, RequiresCondition{SourceAttackedThisTurn}]ability_tag = Exhaust[parsed…, OnlyOnce]activation_zone = Hand[parsed…, DuringYourUpkeep, OnlyOnceEachTurn]Each order was read from that site's current code. The four are not unified behind a shared
builder: they differ in zone, tag, restriction set and restriction order, and a shared builder would
normalize away exactly the asymmetry A0's ordered-
Vecdesign exists to protect.U0-23's
=-not-extendwas verified, not assumed.activation_restrictionsappears incrates/engine/src/parser/oracle_effect/exactly three times, all insideapply_ability_shell_envelopeitself, so the root field is provably empty when the shell runs andextend-onto-empty reproduces the assignment.§5.3: reachability first, then perturbation
A reachability probe ran before any perturbation, because a green perturbation is otherwise ambiguous
between "identical" and "never executed" — the ambiguity that misled A1's first reading.
--libNo
--libzeroes this time. A decoy was caught by probe message rather than by name:power_up_keyword::wonder_man_does_not_raise_non_power_up_once_limitreaches the Exhaust site.ExtractCostReduction, all 4boseiju_who_endures)ExtractManaSpendTrigger, all 4StandaloneThe
ExtractCostReductionred is behavioral rather than serialization — phase A emitsPreLoweredSpell(lower_ability_ir(&ir)), sostagesnever reaches a snapshot:That red is also what makes the stage-reorder green a real inertness measurement rather than an
untested path.
Why the order perturbations were silent: every pre-existing fixture at all four sites is degenerate
on the restriction-order axis. The reminder text stating the restrictions is stripped before
strip_activated_constraints, soconstraints.restrictionsis empty, and theboast_*/exhaust_*/forecast_*assertions use order-insensitive.contains(..). Same defect class A1 found at U0-22 —and it now looks general across the whole keyword-labelled activated family.
Remediation (second commit)
Four real pool cards, Oracle text verbatim from the card pool, each watched go red with the other
three staying green so every red is attributable to its own site:
AsSorceryblockOnlyOncerelocating ahead ofAsSorceryOnlyOnceEachTurntransposing pastRequiresConditionDuringYourUpkeep/OnlyOnceEachTurntransposingGhost-Lit Stalker and Liliana are the genuinely non-degenerate pair: their "Activate only as a
sorcery." sits outside the reminder parens, yielding
[AsSorcery]and[AsSorcery, OnlyOnce]inthe lowered snapshots. Arni and Govern state everything inside reminder text, so they pin
implicit-vs-implicit order only — which is stated plainly rather than papered over.
Gates
Zero existing snapshots changed.
git diff --name-statusis 8 A / 3 M; the three modifiedfiles are all source.
find . -name '*.snap.new'→ 0.oracle.rsunmoved at 16 on the ratchet;scripts/prelowered-ratchet.txtunedited.Parser string-dispatch diff gate: clean — zero
.contains/starts_with/find/rfind/split*inadded non-comment lines under
parser/.CR-annotation diff gate: 11 numbers added, zero unverified, each grep-checked against
docs/MagicCompRules.txtwith the rule text read rather than existence alone — 106.6, 207.2c,601.2f, 602.1a, 602.1b, 602.5d, 603.3, 702.57a, 702.57b, 702.142a, 702.177a. Confirms
channelisin CR 207.2c's ability-word list while boast/exhaust/forecast are not, consistent with those three
being keywords.
Harvest (§5.4)
ShellStage::ExtractManaSpendTriggeris dead at all four sites on today's pool. Dropping it issilent across all 34 reaching tests, and a scan of all 35,516 cards finds zero A2-site lines
whose root effect is
Effect::Mana(Careful Cultivation's "Add {G}" is inside a token's grantedability, not the root). Retained anyway — it reproduces the pre-conversion call sequence, and a
conversion must not quietly drop a fold. This is also why the stage reorder is inert: only one
stage ever fires.
ExtractCostReductionfires only at Channel — the Kamigawa legendary land cycle (Boseiju,Eiganjo, Otawara, Sokenzan, Takenuma). Boast/Exhaust/Forecast run a fold that cannot fire for them.
The rule reads "Activate only if this creature attacked this turn and only once each turn"; the code
pushes
OnlyOnceEachTurnfirst. Semantically harmless — the restrictions are a conjunction — butpre-existing and now pinned by
arni_brokenbrow. Preserved, not fixed.if !…is_empty()guard was already redundant — assigning an empty vec ≡ skipping it.class as A1's U0-22 finding; it generalizes across the keyword-labelled activated family.
was 2 parser tests.
ChainLoweringModeis extensionally inert at all four sites, matching A1's null.Two axes left honestly unwitnessed
Reported rather than designed around:
so parsed-vs-implicit order at U0-24/U0-27 cannot be witnessed by any real card. Inventing one would
violate the verbatim-Oracle-text rule. Documented in the test file and commit message.
ExtractManaSpendTriggercannot be witnessed at any A2 site (harvest chore: update coverage stats and badges #1), so its orderingrelative to
ExtractCostReductionis unwitnessed. The stage-order property is preserved byconstruction but not test-guarded: if a later tranche reorders
stages, nothing goes red today.Summary by CodeRabbit
Bug Fixes
Tests