Skip to content

refactor(parser): Plan 05b T5 — nine trigger recognizer sites through the TriggerNodeIr seam - #6709

Merged
matthewevans merged 10 commits into
mainfrom
ship/plan05b-t5
Jul 27, 2026
Merged

refactor(parser): Plan 05b T5 — nine trigger recognizer sites through the TriggerNodeIr seam#6709
matthewevans merged 10 commits into
mainfrom
ship/plan05b-t5

Conversation

@matthewevans

@matthewevans matthewevans commented Jul 27, 2026

Copy link
Copy Markdown
Member

Plan 05b tranche T5 — nine trigger recognizer sites converted from PreLoweredTrigger to the
TriggerNodeIr seam T4 opened. Follows #6703 (T4) and #6708 (T7).

One commit per row, plus a baseline fixture commit that lands before any conversion.

commit row site
baseline 8 new two-layer fixtures
U0-08 T5b Saga chapter triggers (CR 714)
U0-10 T5b Attraction visit triggers (CR 717)
U0-13 T5a leveler LEVEL-block triggers (CR 711.2a/b)
U0-15 T5a Spacecraft threshold triggers (CR 702.184a)
U0-33 T5c compound exert, bare form (CR 701.43d)
U0-34 T5c compound exert, card-name form
U0-35 T5c conditional exert
U0-36 T5c synthesized flash-cleanup-sacrifice
U0-54 T5d oracle_class.rs level-gained triggers

CR-faithfulness — by construction, for all nine rows

Every conversion reduces to a one-line emission change:

- emitter.trigger_at(item_line, trigger);
+ emitter.trigger_ir_at(item_line, TriggerNodeIr::from_definition(&line, trigger));

trigger is the finished TriggerDefinition the recognizer already built. Nothing about how it was
built changes; it is merely wrapped for source-ordered emission, and lower_trigger_node_ir returns
it untouched. No property of any card's text participates in the argument, so a future MTGJSON
card reaching any of these sites is covered too.

Three consequences worth stating explicitly, because the plan anticipated each as a hazard requiring
mitigation and the identity path removes all three:

The ChainLoweringMode hazard (§7.1) is structurally out of scope in T5c. The recensus flags the
exert trio as the tranche's most dangerous rows — parse_effect_chain_ir/lower_effect_chain_ir
alone runs neither the bypasses nor finalize_effect_chain, silently dropping finalize + anchor on
~113 cards. No chain parsing was moved, re-invoked or re-moded here: grep for
ChainLoweringMode|parse_effect_chain|WithContext across all four T5c commits returns zero changed
lines. The mode was never in scope to change.

T5b's description overwrite cannot fire. lower_trigger_ir does
def.description = Some(ir.source_text) unconditionally, which would corrupt Saga (stamps
"Chapter {n}", not the line) and Attraction (stamps nothing). lower_trigger_node_ir never calls it.
Verified in the committed _lowered snapshots rather than argued:

history_of_benalia_lowered:  "description": "Chapter 1" / "Chapter 2" / "Chapter 3"
bumper_cars_lowered:         "description": null

I, II — correctly yields both Chapter 1 and Chapter 2 from one shared source line (CR 714.2c).

T5a/T5d are by construction, NOT by corpus — a correction to the recensus §5.2 classification.
That document labels these rows correct-by-corpus, relying on "all four Class And conditions in
today's pool are exactly two-element", and names the future card that would break it: one whose
printed condition is already an And, yielding And[And[gate,x],y] instead of the flat
And[gate,x,y] that #6021 deliberately introduced.

That hazard exists only if the CR 711.2a/711.2b graft moves pre-lowering. It does not. The graft
stays exactly where it is, operating on the lowered definition, so the identical code produces the
identical flat shape. grep for extra_conditions|partial_def.condition across the tranche returns
zero — no graft was relocated. Consequently trigger_condition_source_zones
(oracle_trigger.rs:1836) also does not start deriving trigger_zones from the level/station gate,
which was the second half of the plan's concern. There is no corpus dependency to state, and the
future-card shape the plan names cannot arise.

Empirical gate

Snapshot discipline (§5.1.5) — the load-bearing result:

_lowered.snap  MODIFIED after baseline:  0     <- the STOP condition, not triggered
_ir.snap       modified after baseline:  9     <- one per converted row
whole branch:  16 A (8 fixtures x _ir/_lowered), 1 M (wizard_class_two_layer_ir.snap)

The IR representation re-nested on every fixture while every lowered output stayed bit-identical.
The single pre-existing snapshot modified is an _ir file, which is permitted. No churn in
oracle_static/snapshots/*, oracle_replacement.rs parity tests, oracle_trigger_snapshot_tests.rs,
client/public/card-data.json or data/card-data.json.

Hunks read (§5.1.6), not merely counted. The diff shape is pure re-nesting:

-  "PreLoweredTrigger": { "mode": "CounterAdded", "execute": {...
+  "Trigger": { "Assembled": { "definition": { "mode": "CounterAdded", "execute": {...

Same fields, one level deeper, mode and execute preserved. No active_zones drift, no rebound
CountersOn.scope — the charter's STOP conditions.

Other gates: Gate P PASS (ratchet), ✓ oracle-parser skill references valid,
cargo fmt --all --check clean, cherry-picks onto current main with zero conflicts.

Non-vacuity (§5.3) — eight of nine rows had NO witness

This is why the baseline commit exists and why it lands first. Eight of the nine rows had no card
in the two-layer snapshot corpus reaching their site, so a "byte-identical" claim on them would have
been a broken probe rather than a pass. The baseline adds one fixture per uncovered row:

lighthouse_chronologist  1 node   U0-13    entropic_battlecruiser  2 nodes  U0-15
history_of_benalia       3 nodes  U0-08    bumper_cars             1 node   U0-10
ahn_crop_champion        1 node   U0-33    themberchaud            2 nodes  U0-34
combat_celebrant         1 node   U0-35    armor_of_thorns         1 node   U0-36

Probe verified live, not assumed: each new _ir.snap was confirmed to contain at least one
PreLoweredTrigger node before conversion, so each fixture provably reaches the site its row
converts. The node counts above are measured. U0-54 needed no fixture — wizard_class already
witnessed it (2 nodes), verified rather than assumed.

Witness selection was driven by arm predicates, not convenience. Lighthouse Chronologist was chosen
over the pool's other LEVEL-block-trigger levelers because it is the only one printing its own
CR 603.4 intervening-if, making it the witness for the composing arm of the level graft
(Some(existing) => And { .. }) rather than only the None arm — which is the half the flat-vs-nested
question actually turns on. Entropic Battlecruiser carries both a threshold trigger and an ungated
one, so it also witnesses CR 707.9a per-category slot ordering across the preprocessor/dispatch-loop
boundary.

Every card's Oracle text is copied verbatim from the engine-authoritative pool
(data/card-data.json, regenerated 2026-07-27 11:20), never from memory.

Additional verification beyond snapshot diffing

Definition value-identity was machine-proved, not eyeballed. §5.1.6 asks that every regenerated
hunk be read to confirm the definition fields are unchanged modulo indentation. A script that unwraps
Trigger.Assembled.definition back to PreLoweredTrigger reproduces each prior snapshot exactly
across all 12 converted trigger nodes on 9 cards:

PASS ahn_crop_champion 1 · armor_of_thorns 1 · bumper_cars 1 · combat_celebrant 1
PASS entropic_battlecruiser 2 · history_of_benalia 3 · lighthouse_chronologist 1
PASS themberchaud 1 · wizard_class_two_layer 1
ALL DEFINITIONS VALUE-IDENTICAL: True

Two rows convert only ONE of two triggers on the card (U0-34 Themberchaud, U0-54 wizard_class),
leaving the sibling PreLowered. That checks — rather than assumes — that an unconverted sibling's
CR 707.9a printed slot is undisturbed, which is exactly the failure mode a shared trigger_slot
counter would produce.

Full parser suite green by execution, not merely unchanged on disk:

two-layer suite:     98 passed; 0 failed
full parser suite: 8623 passed; 0 failed; 3 ignored

That run covers oracle_trigger_snapshot_tests, the oracle_replacement parity tests and the
oracle_static snapshots.

One prediction failed and was chased rather than rounded off: entropic_battlecruiser was expected
to convert 1 node and converted 2. The second is the unprefixed "Whenever this Spacecraft attacks…"
line, which the preprocessor attaches to the preceding 8+ section. Minima checked individually —
prefixed line min=1, continuation min=8 — matching the physical card's station-bar layout. Correct
behavior, and it makes that fixture cover both arms of U0-15.

Harvest (§5.4)

1. The ratchet cannot see 8 of this tranche's 9 conversions. It converted eight producers in
oracle.rs and that file's count did not move (20 → 20); #6708 converted zero producers and moved
it 20 → 16. The marker text lives in emitter method bodies (trigger_at) and consumer match arms,
never at producer call sites.

Crucially, oracle.rs staying at 20 is correct, not a miss: trigger_at must survive while
drain_result_vectors and the three T10-blocked __item sites still call it. oracle_class.rs moved
6 → 5 only because that site pushes the variant literally into a vector rather than through a helper.
The metric tracks helper deletion, not producer conversion. Do not read the flat 20 as "T5 did
nothing".

So §6.1's premise — "Every tranche that converts a producer must lower its number in the same commit,
which is what makes the burn-down visible in git log"
— does not hold for oracle.rs. The gate is
sound as a ceiling (it would still catch a new producer), but the per-tranche burn-down it was
designed to expose is invisible there, and §6 closing criterion 4 becomes reachable only when the
emitter family is deleted at T9. Flagged; no gate was edited.

2. oracle_spacecraft.rs clobbers where its sibling composes.
trig.condition = Some(self.trigger_cond.clone()) unconditionally discards any printed CR 603.4
intervening-if, while the CR 711.2a leveler graft ~130 lines away composes with And and carries a
comment explaining why. Two counter-threshold gates on printed triggers, opposite policies, one
silently lossy. Latent — no pool card currently prints an intervening-if on a threshold line.

3. A mis-cited CR already in the tree. oracle_class.rs:123 reads
// CR 602.5d + CR 716.4: Level N+1 can only activate at sorcery speed…. CR 716.4 is the
leveler-disambiguation rule
("Some older creature cards, called leveler cards … are not the same as
class level abilities"). The rule that actually says this is CR 716.2a. Independently re-verified
against docs/MagicCompRules.txt. Not fixed here — §7.3 says report, don't fix inside a byte-identity
tranche.

4. Silent line-drop in exert arms 2 and 3. If split_once_on_lower(.., ". when you do, ") returns
None, the arm still runs i += 1; continue — the line is consumed with nothing emitted and no
diagnostic. The exert cost vanishes silently.

5. scan_contains dispatch in U0-34's arm ("as " + "attacks") is exactly the contains()-style
parsing dispatch the nom mandate prohibits.

6. Sibling-cluster smell in the exert trio — the same 5-call builder chain (TriggerMode::Exerted /
SelfRef / [Battlefield] / execute / description) repeated verbatim three times, differing only
in recognition and split.

7. AttractionVisitRoll{min,max} has no pool witness. Zero Attractions in data/card-data.json
print a numbered visit line. The arm is converted and correct by construction but exercised by no test;
recorded in its commit body so it is not mistaken for covered.

8. Unverified observation, flagged as such. The leveler activated branch resets
ctx.subject/ctx.actor; the trigger branch does not and reuses the outer ctx across LEVEL lines,
where oracle_spacecraft.rs passes a fresh ParseContext::default() per trigger. Possible
subject/actor leakage; not proven to change any card's parse.

9. T5a/T5d's by-corpus classification in the recensus is wrong (see above) — a correction to the
plan, not the code.

Scope discipline

The IR-native TriggerNodeIr::Parsed design was considered and rejected for T5a, for two
independent reasons: it is only correct-by-corpus (the And flattening question above), and it would
require a new arm in the finish() match in oracle_ir/doc.rs — the region #6708 was concurrently
editing. If the IR-native decomposition is wanted for its own sake it is a separate chartered change
with a real behavioral delta, and should not ride inside a byte-identity tranche.

Not claimed

Full-pool card-data.json byte-identity was not run from the executor worktree, deliberately: per
§5.0 that rung is batched and Tilt regenerates it automatically from its checkout, so a run from a
worktree would measure a different population (§5.1.3's void-evidence trap). The snapshot-layer
evidence here is strictly stronger per-card — zero _lowered churn plus machine-proved definition
identity across 12 nodes — but the full-pool cmp is a separate read and is not being claimed.

The ratchet was lowered once, in the U0-54 commit, rather than in each of nine, to keep the conflict
surface a single hunk. Every earlier commit sits below its ceiling, which the gate treats as a pass,
so no intermediate commit is red.

Summary by CodeRabbit

  • Bug Fixes

    • Improved trigger processing for Saga chapter triggers, Attraction visit triggers, LEVEL internal triggered abilities, Spacecraft threshold triggers, exerted abilities, and Flashback cleanup sacrifice effects.
    • Preserved trigger identity and description fidelity when parsing class-level “becomes level N” abilities.
  • Tests

    • Expanded two-layer parsing and lowering snapshot coverage for additional Oracle trigger patterns and ordering.
  • Chores

    • Updated the prelowering ratchet threshold used for build validation.

@matthewevans
matthewevans enabled auto-merge July 27, 2026 22:02
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Trigger-producing parser paths now emit TriggerNodeIr through trigger_ir_at, including class-level, pre-parsed, synthesized, exert, and cleanup triggers. New two-layer snapshots cover these conversions, and the pre-lowered ratchet threshold is updated.

Changes

Trigger IR emission

Layer / File(s) Summary
Oracle trigger emission paths
crates/engine/src/parser/oracle.rs
Saga, Attraction, LEVEL, Spacecraft, exert, and flashback cleanup triggers now use TriggerNodeIr with trigger_ir_at.
Class trigger IR conversion
crates/engine/src/parser/oracle_class.rs, scripts/prelowered-ratchet.txt
Class-level triggers are emitted as OracleNodeIr::Trigger, and the pre-lowered ceiling decreases from 6 to 5.
Two-layer trigger snapshots
crates/engine/src/parser/oracle_ir/snapshot_tests.rs
Snapshots cover LEVELER, Spacecraft threshold, Saga, Attraction, exert, and flashback cleanup trigger conversions.

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

Possibly related PRs

  • phase-rs/phase#6703: Introduces the TriggerNodeIr and trigger_ir_at emission seam used by these parser changes.

Suggested labels: enhancement

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the parser refactor converting trigger recognizer sites through the TriggerNodeIr seam.
✨ 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-t5

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

Plan 05b T5 converts nine recognizer sites from `PreLoweredTrigger` to the
`TriggerNodeIr` seam. Eight of the nine had NO witness in the two-layer
snapshot corpus, so a "byte-identical" claim on them would have been a broken
probe rather than a pass (plan 05b-U0 §5.3). These fixtures land BEFORE any
conversion so each row's churn is attributable and non-vacuous.

Every card's Oracle text is copied verbatim from the engine-authoritative pool
(`data/card-data.json`, regenerated 2026-07-27 11:20), never from memory.

Probe verified live, not assumed: each new `*_ir.snap` contains at least one
`PreLoweredTrigger` node today, so each fixture provably reaches the site its
row converts.

  lighthouse_chronologist  1   U0-13 leveler LEVEL-block trigger (CR 711.2a/b)
  entropic_battlecruiser   2   U0-15 Spacecraft threshold trigger (CR 702.184a)
  history_of_benalia       3   U0-08 Saga chapters (CR 714.2c multi-numeral)
  bumper_cars              1   U0-10 Attraction visit (CR 717)
  ahn_crop_champion        1   U0-33 exert, bare-`~` form (CR 701.43d)
  themberchaud             2   U0-34 exert, card-name form + a preceding ETB
  combat_celebrant         1   U0-35 exert, conditional form
  armor_of_thorns          1   U0-36 synthesized flash-cleanup-sacrifice

U0-54 already has a witness (`wizard_class`, 2 nodes) and needs no fixture.

Lighthouse Chronologist is chosen over the two other pool levelers with a
LEVEL-block trigger because its trigger is the only one printing its own
CR 603.4 intervening-if, making it the witness for the COMPOSING arm of the
level graft rather than only the `None` arm.

No existing snapshot changes; all 98 two-layer tests pass.
Plan 05b T5b. `parse_saga_chapters` hand-builds one `TriggerDefinition` per
CR 714.2c numeral; the emission moves from `trigger_at` (`PreLoweredTrigger`)
to `trigger_ir_at` with `TriggerNodeIr::from_definition`.

CR-faithfulness: BY CONSTRUCTION, for the whole class of CR 714 chapter lines.
`lower_trigger_node_ir` (`oracle_trigger.rs:1585`) is `definition.clone()` on
`Assembled` and deliberately does not route through `lower_trigger_ir`, so none
of the nine fields lowering overwrites is re-derived. That is what protects this
row's `B-only stamp`: Saga stamps `description = "Chapter {n}"`, NOT the printed
line, and `lower_trigger_ir:1828` would unconditionally replace it with
`source_text`. No property of any card's text participates in the argument, so a
future Saga printing is covered too.

All five consumers of the node payload already treat `Trigger(Assembled{..})`
and `PreLoweredTrigger(..)` identically: the CR 707.9a printed-slot counter
(`doc.rs:805`), the `finish()` slot stamp (`doc.rs:929/933`), the CR 607.2d
relation reader `item_trigger` (`oracle.rs:1184`, via `definition()`),
`lower_oracle_ir` (`oracle.rs:2892`) and `feature.rs:469`.

Non-vacuity: `history_of_benalia` carries three chapter triggers (I, II, III)
and its `_ir.snap` held three `PreLoweredTrigger` nodes before this commit.
Snapshot churn is confined to `*_ir.snap`; the paired `*_lowered.snap` is
byte-unchanged, which is the byte-identity evidence for this row.
Plan 05b T5b. `parse_attraction_visit_triggers` hand-builds a
`VisitAttraction` trigger (CR 717); the emission moves off `PreLoweredTrigger`
onto `TriggerNodeIr::from_definition`.

CR-faithfulness: BY CONSTRUCTION, same identity property as U0-08. This row is
the complement that makes the property visible: Attraction leaves `description`
at `None` where Saga stamps `"Chapter {n}"`, so a lowering path that derived
`description` from `source_text` would corrupt Saga's value AND invent one for
Attraction. `lower_trigger_node_ir` derives nothing.

Non-vacuity: `bumper_cars` ("Visit — Target creature must be blocked this turn
if able.") held one `PreLoweredTrigger` node before this commit and one
`Trigger::Assembled` after; `bumper_cars_lowered.snap` is byte-unchanged.

Known coverage limit, stated rather than papered over: the NUMBERED visit form
(`"1, 3 — …"`, which stamps `AttractionVisitRoll { min, max }`) has no fixture
because it has no witness in the pool — zero Attractions in `data/card-data.json`
print a numbered visit line. That arm is converted by the same one-line change
and rests on the same by-construction argument, but it is unexercised by any
test, and this comment is the record of that.
Plan 05b T5a. Triggers re-parsed out of a CR 711.2a/711.2b LEVEL block are
gated with a `HasCounters{level, min, max}` condition and then emitted; the
emission moves off `PreLoweredTrigger`.

CR-faithfulness: BY CONSTRUCTION — and this row is by-construction only because
the graft was deliberately LEFT where it is.

The plan flagged this row as the one where the tempting design (hoist the site
onto `parse_trigger_lines_at_index_ir` and graft the gate onto a pre-lowering
`TriggerModifiers`) is correct only BY CORPUS. That design was considered and
rejected. Recorded here so the trade is not re-litigated blind:

  * The printed condition is composed into `def.condition` by lowering, and the
    graft then wraps it: `and_trigger_conditions(Some(gate), if_cond)` yields the
    FLAT `And[gate, if]`. Grafting pre-lowering instead yields `And[And[gate, x], y]`
    whenever the printed condition is ITSELF an `And`. All four Class triggers
    with an `And` condition in today's pool are exactly two-element, so the two
    designs agree today — that is a corpus property, not a guarantee, and the
    flattening branch was added deliberately by #6021.
  * Writing `partial_def.condition` is separately non-equivalent: it would make
    `trigger_condition_source_zones` derive `trigger_zones` from the level gate.

Keeping the graft post-lowering and identity-lowering the node means neither
hazard is live, and the argument cites no property of any card's text.

Non-vacuity: `lighthouse_chronologist` is the only pool leveler whose
LEVEL-block trigger prints its own CR 603.4 intervening-if ("if it's not your
turn"), so it exercises the COMPOSING arm of the graft (`Some(existing) =>
And{..}`), not merely the `None` arm — precisely the arm the flat-vs-nested
question turns on. Its `_ir.snap` held one `PreLoweredTrigger` before this
commit; `lighthouse_chronologist_lowered.snap` is byte-unchanged, which
demonstrates the composed condition survived intact.
Plan 05b T5a. `parse_spacecraft_threshold_lines` stamps a charge-counter
`HasCounters` condition (CR 702.184a + CR 721.2) on each trigger-shaped
threshold line; the emission moves off `PreLoweredTrigger`.

CR-faithfulness: BY CONSTRUCTION. The station gate is written onto the LOWERED
definition inside the preprocessor (`oracle_spacecraft.rs`), and
`lower_trigger_node_ir` is the identity on `Assembled`, so the gate is neither
re-derived nor re-composed. As with U0-13, no card-text property is used.

Non-vacuity: `entropic_battlecruiser` held two `PreLoweredTrigger` nodes before
this commit and converts both, covering BOTH arms of this site:
  * `1+ | Whenever an opponent discards a card, …` — the prefixed threshold line
    (gate min = 1);
  * `Whenever this Spacecraft attacks, …` — an UNPREFIXED continuation line that
    the preprocessor correctly attaches to the preceding `8+` section (gate
    min = 8), matching the physical card's station-bar layout.
That second arm was not something I designed the fixture for; it was found by
checking the emitted node count against prediction, and the minima were verified
individually rather than assumed. `entropic_battlecruiser_lowered.snap` is
byte-unchanged.

Pre-existing defect NOT fixed here (byte-identity tranche), recorded for
follow-up: this preprocessor does `trig.condition = Some(self.trigger_cond)`
UNCONDITIONALLY, discarding any printed CR 603.4 intervening-if. Its sibling —
the CR 711.2a leveler graft converted in U0-13 — composes with `And` instead and
carries a comment explaining why. Two counter-threshold gates on printed
triggers, opposite policies; the Spacecraft one is silently lossy. No pool card
currently prints an intervening-if on a threshold line, so it is latent.
Plan 05b T5c. The CR 701.43d "You may exert this creature as it attacks. When
you do, …" arm hand-builds an `Exerted` trigger shell around a body parsed from
the text suffix; the emission moves off `PreLoweredTrigger`.

ChainLoweringMode (plan §7.1) — the hazard this tranche was flagged for is NOT
armed by this change, and the reason is structural rather than careful: the
`parse_effect_chain_with_context(effect_rest, Spell, &mut ctx)` call is
UNTOUCHED. The conversion changes only how the finished `TriggerDefinition` is
handed to the emitter, so the site keeps its `WithContext` mode verbatim,
including `try_parse_exile_pile_shuffle_cloak` and `parse_face_down_pile_ir`,
and `finalize_effect_chain` + the owner-library anchor run exactly as before.
No mode is crossed, unified, or approximated anywhere in T5.

CR-faithfulness: BY CONSTRUCTION. `lower_trigger_node_ir` is `definition.clone()`,
so the `B-only stamps` this row carries — `mode = Exerted`, `valid_card =
SelfRef`, `trigger_zones = [Battlefield]`, and `description` = the WHOLE printed
line rather than the parsed suffix — are preserved bit-for-bit. The CR 607.2h
linkage between the exert cost and the reflexive "when you do" survives because
no transform runs at all.

Non-vacuity: `ahn_crop_champion` held one `PreLoweredTrigger` node before this
commit. `ahn_crop_champion_lowered.snap` is byte-unchanged.
…U0-34)

Plan 05b T5c. The CR 701.43d arm for "You may exert {Name} as {he/she/it/they}
attacks" — the form the bare-`~` tags in U0-33 cannot match, because self-ref
normalization rewrites the name but leaves the gendered pronoun.

ChainLoweringMode: unchanged, `WithContext`, for the same structural reason as
U0-33 — the `parse_effect_chain_with_context` call is untouched.

CR-faithfulness: BY CONSTRUCTION, identical to U0-33 (this arm builds the same
shell from a different recognizer).

Non-vacuity: `themberchaud` held two `PreLoweredTrigger` nodes before this
commit and converts exactly ONE — the exert trigger. The card's ordinary ETB
trigger prints ABOVE it and still emits `PreLoweredTrigger` from the
(T10-blocked) dispatch-loop site, so this fixture also demonstrates that
converting one trigger does not disturb the CR 707.9a printed slot of a
preceding unconverted one. That is the failure mode a shared `trigger_slot`
counter would produce, and it is checked here rather than assumed.
`themberchaud_lowered.snap` is byte-unchanged.
Plan 05b T5c. The CR 701.43d arm for "If [creature] hasn't been exerted this
turn, you may exert it as it attacks. When you do, …".

ChainLoweringMode: unchanged, `WithContext`, same structural reason as U0-33/34.

CR-faithfulness: BY CONSTRUCTION, identical to U0-33.

Pre-existing gap PRESERVED, not papered over: the leading if-gate this arm
dispatches on is parsed and then DROPPED — the emitted trigger carries no
condition for "hasn't been exerted this turn". The census flagged that the
conversion must not silently correct it, and it does not: the emitted definition
is bit-identical, the gap is now pinned by a snapshot, and a comment at the site
records that the fix belongs in a change allowed to move bytes.

Non-vacuity: `combat_celebrant` is the ONLY card in the pool that reaches this
arm (verified by scanning `data/card-data.json`, not assumed), and held one
`PreLoweredTrigger` node before this commit. `combat_celebrant_lowered.snap` is
byte-unchanged — which is also the evidence that the dropped if-gate is still
dropped rather than accidentally restored.
… IR (U0-36)

Plan 05b T5c, the `synthesized-effect-trigger` row.
`parse_flash_cleanup_sacrifice_casting_option` recognizes a fixed three-`tag()`
sentence and returns a casting option plus a `ChangesZone` trigger whose
`execute` is FULLY synthesized — `CreateDelayedTrigger{AtNextPhase(Cleanup)} ->
Sacrifice{SelfRef}` — with no parseable source text at all.

CR-faithfulness: BY CONSTRUCTION, and this row has the cleanest form of the
argument in the tranche. There is no parsed body to preserve, so the only thing
that could change is the assembled definition itself, and
`lower_trigger_node_ir` clones it. `condition = CastTimingPermission
{AsThoughHadFlash}`, `destination = Battlefield`, `valid_card = SelfRef` and
`description` all pass through untouched.

Design note: the plan named `VoteIr`/`PileIr` as the precedent for a synthesized
body, i.e. wrapping the hand-built `Effect` and exposing it via
`EffectChainIr::single_clause`. That precedent is NOT followed here, deliberately.
It applies to a `TriggerBody` that must still flow through trigger lowering;
this site needs the opposite — no lowering at all — and `TriggerNodeIr::Assembled`
already expresses exactly that. Using `single_clause` would have re-introduced
the ~20 execute-mutating passes over an already-assembled tree, which is the
failure mode `TriggerIr::from_definition` was forbidden to enable.

Non-vacuity: `armor_of_thorns` held one `PreLoweredTrigger` node before this
commit; it is the alphabetically-first of the pool Auras printing this exact
sentence. `armor_of_thorns_lowered.snap` is byte-unchanged.
Plan 05b T5d, the one row of this tranche in `oracle_class.rs`.
`parse_class_level_trigger` hand-builds the "When this Class becomes level N"
trigger; the item pushed into the Class preprocessor's ordered vector moves from
`OracleNodeIr::PreLoweredTrigger` to `OracleNodeIr::Trigger`.

CR-faithfulness: BY CONSTRUCTION. Same identity property as the other eight
rows. The plan listed T5d alongside T5a as "by corpus", on the assumption that
the `ClassLevelGE` graft would move pre-lowering; it does not — the graft stays
inside `parse_class_level_trigger`, operating on the definition it just built,
so the flat-vs-nested `And` question never arises here either. Labelled
by-construction deliberately and with that reasoning stated, rather than
inheriting the plan's label unexamined.

This row does NOT go through `DocEmitter`, so it correctly continues to skip the
`last_trigger` peek mirror that `trigger_ir_at` maintains — the Class path
returns before any dispatch loop runs, and no mid-loop reader exists to serve.

Ratchet: `oracle_class.rs` 6 -> 5. Lowered once, for the whole tranche, in this
commit rather than in each of the nine, so the one-line conflict with the
concurrent T7 branch stays a single hunk. Every earlier commit in this tranche
sits BELOW its ceiling, which the gate treats as a pass, so bisect is unaffected.

Worth recording, because the number under-reports the work: T5 converts EIGHT
producer call sites in `oracle.rs` and the ledger cannot see any of them. The
grep needle lives in `trigger_at`'s BODY (`oracle.rs:3576`), not at its call
sites, and `trigger_at` must survive while `drain_result_vectors` and the three
T10-blocked `__item` sites still call it. So `oracle.rs` legitimately stays at
20. The ratchet measures helper deletion, not producer conversion; do not read
the flat number as "T5 did nothing".

Non-vacuity: `wizard_class` held two `PreLoweredTrigger` nodes and converts
exactly ONE — the level-gained trigger. The card's "Whenever you draw a card"
Level-3 trigger still emits `PreLoweredTrigger` from a different, unconverted
`oracle_class.rs` site, so this fixture also shows the conversion is scoped to
the arm it claims. `wizard_class_two_layer_lowered.snap` is byte-unchanged.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 4866-4871: The trigger construction around
TriggerNodeIr::from_definition must preserve and model the printed “hasn’t been
exerted this turn” permission condition. Add this condition to the reusable
exert-as-attacks permission or linked-trigger building block before creating the
resulting Exerted trigger, rather than attaching it to the post-choice Exerted
trigger or special-casing Combat Celebrant.
🪄 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: 7160232c-200f-440b-98fb-125dc5b63ee8

📥 Commits

Reviewing files that changed from the base of the PR and between 8c4ea47 and 29414b1.

⛔ Files ignored due to path filters (17)
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__ahn_crop_champion_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__ahn_crop_champion_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__armor_of_thorns_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__armor_of_thorns_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bumper_cars_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bumper_cars_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__entropic_battlecruiser_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__entropic_battlecruiser_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__history_of_benalia_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__history_of_benalia_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__lighthouse_chronologist_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__lighthouse_chronologist_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__themberchaud_ir.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__themberchaud_lowered.snap is excluded by !**/*.snap, !**/snapshots/**
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__wizard_class_two_layer_ir.snap is excluded by !**/*.snap, !**/snapshots/**
📒 Files selected for processing (4)
  • crates/engine/src/parser/oracle.rs
  • crates/engine/src/parser/oracle_class.rs
  • crates/engine/src/parser/oracle_ir/snapshot_tests.rs
  • scripts/prelowered-ratchet.txt

Comment on lines +4866 to +4871
// The leading if-gate this arm dispatches on is still DROPPED —
// no condition is stamped for "hasn't been exerted this turn".
// That gap is pre-existing and deliberately preserved here: the
// conversion is behavior-identical, and the fix belongs in a
// change that is allowed to move bytes.
emitter.trigger_ir_at(item_line, TriggerNodeIr::from_definition(&line, trigger));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Model the conditional exert permission instead of dropping it.

This permits Combat Celebrant’s linked effect after it is untapped and exerted again in the same turn. CR 701.43b permits repeat exertion, so the printed pre-exert condition is the required once-per-turn gate. Represent it on the exert-as-attacks permission/linked-trigger building block—not on the resulting Exerted trigger, which occurs after the choice. (blogs.magicjudges.org)

As per coding guidelines, “Build reusable card-class building blocks rather than one-card special cases.”

🤖 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 4866 - 4871, The trigger
construction around TriggerNodeIr::from_definition must preserve and model the
printed “hasn’t been exerted this turn” permission condition. Add this condition
to the reusable exert-as-attacks permission or linked-trigger building block
before creating the resulting Exerted trigger, rather than attaching it to the
post-choice Exerted trigger or special-casing Combat Celebrant.

Sources: Coding guidelines, Path instructions

@matthewevans
matthewevans added this pull request to the merge queue Jul 27, 2026
@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

Merged via the queue into main with commit e9a268a Jul 27, 2026
16 checks passed
@matthewevans
matthewevans deleted the ship/plan05b-t5 branch July 27, 2026 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant