fix(parser): Plan 05b T7 — Unit 3b prerequisites (printed slot, item_ability, unsound destructures) - #6708
Merged
Merged
Conversation
`OracleDocBuilder::finish()` is the live CR 707.9a authority: it resolves every "...except it has this ability" printed slot by walking items in source order and counting each category separately. `OracleNodeIr::Spell(_)` sat in that walk's no-op arm, so it consumed no ability slot. The set of node variants that advance `ability_slot` must equal the set `lower_oracle_ir` (`oracle.rs`) pushes into `result.abilities`, which is in turn the set `emit` pushes onto `spells_emitted` — both spell shapes. Three matches over one set, and this one disagreed. Every ability printed AFTER an IR-native spell was therefore stamped one slot low, so a copy's "except it has this ability" grafts the wrong printed ability. `Spell(_)` is not a hypothetical variant: the instant/sorcery prevent-damage recognizer in `oracle.rs` has emitted it since before this commit, which is precisely why the defect existed rather than merely being latent. The new arm advances the counter WITHOUT stamping, on purpose. An IR-native spell body has no `AbilityDefinition` until lowering builds one, and `stamp_retained_printed_slot` takes `&mut AbilityDefinition` — there is nothing to stamp. The printed slot is consumed all the same. Also corrects two comments that asserted the `Spell` IR variant is never constructed. Both were already false. CR 707.9a grep-verified against docs/MagicCompRules.txt. Plan 05b T7-P1.
`emit` (`oracle_ir/doc.rs`) pushes BOTH `OracleNodeIr::Spell(_)` and the
pre-lowered shape onto `spells_emitted`, and `take_last_spell` pops that
stack with no variant filter. Three readers were nonetheless written against
a single shape.
`item_ability` — the CR 607.2d ability side of document-relation discovery —
had a bare `_ => None` arm, so every relation that pairs an ability item went
blind on an IR-native spell, silently and with no compiler help. It now
returns `Cow<'_, AbilityDefinition>`: the pre-lowered shape still lends its
definition, while an IR-native item owns none and must lower one. That
lowering is the identity conversion `lower_oracle_ir` performs for the same
item, so a relation predicate sees exactly the definition the relation is
later applied to. `Cow` rather than a borrow because there is nothing to
borrow from in the second case, and rather than an owned value because that
would clone the first case at all seven call sites, most of which scan every
item on the card.
This is where `item_trigger`'s reasoning stops applying: it keeps a borrow
and pushes exhaustiveness down onto `TriggerNodeIr::definition()`, which
works because both of its shapes own a `TriggerDefinition`. A spell node's IR
payload is an `EffectChainIr`, not an enum of definition-owning
representations, so there is no lower layer to carry the obligation and both
shapes are named in the accessor.
`mutate_last_spell` and the cross-line "instead" fold both destructured the
popped item with `unreachable!("... returns only PreLowered... items")`. That
claim was false. The first IR-native spell preceding a min-X annotation line
or an "instead" fold line panics the parser mid-card, and `oracle_gen` has no
`catch_unwind`, so it aborts full-pool generation rather than degrading one
card. Both now read through `lower_spell_node` — the existing
node-shape-agnostic reader that already sat in this file and was already used
by `last_ability_definition`. The surviving `unreachable!` states an
invariant that is actually true: `spells_emitted` holds only spell nodes.
Not firing on today's pool, proven rather than assumed: full-pool generation
over ~35.4k cards is green, and the single live `Spell` producer is a
prevent-damage spell line that no current printing pairs with a mutating
sibling line. It detonates when T8 raises `Spell` producers from 1 to ~14.
Regression test (recensus 5.1.8, the `8f4826e0ca` pattern): Siren's Call's
`ActivePlayerPunisher` relation pairs two ability items on one instant, so it
drives `item_ability` twice; the test asserts the punisher's destroyed set is
still rebound off its line-local `You`, with a reach-guard proving the
punisher line alone is not. The test file records honestly that this
witnesses the BORROWED arm only — no text can currently reach the new
IR-native arm.
PreLowered ratchet: crates/engine/src/parser/oracle.rs 20 -> 16.
CR 607.2d, CR 707.9a, CR 102.1, CR 603.7c, CR 608.2c, CR 302.6 and CR 508.1a
grep-verified against docs/MagicCompRules.txt.
Plan 05b T7-P2 + T7-P3.
matthewevans
enabled auto-merge
July 27, 2026 21:22
Contributor
📝 WalkthroughWalkthroughThe parser now supports pre-lowered and IR-native spell nodes during ability extraction, relation discovery, mutation, and printed-slot accounting. New integration tests cover slot retention, punisher rebinding, exemption folding, and sibling reach guards. ChangesIR spell reader compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes 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: 1
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle.rs (1)
1163-1192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant re-lowering of IR-native spells across call sites.
item_abilityre-runslower_effect_chain_irfrom scratch on every call for anOracleNodeIr::Spellitem, and it's called independently at 7 sites (lines 1450, 1571, 1672, 1681, 1734, 3104, 3109), several of which run back-to-back over the sameitemsslice for the same card insidefinalize_document_relations. Today this is cheap (one live producer: the prevent-damage line), but the test suite's own scope note flags that IR-nativeSpellproducers are expected to grow from 1 to ~14 in a later tranche — at that point every relation-detector pass pays the lowering cost repeatedly per card.Worth memoizing once per document-relation pass now, while there's a single call pattern to change, rather than after more call sites accumulate.
♻️ Sketch: precompute once, read from cache in detectors
// In finalize_document_relations (or a caller), once per document: let lowered: HashMap<OracleItemId, AbilityDefinition> = items .iter() .filter_map(|item| Some((item.id, item_ability(item)?.into_owned()))) .collect(); // Detectors then read `lowered.get(&item.id)` instead of calling // `item_ability(item)` themselves.🤖 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 1163 - 1192, Memoize lowered ability definitions once per document-relation pass instead of re-running lower_effect_chain_ir through item_ability at each detector call. In finalize_document_relations, precompute owned definitions for IR-native OracleNodeIr::Spell items keyed by OracleItemId, preserve borrowed PreLoweredSpell behavior as equivalent cached values, and update all item_ability call sites to read the cache while retaining None for unsupported node variants.
🤖 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/tests/integration/ir_spell_node_readers.rs`:
- Around line 239-255: Update
the_punisher_line_alone_keeps_its_line_local_controller so it first positively
asserts that controllers is non-empty, using the same reach-guard pattern as the
sibling tests, then retains the existing negative ActivePlayer assertion.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 1163-1192: Memoize lowered ability definitions once per
document-relation pass instead of re-running lower_effect_chain_ir through
item_ability at each detector call. In finalize_document_relations, precompute
owned definitions for IR-native OracleNodeIr::Spell items keyed by OracleItemId,
preserve borrowed PreLoweredSpell behavior as equivalent cached values, and
update all item_ability call sites to read the cache while retaining None for
unsupported node variants.
🪄 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: 841b2633-0ae8-4e33-8944-5853a3599eff
📒 Files selected for processing (5)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/tests/integration/ir_spell_node_readers.rscrates/engine/tests/integration/main.rsscripts/prelowered-ratchet.txt
Comment on lines
+239
to
+255
| fn the_punisher_line_alone_keeps_its_line_local_controller() { | ||
| let parsed = parse_oracle_text( | ||
| "At the beginning of the next end step, destroy all non-Wall creatures that player controls that didn't attack this turn.", | ||
| "Probe", | ||
| &[], | ||
| &["Instant".to_string()], | ||
| &[], | ||
| ); | ||
| let (target, _) = punisher_destroy_target(&parsed); | ||
| let mut controllers = Vec::new(); | ||
| typed_controllers(&target, &mut controllers); | ||
| assert!( | ||
| !controllers.contains(&Some(ControllerRef::ActivePlayer)), | ||
| "with no coerce sibling there is no relation, so the anaphor must NOT be \ | ||
| rebound to ActivePlayer, got {controllers:?}" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing reach-guard for the negative assertion.
Unlike tests 1 and 2, this test asserts only the negative (!controllers.contains(...)) with no guard that controllers is actually non-empty first. If a regression made the destroy-target filter stop lowering to TargetFilter::Typed at all, controllers would be empty and this assertion would pass vacuously, hiding the real breakage.
✅ Proposed fix: add the same reach-guard used in the sibling test
let (target, _) = punisher_destroy_target(&parsed);
let mut controllers = Vec::new();
typed_controllers(&target, &mut controllers);
+ assert!(
+ !controllers.is_empty(),
+ "reach-guard: the destroyed set must carry at least one typed node, or the \
+ assertion below is vacuous, got {target:?}"
+ );
assert!(
!controllers.contains(&Some(ControllerRef::ActivePlayer)),
"with no coerce sibling there is no relation, so the anaphor must NOT be \
rebound to ActivePlayer, got {controllers:?}"
);As per path instructions, "For every negative assertion... require a paired positive reach-guard proving the input actually reached the code under test... An upstream short-circuit makes a negative pass for the wrong reason."
📝 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
| fn the_punisher_line_alone_keeps_its_line_local_controller() { | |
| let parsed = parse_oracle_text( | |
| "At the beginning of the next end step, destroy all non-Wall creatures that player controls that didn't attack this turn.", | |
| "Probe", | |
| &[], | |
| &["Instant".to_string()], | |
| &[], | |
| ); | |
| let (target, _) = punisher_destroy_target(&parsed); | |
| let mut controllers = Vec::new(); | |
| typed_controllers(&target, &mut controllers); | |
| assert!( | |
| !controllers.contains(&Some(ControllerRef::ActivePlayer)), | |
| "with no coerce sibling there is no relation, so the anaphor must NOT be \ | |
| rebound to ActivePlayer, got {controllers:?}" | |
| ); | |
| } | |
| fn the_punisher_line_alone_keeps_its_line_local_controller() { | |
| let parsed = parse_oracle_text( | |
| "At the beginning of the next end step, destroy all non-Wall creatures that player controls that didn't attack this turn.", | |
| "Probe", | |
| &[], | |
| &["Instant".to_string()], | |
| &[], | |
| ); | |
| let (target, _) = punisher_destroy_target(&parsed); | |
| let mut controllers = Vec::new(); | |
| typed_controllers(&target, &mut controllers); | |
| assert!( | |
| !controllers.is_empty(), | |
| "reach-guard: the destroyed set must carry at least one typed node, or the \ | |
| assertion below is vacuous, got {target:?}" | |
| ); | |
| assert!( | |
| !controllers.contains(&Some(ControllerRef::ActivePlayer)), | |
| "with no coerce sibling there is no relation, so the anaphor must NOT be \ | |
| rebound to ActivePlayer, got {controllers:?}" | |
| ); | |
| } |
🤖 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/tests/integration/ir_spell_node_readers.rs` around lines 239 -
255, Update the_punisher_line_alone_keeps_its_line_local_controller so it first
positively asserts that controllers is non-empty, using the same reach-guard
pattern as the sibling tests, then retains the existing negative ActivePlayer
assertion.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 27, 2026
Merged
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 T7 — the Unit 3b prerequisites. Three defect fixes, zero conversions.
Follows #6703 (T4). All three defects are on the already-live
Spell(EffectChainIr)variant, soall three are live today; they escalate from 1 producer to ~14 the moment T8 lands.
P1 —
finish()did not consume a printed slot forOracleNodeIr::Spell(_)The invariant this restores is worth stating as a class, because nothing in the type system enforces
it: the set of variants that advance
ability_slotinfinish()must equal the setlower_oracle_irpushes intoresult.abilities, which must equal the setemitpushes ontospells_emitted. Three matches over one set, coupled only by convention — which is exactly howthey drifted.
Spell(_)was in the first match's no-op list and in the other two's live lists.Consequence: every ability printed after a
Spellitem was stamped one CR 707.9a slot low, so acopy-except clause grafted the wrong ability.
The arm advances without stamping, deliberately and un-symmetrically with its
PreLoweredSpellsibling:
stamp_retained_printed_slottakes&mut AbilityDefinition, and an IR-native spell body hasno definition until lowering builds one. There is nothing to stamp; the printed slot is consumed all
the same. The comment says so inline so the next reader does not "fix" the asymmetry.
P2 —
item_abilitywas blind toSpell(_)The CR 607.2d ability side of relation discovery had a bare
_ => None, so every relation pairing anability item went blind on an IR-native spell — silently, with no compiler help.
Now returns
Cow<'_, AbilityDefinition>. This is whereitem_trigger's reasoning stops applying,and the asymmetry is structural rather than stylistic:
item_triggerkeeps a borrow and pushesexhaustiveness down onto
TriggerNodeIr::definition(), which works because both of its shapes own aTriggerDefinition. A spell node's payload is anEffectChainIr— not an enum of definition-owningrepresentations — so there is no lower layer to carry the obligation, and both shapes are named in the
accessor.
Cowrather than a borrow because the second case has nothing to borrow from; rather than an ownedvalue because that would clone the first case at all seven call sites, most of which scan every item
on the card.
Cost, verified rather than asserted: seven call sites, zero regressed into a per-item clone.
The pre-lowered arm returns
Cow::Borrowed(a clone is not representable there); every consumer takes&def, which deref-coerces at the argument position; andinto_owned/to_mut()— the only two waysto force a
Cowto clone — appear nowhere inoracle.rs. On the pre-lowered path the cost isbyte-identical to the old
Option<&AbilityDefinition>.P3 — two
unreachable!destructures were unsoundemitpushes both spell shapes ontospells_emittedandtake_last_spellpops with no variantfilter, so
unreachable!("... returns only PreLowered... items")was simply false. The firstIR-native spell preceding a min-X annotation or an "instead" fold line panics the parser mid-card —
and
oracle_genhas nocatch_unwind, so it aborts full-pool generation rather than degrading onecard.
The fix turned out to be reuse, not new code:
lower_spell_node(oracle.rs:631) is exactly thenode-shape-agnostic reader both sites needed, and was already consumed by
last_ability_definition~3,000 lines above. A building-block search miss, not a design gap. The surviving
unreachable!nowstates an invariant that is actually true (
spells_emittedholds only spell nodes).Empirical gate
21953 tests run: 21953 passed (11 slow), 8 skipped. Caveat stated plainly:that run predates a test-file rename; the production sources were confirmed byte-identical
(
diff -q) to what passed it, and only test-file naming changed after.3 tests run: 3 passed.*_ir.snap, no*_lowered.snap, nooracle_static/snapshots/*, no card-data.Gate P PASS(ratchet,oracle.rs20 → 16);✓ oracle-parser skill references valid;cargo fmt --allclean.P1's byte delta: MEASURED ZERO — no bug report
The plan predicts zero and says nonzero means a live mis-indexed card exists. Measured by direct
corpus probe rather than full-pool regen, which is sharper for this defect: across all 35,516 cards in
data/card-data.json, 173 areSpell-node candidates (Instant/Sorcery whose text contains"prevent" and "damage" — deliberately a superset of the real producer). Of those 173, zero
carry a
RetainPrintedAbilityFromSourceanywhere in their parse.P1 can only move a stamp on a card having both a
Spellnode and a copy-except clause. No such cardexists in the pool, and because the probe ran over a superset of producers the conclusion holds
a fortiori.
Full-pool regeneration was NOT run, and that is deliberate: the executor worktree carries only
data/mtgjson/test_fixture.json(the realAtomicCards.jsonis gitignored and exists only in themain checkout), so generating there would have measured a fixture population — the §5.1.3
void-evidence trap. A manufactured green was declined in favour of saying so. Pre-edit baseline for
whoever runs it on the main checkout:
a717afe7328faad9d074cf517abc598120b0a7fcf5bdcca38400d32bb42185db,99,201,816 bytes, 2026-07-27T11:20:54-0700. Side-observation: Tilt regenerated card-data at 13:39 and
the hash was unchanged, so concurrent agents' game-layer edits are not moving card-data and that
baseline is sound.
Harvest (§5.4)
mutate_last_spellcallers; the basecommit had already deleted the second. There is exactly one today. (Independently re-verified.)
doc.rs:916-924also claimed theSpellIRvariant is "still never-constructed". Both it and the
941-944comment were false at base; bothfixed.
finish(), which runs before lowering, andlower_oracle_irdoes no stamping. So a copy-exceptclause inside an IR-native
Spellbody keepsplaceholder()(= 0) permanently. P1 fixes everylater ability; the IR-native item's own clause is never stamped. Unreachable today, reachable at
T8. This is the same question the recensus flags for T9 ("move the stamp to
lower_oracle_ir").ability_index()/trigger_index()are dead in production — zero callers outsidedoc.rs(grep-verified); the only readers are
doc.rs's own tests, whilefinish()is the live authorityand says so. Both still carry
PLAN-05 DEBTallows.doc.rssits at its ceiling (16/16), and any#[cfg(test)]helper constructing a stamp carrier must name the marker word. The ceiling was notraised; the test moved to
crates/engine/tests/integration/(outside the gate's scope), which alsoupgraded it to driving the real
parse_oracle_textpipeline.Known limitation, stated rather than papered over
P2's new IR-native arm has no test witness — it is vacuously covered. Removing the arm leaves all
three tests green. No relation predicate matches a prevention chain, and an attempt to synthesize a
card pairing the two failed because the line splitter emits the choice as its own item, so the chooser
is never the
Spellnode. The arm is forward hardening for T8. The Siren's Call test is a genuinenon-regression witness for the borrowed arm, which is the change class §5.1.8 actually asks for.
Deliberate allocations introduced, flagged for review
lower_spell_node(&node)clones the definition on the pre-lowered arm atmutate_last_spelland atthe "instead" fold, where the old code moved it — one clone per occurrence on two rare
once-per-card-line parse paths. Chosen over duplicating the match inline twice (DRY); the alternative
is minting a by-value sibling of
lower_spell_nodefor two callers.CR numbers grep-verified against
docs/MagicCompRules.txtbefore entering code or commit messages:707.9a, 607.2d, 102.1, 603.7c, 608.2c, 302.6, 508.1a.
Summary by CodeRabbit
Bug Fixes
Tests