fix(parser): gate Ketramose's batched exile trigger on source zones + own turn (#4952)#5757
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing player-agnostic source zones (such as 'graveyards' or 'the battlefield') and optional 'during your turn' constraints on batched exile triggers, along with a corresponding test case. The feedback requests correcting the Magic Comprehensive Rules (CR) citations in the comments to adhere to Rule R6 of the style guide: changing CR 400.1 to CR 400.3 for zone types, and CR 603.4 to CR 603.2 for the turn-based trigger restriction.
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.
| // CR 400.1: source zones expressed player-agnostically — bare plural | ||
| // "graveyards"/"libraries" (any player's), "a graveyard", or "the | ||
| // battlefield" (Ketramose, the New Dawn: "…from graveyards and/or the | ||
| // battlefield"). The batched trigger keys on the zone TYPE the exile | ||
| // came from, so these lower to the same `Zone` as the "your" forms. |
There was a problem hiding this comment.
The comment cites CR 400.1, which only lists the game's zones. However, the distinction between the singular shared zone ('the battlefield') and the plural player-specific zones ('graveyards', 'libraries') is specifically defined in CR 400.3 ('...the battlefield and the stack are shared zones. Each player has their own library, hand, graveyard, and exile zone.'). Please update the citation to CR 400.3 to adhere to style guide rule R6.
| // CR 400.1: source zones expressed player-agnostically — bare plural | |
| // "graveyards"/"libraries" (any player's), "a graveyard", or "the | |
| // battlefield" (Ketramose, the New Dawn: "…from graveyards and/or the | |
| // battlefield"). The batched trigger keys on the zone TYPE the exile | |
| // came from, so these lower to the same `Zone` as the "your" forms. | |
| // CR 400.3: source zones expressed player-agnostically — bare plural | |
| // "graveyards"/"libraries" (any player's), "a graveyard", or "the | |
| // battlefield" (Ketramose, the New Dawn: "…from graveyards and/or the | |
| // battlefield"). The batched trigger keys on the zone TYPE the exile | |
| // came from, so these lower to the same `Zone` as the "your" forms. |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . The cited rule's body must describe what the code is doing. (link)
| // CR 603.4: an optional trailing " during your turn" restricts the batched | ||
| // trigger to the controller's own turn (Ketramose, the New Dawn — "…are | ||
| // put into exile from graveyards and/or the battlefield during your | ||
| // turn"). Peel it here rather than bailing, so the disjunctive origin | ||
| // (graveyards + battlefield) is captured AND the own-turn gate applies — | ||
| // otherwise the line falls through to a less-precise parser that drops | ||
| // both (firing on any turn and on exile from any zone, e.g. hand). |
There was a problem hiding this comment.
The comment cites CR 603.4, which governs intervening 'if' clauses. However, 'during your turn' is not an intervening 'if' clause (which requires the literal word 'if' and is checked both on trigger and resolution). Instead, it is a restriction on the trigger event itself, which is governed by CR 603.2 ('These abilities trigger when the game state matches the trigger event.'). Please update the citation to CR 603.2 to adhere to style guide rule R6.
| // CR 603.4: an optional trailing " during your turn" restricts the batched | |
| // trigger to the controller's own turn (Ketramose, the New Dawn — "…are | |
| // put into exile from graveyards and/or the battlefield during your | |
| // turn"). Peel it here rather than bailing, so the disjunctive origin | |
| // (graveyards + battlefield) is captured AND the own-turn gate applies — | |
| // otherwise the line falls through to a less-precise parser that drops | |
| // both (firing on any turn and on exile from any zone, e.g. hand). | |
| // CR 603.2: an optional trailing " during your turn" restricts the batched | |
| // trigger to the controller's own turn (Ketramose, the New Dawn — "…are | |
| // put into exile from graveyards and/or the battlefield during your | |
| // turn"). Peel it here rather than bailing, so the disjunctive origin | |
| // (graveyards + battlefield) is captured AND the own-turn gate applies — | |
| // otherwise the line falls through to a less-precise parser that drops | |
| // both (firing on any turn and on exile from any zone, e.g. hand). |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . The cited rule's body must describe what the code is doing. (link)
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the parser fix is correct in shape, but its trigger behavior lacks production-pipeline proof.
🔴 Blocker
crates/engine/src/parser/oracle_trigger_tests.rs:23705-23739only inspects the parsedTriggerDefinition; it never drivesChangesZoneAllthroughprocess_triggers/the scenario runner. The new fields are consumed atcrates/engine/src/game/trigger_matchers.rs:1133-1154(origin_zones) andcrates/engine/src/game/triggers.rs:6411(OnlyDuringYourTurn), so an AST assertion cannot prove the issue’s two runtime claims. Add a registered integration test using the real Ketramose Oracle text and actual zone-change events: positive for graveyard/battlefield exile on its controller's turn, negative for exile from hand and for the opponent's turn. The verified Oracle trigger is: “Whenever one or more cards are put into exile from graveyards and/or the battlefield during your turn”.
🟡 Non-blocking
crates/engine/src/parser/oracle_trigger.rs:12089citesCR 603.4forduring your turn, but 603.4 is only the immediate intervening-ifrule. The official rules define trigger conditions/events inCR 603.1and event matching inCR 603.2; update the annotation while adding the runtime coverage. Gemini's proposedCR 400.3is not correct for zone kinds (that rule concerns an object going to another player's library, graveyard, or hand); the existingCR 400.1zone reference is appropriate.
✅ Clean
parse_your_zone_tokenandtry_parse_one_or_more_put_into_exile_fromuse the existing composable zone-set and trigger-constraint vocabulary at the correct parser seam. The current parse artifact is scoped to Ketramose alone and records the intendedChangesZoneAllplus own-turn constraint.
Recommendation: request-changes — add the discriminating real-event integration coverage and correct the CR annotation; then resubmit the current head.
… own turn (phase-rs#4952) Ketramose, the New Dawn's "Whenever one or more cards are put into exile from graveyards and/or the battlefield during your turn" trigger fired on the opponent's turn and when exiling a card from hand. try_parse_one_or_more_put_into_exile_from bailed on the trailing " during your turn" clause, so the line fell through to a less-precise parser that dropped the source-zone set and the own-turn gate (origin null, battlefield-only, no constraint). And parse_your_zone_token only recognized "your library"/"your graveyard", not the player-agnostic "graveyards" / "the battlefield" forms. - parse_your_zone_token: accept bare-plural "graveyards"/"libraries", "a graveyard", and "the battlefield". - try_parse_one_or_more_put_into_exile_from: peel an optional trailing " during your turn" and set TriggerConstraint::OnlyDuringYourTurn instead of bailing. Ketramose now parses to ChangesZoneAll with origin_zones = [Graveyard, Battlefield] and constraint = OnlyDuringYourTurn.
7b48f62 to
da3ce2f
Compare
|
Both review points addressed in 🔴 Blocker — registered game-pipeline integration test. Added
🟡 Non-blocking — CR annotation. Corrected Local: all 4 integration tests pass, parser unit test passes, |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
APPROVED — the prior runtime-proof and CR-annotation blockers are resolved on the current head.
🔴 Blocker
- None.
🟡 Non-blocking
- The remaining Gemini thread at
crates/engine/src/parser/oracle_trigger.rs:12053is refuted: verified CR 400.1 describes the zone classes and shared/player-owned distinction used by the parser, while CR 400.3 governs an object moving to another player's library, graveyard, or hand.
✅ Clean
ketramose_exile_trigger_gate_4952.rs:66-222drives real spells, zone-change events, trigger matching, and stack resolution from Ketramose's real Oracle text. It discriminates both valid source zones and both excluded paths (hand and opponent's turn).- The current parse artifact changes Ketramose alone from the imprecise
ChangesZoneshape toChangesZoneAllwith the own-turn constraint; no unexplained card impact remains.
Recommendation: approve and enqueue as a parser bug fix.
2 participants
Fixes #4952.
Summary
Ketramose, the New Dawn — "Whenever one or more cards are put into exile from graveyards and/or the battlefield during your turn, you draw a card and lose 1 life" — fired on the opponent's turn and when a card was exiled from hand. Both restrictions were dropped by the parser.
Root cause
try_parse_one_or_more_put_into_exile_fromparsed the zone-set, then bailed on the trailingduring your turnclause — so the line fell through to a less-precise parser that producedorigin: null, battlefield-onlytrigger_zones, and no constraint (→ fires on any turn, and on exile from any zone incl. hand).parse_your_zone_tokenonly recognizedyour library/your graveyard, not the player-agnosticgraveyards/the battlefieldforms Ketramose uses.Fix (parser-only; existing runtime-enforced fields)
parse_your_zone_token: also accept bare-pluralgraveyards/libraries,a graveyard, andthe battlefield(all lower to the sameZonetype the "your" forms do).try_parse_one_or_more_put_into_exile_from: peel an optional trailingduring your turnand setTriggerConstraint::OnlyDuringYourTurninstead of bailing.Ketramose now parses to
ChangesZoneAllwithorigin_zones = [Graveyard, Battlefield]andconstraint = OnlyDuringYourTurn— both are already runtime-enforced mechanisms (OnlyDuringYourTurnset elsewhere for "only during your turn" cards;origin_zonesused by the batched exile family).CR references
CR 603.4 (own-turn intervening restriction) + CR 400.1 (source zones). Touched, not invented.
Verification (local, head
91f46db1e181d55b50aa229beb29f41a637438b8)cargo test -p engine --lib ketramose_exile_trigger_gated_on_source_zones_and_own_turn— ok (assertsorigin_zones⊇ {Graveyard, Battlefield} andconstraint == OnlyDuringYourTurn)cargo test -p engine --lib parser::oracle— 7915 passed, 0 failed (full sweep;parse_your_zone_tokenis used only by the batched-exile family, low blast radius)cargo fmt -p engine -- --check,cargo clippy -p engine --lib(-D warnings) — cleanscripts/check-parser-combinators.sh,scripts/check-engine-authorities.sh— passTransparency: did not run
scripts/ai-gate.sh(Gate A — fresh release build, disk tight). Happy to add a game-pipeline runtime witness (exile-from-graveyard-on-own-turn fires; from-hand / opponent-turn don't) if you'd like it, but this populates existing runtime-enforced fields.Claimed impact