fix(parser): guard bounded counter distribution choices by Lowtrixx · Pull Request #5675 · phase-rs/phase

Lowtrixx · 2026-07-12T12:29:41Z

Summary

Fixes parser: binary-choice splitter guards damage distribution but not counters (32 cards), and the underlying re-entry is unbounded #5613 for bounded counter distribution parsing.
Implements the selected card: Ajani, Mentor of Heroes.
Keeps one, two, or three target ... cardinality text from being misclassified as an inline ChooseOneOf separator.
Preserves real external binary choices after bounded target branches by skipping only separators inside bounded-cardinality phrases and continuing the separator scan.

Card / Oracle

Selected card: Ajani, Mentor of Heroes

Scryfall Oracle verified:

+1: Distribute three +1/+1 counters among one, two, or three target creatures you control.
+1: Look at the top four cards of your library. You may reveal an Aura, creature, or planeswalker card from among them and put it into your hand. Put the rest on the bottom of your library in any order.
-8: You gain 100 life.

Review Notes

Followed the non-developer track because no Rust toolchain is available on this machine.
Checked open PRs/issues for an existing Ajani / parser: binary-choice splitter guards damage distribution but not counters (32 cards), and the underlying re-entry is unbounded #5613 / bounded counter-distribution implementation; no duplicate or claimed PR found.
Plan review completed with a clean final pass.
Implementation review initially found two issues around external choice scanning and test coverage; both were fixed.
Final implementation review: Maintainer-Simulation Gate PASS, no blocking findings.

Validation

C:\Program Files\Git\bin\bash.exe scripts/check-parser-combinators.sh
git diff --check
Added focused parser regression tests for Ajani and bounded-target choice separator behavior.

Not Run

cargo fmt --all: cargo is not available on PATH.
Rust unit tests / card-data generation / coverage: Rust toolchain and generated card-data artifacts are unavailable in this environment.

gemini-code-assist

Code Review

This pull request introduces helper functions, constants, and unit tests to detect and parse bounded target cardinality phrases, specifically checking if a separator falls within such phrases. The code review feedback highlights two main issues: first, the use of static prefix lists and verbatim string matching for parsing Oracle phrases violates the style guide's requirement for modular, nom-based combinators; second, several newly added rules-touching parser functions lack the mandatory CR (Comprehensive Rules) annotations required by the style guide.

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.

gemini-code-assist · 2026-07-12T12:32:01Z

+pub(super) const BOUNDED_TARGET_PREFIX_PHRASES: &[(&str, usize, usize)] = &[
+    ("one or two target ", 1, 2),
+    ("one, two, or three target ", 1, 3),
+];


Avoid verbatim string equality and static prefix lists (like BOUNDED_TARGET_PREFIX_PHRASES) for parsing Oracle phrases, as this bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., numbers, conjunctions, and 'target') and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

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 to prevent combinatorial explosion and improve maintainability.

gemini-code-assist · 2026-07-12T12:32:01Z

    None
 }

+fn is_bounded_cardinality_boundary(c: char) -> bool {


[HIGH] Missing mandatory CR annotation for rules-touching parser function. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:5039.

Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.

Suggested fix: Add a CR 115.1d comment above the function definition.

Suggested change

fn is_bounded_cardinality_boundary(c: char) -> bool {

/// CR 115.1d: Check if a character is a boundary for bounded target cardinality.

fn is_bounded_cardinality_boundary(c: char) -> bool {

References

Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

gemini-code-assist · 2026-07-12T12:32:02Z

+fn parse_bounded_target_placeholder<'a>(
+    phrase: &'static str,
+    input: &'a str,
+) -> OracleResult<'a, &'a str> {


[HIGH] Missing mandatory CR annotation for rules-touching parser function. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:5043.

Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.

Suggested fix: Add a CR 115.1d comment above the function definition.

/// CR 115.1d: Parse a bounded target placeholder. fn parse_bounded_target_placeholder<'a>( phrase: &'static str, input: &'a str, ) -> OracleResult<'a, &'a str> {

References

Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

gemini-code-assist · 2026-07-12T12:32:02Z

+    .parse(input)
+}
+
+fn parse_bounded_target_cardinality_phrase(input: &str) -> OracleResult<'_, ()> {


[HIGH] Missing mandatory CR annotation for rules-touching parser function. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:5057.

Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.

Suggested fix: Add a CR 115.1d comment above the function definition.

/// CR 115.1d: Parse a bounded target cardinality phrase. fn parse_bounded_target_cardinality_phrase(input: &str) -> OracleResult<'_, ()> {

References

Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

gemini-code-assist · 2026-07-12T12:32:02Z

+pub(super) fn selected_separator_inside_bounded_target_cardinality(
+    lower: &str,
+    separator: Range<usize>,
+) -> bool {


[HIGH] Missing mandatory CR annotation for rules-touching parser function. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:5071.

Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to ensure strict fidelity to the Magic Comprehensive Rules.

Suggested fix: Add a CR 115.1d comment above the function definition.

/// CR 115.1d: Check if a selected separator is inside a bounded target cardinality phrase. pub(super) fn selected_separator_inside_bounded_target_cardinality( lower: &str, separator: Range<usize>, ) -> bool {

References

Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

matthewevans · 2026-07-12T12:35:22Z

Thanks for this, and welcome — the diagnosis here is correct, which is worth saying plainly before the outcome.

Closing as already fixed. Issue #5613 was closed as completed by #5619 (merged 700d4364, 2026-07-12T00:39Z), which landed the same guard roughly twelve hours before this branch was authored. That's the whole reason the branch shows CONFLICTING: it's based on a pre-#5619 main, so the code it edits no longer looks the way it does here.

Concretely, main already carries all three pieces:

BOUNDED_TARGET_CARDINALITIES (oracle_effect/lower.rs) — the single authority for the cardinality vocabulary, measured against the full 34,632-card pool.
The splitter guard in try_parse_choose_one_of_inline (oracle_effect/mod.rs), keyed on that cardinality axis rather than on a distribution verb — which is exactly the CR 601.2d "damage or counters" generalization you identified.
The regression test distribute_cardinality_clause_parses_within_a_bounded_stack.

One note for next time, because it would matter even on a rebased branch: this PR introduces BOUNDED_TARGET_PHRASES and BOUNDED_TARGET_PREFIX_PHRASES — two lists that re-spell the same cardinality vocabulary. main's constant is deliberately a single authority, with each consumer composing the trailing noun it needs (" targets", " target ") off one stem, so that a future cardinality like "one, two, three, or four …" is added in exactly one place. Forking a parallel list is the pattern that authority exists to prevent.

Two things that would have caught this before you wrote the code, and are worth building into your habit here:

Check the issue is still open, and skim its cross-references — fix(parser): guard the binary-choice splitter on the target-cardinality list #5619 was linked on parser: binary-choice splitter guards damage distribution but not counters (32 cards), and the underlying re-entry is unbounded #5613 before this branch existed.
Branch from a freshly fetched main. This repo moves fast; a stale base is what turned a correct idea into a conflicting duplicate.

The reasoning was sound and the CR 601.2d insight was the right one. Please do pick up another issue — ideally one with no open PR linked against it, and I'd be glad to look at it.

fix(parser): guard bounded counter distribution choices

795df31

Lowtrixx requested a review from matthewevans as a code owner July 12, 2026 12:29

gemini-code-assist Bot reviewed Jul 12, 2026

View reviewed changes

matthewevans closed this Jul 12, 2026

Navigation Menu

Uh oh!

Uh oh!

fix(parser): guard bounded counter distribution choices - #5675

fix(parser): guard bounded counter distribution choices#5675
Lowtrixx wants to merge 1 commit into
phase-rs:mainfrom
Lowtrixx:fix/issue-5613-counter-distribution-guard

Lowtrixx commented Jul 12, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Uh oh!

matthewevans commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	fn is_bounded_cardinality_boundary(c: char) -> bool {
	/// CR 115.1d: Check if a character is a boundary for bounded target cardinality.
	fn is_bounded_cardinality_boundary(c: char) -> bool {

Uh oh!

Uh oh!

Conversation

Lowtrixx commented Jul 12, 2026

Summary

Card / Oracle

Review Notes

Validation

Not Run

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

matthewevans commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants