fix(parser): token Flying keyword before equal-to count - #3151
Conversation
Strip "equal to …" before token keyword parsing so Broodspinner-style "flying equal to the number of …" insects keep the Flying keyword. Fixes phase-rs#2854 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request addresses issue #2854 by updating the token keyword clause parser to handle "equal to" count suffixes, ensuring that tokens with flying (like those from Broodspinner) are correctly parsed with the flying keyword. It also adds a card fixture for Wren's Run Packmaster and an integration test to verify that flying attackers cannot be blocked by creatures without flying or reach. Feedback on the pull request points out a violation of the repository's hard architectural rule (R1) due to the use of raw string split operations instead of nom combinators for parser 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.
| .split(" where ") | ||
| .next() | ||
| .unwrap_or(after_with) | ||
| .split(" equal to ") | ||
| .next() | ||
| .unwrap_or(after_with) | ||
| .split(" attached ") | ||
| .next() | ||
| .unwrap_or(after_with) |
There was a problem hiding this comment.
[HIGH] Avoid raw string split operations or verbatim string matching for parsing Oracle phrases.
Why it matters: Manual string splitting and verbatim matching bypass the robust nom-based parser architecture, creating fragile matches and violating repository guidelines.
Suggested fix: Decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic nom combinator aggregates (such as nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
// Use nom combinators to parse the clauses modularly
let (input, clause) = alt((
terminated(take_until(" where "), tag(" where ")),
terminated(take_until(" equal to "), tag(" equal to ")),
terminated(take_until(" attached "), tag(" attached ")),
))(raw_clause).unwrap_or(("", raw_clause));References
- Every new parser dispatch under crates/engine/src/parser/ must use nom combinators. Raw string split operations or manual string splitting for parsing dispatch are prohibited. (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.
Replace forbidden .split(" equal to ") dispatch with take_until-based
helpers so the parser combinator gate passes on PR phase-rs#3151.
Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer pass on current head ca07817143a0:
- The parser fix is at the right seam: token keyword clause parsing now strips dynamic suffix clauses before keyword mapping, so
with flying equal to ...keepsFlyingwithout changing the count expression. - I pushed a maintainer test cleanup so the runtime regression now parses Broodspinner's actual Oracle through
parse_oracle_text; reverting the parser fix makes the parsed token effect loseFlyingbefore the combat assertion. - Verified locally:
cargo fmt --all,./scripts/check-parser-combinators.sh, targeted parser unit tests, targeted #2854 integration test, and the pre-push hook through clippy/card-data/parser/phase-ai before SSH transport dropped; final push used--no-verifyonly to avoid rerunning the same hook after that disconnect.
For future PRs, please sync with origin/main before opening/updating, and use the /engine-implementer skill for non-trivial parser/engine work so the first pass includes seam analysis and discriminating production-path tests.
The nom take_until quote strip used the remaining input instead of the head, so "with toxic 1 and \"~ can't block.\"" lost Toxic(1) and broke White Sun's Twilight integration tests on PR phase-rs#3151. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
There was a problem hiding this comment.
Approved at the current head after re-reviewing the post-approval test-strengthening commit. The production parser change remains the reviewed suffix-strip fix, and the added tests now explicitly cover both "This token can't block." and "~ can't block." quoted static suffixes preserving toxic 1.
For future PRs, please sync with origin/main before handoff and use the /engine-implementer skill for non-trivial engine/parser work so sibling cases and production-path tests are covered before review.
Summary
Test plan
cargo test -p engine --lib broodspinner_insectcargo test -p engine --lib keyword_clause_with_equal_to_count_suffixcargo test -p engine --test integration issue_2854cargo clippy -p engine -- -D warningsFixes #2854
Made with Cursor