fix(parser): strip "additional" from "N additional <type> counters" so the counter type is canonical (CR 122.1)#5199
Conversation
…ounters"
"Enters with N additional +1/+1 counters" leaked the word "additional" into the
counter TYPE: parse_enters_with_counters consumed the count ("two") but then
sliced the counter type from "additional +1/+1 counters on it", producing
Effect::PutCounter { counter_type: Generic("additional +1/+1"), count: 2 } — a
counter of a type no real counter has. So the permanent entered with two
counters of a nonexistent kind and never actually gained +2/+2 (Necromantic
Summons, Heroic Return, Turntimber Symbiosis, Curator Beastie), and Ravaging
Riftwurm got Generic("additional time") instead of CounterType::Time (breaking
its Vanishing count).
The leading-"additional" strip already handled the count-less "an additional
+1/+1 counter" form, but not the "N additional ..." form where the count word
precedes "additional". CR 122.1 + CR 614.1c: "additional" is a stacking
qualifier, not part of the counter type (the engine models additivity by this
being a distinct PutCounter replacement). Strip it (nom tag) after the count is
parsed at both counter-type slice sites so the canonical CounterType is
produced. Parser-only — the engine already resolves Plus1Plus1/Time correctly.
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function strip_additional_counter_qualifier to strip the "additional " qualifier from phrases like "N additional counters" during parsing, preventing "additional" from leaking into the parsed counter type. Unit tests were added to verify correct parsing behavior. The review feedback points out a violation of Rule R6 regarding the format of CR annotations, where multiple annotations were combined on a single line using a + sign, and suggests splitting them into separate lines.
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.
|
Maintainer note: I reviewed the current head locally. The implementation is at the right parser seam, the I also checked the Gemini CR-format thread against the repo annotation rules and resolved it as a false positive: Holding approval until the card-data job publishes the required parse-diff sticky for this parser-surface change. No author action requested yet. |
Parse changes introduced by this PR · 5 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: approved. I re-reviewed the current head and the previously missing parser evidence is now present. The parse-diff sticky is scoped to the intended five cards and canonicalizes only the affected counter types (additional +1/+1 → P1P1, additional time → time), matching the PR's claim. The implementation is at the replacement-parser seam, strips additional after the count expression in both single-counter and multi-entry paths, and the tests assert semantic counter types rather than string shape. Gemini's CR-format finding was resolved as a false positive under this repo's documented CR X + CR Y convention. CI is green.
Tier: Frontier
Model: claude-opus-4-8
Thinking: high
Summary
CR 122.1 + CR 614.1c: a misparse in the "enters with N additional
<type>counters" class.parse_enters_with_countersconsumed the count word ("two") but then sliced the counter TYPE out of the remaining"additional +1/+1 counters on it"— so "additional" leaked into the counter type, producingEffect::PutCounter { counter_type: Generic("additional +1/+1"), count: 2 }. That's a counter of a type no real counter has: the permanent entered with two counters of a nonexistent kind and never actually gained +2/+2.Affected (5, marked "supported" but silently wrong): Necromantic Summons, Heroic Return, Turntimber Symbiosis, Curator Beastie (
+1/+1), and Ravaging Riftwurm (Generic("additional time")instead ofCounterType::Time— which breaks its Vanishing count).Fix (parser-only): the existing leading-
additionalstrip handles the count-less form ("an additional +1/+1 counter") but not the "N additional …" form, where the count word precedes "additional". Add a small nom helper (tag("additional ")) and apply it to the remainder afterparse_count_exprconsumes the count, at both counter-type slice sites (the single-entry path and theparse_enters_counter_entriesloop). "additional" is a stacking qualifier, not part of the type (CR 614.12 — the engine models additivity by this being a distinctPutCounterreplacement), so stripping it yields the canonicalCounterType. The engine already resolvesPlus1Plus1/Timecorrectly — no engine change.Files changed
CR references
CR 122.1— counters (a counter has a kind).CR 614.1c+CR 614.12— "enters with additional counters" replacement; "additional" signals stacking, not a counter kind.Gate A
Also ran the authoritative check on my parser diff — combinator grep on
git diff -- 'crates/engine/src/parser/**'— printed nothing. The newstrip_additional_counter_qualifieris combinator-only (tag("additional ")+map_or), reusing the same idiom as the existing leading-additionalstrip.Anchored on
additionalstrip inparse_enters_with_counters(alt((tag("an additional "), tag("additional ")))); the newstrip_additional_counter_qualifiermirrors that exacttag-based qualifier-strip idiom, applied at the post-count position the leading strip never reaches.parse_enters_counter_entries, the multi-entry loop that slices each counter type afterparse_count_expr; the strip is applied there too so multi-entry "N additional …" clauses lower identically.Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Verification
Developer track (Tilt not running → cargo directly with the repo
CClinker):cargo fmt --all— clean./scripts/check-parser-combinators.sh— exit 0; parser-diff combinator grep — emptycargo clippy -p engine --tests -- -D warnings— cleancargo test -p engine --lib -- parser::— 7544 pass / 0 fail; new fail-on-revert testenters_with_n_additional_counters_parses_canonical_typeasserts the SEMANTICcounter_typevalue (Plus1Plus1for the four +1/+1 cards,Timefor Ravaging Riftwurm) — reverting the strip flips it toGeneric("additional +1/+1")and failsUnimplementedcount unchanged (3583→3583, no new gaps); a global diff of everyPutCounter.counter_typeacross all 34.6k cards shows exactly the 5 target cards changed, nothing else (no over-match — non-"additional" enters-with-counter cards like Kalonian Hydra, Avatar of the Resolute, Renata, Spark Double are byte-identical to baseline)Scope Expansion
None.