fix(parser): support N-ary "your choice of" keyword-grant lists#6192
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extends the MTG engine's parser to support N-branch keyword choice grants (such as Golem Artisan's three-way choice of flying, trample, or haste) by utilizing the nom-based list splitter, and adds corresponding integration tests. The review feedback points out a style guide violation where a rules-touching comment block in parse_keyword_choice_grant lacks the mandatory CR 608.2d annotation.
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.
| // Shape 1: "gain your choice of X, Y, or Z" — an explicit keyword-grant menu | ||
| // of two OR MORE options (Golem Artisan: "flying, trample, or haste"). Reuse | ||
| // the nom-based `split_choice_list_items` splitter (shared with the counter- | ||
| // choice and "from among" paths) so an Oxford-comma N-ary list parses without | ||
| // manual byte slicing. |
There was a problem hiding this comment.
According to the repository style guide, every rules-touching line of engine code must carry a comment of the form CR : . Since this block parses the choice of keywords governed by CR 608.2d, please add the appropriate CR annotation to the comment. Additionally, ensure we reuse the robust nom-based parser combinators rather than manual byte slicing or verbatim string matching to parse these Oracle phrases.
| // Shape 1: "gain your choice of X, Y, or Z" — an explicit keyword-grant menu | |
| // of two OR MORE options (Golem Artisan: "flying, trample, or haste"). Reuse | |
| // the nom-based `split_choice_list_items` splitter (shared with the counter- | |
| // choice and "from among" paths) so an Oxford-comma N-ary list parses without | |
| // manual byte slicing. | |
| // CR 608.2d: "gain your choice of X, Y, or Z" — an explicit keyword-grant menu | |
| // of two OR MORE options (Golem Artisan: "flying, trample, or haste"). Reuse | |
| // the nom-based split_choice_list_items splitter (shared with the counter- | |
| // choice and "from among" paths) so an Oxford-comma N-ary list parses without | |
| // manual byte slicing. |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (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 to prevent combinatorial explosion and improve maintainability.
Parse changes introduced by this PR · 15 card(s), 19 signature(s) (baseline: main
|
|
Maintainer update — nested keyword choices now remain honestly unsupported; fresh CI is pending. The original N-ary menu handling is correct for fixed keyword options such as Golem Artisan and Assassin Initiate. However, the current parse-diff also included Greater Morphling, whose Oracle text has nested selections: I pushed No approval yet: the new head needs the required checks and an updated parse-diff artifact. Once green, the fixed-option N-ary class can be re-reviewed without claiming unsupported nested choices. |
|
Maintainer CI repair — fresh checks running. GitHub CI identified two compile errors in the previous maintainer follow-up: both Fresh required CI and the parse-diff artifact are now pending; this PR remains unapproved until they complete successfully. |
|
Maintainer CI follow-up: the replacement run exposed that the new coverage-honesty test in |
|
Maintainer CI follow-up: the prior test-only visibility repair exposed two linked issues — the test module is a sibling of |
|
Maintainer CI repair — strict nested choices now stop before generic fallback. The previous strict-failure plumbing still returned Evidence: the failing current-head CI assertion in |
|
Maintainer status — processed current head The strict-fallback repair is now the PR head. The nested-choice regression is intentionally preserved as unsupported, parser gates plus independent adversarial source review passed, and the required GitHub run is still pending. No approval or queue action will occur until that run and its refreshed parse-diff complete. |
|
Maintainer CI repair — fixed the current Rust-lint failure; fresh CI required. GitHub's Rust-lint job failed only on |
|
Maintainer fixup on current head
|
|
Maintainer follow-up on current head
|
|
Reprocessed current head This PR remains held, not approved or enqueued: the required Please publish a parse-diff generated for this exact head, then this evidence gate can be re-run. |
Fixes phase-rs#5992. Golem Artisan's second ability ("Target artifact creature gains your choice of flying, trample, or haste until end of turn.") is a 3-way Oxford-comma keyword choice. parse_keyword_choice_grant only handled a binary "X or Y" split via split_once_on(text, " or "), so the 3-way list split into "flying, trample," / "haste" -- the left half failed parse_granted_keyword_fragment and the whole clause fell through to Unimplemented. Also corrects the issue's own paraphrase: the real card has no "you control" restriction on its target (any artifact creature is a legal target), verified against Scryfall. parse_keyword_choice_grant now reuses the existing nom-based N-ary Oxford-comma splitter (split_choice_list_items, already shared by the counter-choice and "from among" paths) instead of a binary split_once_on, and returns Vec<Keyword> instead of a fixed pair. Its two callers (build_keyword_choice_clause, build_keyword_choice_sub_ability) build branches via .into_iter().map(...).collect() instead of a hardcoded 2-element vec. The sibling protection-choice shape ("protection from X or from the color of your choice") keeps its existing binary logic unchanged, just adapted to the new Vec-based return type. Effect::ChooseOneOf's runtime (choose_one_of.rs, GameAction::ChooseBranch) already takes Vec<AbilityDefinition> generically with no arity assumption, confirmed by grepping every ChooseOneOf reference in the crate -- this is a parser-only fix. - New parser tests: Golem Artisan's verbatim 3-way clause (asserts 3 branches + a positive `target.controller == None` check), plus Assassin Initiate's verbatim 3-way clause to prove the fix generalizes beyond the one reported card. Existing 2-way test kept unchanged as a regression guard. - New end-to-end integration tests (issue_5992_golem_artisan.rs) drive the real activation pipeline: GameRunner::activate(...).resolve() halts at the real ChooseOneOfBranch prompt (3 branches), GameAction::ChooseBranch picks Trample, evaluate_layers confirms the keyword actually lands (and the other two do not). A second test targets an OPPONENT-controlled artifact creature to prove the no-"you control" correction holds through the real targeting-legality pipeline, not just the parsed AST. - Verification (CARGO_TARGET_DIR isolated): cargo fmt --check clean, cargo clippy -p engine --all-targets --features test-support -- -D warnings clean, full cargo test -p engine --test integration --features test-support: 3544 passed, 0 failed, 2 ignored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
031e7be to
53d70d4
Compare
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesN-ary keyword choice support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GolemArtisan
participant ChooseOneOfBranch
participant GameEngine
participant evaluate_layers
GolemArtisan->>GameEngine: activate keyword-choice ability
GameEngine-->>ChooseOneOfBranch: present three keyword branches
ChooseOneOfBranch->>GameEngine: select requested keyword
GameEngine->>evaluate_layers: settle resolution and evaluate layers
evaluate_layers-->>GolemArtisan: apply selected keyword to target
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/tests/integration/issue_5992_golem_artisan.rs (1)
97-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoop exits silently without asserting full resolution.
If 16 passes never drain the stack/
deferred_triggers, the loop just ends andevaluate_layersruns anyway — the keyword assertions still pass becauseresolve_branchapplies the continuous effect synchronously before this loop starts. A future regression that causes the stack to hang would go undetected by this test.♻️ Proposed fix: assert convergence
- for _ in 0..16 { + for i in 0..16 { match &runner.state().waiting_for { WaitingFor::Priority { .. } => { if runner.state().stack.is_empty() && runner.state().deferred_triggers.is_empty() { - break; + evaluate_layers(runner.state_mut()); + return; } runner.pass_both_players(); } other => panic!("unexpected waiting state after choosing the branch: {other:?}"), } + if i == 15 { + panic!("stack/deferred_triggers did not settle within 16 priority passes"); + } } - - evaluate_layers(runner.state_mut());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/issue_5992_golem_artisan.rs` around lines 97 - 107, Update the bounded loop after branch selection to assert that resolution has converged after all 16 passes, requiring both runner.state().stack and runner.state().deferred_triggers to be empty before evaluate_layers proceeds. Preserve the existing early break when both are already empty and retain the unexpected WaitingFor panic.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/tests/integration/issue_5992_golem_artisan.rs`:
- Around line 97-107: Update the bounded loop after branch selection to assert
that resolution has converged after all 16 passes, requiring both
runner.state().stack and runner.state().deferred_triggers to be empty before
evaluate_layers proceeds. Preserve the existing early break when both are
already empty and retain the unexpected WaitingFor panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 73b8617a-3f29-45ae-b3fb-f3fa148faa97
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/issue_5992_golem_artisan.rscrates/engine/tests/integration/main.rs
|
Maintainer test hardening is now on the current head The required GitHub CI checks and CodeRabbit review have restarted for this head. The existing |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the N-ary fixed-keyword choice reuses the existing list splitter, preserves nested choices as unsupported, and has discriminating runtime coverage.
2 participants
Fixes #5992.
Golem Artisan: "{2}: Target artifact creature gains your choice of flying, trample, or haste until end of turn."
Root cause & fix
This is a 3-way Oxford-comma keyword choice.
parse_keyword_choice_grant(crates/engine/src/parser/oracle_effect/subject.rs) only handled a binary "X or Y" split viasplit_once_on(text, " or ")— for "flying, trample, or haste" that splits into"flying, trample,"/"haste", the left half failsparse_granted_keyword_fragment, and the whole clause falls through toUnimplemented. (Also: the real card has no "you control" restriction on its target — verified against Scryfall, correcting the issue's own paraphrase.)Fixed by reusing the existing nom-based N-ary Oxford-comma splitter (
split_choice_list_items, already shared by the counter-choice and "from among" paths) instead of the binarysplit_once_on, changing the return type toVec<Keyword>. Its two call sites (build_keyword_choice_clause,build_keyword_choice_sub_ability) now build branches via.into_iter().map(...).collect()instead of a hardcoded 2-elementvec!. The sibling protection-choice shape ("protection from X or from the color of your choice") keeps its existing binary logic, just adapted to the newVec-based return type — no behavior change there.Effect::ChooseOneOf's runtime (choose_one_of.rs,GameAction::ChooseBranch) already takesVec<AbilityDefinition>generically with no arity assumption anywhere in the crate (confirmed by grepping everyChooseOneOfreference) — this is a parser-only fix.Testing
target.controller == Nonecheck proving the no-"you control" correction), plus Assassin Initiate's verbatim 3-way clause proving the fix generalizes beyond the one reported card. Existing 2-way test kept unchanged as a regression guard.issue_5992_golem_artisan.rs) drive the real activation pipeline:GameRunner::activate(...).resolve()halts at the realChooseOneOfBranchprompt (3 branches),GameAction::ChooseBranchpicks Trample,evaluate_layersconfirms the keyword actually lands (and the other two do not). A second test targets an OPPONENT-controlled artifact creature to prove the no-"you control" correction holds through the real targeting-legality pipeline, not just the parsed AST.CARGO_TARGET_DIR):cargo fmt --checkclean,cargo clippy -p engine --all-targets --features test-support -- -D warningsclean, fullcargo test -p engine --test integration --features test-support: 3544 passed, 0 failed, 2 ignored, 0 regressions.Summary by CodeRabbit
New Features
Bug Fixes
Tests