fix(parser): recognize "you exile ... this way" reflexive triggers - #6193
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for parsing reflexive 'when you exile a card this way' clauses (CR 603.12 + CR 701.16a) to resolve issues like Ardyn, the Usurper's ability (issue #5989). It introduces the parse_you_exile_this_way_clause parser using nom combinators, integrates it into strip_if_you_do_conditional, and adds a comprehensive integration test to verify the functionality. There are no review comments, and I have no feedback to provide.
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.
Parse changes introduced by this PR · 5 card(s), 7 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Blocking: the added grammar does not match Ardyn's current Oracle text
Scryfall lists Ardyn's reflexive clause as “If you exiled a card this way, …” (card API). This PR instead adds a when you exile … this way parser and its integration test replaces the printed triggered ability with a synthetic activated ability containing that same When wording (and an extra may).
strip_if_you_do_conditional reaches the new arm only inside if prefix == "when "; the existing passive parser starts with an article/type phrase, so it cannot consume the actual active-past if you exiled a card this way form either. Consequently the real card still falls through instead of gaining the conditional token-copy effect, despite the green test.
Please parse the actual active-past if you exiled … this way construction (at the shared reflexive-condition seam, without regressing the existing passive forms), and replace/add a regression that uses Ardyn's exact current Oracle text and its triggered combat ability. The test should prove the printed path, not a hand-written activated approximation.
Fixes phase-rs#5989. Ardyn, the Usurper: "At the beginning of combat on your turn, you may exile up to one target creature card from a graveyard. When you exile a card this way, create a token that's a copy of it, except it's a 5/5 black Demon." ## Root cause & fix strip_if_you_do_conditional (the reflexive "when you <verb> this way" gate recognizer) already had active-voice combinators for "you discard"/"you sacrifice" this way, but no "you exile" arm. The entire "When you exile a card this way, create a token..." clause fell through to Effect::Unimplemented -- the ability exiled the target and then silently did nothing, matching the reported "I have to make the copy myself" symptom. Fixed by adding parse_you_exile_this_way_clause (parser/oracle_nom/condition.rs), mirroring the existing parse_you_discard_this_way_clause / parse_you_sacrifice_this_way_clause siblings (same active-voice grammar, differing only in the verb and that exile's destination isn't fixed to a single zone the way discard/sacrifice are), and wiring it into strip_if_you_do_conditional (parser/oracle_effect/conditions.rs) alongside the existing two. Also investigated the issue's own paraphrase -- "only works if I do it to my graveyard" -- and disproved it: the parsed exile target filter carries no controller restriction (any player's graveyard is a legal source), so this reads as an imprecise description of the same underlying "nothing happens" bug, not a second, narrower defect. ## Testing New end-to-end test drives the real activate -> target-selection -> resolve -> optional-effect-decision -> reflexive-copy pipeline (not a hand-built ResolvedAbility), with two legal graveyard targets -- one in EACH player's graveyard -- so target selection genuinely pauses and the opponent's-graveyard case is explicitly exercised. Confirms the resulting token is a distinct 5/5 black Demon. Verified locally: fmt clean, clippy clean (-D warnings), full integration suite 3546 tests (3543 pre-existing + 2 pre-existing ignored + 1 new), 0 regressions.
Review follow-up (phase-rs#5989): Ardyn's current Oracle text is the active-PAST "If you exiled a card this way," — not the "When you exile ... this way" form the previous head parsed and tested against a synthetic activated ability. Parser: parse_you_exile_this_way_clause now accepts both tenses ("exile"/"exiled") and runs under BOTH the "if " and "when " prefixes (hoisted out of the when-only block, like the other active arms). Separately, the context-free condition grammar was reading "you exiled a card this way" as CostPaidObjectMatchesFilter (the Shilgengar cost-paid class); with no cost snapshot its runtime fallback constrains the moved object's controller to the ability controller, so a card exiled from an OPPONENT's graveyard failed the gate — the reported "only works on my own graveyard" symptom. Added rewrite_cost_paid_exiled_reflexive_for_effect_exile_parent: when the preceding non-continuation clause is a ChangeZone{Exile} EFFECT, the gate rebinds to ZoneChangedThisWay; the cost-paid class (no such preceding clause) and the subject-scoped sacrifice class (Deadly Brew) are untouched. Tests now prove the PRINTED path: the integration test drives Ardyn's full exact Oracle text (Demon anthem line + Starscourge ability word) through the real begin-combat trigger — phase advance, TriggerTarget- Selection between two graveyard candidates (one per player), mandatory exile of the opponent's card, automatic reflexive copy, and asserts the token is a copy of THAT card (name), a 5/5 Demon, and receives haste from Ardyn's own anthem. A parser-level test pins the exact reflexive sentence pair lowering to a ZoneChangedThisWay-gated CopyTokenOf over the tracked set. Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17516 passed; engine integration 3749 passed.
272d7fe to
044a36b
Compare
|
Addressed the blocker — the PR now parses and proves Ardyn's actual printed text. Grammar: A second, deeper mis-binding found while fixing this: the context-free condition grammar was already consuming "you exiled a card this way" as Tests: the integration test now drives Ardyn's full exact Oracle text (Demon anthem + Verified locally in an isolated target dir: fmt clean, workspace clippy |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser now recognizes active-voice “you exile[d] … this way” clauses, rewrites matching conditions based on preceding exile effects, and adds unit and integration coverage for Ardyn’s exile-and-copy ability. ChangesExile reflexive condition handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant ConditionParser
participant EffectBuilder
participant GameTest
OracleText->>ConditionParser: Parse “you exiled a card this way”
ConditionParser-->>EffectBuilder: ZoneChangedThisWay condition
EffectBuilder->>EffectBuilder: Inspect preceding exile ChangeZone clause
EffectBuilder-->>GameTest: Parsed exile-and-copy effect
GameTest->>GameTest: Resolve exile and create copied token
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
There was a problem hiding this comment.
Re-reviewed the refreshed head: the exact-Ardyn regression and the clause-context rewrite address the prior blocker, and the current CodeRabbit review has no actionable finding. Holding rather than approving while the refreshed CI run and a parse-diff artifact tied to this head complete.
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head cf6c2d2f39aa79e4e2c6c62bfb77ed67d158606b: the exact-Ardyn regression, shared active-voice reflexive parser, and clause-context rebind resolve the prior finding. Current CI and parse-diff evidence are clean.
…hase-rs#6193) * fix(parser): recognize "you exile ... this way" reflexive triggers Fixes phase-rs#5989. Ardyn, the Usurper: "At the beginning of combat on your turn, you may exile up to one target creature card from a graveyard. When you exile a card this way, create a token that's a copy of it, except it's a 5/5 black Demon." ## Root cause & fix strip_if_you_do_conditional (the reflexive "when you <verb> this way" gate recognizer) already had active-voice combinators for "you discard"/"you sacrifice" this way, but no "you exile" arm. The entire "When you exile a card this way, create a token..." clause fell through to Effect::Unimplemented -- the ability exiled the target and then silently did nothing, matching the reported "I have to make the copy myself" symptom. Fixed by adding parse_you_exile_this_way_clause (parser/oracle_nom/condition.rs), mirroring the existing parse_you_discard_this_way_clause / parse_you_sacrifice_this_way_clause siblings (same active-voice grammar, differing only in the verb and that exile's destination isn't fixed to a single zone the way discard/sacrifice are), and wiring it into strip_if_you_do_conditional (parser/oracle_effect/conditions.rs) alongside the existing two. Also investigated the issue's own paraphrase -- "only works if I do it to my graveyard" -- and disproved it: the parsed exile target filter carries no controller restriction (any player's graveyard is a legal source), so this reads as an imprecise description of the same underlying "nothing happens" bug, not a second, narrower defect. ## Testing New end-to-end test drives the real activate -> target-selection -> resolve -> optional-effect-decision -> reflexive-copy pipeline (not a hand-built ResolvedAbility), with two legal graveyard targets -- one in EACH player's graveyard -- so target selection genuinely pauses and the opponent's-graveyard case is explicitly exercised. Confirms the resulting token is a distinct 5/5 black Demon. Verified locally: fmt clean, clippy clean (-D warnings), full integration suite 3546 tests (3543 pre-existing + 2 pre-existing ignored + 1 new), 0 regressions. * fix(parser): bind Ardyn's actual active-past reflexive exile gate Review follow-up (phase-rs#5989): Ardyn's current Oracle text is the active-PAST "If you exiled a card this way," — not the "When you exile ... this way" form the previous head parsed and tested against a synthetic activated ability. Parser: parse_you_exile_this_way_clause now accepts both tenses ("exile"/"exiled") and runs under BOTH the "if " and "when " prefixes (hoisted out of the when-only block, like the other active arms). Separately, the context-free condition grammar was reading "you exiled a card this way" as CostPaidObjectMatchesFilter (the Shilgengar cost-paid class); with no cost snapshot its runtime fallback constrains the moved object's controller to the ability controller, so a card exiled from an OPPONENT's graveyard failed the gate — the reported "only works on my own graveyard" symptom. Added rewrite_cost_paid_exiled_reflexive_for_effect_exile_parent: when the preceding non-continuation clause is a ChangeZone{Exile} EFFECT, the gate rebinds to ZoneChangedThisWay; the cost-paid class (no such preceding clause) and the subject-scoped sacrifice class (Deadly Brew) are untouched. Tests now prove the PRINTED path: the integration test drives Ardyn's full exact Oracle text (Demon anthem line + Starscourge ability word) through the real begin-combat trigger — phase advance, TriggerTarget- Selection between two graveyard candidates (one per player), mandatory exile of the opponent's card, automatic reflexive copy, and asserts the token is a copy of THAT card (name), a 5/5 Demon, and receives haste from Ardyn's own anthem. A parser-level test pins the exact reflexive sentence pair lowering to a ZoneChangedThisWay-gated CopyTokenOf over the tracked set. Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17516 passed; engine integration 3749 passed. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Fixes #5989.
Ardyn, the Usurper: "At the beginning of combat on your turn, you may exile up to one target creature card from a graveyard. When you exile a card this way, create a token that's a copy of it, except it's a 5/5 black Demon."
Root cause & fix
strip_if_you_do_conditional(the reflexive "when you this way" gate recognizer) already had active-voice combinators for "you discard"/"you sacrifice" this way, but no "you exile" arm. The entire "When you exile a card this way, create a token..." clause fell through toEffect::Unimplemented— the ability exiled the target and then silently did nothing, matching the reported "I have to make the copy myself" symptom.Fixed by adding
parse_you_exile_this_way_clause(parser/oracle_nom/condition.rs), mirroring the existingparse_you_discard_this_way_clause/parse_you_sacrifice_this_way_clausesiblings (same active-voice grammar, differing only in the verb and that exile's destination isn't fixed to a single zone the way discard/sacrifice are), and wiring it intostrip_if_you_do_conditional(parser/oracle_effect/conditions.rs) alongside the existing two.Also investigated the issue's own paraphrase — "only works if I do it to my graveyard" — and disproved it: the parsed exile target filter carries no controller restriction (any player's graveyard is a legal source), so this reads as an imprecise description of the same underlying "nothing happens" bug, not a second, narrower defect.
Testing
New end-to-end test drives the real activate → target-selection → resolve → optional-effect-decision → reflexive-copy pipeline (not a hand-built
ResolvedAbility), with two legal graveyard targets — one in EACH player's graveyard — so target selection genuinely pauses and the opponent's-graveyard case is explicitly exercised. Confirms the resulting token is a distinct 5/5 black Demon.Verified locally: fmt clean, clippy clean (
-D warnings), full integration suite 3546 tests (3543 pre-existing + 2 pre-existing ignored + 1 new), 0 regressions.Summary by CodeRabbit