Fix Dwarven Armorer counter choice parsing - #6934
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe parser centralizes counter-choice classification and validation. It accepts valid bare distributed counter phrases, preserves original-case branches, and adds parser and integration coverage for Dwarven Armorer choices. ChangesCounter-choice parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CounterChoiceParser
participant CounterChoiceClassifier
participant BranchReparser
participant DwarvenArmorer
participant GameState
DwarvenArmorer->>CounterChoiceParser: parse counter-choice branches
CounterChoiceParser->>CounterChoiceClassifier: classify and validate items
CounterChoiceClassifier-->>CounterChoiceParser: return counter-choice shape
CounterChoiceParser->>BranchReparser: reparse valid branches
BranchReparser-->>CounterChoiceParser: return selectable branches
CounterChoiceParser->>GameState: resolve selected branch
GameState-->>DwarvenArmorer: apply counter and activation effects
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Add a discriminating production-pipeline test for Dwarven Armorer. Evidence: crates/engine/src/parser/oracle_effect/tests.rs:39765 only asserts the parsed AST; this change turns the real card from unsupported into a supported activated ability. Why it matters: the test never exercises target selection, the resolution-time ChooseOneOf prompt, or applying either selected P/T counter to the chosen creature, so a regression in the production activation/resolution wiring could still mark the card supported. Suggested fix: add an integration test that activates the card through the engine, chooses each branch in turn, and asserts the selected target receives exactly the corresponding +0/+1 or +1/+0 counter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 5450-5482: Update try_parse_put_counter_choice and
classify_counter_choice_list so grammar detection and counter validation use
lowercased input/item slices, while retaining the original text for returned
display values. Ensure mixed-case “from among,” shared-noun, and distributed
lists classify and validate identically to lowercase input without changing
output casing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fabc4664-1217-4cd7-9942-c32daa0b9921
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rs
|
Generated for head Parse changes introduced by this PR · 2 card(s), 4 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The new validation rejects the very distributed counter-choice grammar this PR adds, and it regresses existing your choice counter parsing. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:5405-5413 requires every distributed item to pass the new full-noun parser, and :5453-5480 makes that guard authoritative for both the new bare path and existing context-free callers. Current-head CI shows the new Dwarven Armorer test and its positive reach-guard fail (tests.rs:39765, :40060), the distributed classifier test fails (:39963), and the existing replacement test now receives a generic PutCounter instead of ChooseOneOf (oracle_replacement.rs:15724). The current parse-diff confirms the blast radius: 15 cards / 18 signatures, including six ChooseOneOf removals. Why it matters: valid controller choices are no longer represented as ChooseOneOf, so cards lose their required resolution-time choice semantics while the PR claims a Dwarven-only parser fix. Suggested fix: correct the complete-counter-noun grammar and preserve the existing shared-noun/from-among behavior, then add direct regression tests for the affected choice forms and rerun the parse-diff until the card-level scope is explained and intentional.
matthewevans
removed
the
needs-maintainer
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
5550-5560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the parsed entries from the classifier instead of re-parsing them.
classify_counter_choice_listalready parses every item to validate it. It then discards the results.classify_and_parse_counter_choice_listat Lines 5637-5648 runsparse_full_counter_nounandparse_strict_counter_typea second time on the same items. This duplicates the grammar and lets the two paths drift apart.Consider returning
Vec<(CounterType, QuantityExpr)>alongside the shape, so the classifier stays the single authority for both validation and extraction.🤖 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/src/parser/oracle_effect/mod.rs` around lines 5550 - 5560, The classifier currently discards parsed counter entries, causing classify_and_parse_counter_choice_list to parse each item again. Update classify_counter_choice_list to retain and return the parsed Vec<(CounterType, QuantityExpr)> alongside the ChoiceListShape, then make classify_and_parse_counter_choice_list reuse those results instead of invoking parse_full_counter_noun or parse_strict_counter_type a second time.
🤖 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/src/parser/oracle_effect/mod.rs`:
- Around line 5550-5560: The classifier currently discards parsed counter
entries, causing classify_and_parse_counter_choice_list to parse each item
again. Update classify_counter_choice_list to retain and return the parsed
Vec<(CounterType, QuantityExpr)> alongside the ChoiceListShape, then make
classify_and_parse_counter_choice_list reuse those results instead of invoking
parse_full_counter_noun or parse_strict_counter_type a second time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 00a203c0-bb71-477d-8b14-5dcdf9282f95
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/integration_cards.json.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/tests/integration/dwarven_armorer_counter_choice.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head e050f189708976e956d7618a42513943e20563b2
[HIGH] Current-head parser-impact evidence is missing. The only <!-- coverage-parse-diff --> artifact is generated for 32d4ef47d73e0d05cb6b31aebc4d7bba81c31da8, not this parser-changing head. Its 15-card / 18-signature report therefore cannot validate the current parser surface or reveal any later blast radius.
Suggested fix: publish a fresh parse-diff sticky/artifact bound to e050f189708976e956d7618a42513943e20563b2, then account for every gained/lost/changed card before re-review. Also wait for the currently pending Rust lint and Rust test shards to complete green before requesting approval.
[LOW] Counter-choice list items are parsed twice through separate authorities. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:5550-5559 validates every item with parse_full_counter_noun / parse_strict_counter_type, then :5628-5653 repeats the same parsing to construct typed entries. The two passes can drift as this grammar expands.
Suggested fix: make classification return the already parsed typed entries (with the list shape where needed), so counter-choice recognition and typed parsing have one authority.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Updated changes requested — current head e050f189708976e956d7618a42513943e20563b2
The parse-diff artifact is now current-head evidence, so this supersedes my earlier evidence-only hold.
[HIGH] The counter-choice grammar regresses established resolution-time choices and fails the PR’s own parser surface. Evidence: the exact-head parse artifact reports 15 affected cards / 18 signatures, including six removed ChooseOneOf signatures and malformed PutCounter output such as counter=1 your choice of a flying for Helica Glider and Wingfold Pteron. Required Rust test run 31270662539 fails both shards: shard 1 fails the distributed-list classifier, Dwarven Armorer branches, mixed-case target preservation, and enters with your choice replacement parsing; shard 2 fails the bare P/T distributed-choice regression. The changed grammar is at crates/engine/src/parser/oracle_effect/mod.rs:5485-5494, :5533-5560, and :5681-5724.
Why it matters: a parser extension for Dwarven Armorer changes existing explicit/shared choice forms from ChooseOneOf resolution-time decisions into malformed counters, TargetOnly, or unimplemented effects. That is a rules-semantic regression across a card class, not a single-card failure.
Suggested fix: preserve the established explicit/shared resolution-time choice paths and constrain the new bare distributed grammar so it does not capture them. Add discriminating regressions for bare P/T choices, multiword keyword-counter choices, and enters with your choice forms, in addition to Dwarven Armorer, before re-review.
[LOW] Counter-choice members still have two parsing authorities. mod.rs:5550-5560 validates items, then :5628-5653 parses them again to build typed entries. Return the classified parsed entries from the first authority to prevent grammar drift.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 5736-5744: Update the rules annotation for the counter-choice
handling around classify_counter_choice_list and
original_counter_choice_list_items to cite CR 122.1/122.1a, covering both
keyword and distributed +X/+Y counter forms. Retain CR 608.2d only for the
resolution-time choice annotation, and ensure each citation’s description
matches the behavior implemented by this path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cac9abb9-1f57-427a-a1e3-b6485c7209fc
📒 Files selected for processing (1)
crates/engine/src/parser/oracle_effect/mod.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 699f390ed9e7e5549b7bf074f333537e489a18e7.
[HIGH] The broad counter-choice classifier is invoked before normal imperative parsing (crates/engine/src/parser/oracle_effect/mod.rs:8514, classifier :5472-5770) and is claiming/rejecting ordinary counter text that must fall through to the existing parser. Exact CI run 31306328452 has semantic regressions in both Rust test shards: AWBO loses its target/multi-type behavior, and Abigale and Stalwart lose their counter effects.
Restore the normal parse path for non-exact counter-choice grammar; the choice classifier must be a narrow recognizer, not a pre-dispatch sink. Add a discriminating matrix that proves the affected AWBO target/multi-type form and Abigale/Stalwart counter effects are present, while preserving a genuine shared-target counter choice and proving a near-miss falls through rather than being swallowed.
This is a substantive parser regression even though the branch is maintainer-behind. After the fix, provide current-head CI and a SHA-bound coverage-parse-diff artifact; the available artifact is for old head 8b166c….
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
16590-16603: 📐 Maintainability & Code Quality | 🟠 MajorUse typed
Effectmatches instead of wildcards.Bind parser result fields in the outer
if letpatterns and handlePutCounter/ReproduceEventCountersexhaustively instead of_ => unreachable!or_ => None. The wildcard arms still violate typed-enum exhaustiveness and keep future enum variants outside compiler diagnostics.🤖 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/src/parser/oracle_effect/mod.rs` around lines 16590 - 16603, The typed Effect handling at crates/engine/src/parser/oracle_effect/mod.rs lines 16590-16603 and 17105-17122 must be made exhaustive. Bind target and per-kind_count directly in the outer if let patterns for ReproduceEventCounters and PutCounter, then match only the corresponding typed variants without wildcard or unreachable arms; preserve the existing parser result behavior at both sites.Sources: Coding guidelines, 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.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 5734-5737: Add a verified rules annotation beside the bare
counter-choice helper’s documentation, using the format “CR 608.2c + CR 608.2d:
…”. Describe that normal-English instructions determine whether counters are
applied together or disjunctively, and that an `or` choice is made as the effect
resolves.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 16590-16603: The typed Effect handling at
crates/engine/src/parser/oracle_effect/mod.rs lines 16590-16603 and 17105-17122
must be made exhaustive. Bind target and per-kind_count directly in the outer if
let patterns for ReproduceEventCounters and PutCounter, then match only the
corresponding typed variants without wildcard or unreachable arms; preserve the
existing parser result behavior at both sites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c40ee933-963b-48c0-bab4-3bc7580da0df
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_effect/tests.rs
|
Maintainer hold — current head The prior counter-choice parser regression is resolved and the current source is clean. The existing changes-requested review is for older head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for exact head 7ca67f738536424fd901e38d1e7bd0f7566911c7.
Maintainer port to main 7b17f21c1af5 is clean. All required hosted checks are successful, and the refreshed parse-diff artifact is current for this head: the intended Dwarven Armorer target-only counter-choice signature is added, with the associated stale put signatures removed. The previous changes-requested review applied only to 699f390….
2 participants
Summary
Parses bare, distributed full-counter choices such as Dwarven Armorer's
+0/+1or+1/+0options. The shared parser now validates every complete counter noun before building the existing choice branches, preventing non-counter disjunctions from being accepted.Files changed
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rsTrack
Developer
LLM
Model: gpt-5.6
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 602.2b, CR 601.2c, CR 608.2d, CR 122.1, and CR 122.1a were verified for the activation target, resolution-time branch choice, and P/T counter semantics. No CR annotations were added or changed.
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— PASS./scripts/check-parser-combinators.sh— PASSgit diff --check— PASSFocused
cargo test -p phase-engine dwarven_armorer_bare_distributed_counter_choice_preserves_cost_and_branches -- --exactand./scripts/gen-card-data.sh— environment terminatedrustcwith SIGTERM during compilation; GitHub Actions is the CI-owned verification alternative.Gate A
Gate A PASS head=32d4ef47d73e0d05cb6b31aebc4d7bba81c31da8 base=b9023330ef4b5d091f438eb27fbfbfe321983b85
Anchored on
crates/engine/src/parser/oracle_effect/mod.rs:5509— existing typed counter-choice classifier consumes the shared list classification.crates/engine/src/parser/oracle_effect/mod.rs:5558— existing shared-target choice parser reparses validated branches asTargetOnlyplusChooseOneOf.Final review-impl
Final review-impl PASS head=32d4ef47d73e0d05cb6b31aebc4d7bba81c31da8
Claimed parse impact
Dwarven Armorer.
Scope Expansion
None.
Validation Failures
The isolated environment terminated the attempted focused engine test and card-data generation compilation with SIGTERM before a test/card-data result was produced. This was not a source diagnostic; GitHub Actions must complete those checks.
CI Failures
None; CI has not run for this branch.
Summary by CodeRabbit