refactor(parser): Plan 05b T8-A1 — the first two producer conversions (U0-22, U0-26) by matthewevans · Pull Request #6715 · phase-rs/phase

matthewevans · 2026-07-28T01:08:02Z

Plan 05b tranche T8-A1 — the first two producer conversions through A0's ability shell.
Follows #6714 (A0), which built the mechanism this consumes.

Two sites, both recipe R1, both with no stages: U0-22 (CR 719.3c, activated Solved) and
U0-26 (CR 702.193a-b, Power-up).

Two commits, deliberately separable: the conversion touches only oracle.rs (+47/−26); the §5.3
remediation guard is purely additive.

The conversion

The identity is already in the tree, so this is a rewrite rather than a redesign:

parse_effect_chain_with_context(t,k,cx)  ==  lower_ability_ir(&parse_ability_ir_with_context(t,k,cx))
phase-A  ability_ir_at(l, ir)            ==  ability_at(l, lower_ability_ir(&ir))

Both sites' original entry point was parse_effect_chain_with_context(&effect_text, AbilityKind::Activated, &mut ctx), so both take parse_ability_ir_with_context — chosen by name
per §7.1, not by re-deriving the mode at the call site. That is the entire point of A0's mode-pinned
wrappers.

ability_ir_at's #[allow(dead_code)] is deleted, not moved — A0 assigned it to whichever
tranche added the first producer, and this is that tranche. ShellStage's own allow stays; A2 owns it.

Restriction order is preserved, and the two sites disagree on purpose

// U0-22 — implicit LEADS
let mut activation_restrictions = vec![ActivationRestriction::IsSolved];
activation_restrictions.extend(constraints.restrictions);

// U0-26 — parsed LEADS
let mut activation_restrictions = constraints.restrictions;
activation_restrictions.push(ActivationRestriction::OnlyOnce);

These are exact reverses of one another, and both reproduce their original push/extend sequence.
This is why A0 carries one ordered Vec composed by the site rather than separate
"implicit"/"parsed" fields — a split shell would silently normalize the asymmetry away. Each site now
carries a comment pointing at the other, so a future tidying refactor has to read the asymmetry before
unifying it.

U0-22's ParseContext reset stays at the call site, before the parse, untouched: it is a parser side
effect with different timing, not part of the CR 602.1 envelope.

U0-26 stamps cost_reduction explicitly from the keyword definition and leaves stages empty.
Listing ShellStage::ExtractCostReduction there would both overwrite the keyword-defined value and
strip a node out of the sub_ability chain. Commented in place.

§5.3 non-vacuity: two null results reported as findings, one of them remediated

A reachability probe ran first, because a green perturbation is ambiguous between "identical" and
"never executed".

Reachability (temporary panic! at both sites): U0-22 fires in zero of 17,844 --lib tests
but is reached by 2 integration tests; U0-26 by 1 --lib and 10 integration tests. The --lib
zero initially read as "wholly untested" and that reading was falsified by the integration run —
recorded here because the narrower true statement is the useful one.

Null result 1 — wrong ChainLoweringMode at both sites: no divergence. The two modes differ only
by parse_face_down_pile_ir / try_parse_exile_pile_shuffle_cloak, and no Solved or Power-up fixture
reaches either. The mode axis is extensionally inert at these two sites, so mode preservation here
rests entirely on the by-construction argument and §7.1's name-matching rule — not on any test. §5.3's
suggested probe ("watch full-pool regeneration go red on die-result cards") does not apply here; the
~113 die-roll cards §7.1 warns about are unreachable from these recognizers. A2/A3 should not expect a
red from it either, and should re-run the probe per-tranche rather than inheriting this null.

Null result 2 — deleting U0-22's implicit IsSolved: no divergence. That is a genuine semantic
change (a Solved ability becomes activatable while unsolved) and nothing caught it. Root cause,
measured: the only fixture reaching U0-22 is "Solved — {T}: Add {R}." — no trailing activation
instruction, so constraints.restrictions is empty, making the fixture degenerate on exactly the
axis this conversion touches. Its assertions target is_solved, not the activation envelope. The
sibling case_of_the_crimson_pulse fixture is a decoy: its Solved clause is triggered, so it never
passes find_activated_colon.

Perturbation that did go red — U0-26's cost_reduction blanked:

FAIL power_up_keyword::power_up_cost_reduced_by_source_mana_value_when_entered_this_turn
ActivateAbility must be accepted by the engine: ActionNotAllowed("Cannot pay mana cost")

The remediation (second commit)

A two-layer snapshot for Case of the Stashed Skeleton, whose Oracle text was taken verbatim from
the card pool:

Solved — {1}{B}, Sacrifice this Case: Search your library for a card, put it into your hand, then
shuffle. Activate only as a sorcery.

Chosen for being non-degenerate: the trailing activation instruction makes
constraints.restrictions non-empty, so the fixture reaches the two-element arm and pins both halves
and their order:

"activation_restrictions": [ { "type": "IsSolved" }, { "type": "AsSorcery" } ]

A card with only one restriction would witness that the field is populated while saying nothing about
sequencing — and sequencing is the property at risk. Watched go red on a pure order swap:

229  232 │   "type": "IsSolved"
230      │-  },
231      │-  { "type": "AsSorcery"
FAIL engine parser::oracle_ir::snapshot_tests::case_of_the_stashed_skeleton

It lives in oracle_ir/snapshot_tests.rs rather than tests/integration/ on purpose: as an inline
#[cfg(test)] src module it runs under --lib, which is what closes the measured reachability hole,
and it reuses the existing parse_two_layer harness next to its near-miss sibling. No new test binary
is created. 4 pool cards reach this recognizer (Case of the Burning Masks / Filched Falcon / Stashed
Skeleton / Uneaten Feast); 37 reach Power-up.

Gates

cargo fmt --all                                      clean
cargo clippy -p engine --lib -- -D warnings          zero warnings
cargo nextest run -p engine --lib                    17845 tests run: 17845 passed, 6 skipped
./scripts/check-prelowered-ratchet.sh                Gate P PASS
./scripts/check-skill-doc.sh                         ✓ oracle-parser skill references valid
./scripts/check-parser-combinators.sh                Gate G PASS / Gate A PASS

Snapshot churn: ZERO existing snapshots changed. git diff --name-status <base>..HEAD is 2 A /
2 M, both modified files are source; every snapshot in the diff is an add, for a card that had no
prior fixture. find . -name '*.snap.new' → empty.

oracle.rs stayed at 16 on the ratchet, which is correct and worth restating: all 16 occurrences
are consumer match arms, emitter method bodies and prose — none at producer call sites. The metric
tracks helper deletion, not producer conversion, so a tranche that converts producers can and does
leave it unmoved. Ledger not edited; no count rose.

Harvest (§5.4)

§5.3's own suggested probe does not work at these sites — see null result 1. Recommend the plan
qualify it as applicable only to sites whose effect text can reach the two WithContext-only
recognizers.
case_of_the_crimson_pulse is a decoy fixture — its name and Case/Solved shape imply coverage
of the Solved family, but it exercises only the triggered branch. Worth a note so nobody else
counts it as coverage for U0-22.
Pre-existing CR-fidelity gap at U0-26, not fixed. CR 702.193a reduces the cost by the
permanent's mana cost, and 702.193b splits colored/colorless from generic. The implementation
reduces generic by QuantityRef::SelfManaValue — the total mana value. For a {2}{R}{R}
source that is 4 generic rather than 2 generic + 2 red. Pre-existing and unchanged here; fixing it
inside a byte-identity tranche would contaminate the gate.
Pre-existing questionable citation at U0-26: // CR 702.193b + CR 602.2b + CR 601.2f + CR 302.6
— CR 302.6 is the summoning-sickness rule and does not describe a cost reduction. The number exists;
the text does not support the use. Left verbatim to keep the diff surgical.
Dead guard removed: U0-22's if !constraints.restrictions.is_empty() wrapped an extend, which
is already a no-op on an empty vec — provably behavior-identical.
Census rows 142 and 146 are accurate, and their needs-design label was pessimistic. Both were
mechanical under A0's shell; recipe R1 worked exactly as specified.

Known limitation

Full-pool card-data regeneration was not run locally — Tilt owns it and §5.0 says read it, don't run
it. Byte-identity here rests on the by-construction argument, zero snapshot churn, and 17,845 green
lib tests plus the integration binary. CI's card-data job is the empirical check.

CR numbers grep-verified against docs/MagicCompRules.txt before entering code, text checked rather
than existence alone: 602.1, 602.1a, 602.1b, 602.5d, 719.3c added; 702.193a, 702.193b, 602.2b, 601.2f,
302.6 verified as preserved context lines.

Summary by CodeRabbit

Bug Fixes
- Improved parsing of “Solved” and “Power-up” activated abilities.
- Ensured activation costs, restrictions, descriptions, ability tags, and cost reductions are applied consistently.
Tests
- Added coverage for a “Solved — {cost}: {effect}” ability pattern.
- Expanded snapshots to verify both intermediate parsing and final ability output.

… (U0-22, U0-26) The first consumers of A0's widened `AbilityShellIr`. Two recipe-R1 sites, both with **no stages**, converted from "lower, then stamp the lowered def" to "stamp the shell, then lower". * **U0-22** — CR 719.3c Case `"Solved — {cost}: {effect}"`. Composes its restriction vec implicit-first (`IsSolved`, then the parsed constraints), reproducing the site's original push-then-extend order. * **U0-26** — CR 702.193a-b Power-up. Composes its vec parsed-first (constraints, then `OnlyOnce`) — the **reverse** of U0-22. A0 deliberately carries ONE ordered `Vec` composed by the site rather than separate implicit/parsed fields precisely so this asymmetry survives conversion instead of being normalized. It also stamps `cost_reduction` explicitly from the keyword definition, so `shell.stages` stays EMPTY: `ShellStage::ExtractCostReduction` would overwrite that value AND strip a node out of the `sub_ability` chain. Both sites called `parse_effect_chain_with_context`, so both 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). U0-22's `ParseContext` reset stays at the call site: it is a parser side effect with parse-time ordering, not part of the CR 602.1 activation envelope. 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`, `parse_effect_chain_with_context`), and phase-A `ability_ir_at(l, ir)` IS `ability_at(l, lower_ability_ir(&ir))` (`oracle.rs`). The old site computed `stamp(lower_ability_ir(ir))`; the converted site computes `lower_ability_ir(ir_with_shell)`, and the shell reproduces `stamp` in the same order. `activation_restrictions` is applied with `extend`, so lowered-then-site ordering is preserved exactly as `push`/`extend` did. CR numbers grep-verified against docs/MagicCompRules.txt: 702.193a (:5423, "Activate this ability only once"), 702.193b (:5425, generic-mana reduction), 719.3c (:6086), 602.1a (:2516), 602.1b (:2519), 602.5d (:2549), 602.2b (:2531), 601.2f (:2468). `ability_ir_at`'s `#[allow(dead_code)]` is DELETED, not moved — A0 said this tranche retires it, and it now has a real producer. Gates: `Gate P PASS` (oracle.rs unmoved at 16 — the marker text lives in consumer arms and emitter bodies, not producer call sites), skill-doc `✓`, Gate G + Gate A PASS, `cargo clippy -p engine --lib -- -D warnings` clean, `cargo nextest run -p engine --lib` 17844 passed / 6 skipped, and **zero snapshot churn** (no `*.snap.new`; `git status` shows only `oracle.rs`).

… remediation) The §5.3 non-vacuity probe on T8-A1 measured a real gap, so this lands the missing guard rather than reporting a clean run. **What the probe measured.** With a temporary `panic!` at the CR 719.3c `"Solved — {cost}: {effect}"` recognizer, **zero of the 17844 `--lib` tests fired.** Only two tests in the whole repo reach it (`case_solve_condition`), and their fixture is `"Solved — {T}: Add {R}."` — no trailing activation instruction, so `strip_activated_constraints` yields an **empty** `constraints.restrictions` and the fixture is degenerate on exactly the axis T8-A1 touches. Consequences, both measured, not argued: * deleting the implicit `ActivationRestriction::IsSolved` entirely stayed **green** across every reaching test; * so did swapping the restriction order. The sibling `case_of_the_crimson_pulse` snapshot is a near-miss: its Solved clause is a *triggered* ability, so it never passes `find_activated_colon`. It passes unchanged under both perturbations, which is why it could not stand in. **The fixture.** Case of the Stashed Skeleton, verbatim Oracle text from MTGJSON `AtomicCards`. It is chosen for being NON-degenerate: the trailing "Activate only as a sorcery." makes `constraints.restrictions` non-empty, so the snapshot pins both halves of the vector **and their order** — implicit `IsSolved` first (CR 719.3c, MagicCompRules.txt:6086), parsed `AsSorcery` second (CR 602.5d, :2549). That order is the one property T8's shell conversion could silently normalize, because the Power-up recognizer composes the same vector in the opposite order and a "tidying" refactor would unify them. **Watched red, not assumed.** Under a pure order swap the new snapshot fails with `"IsSolved"` relocated after `"AsSorcery"`; under the drop it fails with `"IsSolved"` absent. A gate never watched go red is not a gate. This also closes the `--lib` reachability hole: the recognizer now has coverage in the default lib run, not only in the integration binary. Additive only — two NEW `*.snap` files, **no existing snapshot changed**, so the tranche's zero-churn property is preserved. 4 real cards reach this recognizer today (Case of the Burning Masks, Filched Falcon, Stashed Skeleton, Uneaten Feast).

coderabbitai · 2026-07-28T01:08:27Z

📝 Walkthrough

Walkthrough

Oracle activated-ability handlers now build and emit AbilityIr shells for Solved and Power-up patterns. Power-up abilities explicitly record cost reduction metadata. A new snapshot test covers the Solved recognition path and both IR output layers.

Changes

Oracle ability IR routing

Layer / File(s)	Summary
Activated ability IR routing `crates/engine/src/parser/oracle.rs`	Solved and Power-up handlers now use `parse_ability_ir_with_context`, populate shell fields, and emit through `ability_ir_at`; Power-up also stamps its cost-reduction rule.
Solved ability snapshot coverage `crates/engine/src/parser/oracle_ir/snapshot_tests.rs`	Adds coverage for the stashed skeleton’s Solved activated-ability shape, snapshotting IR and lowered outputs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SolvedHandler
  participant PowerUpHandler
  participant AbilityIRParser
  participant DocEmitter
  participant ParsedAbilities
  SolvedHandler->>AbilityIRParser: Parse effect into AbilityIr
  PowerUpHandler->>AbilityIRParser: Parse effect into AbilityIr
  AbilityIRParser-->>SolvedHandler: Return IR
  AbilityIRParser-->>PowerUpHandler: Return IR
  SolvedHandler->>DocEmitter: Emit shell fields
  PowerUpHandler->>DocEmitter: Emit shell fields and cost reduction
  DocEmitter->>ParsedAbilities: Lower and emit ability

Possibly related PRs

phase-rs/phase#6714: Updates related Oracle ability parsing and AbilityIr emission paths.

Suggested labels: quality

Suggested reviewers: lgray, ntindle

🚥 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 matches the main change: refactoring the first two Oracle producer conversions for U0-22 and U0-26.
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-t8a1

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

coderabbitai

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.rs`:
- Around line 4636-4651: Replace the Power-up assignment to
ir.shell.cost_reduction with the shared mana-cost modification model so the
source permanent’s full mana cost, including colored and colorless symbols, is
reduced and resolved centrally. Preserve the SourceEnteredThisTurn condition and
avoid routing through ShellStage::ExtractCostReduction if that would alter the
sub_ability chain. Update the annotation to remove CR 302.6, retaining only
citations whose rule text defines the cost reduction.

🪄 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: 1fc9eb8b-8969-4b7f-a33b-2f270a518cf1

📥 Commits

Reviewing files that changed from the base of the PR and between 743da6d and 54e7b02.

⛔ Files ignored due to path filters (2)

crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.snap is excluded by !**/*.snap, !**/snapshots/**
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_lowered.snap is excluded by !**/*.snap, !**/snapshots/**

📒 Files selected for processing (2)

crates/engine/src/parser/oracle.rs
crates/engine/src/parser/oracle_ir/snapshot_tests.rs

coderabbitai · 2026-07-28T01:13:17Z

                // CR 702.193b + CR 602.2b + CR 601.2f + CR 302.6: the activation cost's
                // generic mana is reduced by the source's mana value if it entered this turn.
-                def.cost_reduction = Some(CostReduction {
+                //
+                // Stamped explicitly from the keyword definition, which is why
+                // `shell.stages` stays EMPTY here: this is the one site in the
+                // family that does not derive the reduction from the chain, and
+                // `ShellStage::ExtractCostReduction` would both overwrite this
+                // value and strip a node out of the `sub_ability` chain.
+                ir.shell.cost_reduction = Some(CostReduction {
                    mode: crate::types::statics::CostModifyMode::Reduce,
                    amount_per: 1,
                    count: QuantityExpr::Ref {
                        qty: QuantityRef::SelfManaValue,
                    },
                    condition: Some(ParsedCondition::SourceEnteredThisTurn),
                });


🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Model Power-up reduction as a mana-cost reduction, not a mana-value generic reduction.

CostReduction is generic-mana-only, but Power-up reduces the activation cost by the permanent’s mana cost. A {1}{R} permanent must reduce {1} generic and {R}, whereas SelfManaValue records a generic reduction of 2; colored/colorless components and excess handling are therefore wrong. Reuse or extend the shared cost-modification model so it preserves mana symbols and is resolved centrally. Also remove CR 302.6 from this annotation— that rule is about summoning sickness, not Power-up cost reduction. (media.wizards.com)

As per path instructions, engine rules implementations must faithfully encode the Comprehensive Rules and use citations whose rule bodies describe the code.

🤖 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.rs` around lines 4636 - 4651, Replace the Power-up assignment to ir.shell.cost_reduction with the shared mana-cost modification model so the source permanent’s full mana cost, including colored and colorless symbols, is reduced and resolved centrally. Preserve the SourceEnteredThisTurn condition and avoid routing through ShellStage::ExtractCostReduction if that would alter the sub_ability chain. Update the annotation to remove CR 302.6, retaining only citations whose rule text defines the cost reduction.

Source: Path instructions

github-actions · 2026-07-28T01:23:06Z

Parse changes introduced by this PR

✓ No card-parse changes detected.

matthewevans added 2 commits July 27, 2026 18:06

matthewevans enabled auto-merge July 28, 2026 01:08

coderabbitai Bot reviewed Jul 28, 2026

View reviewed changes

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

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

matthewevans deleted the ship/plan05b-t8a1 branch July 28, 2026 01:38

matthewevans mentioned this pull request Jul 28, 2026

refactor(parser): Plan 05b T8-A2 — the four R1 sites that run both extract_* stages #6716

Merged

This was referenced Jul 28, 2026

refactor(parser): Plan 05b T8-A3 — the R1 remainder (U0-50, U0-12) #6719

Merged

refactor(parser): Plan 05b T8-A4 — U0-53, the one R2 row under the cap #6720

Merged

fix(parser): Plan 05b T9a — route U0-39 through lower_ability_ir #6722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(parser): Plan 05b T8-A1 — the first two producer conversions (U0-22, U0-26) - #6715

refactor(parser): Plan 05b T8-A1 — the first two producer conversions (U0-22, U0-26)#6715
matthewevans merged 2 commits into
mainfrom
ship/plan05b-t8a1

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!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 28, 2026

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!

The conversion

Restriction order is preserved, and the two sites disagree on purpose

§5.3 non-vacuity: two null results reported as findings, one of them remediated

The remediation (second commit)

Gates

Harvest (§5.4)

Known limitation

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!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 28, 2026

Choose a reason for hiding this comment

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

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading