refactor(parser): Plan 05b T8-A0 — the ability shell widening (infrastructure only) - #6714
Merged
Conversation
…tructure only)
Builds the mechanism every later T8 sub-tranche stands on. Converts ZERO
producers: no call site changes, so the tranche is byte-identical BY
CONSTRUCTION and shows zero snapshot churn.
What lands:
* `AbilityShellIr` widens from `{ sub_link }` to the CR 602.1 activation
envelope — 10 of `AbilityDefinition`'s 38 root fields, deliberately not a
mirror of the root. CR 602.1 (MagicCompRules.txt:2514) draws exactly this
seam: cost (CR 602.1a, :2516) / activation instructions (CR 602.1b, :2519) /
effect, with CR 113.3b (:761) repeating it for abilities generally and
CR 601.2b (:2459) grounding `min_x_value`. `EffectChainIr` keeps the CR 608.2
(:2785) resolution instructions. The root-vs-clause axis therefore follows a
seam the rules already draw rather than straddling rule sections.
* `ShellStage`, an ORDERED `Vec` — not a set of flags. Both stages are chain-
STRUCTURE folds that also write root fields, so their position relative to the
field stamps is behavior-load-bearing: `ExtractCostReduction` writes
`cost_reduction` (so it must not run where a site stamped it explicitly — the
Power-up recognizer does), and `ExtractManaSpendTrigger` is a no-op unless the
lowered root effect is already `Effect::Mana`, so it is meaningful only
post-lowering.
* `lower_ability_ir` gains the envelope applier and the stage loop, in the
pinned order chain -> finalize -> anchor -> sub_link -> stamps -> stages,
reproducing what every family-A recognizer does by hand.
* `parse_ability_ir` published as `pub(crate)` plus the two mode-pinned wrappers
`parse_ability_ir_standalone` / `parse_ability_ir_with_context`, whose
argument lists match the entry points they replace so a converted site
inherits its `ChainLoweringMode` mechanically instead of by reviewer judgment.
* `DocEmitter::ability_ir_at`, which lowers eagerly and still emits the
pre-lowered node, so producers convert tranche by tranche with the node
payload, `finish()`, `item_ability` and the mutators all untouched.
Applier semantics are defer-on-default throughout, which is what makes the
widening inert until a producer opts in: Option fields override only when
`Some`, `activation_restrictions` uses `extend` (never `=`),
`min_x_value` uses `max`, `cant_be_copied` is a monotone OR.
`extend` is verified, not assumed: `rg activation_restrictions
crates/engine/src/parser/oracle_effect/` returns zero hits, and that directory
holds the whole of `lower_ability_ir` — `lower_effect_chain_ir`,
`finalize_effect_chain`, the owner-library anchor, and all five whole-body
bypasses `parse_ability_ir` dispatches to. Every reachable write lands on a
NESTED granted/static definition inside an `Effect` payload, never on the root
the shell stamps. So the sites whose original was `= constraints` provably start
from empty and `extend` reproduces them.
`ability_ir_at` delegates to `ability_at` rather than emitting its own node.
That states the phase-A identity literally and keeps the textual burn-down
ratchet monotone — `oracle.rs` sits exactly at its ceiling, and the ratchet
counts the token in prose as well as in code.
CR 707.9a: no gap opens here. Stamping runs in `finish()` before lowering, so an
IR-native `Spell` body cannot be stamped — but phase A emits the pre-lowered
variant, so every converted producer is still stamped by
`stamp_retained_printed_slot`, which recurses the whole definition. The gap
becomes reachable only when the emitter switches payloads in T9, which is also
the only place the stamp can move without a byte delta on the one pre-existing
`Spell` producer.
Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS with
per-file counts unchanged (no ledger edit); check-skill-doc PASS; 399/399
snapshot tests pass with zero `.snap.new`.
matthewevans
enabled auto-merge
July 27, 2026 23:59
Contributor
📝 WalkthroughWalkthroughThe PR defines serialized ability-shell fields and ordered lowering stages, centralizes shell-envelope application, adds mode-specific parsing wrappers, and exposes helpers for IR-based emission and mana-spend trigger extraction. ChangesAbility lowering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AbilityIr
participant lower_ability_ir
participant AbilityDefinition
AbilityIr->>lower_ability_ir: lower ability chain
lower_ability_ir->>AbilityDefinition: apply shell envelope
lower_ability_ir->>AbilityDefinition: extract cost reduction
lower_ability_ir->>AbilityDefinition: extract mana-spend trigger
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_effect/mod.rs`:
- Around line 26600-26612: In the lowering flow around
apply_ability_shell_envelope and the ShellStage::ExtractCostReduction branch,
add a debug_assert! that rejects configurations where
AbilityShellIr::cost_reduction is explicitly set while the shell stages also
include ExtractCostReduction. Place the guard before extraction can overwrite
the stamped value, while preserving normal lowering when only one source is
present.
In `@crates/engine/src/parser/oracle_ir/effect_chain.rs`:
- Around line 126-156: Correct the CR citations in the `EffectChainIr` partition
table and corresponding per-field documentation: classify `cost_reduction` under
CR 601.2f rather than CR 602.1a, and cite `min_x_value` with both CR 601.2b and
CR 602.2b wherever its applicability is documented. Keep the runtime
implementation unchanged.
- Around line 220-231: Correct the rule citation in the documentation for the
EffectChain `activation_zone` field from CR 113.6m to CR 113.6b, keeping the
existing explanation of printed-zone declarations such as Channel and Forecast
unchanged.
🪄 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: 7c39179c-94b4-4ef2-aa9c-356541d37fb7
📒 Files selected for processing (3)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/effect_chain.rs
Comment on lines
+26600
to
26612
| apply_ability_shell_envelope(&mut def, &ir.shell); | ||
| for stage in &ir.shell.stages { | ||
| match stage { | ||
| ShellStage::ExtractCostReduction => { | ||
| crate::parser::oracle::extract_cost_reduction_from_chain(&mut def); | ||
| } | ||
| ShellStage::ExtractManaSpendTrigger => { | ||
| crate::parser::oracle::extract_mana_spend_trigger_from_chain(&mut def); | ||
| } | ||
| } | ||
| } | ||
| def | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unenforced invariant: explicit cost_reduction stamp + ExtractCostReduction stage can silently collide.
The docs on both AbilityShellIr::cost_reduction and ShellStage::ExtractCostReduction state a site must never set both, because the stage would silently overwrite an explicitly-stamped reduction with whatever it finds in the chain. Nothing in lower_ability_ir/apply_ability_shell_envelope enforces this — it's discipline-only. Currently unreachable (only AbilityShellIr::default() is ever constructed), but this is exactly the kind of latent trap that becomes a real silent-data-loss bug the moment a T8-A2 site mistakenly sets both.
A cheap debug_assert! at the point where both interact would convert a silent-overwrite bug into an immediate, loud failure in dev/test builds.
🛡️ Proposed guard
apply_ability_shell_envelope(&mut def, &ir.shell);
for stage in &ir.shell.stages {
match stage {
ShellStage::ExtractCostReduction => {
+ debug_assert!(
+ ir.shell.cost_reduction.is_none(),
+ "ExtractCostReduction stage must not run when the shell explicitly stamps cost_reduction"
+ );
crate::parser::oracle::extract_cost_reduction_from_chain(&mut def);
}Also applies to: 26622-26657
🤖 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_effect/mod.rs` around lines 26600 - 26612, In
the lowering flow around apply_ability_shell_envelope and the
ShellStage::ExtractCostReduction branch, add a debug_assert! that rejects
configurations where AbilityShellIr::cost_reduction is explicitly set while the
shell stages also include ExtractCostReduction. Place the guard before
extraction can overwrite the stamped value, while preserving normal lowering
when only one source is present.
Comment on lines
+126
to
+156
| /// # The partition is the rules' own, not an engineering convenience | ||
| /// | ||
| /// CR 602.1 (`MagicCompRules.txt:2514`) — *"Activated abilities have a cost and | ||
| /// an effect. They are written as `[Cost]: [Effect.] [Activation instructions | ||
| /// (if any).]`"* — draws exactly the seam this type sits on, and CR 113.3b | ||
| /// (:761) repeats the tripartite form for abilities generally. So: | ||
| /// | ||
| /// | shell field group | CR | | ||
| /// |---|---| | ||
| /// | `cost`, `cost_reduction` | CR 602.1a — everything before the colon (:2516) | | ||
| /// | `activation_restrictions`, `activation_mana_payment_restriction`, `activator_filter`, `activation_zone` | CR 602.1b — activation instructions, *"not part of the ability's effect"* (:2519) | | ||
| /// | `min_x_value` | CR 601.2b — the announced value of a variable cost (:2459) | | ||
| /// | `ability_tag`, `cant_be_copied`, `description` | ability-level identity/provenance, not resolution steps | | ||
| /// | ||
| /// while `EffectChainIr` holds the CR 608.2 (:2785) resolution instructions. | ||
| /// Because the root-vs-clause axis follows a seam CR 602.1 already draws, the | ||
| /// widening satisfies the categorical-boundary rule rather than straddling rule | ||
| /// sections. | ||
| /// | ||
| /// **This is 10 of `AbilityDefinition`'s 38 root fields, deliberately not a | ||
| /// mirror of the root.** Fields excluded on purpose — `effect`, `sub_ability`, | ||
| /// `else_ability`, `condition` — are all CR 608.2 resolution tree and are | ||
| /// already expressible as `ClauseIr`/`ClauseDisposition`. A shell that mirrored | ||
| /// the root would re-open the escape hatch this type exists to close. | ||
| /// | ||
| /// # Applier semantics (see `lower_ability_ir`) | ||
| /// | ||
| /// Every field is **defer-on-default**: an unset field leaves whatever lowering | ||
| /// produced, so a `default()` shell is exactly today's behavior. That is what | ||
| /// makes the widening byte-identical by construction — see the per-field docs | ||
| /// for the one-line rule each obeys. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor CR-citation gaps between the summary table and the per-field docs.
Two small inconsistencies in the CR mapping:
- The table (line 135) groups
cost_reductionunder CR 602.1a ("everything before the colon"), but the field's own doc (line 180) correctly cites CR 601.2f (cost reduction is determined while computing the total cost, not part of the literal pre-colon cost text). The table oversimplifies and disagrees with the more precise field-level citation. min_x_value(line 137, line 239) cites only CR 601.2b, which governs the announcement step for casting a spell. This same shell field is shared byAbilityKind::Activated(as evidenced byactivation_restrictions/activation_zonebeing cited under CR 602.1b specifically for the activated-ability half of the type). CR 602.2b is what actually extends 601.2b's announcement steps to activated abilities, so citing 601.2b alone is incomplete for half of this field's stated applicability.
Neither changes runtime behavior, but this doc is the reference future tranches (T8-A2/T9) will read when wiring producers, so a precise/compound citation (e.g. CR 601.2b + CR 602.2b) would avoid propagating the gap.
Also applies to: 175-247
🤖 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_ir/effect_chain.rs` around lines 126 - 156,
Correct the CR citations in the `EffectChainIr` partition table and
corresponding per-field documentation: classify `cost_reduction` under CR 601.2f
rather than CR 602.1a, and cite `min_x_value` with both CR 601.2b and CR 602.2b
wherever its applicability is documented. Keep the runtime implementation
unchanged.
Source: Path instructions
Comment on lines
+220
to
+231
| /// CR 113.6m: the zone the ability functions in. `Some(_)` overrides; | ||
| /// `None` defers. | ||
| /// | ||
| /// Note for later tranches: the generic activated-ability recognizer derives | ||
| /// this field by *reading the lowered def* | ||
| /// (`activation_zone_from_self_cost` / `activation_zone_from_self_effect`), | ||
| /// which a shell stamped before lowering cannot express. That is one of the | ||
| /// reasons `parse_activated_ability_definition` is scoped to its own unit | ||
| /// rather than to T8 — this field is here for the recognizers that know | ||
| /// their zone from the printed keyword (Channel, Forecast), not for that one. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub(crate) activation_zone: Option<Zone>, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CR 113.6m is the wrong rule for activation_zone.
CR 113.6m governs the narrow "ability whose effect moves the object out of a zone functions only in that zone" case (the Reassembling Skeleton example — self-referential zone-exit). The generic "an ability that states which zones it functions in" rule this field actually needs is CR 113.6b: "An ability that states which zones it functions in functions only from those zones." The field's own doc names Channel/Forecast as the target use case — both are printed-zone declarations, i.e. textbook CR 113.6b, not 113.6m.
🐛 Proposed fix
- /// CR 113.6m: the zone the ability functions in. `Some(_)` overrides;
+ /// CR 113.6b: the zone the ability functions in. `Some(_)` overrides;
/// `None` defers.As per path instructions, "rules-touching code with no verified CR annotation, or a CR citation whose rule body does not describe the code" is a flagged finding for crates/engine/**.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// CR 113.6m: the zone the ability functions in. `Some(_)` overrides; | |
| /// `None` defers. | |
| /// | |
| /// Note for later tranches: the generic activated-ability recognizer derives | |
| /// this field by *reading the lowered def* | |
| /// (`activation_zone_from_self_cost` / `activation_zone_from_self_effect`), | |
| /// which a shell stamped before lowering cannot express. That is one of the | |
| /// reasons `parse_activated_ability_definition` is scoped to its own unit | |
| /// rather than to T8 — this field is here for the recognizers that know | |
| /// their zone from the printed keyword (Channel, Forecast), not for that one. | |
| #[serde(default, skip_serializing_if = "Option::is_none")] | |
| pub(crate) activation_zone: Option<Zone>, | |
| /// CR 113.6b: the zone the ability functions in. `Some(_)` overrides; | |
| /// `None` defers. | |
| /// | |
| /// Note for later tranches: the generic activated-ability recognizer derives | |
| /// this field by *reading the lowered def* | |
| /// (`activation_zone_from_self_cost` / `activation_zone_from_self_effect`), | |
| /// which a shell stamped before lowering cannot express. That is one of the | |
| /// reasons `parse_activated_ability_definition` is scoped to its own unit | |
| /// rather than to T8 — this field is here for the recognizers that know | |
| /// their zone from the printed keyword (Channel, Forecast), not for that one. | |
| #[serde(default, skip_serializing_if = "Option::is_none")] | |
| pub(crate) activation_zone: Option<Zone>, |
🤖 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_ir/effect_chain.rs` around lines 220 - 231,
Correct the rule citation in the documentation for the EffectChain
`activation_zone` field from CR 113.6m to CR 113.6b, keeping the existing
explanation of printed-zone declarations such as Channel and Forecast unchanged.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 28, 2026
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-A0 — the ability shell widening. Infrastructure only: zero producers
converted, zero call sites changed.
Follows #6709 (T5). This is the foundation the rest of T8 stands on: it builds the mechanism that
makes A1–A4's recognizer conversions mechanical rather than a per-site judgment call.
What landed
AbilityShellIrwidened from{ sub_link }to the CR 602.1 activation envelope — 10 ofAbilityDefinition's 38 root fields, deliberately not a mirror of the root.stages: Vec<ShellStage>, because two of the transforms the call sites run are chainstructure folds that write root fields, and their order relative to the field stamps is
load-bearing. A field bag cannot express "run these two, in this order, after the stamps."
lower_ability_irextended to apply the envelope and the stages in the pinned orderchain → finalize → anchor →
sub_link→ envelope → stages.parse_ability_irpublished aspub(crate), plus the two mode-pinned wrappersparse_ability_ir_with_context/parse_ability_ir_standalone. These exist so a converted siteinherits its original
ChainLoweringModeby name rather than by a reviewer re-deriving it atnine call sites.
ability_ir_atemitter, which still emits the pre-lowered node.Byte-identity is by construction, and the property has a name
Defer-on-default: every shell field's applier is a no-op at its default, so
AbilityShellIr::default()— the only shell any producer builds until A1 — leaves the lowereddefinition bit-for-bit unchanged. In
apply_ability_shell_envelope:Optionfields write only underif let Some;activation_restrictionsextends from an empty vec;min_x_valueusesmaxagainst0;cant_be_copiedORs withfalse; the stage loop iterates an emptyVec.No property of any card's text participates, so a future MTGJSON card reaching one of these sites is
covered by the same argument.
The identity is now also literal in the tree rather than merely argued:
parse_effect_chainisexactly
lower_ability_ir(&parse_ability_ir_standalone(t, k)), andparse_effect_chain_with_contextits
WithContexttwin.H3 — the verification the plan explicitly deferred to the implementer: PASSED, no site excluded
The live sites split between
activation_restrictions = constraints(U0-23 Channel, U0-12 leveler)and
.extend(constraints)(U0-24/25/27).extendis the only correct shell semantic, but that isonly sound if nothing in
lower_ability_ir's closure writes the root field first.parse_ability_ircan dispatch to exactly five whole-body paths, plus three post-parse steps.Every one lives in
crates/engine/src/parser/oracle_effect/, and that entire directory containszero occurrences of
activation_restrictionsat the base commit. That is the whole closure, sono bypass writes the root's restrictions.
The check did not stop at the file grep, because
oracle_effect/does call into modules that writethe field. All six such calls go to
oracle_static, and every result is boxed into aGrantStaticAbility/StaticDefinition/CreateEmbleminside anEffectpayload — those landon nested definitions, never the root the shell stamps, and
StaticDefinitionhas noactivation_restrictionsfield at all.Verdict:
extend≡=for the root at every T8 R1 site; both=sites provably start from empty;nothing is excluded from T8. The invariant is maintainable — any future violation has to write the
field inside
oracle_effect/, which the same one-line grep catches.One thing A1–A4 must not "tidy"
Restriction order is site-specific. The Solved recognizer pushes
IsSolvedbefore extendingwith parsed constraints; Boast/Exhaust/Forecast/Power-up extend parsed constraints first, then push
their implicit ones. A shell with separate "implicit"/"parsed" fields would silently normalize that
away. Hence one ordered
Veccomposed by the site, applied by a singleextend.CR 707.9a is not gapped by this seam
ability_ir_atemits the pre-lowered spell variant, so every phase-A-converted producer lands infinish()'s pre-lowered arm and is stamped bystamp_retained_printed_slot— which recurses thewhole definition (effect,
sub_ability,else_ability,mode_abilities). A copy-except clauseinside a converted body is stamped, at the same slot it got before. A0 adds zero new reachable
instances.
Moving the stamp to
lower_oracle_ir(the recensus's T9 recommendation) would be actively unsafehere: it would newly stamp U0-39's
Spellnode, which #6708 deliberately leaves unstamped. Thatchanges output for any card pairing a prevention spell with a copy-except clause — an intentional byte
delta, which this tranche is chartered not to land. T9 is the right home, because that is where the
emitter switches payloads and where U0-39's delta is already being adjudicated.
Gates
Snapshot churn: ZERO, as an infrastructure-only tranche requires.
find . -name '*.snap.new'→ 0files after the full 17844-test run; the complete branch delta is three parser files, no
*_ir.snap,no
*_lowered.snap, no card-data. Gate P per-file counts are identical to baseline with no ledgeredit — every file sits exactly at its ceiling, so nothing was silently loosened.
Gate P caught a real defect in the first draft, which is worth recording. It raised
oracle.rs16 → 21, and not one of the five came from code — all five were the token appearing in a new doc
comment. The gate is textual; your own prose counts. The prose was rewritten to carry the meaning
without the token.
That is also why
ability_ir_atis written asself.ability_at(line, lower_ability_ir(&ir))ratherthan spelling out the
emit_at(.., PreLoweredSpell(..))form:ability_atis exactly thatemit_at, so the two are behaviorally identical, but the spelled-out form would pushoracle.rsto17 against a ceiling of 16. Delegation adds zero tokens and states the phase-A identity once instead
of maintaining a second copy of it. T9's one-line body swap is unaffected. No ceiling was raised.
Deliberate
#[allow(dead_code)], both with a named ownerShellStage(first producer arrives in A2, the R1 sites running both folds) andability_ir_at(first producer in A1). Unlike
TriggerNodeIr::Parsed, neither allow moves — both aredeleted by the tranche that adds the first producer, and the code comments say so.
Not run, and why
Full-pool
card-data.jsonbyte-identity. For a tranche that converts no producers it is vacuouslygreen and therefore supplies no signal; manufacturing a green that cannot be interpreted was declined
in favour of saying so. The non-vacuity probe belongs to A1, which is the first tranche with a
converted site to mis-mode.
CR numbers grep-verified against
docs/MagicCompRules.txtbefore entering code: 602.1, 602.1a,602.1b, 113.3b, 601.2b, 608.2, 601.2f, 113.6m, 707.9a, 702.142a, 702.193b, 106.6, 603.3, 707.10.
One was corrected in draft:
cant_be_copiedwas annotated CR 707.9a, which is the unrelated rule forcopy effects that cause a copy to gain an ability. The printed line "This ability can't be copied"
restricts putting a copy on the stack — CR 707.10. Both the field doc and the applier now cite
707.10 and say why it is not 707.9a.
Summary by CodeRabbit
Enhancements
Bug Fixes