fix(parser): split Gathering Stone ETB/upkeep trigger and chosen-type gate (#1525) - #2769
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces parsing support and unit tests for the Oracle phrase "card of the chosen type" (CR 608.2c) within strip_card_type_conditional. The review feedback notes that using verbatim string matching bypasses the robust nom-based parser and fails to cover sibling variations such as "permanent card of the chosen type". It is recommended to decompose this phrase into modular, reusable nom combinators to ensure parser robustness and complete coverage.
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.
| if let Ok((after_chosen, _)) = | ||
| tag::<_, _, OracleError<'_>>("card of the chosen type").parse(rest) | ||
| { | ||
| let remainder = remainder_after_optional_comma(after_chosen); | ||
| let offset = text.len() - remainder.len(); | ||
| return ( | ||
| Some(maybe_negate( | ||
| AbilityCondition::RevealedHasCardType { | ||
| card_types: vec![], | ||
| additional_filter: Some(FilterProp::IsChosenCreatureType), | ||
| subtype_filter: None, | ||
| }, | ||
| negated, | ||
| )), | ||
| text[offset..].to_string(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing sibling coverage for "permanent card of the chosen type"
While CoreTypes (like Creature, Land, Artifact) handle the " of the chosen type" suffix, the type_word == "permanent" block does not handle this. If a card contains the phrase "If it's a permanent card of the chosen type, ...", parsing will fail downstream.
To resolve this, avoid adding verbatim string or suffix checks (such as checking for " of the chosen type" inside a string-matching block), as this bypasses the robust nom-based parser and creates fragile matches. Instead, decompose this compound phrase into modular, reusable parsers for its constituent parts and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to maintain parser robustness.
References
- Sibling coverage: If a parser arm or string was extended, are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? (link)
- 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.
mike-theDude
left a comment
There was a problem hiding this comment.
Architecture Review
The chosen-type gate is a correct, targeted fix and the enters+upkeep split mirrors the established compound-split family. Three items.
[MED] The split test checks modes but not the shared effect — the real risk. Evidence: trigger_compound_enters_and_upkeep_splits asserts defs.len()==2, defs[0].mode==ChangesZone, defs[1].mode==Phase, but not that both triggers carry the "look at the top card" effect. The split (split_once_on(cond_lower, " and at the beginning of ")) puts first = "When this artifact enters" (the part before the join) and second = "At the beginning of your upkeep, look at the top card…" — so the effect tail lives only in the second half. The new arm is structurally identical to the existing " and when " split (oracle_trigger.rs:~4096), so if that family re-shares the effect onto the first trigger this is fine; but as written the test would pass even if the ETB trigger ended up effect-less (fires, does nothing). Suggested fix: assert defs[0].execute carries the look-at-top-card effect (and ideally the chosen-type gate), not just its mode — that's what proves the split didn't drop the shared effect from the enters trigger.
[MED] (confirming Gemini) "permanent card of the chosen type" isn't handled. Evidence: oracle_effect/conditions.rs matches CoreType words (Creature/Land/Artifact) with the "of the chosen type" suffix, but the type_word == "permanent" block doesn't, so "if it's a permanent card of the chosen type" silently fails the gate. Add the suffix handling to the permanent arm so the chosen-type condition covers the permanent class too.
[LOW] Cross-PR shape drift on RevealedHasCardType. This PR constructs RevealedHasCardType { card_types: vec![], additional_filter, subtype_filter } (plural card_types), while open PRs #2754 and #2752 use/add card_type: CoreType + alt_card_types: Vec<CoreType>. Three concurrent PRs are shaping this variant differently — coordinate so it lands one consistent shape (the card_types: Vec generalization here is the cleaner one, and supersedes the card_type + alt_card_types split I flagged on #2754).
Net: the IsChosenCreatureType gate correctly stops the look/reveal chain from treating every top card as eligible, and the split follows the house pattern — but pin the shared effect on the ETB trigger in the test, cover "permanent … of the chosen type", and reconcile the RevealedHasCardType shape across the in-flight PRs.
|
Pushed maintainer follow-up changes for the review feedback:
I did not run local cargo builds/tests; leaving broad validation to GitHub CI per maintainer workflow. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer follow-up.
Evidence checked:
- Maintainer MED/Gemini sibling coverage feedback is addressed for
permanent card of the chosen typeat the existing permanent-card condition seam. - The compound enters/upkeep trigger test now verifies both triggers retain the shared
Digeffect and chosen-type reveal gate, not just their modes. - Current
origin/mainalready resolves theRevealedHasCardTypeshape drift to pluralcard_types. - Verified CR references locally for 608.2c, 603.2, and 205.3m.
- Verified
cargo fmt --all,./scripts/check-parser-combinators.sh, andgit diff --checkafter merging currentorigin/main.
Broad build/test validation is left to GitHub CI.
…e test Use unit-variant matches for TriggerMode::ChangesZone and TriggerMode::Phase.
Head branch was pushed to by a user without write access
Summary
RevealedHasCardTypewithIsChosenCreatureType, preventing the look/reveal chain from treating every top card as eligible.Anchored on
crates/engine/src/parser/oracle_trigger.rs:4112— existingand whenevercompound trigger splitcrates/engine/src/parser/oracle_effect/conditions.rs:762— Herald's Horn "creature card of the chosen type" conditionTest plan
cargo test -p engine trigger_compound_enters_and_upkeepcargo test -p engine card_of_the_chosen_typeFixes #1525
Made with Cursor