fix(parser): scope enchanted-creature's-controller phase triggers (#5275)#5317
Conversation
…ase-rs#5275) Super Intelligence -- "At the beginning of the upkeep of enchanted creature's controller, that player draws a card." -- behaved like a Howling Mine: the phase trigger carried no player scope, so it fired on EVERY player's upkeep instead of only the enchanted creature's controller's. try_parse_phase_trigger scoped "enchanted player's <phase>" (player- enchant Auras) to AttachedTo but had no handling for the creature-enchant "the <phase> of enchanted creature's controller" form, leaving valid_target unset (any player). Recognize that form and scope valid_target to ParentTargetController, and add a trigger-side player_matches_filter arm that resolves it to the controller of the permanent the Aura is attached to. Per CR 303.4e the Aura's controller and the enchanted permanent's controller may differ, so this cannot reuse the source-controller turn constraint. No new engine variant -- reuses the existing ParentTargetController filter whose documented resolution already covers the AttachedTo host of an Aura phase trigger. Adds tests: the Super Intelligence upkeep scope (ParentTargetController) and a Howling-Mine regression ("each player's upkeep" stays unscoped). CR 303.4e (Aura vs enchanted-permanent controller) + CR 109.4 (controller of an object) + CR 503.1 (upkeep step).
There was a problem hiding this comment.
Code Review
This pull request implements scoping for Aura phase triggers to the controller of the enchanted permanent or creature (e.g., "enchanted creature's controller") by introducing TargetFilter::ParentTargetController and parsing the corresponding Oracle phrases. However, the parser implementation in oracle_trigger.rs uses fragile substring checks (scan_contains) instead of idiomatic nom combinators, which violates the repository's strict architectural rule (R1) requiring combinators for parsing dispatch.
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.
| else if scan_contains(phase_text, "enchanted creature's controller") | ||
| || scan_contains(phase_text, "enchanted creature\u{2019}s controller") | ||
| || scan_contains(phase_text, "enchanted permanent's controller") | ||
| || scan_contains(phase_text, "enchanted permanent\u{2019}s controller") |
There was a problem hiding this comment.
Avoid using fragile substring checks like scan_contains for parsing Oracle phrases, as this bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases (such as "enchanted 's controller") into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
| else if scan_contains(phase_text, "enchanted creature's controller") | |
| || scan_contains(phase_text, "enchanted creature\u{2019}s controller") | |
| || scan_contains(phase_text, "enchanted permanent's controller") | |
| || scan_contains(phase_text, "enchanted permanent\u{2019}s controller") | |
| let mut enchanted_controller = preceded( | |
| tag("enchanted "), | |
| terminated(card_type, alt((tag("'s controller"), tag("’s controller")))) | |
| ); |
References
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Parse the enchanted-permanent controller phrase with composed nom axes instead of literal phrase scans. Evidence: crates/engine/src/parser/oracle_trigger.rs:11590 adds four scan_contains checks for the creature/permanent and ASCII/curly-apostrophe variants. Why it matters: this is new Oracle parsing dispatch, and the project rule is to compose the grammar dimensions (enchanted + object kind + possessive controller) with nom combinators so future sibling forms do not require another cartesian list of literals. Suggested fix: add a small phase-trigger helper that parses enchanted {creature|permanent}{'s|’s} controller with tag/alt/preceded/terminated (or an existing possessive helper if one fits), then set ParentTargetController from that parsed result.
Parse changes introduced by this PR · 27 card(s), 1 signature(s) (baseline: main
|
…ators Addresses review feedback (R1) on phase-rs#5317. The enchanted-permanent controller scope was matched with four scan_contains literals (creature/ permanent x ASCII/curly apostrophe) — a cartesian list. Replace it with a single composed combinator, parse_enchanted_controller_phrase, that parses the axes with tag/alt ("enchanted " + {creature|permanent} + {'s|’s} + " controller"), matched at any word boundary via scan_at_word_boundaries (the phrase trails the phase noun, e.g. "the upkeep of …"). A new object- kind sibling is now one alt() arm. Behavior-preserving; no logic change.
|
Thanks — addressed in |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: current head resolves the prior nom-combinator finding; parse diff is scoped to enchanted permanent controller phase triggers.
Summary
[[Super Intelligence]] —
"Enchant creature\nAt the beginning of the upkeep of enchanted creature's controller, that player draws a card."— behaved like a Howling Mine, giving the extra card on every player's upkeep instead of only the enchanted creature's controller's upkeep.Root cause:
try_parse_phase_triggerscopes"enchanted player's <phase>"(player-enchant Auras) toAttachedTo, but had no handling for the creature-enchant"the <phase> of enchanted creature's controller"form. That leftvalid_targetunset, andmatch_phasetreats an unset player scope as "any active player" — so the trigger fired on every upkeep.Fixes #5275
Fix
Two small, localized changes — no new engine variant:
try_parse_phase_trigger): recognize"enchanted creature's controller"/"enchanted permanent's controller"(ASCII +’) in the phase text and setvalid_target = TargetFilter::ParentTargetController, mirroring the existing"enchanted player's"→AttachedToarm.player_matches_filter, trigger side): add aParentTargetControllerarm that resolves to the controller of the permanent the source Aura is attached to and compares it to the active player.Per CR 303.4e, the Aura's controller and the enchanted permanent's controller need not be the same, so this can't reuse the source-controller turn constraint (
OnlyDuringYourTurn).ParentTargetController's documented resolution already covers "the AttachedTo host on source for Aura phase triggers," so it's the intended representation here; no trigger currently used it asvalid_target, so the new matcher arm is additive.CR references
Verification
cargo fmt --all— cleanscripts/check-parser-combinators.sh— cleanparser::oracle::tests:super_intelligence_upkeep_scoped_to_enchanted_creature_controller— assertsphase == Upkeep,valid_target == ParentTargetController, and the card fully parses (noUnimplemented).each_player_upkeep_phase_trigger_stays_unscoped— regression: a genuine "each player's upkeep" Howling-Mine trigger keepsvalid_target == None(fires every turn).parser::oracle::testssuite green.Scope note
The
"enchanted player's <phase>"player-enchant path (AttachedTo) is unchanged; the new arm iselse if, so player-enchant Auras are unaffected. The existing Aggression card ("the end step of enchanted creature's controller") now also gains correct turn-owner scoping as a member of the same class.