fix(parser): support definite-article back-reference reveal ("Reveal the card") - #4804
Conversation
"Reveal the card." / "Reveal the cards." fell through to
Effect::Unimplemented, unlike the semantically identical pronoun/
demonstrative forms ("Reveal it." / "Reveal that card." / "Reveal those
cards.") which already lower to Effect::Reveal { target: ParentTarget }
(CR 701.20a). Surfaced by the repo`s own `cargo parser-gaps` audit
(A_verb_variation / reveal).
Add the definite-article forms to parse_hand_reveal_ast, matched as a
whole-clause (anchored) match rather than a prefix so collision-prone
compound clauses ("reveal the cards you want to splice onto it",
"reveal the cards in your library") are left to their own recognizers.
Adds a positive parser test and an anti-hijack guard test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the back-reference reveal parsing logic in crates/engine/src/parser/oracle_effect/imperative.rs to support definite-article forms ("the card" / "the cards") as whole-clause matches. Feedback on this change highlights a violation of repository style guides: the implementation uses verbatim string matching with matches! instead of the required nom combinators (Rule R1) and lacks the mandatory CR annotation (Rule R6). Refactoring using nom combinators is recommended to align with the project's architectural rules.
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.
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Definite-article reveal is implemented with exact string matching instead of parser combinators. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:2858. Why it matters: parser dispatch in this repo is required to compose nom axes rather than add verbatim Oracle string matches, and this branch can be expressed with the already-imported all_consuming/alt/tag combinators while preserving the whole-clause anti-hijack behavior. Suggested fix: replace the matches!(after_reveal_lower.trim().trim_end_matches('.').trim(), "the card" | "the cards") branch with an all_consuming nom parser for the card/the cards plus optional period/trimming.
Reviewed current head 7e22e7d10b4afd84cb68c8857d9d1088d7283c62.
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Definite-article reveal is still implemented with exact string matching instead of nom combinators. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:2858. Why it matters: parser dispatch in this repo requires composable nom axes, and this branch still uses matches!(after_reveal_lower.trim().trim_end_matches('.').trim(), "the card" | "the cards") for the new grammar form. Suggested fix: replace this branch with an all_consuming nom parser for the card / the cards plus optional trailing punctuation/space handling.
Address review feedback on the definite-article back-reference reveal
branch: replace the verbatim `matches!` string comparison with an
`all_consuming` nom parser (Rule Zero — nom combinators are mandatory),
preserving the whole-clause anti-hijack anchoring, and lead the branch
comment with the CR 701.20a annotation.
No behavior change: "Reveal the card." / "Reveal the cards." still lower
to Effect::Reveal { ParentTarget }; compound clauses still fall through.
Full engine suite green (14459 passed).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review — addressed both points in b1d96ef:
No behavior change; full engine suite green (14460 passed, 0 failed). |
matthewevans
left a comment
There was a problem hiding this comment.
Approved at this head. The definite-article reveal back-reference is now anchored as a whole-clause nom parse, so bare "Reveal the card/cards." lowers to Reveal { ParentTarget } without hijacking compound reveal clauses. The positive and negative parser tests cover the regression shape.
Summary
Bare back-reference reveal clauses using the definite article —
Reveal the card./Reveal the cards.— fell through toEffect::Unimplementedin the oracle-text parser, even though the semantically identical pronoun/demonstrative forms (Reveal it./Reveal that card./Reveal those cards.) were already supported and lower toEffect::Reveal { target: ParentTarget }(CR 701.20a).This was surfaced by the repo's own
cargo parser-gapsaudit (A_verb_variation / reveal), which reportsreveal the cardandreveal the cardsas distinct unsupported patterns. Thesequence.rs::is_search_result_reveal_clausecontinuation recognizer already listsreveal the cardin its vocabulary, so the imperative recognizerparse_hand_reveal_astwas simply inconsistent with it.Change
In
parse_hand_reveal_ast(crates/engine/src/parser/oracle_effect/imperative.rs), add the definite-article forms so a bareReveal the card./Reveal the cards.clause lowers to the sameEffect::Reveal { target: ParentTarget }back-reference as the existing pronoun forms.These forms are matched as a whole-clause (anchored) match, not a prefix. The pronoun forms can stay loose prefix matches because no real card continues them divergently, but the definite-article forms are highly collision-prone at the prefix — e.g.:
reveal the cards you want to splice onto it(splice selection)reveal the cards in your library(library reveal)These are distinct effects that merely share the
reveal the card(s)prefix. Anchoring restricts the new lowering to a bare back-reference clause and leaves compound clauses to their own recognizers.Tests
crates/engine/src/parser/oracle_effect/tests.rs:reveal_the_card_backref_lowers_to_reveal_parent_target— the definite-article forms lower toEffect::Reveal { ParentTarget }, and the pre-existing pronoun/demonstrative forms keep working (regression anchors).reveal_the_card_backref_does_not_hijack_compound_clauses— compound clauses that start withreveal the card(s)are not misclassified as a bare back-reference.Verified against the full engine unit-test suite (
cargo test -p engine --lib): the new positive test fails before the change (Reveal the card.→Unimplemented) and passes after, with no regressions across the suite.