fix(parser): compose Class-level trigger conditions instead of overwriting them (#5638) - #6021
Merged
Merged
Conversation
…iting them (phase-rs#5638) parse_class_oracle_text gated level-2+ triggers by unconditionally assigning trigger.condition = ClassLevelGE, which silently dropped any printed intervening-if already parsed onto the trigger. Intermediate Chirography's level-3 ability ("if a modified creature died under your control this turn") lost its death check entirely and always created a token at end step. Compose with TriggerCondition::And instead, mirroring the existing static/replacement class-level wrappers. Closes phase-rs#5638
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where class-level triggers with printed intervening-if conditions had their conditions unconditionally overwritten by the class level gate. The fix introduces wrap_trigger_with_class_level to compose both conditions using TriggerCondition::And instead of overwriting, and updates the corresponding tests and fixtures. The review feedback suggests flattening nested TriggerCondition::And structures to improve composability and using structured fields instead of fragile substring matching in tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+424
to
+428
| .find(|t| { | ||
| t.description | ||
| .as_deref() | ||
| .is_some_and(|d| d.contains("end step")) | ||
| }) |
Contributor
There was a problem hiding this comment.
Locating the trigger by searching for the substring "end step" in its description is a bit fragile. This could break if the description text is changed in the future. A more robust approach would be to match on the structured TriggerDefinition fields, such as mode and phase, which are less likely to change.
.find(|t| {
t.mode == crate::types::triggers::TriggerMode::Phase
&& t.phase == Some(crate::types::phase::Phase::End)
})
Parse changes introduced by this PR · 4 card(s), 4 signature(s) (baseline: main
|
…evel trigger Addresses Gemini review feedback on PR phase-rs#6021 (issue phase-rs#5638): - wrap_trigger_with_class_level now flattens into the existing And's conditions vec instead of nesting a new And around it, matching the composability convention used elsewhere in this module. - the level-3 trigger regression test now locates the end-step trigger by its structured mode/phase fields instead of a fragile substring match on the rendered description.
matthewevans
approved these changes
Jul 16, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved: the Class-level gate now composes with each printed intervening-if at the parser authority, and the current parse-diff is limited to the four expected Class triggers.
matthewevans
added a commit
that referenced
this pull request
Jul 27, 2026
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.
matthewevans
added a commit
to andriypolanski/phase
that referenced
this pull request
Jul 27, 2026
… the TriggerNodeIr seam (phase-rs#6709) * test(parser): baseline IR fixtures for the nine T5 trigger sites 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. * refactor(parser): emit Saga chapter triggers as IR (U0-08) 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. * refactor(parser): emit Attraction visit triggers as IR (U0-10) 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. * refactor(parser): emit leveler LEVEL-block triggers as IR (U0-13) 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 phase-rs#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. * refactor(parser): emit Spacecraft threshold triggers as IR (U0-15) 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. * refactor(parser): emit compound exert trigger as IR, bare form (U0-33) 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. * refactor(parser): emit compound exert trigger as IR, card-name form (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. * refactor(parser): emit conditional exert trigger as IR (U0-35) 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. * refactor(parser): emit synthesized flash-cleanup-sacrifice trigger as 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. * refactor(parser): emit Class level-gained triggers as IR (U0-54) 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. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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
2 participants
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.
fix(parser): compose Class-level trigger conditions instead of overwriting them (#5638)
parse_class_oracle_text gated level-2+ triggers by unconditionally
assigning trigger.condition = ClassLevelGE, which silently dropped any
printed intervening-if already parsed onto the trigger. Intermediate
Chirography's level-3 ability ("if a modified creature died under your
control this turn") lost its death check entirely and always created
a token at end step. Compose with TriggerCondition::And instead,
mirroring the existing static/replacement class-level wrappers.
Closes #5638