Partial: Batroc the Leaper - #7000
Conversation
Co-Authored-By: Claude Opus 4.8 <[email protected]>
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR centralizes object-recipient pronoun parsing across Oracle parsers, expands kicker-count and replacement parsing, adds Batroc multikicker integration tests, and normalizes relative path separators before pinned-coordinate comparisons. ChangesOracle parser updates
Path normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_quantity.rs (2)
3508-3535: 📐 Maintainability & Code Quality | 🔵 TrivialConsider sharing the "was/were kicked" verb-anchor scan with
parse_where_x_kicker_count.The comment above
parse_for_each_kicker_countstates it "mirrors the permissive subject match already used byoracle_effect::lower::parse_where_x_kicker_count." Two independent implementations of the same "subject was/were kicked" grammar can drift apart over time (for example, if one recognizer later needs a new subject form and the other does not get updated). Extract the sharedtake_until(" was kicked")/take_until(" were kicked")scan into one combinator in a shared primitives module, and have both recognizers call it.This mirrors the single-authority principle this PR already applies to
parse_object_recipient_pronoun.
[medium_effort_and_high_reward]Run the following script to confirm the duplication and locate the sibling function:
#!/bin/bash # Description: Locate parse_where_x_kicker_count and compare its verb-anchor grammar # to parse_for_each_kicker_count. rg -n -B3 -A25 'fn parse_where_x_kicker_count' crates/engine/src/parser🤖 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_quantity.rs` around lines 3508 - 3535, Extract the shared “was kicked”/“were kicked” subject scan from parse_for_each_kicker_count and parse_where_x_kicker_count into a reusable combinator in the shared parser primitives module. Update both recognizers to call this single combinator while preserving full-consumption validation and QuantityRef::KickerCount behavior.
4150-4154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the CR 201.5 citation.
CR 201.5 governs name-based self-reference, while this test documents grammatical pronoun agreement for kicker state ("he was kicked" / "it was kicked"). No rule codifies pronoun gender/number as kicker-count equality, so keep the test comment limited to CR 702.33c-d, or cite the actual governing source if one exists.
🤖 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_quantity.rs` around lines 4150 - 4154, Update the comment in for_each_time_gendered_pronoun_was_kicked_maps_to_kicker_count to remove the incorrect CR 201.5 citation and describe the test as governed by CR 702.33c-d only; do not imply that the rules explicitly establish gendered/neuter pronoun kicker-count equality.
🤖 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_nom/primitives.rs`:
- Around line 189-195: Update the documentation comment above the anaphoric
pronoun handling in the relevant parser code to remove the inaccurate CR
608.2c/608.2k citations and unsupported claims about earlier clauses and pronoun
categories. Either narrow the comment to behavior directly supported by a
verified Comprehensive Rules citation or replace it with a verified rule whose
text matches the implementation, while preserving the explanation that these
pronouns resolve to the established referent.
- Around line 203-205: Update parse_object_recipient_pronoun to require a word
boundary after each pronoun, preventing matches for prefixes such as item and
itself while preserving valid pronoun parsing. Ensure callers such as
parse_counter_object_scope still require full-clause consumption, and add
regression coverage for both rejected inputs.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_quantity.rs`:
- Around line 3508-3535: Extract the shared “was kicked”/“were kicked” subject
scan from parse_for_each_kicker_count and parse_where_x_kicker_count into a
reusable combinator in the shared parser primitives module. Update both
recognizers to call this single combinator while preserving full-consumption
validation and QuantityRef::KickerCount behavior.
- Around line 4150-4154: Update the comment in
for_each_time_gendered_pronoun_was_kicked_maps_to_kicker_count to remove the
incorrect CR 201.5 citation and describe the test as governed by CR 702.33c-d
only; do not imply that the rules explicitly establish gendered/neuter pronoun
kicker-count equality.
🪄 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: dd077b02-f69d-4079-bd47-81f6b6e6ff15
📒 Files selected for processing (10)
crates/engine/src/game/engine.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/counter.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_nom/primitives.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/src/parser/oracle_quantity.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/tests/integration/batroc_the_leaper_multikicker_counters.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
High — Kicker-count grammar accepts unrelated or empty subjects
parse_for_each_kicker_count in crates/engine/src/parser/oracle_quantity.rs:3508-3534 discards every token between time and was/were kicked, then always emits QuantityRef::KickerCount. The resolver in crates/engine/src/game/quantity.rs:4395-4406 has no subject parameter: it reads only the trigger or self source's kickers_paid. Consequently, unsupported inputs such as for each time a creature was kicked (and an empty subject) silently resolve to the source's kick count instead of remaining explicit unsupported syntax or carrying an authoritative subject.
Please put this grammar in crates/engine/src/parser/oracle_nom/quantity.rs as an all-consuming, word-boundary-safe self-subject combinator, and delegate to it from the semantic entry point. It should admit only established self forms (it, this spell, ~, plus a correctly bound named-self pronoun if the existing architecture supports that); non-self and empty subjects must be rejected. Add negative reach-guard coverage for a generic creature and an empty subject, while retaining the Batroc runtime behavior for the real self form.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
MED — CR annotation overclaims the pronoun-reference rule
crates/engine/src/parser/oracle_nom/primitives.rs:189-195 attributes the broad statement that these pronouns bind objects introduced by a trigger, cost, or earlier clause to CR 608.2c + CR 608.2k. The verified rules do not support that scope: CR 608.2c governs instruction ordering and applying the rules of English, while CR 608.2k is limited to a specific untargeted object previously referred to by that ability's cost or trigger condition across characteristic changes.
Please remove or narrow the CR claim to its verified scope. The generic parser-grammar explanation may remain, but it should not assert that those CR sections define the full anaphora behavior implemented here.
|
Generated for head Parse changes introduced by this PR · 4 card(s), 4 signature(s) (baseline: main
|
matthewevans
removed
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head c5e70a7: feedback is addressed and the latest CI workflow is green.
2 participants
Summary
Fixes a parse-fidelity defect on Batroc the Leaper.
Issue: Enters-with-counter replacement parses count as Fixed 1 instead of dynamic KickerCount, so "a +1/+1 counter for each time he was kicked" wrongly gives exactly one counter regardless of kicks (parser flagged SwallowedClause/DynamicQty).
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean (exit 0)./scripts/check-parser-combinators.sh— pass with env-skip: Gate G PASS and families A/B/C/E/F verified clean on the parser diff (base 9169d8f); Family-D skipped because python3 is the Windows-Store stub (exit 126, Permission denied) — the documented env limitation, not a failure. The script's raw exit was 1 solely because the Family-D self-test precondition (D0) hard-exits when python3 is unavailable, before the (already-clean) A-F diff scan. My in-loop fixes touched only a test file and crates/engine/src/game/engine.rs (neither under crates/engine/src/parser), so the gate verdict is unaffected.cargo clippy-strict— clean after in-loop fix (exit 0). Initial run failed with clippy::while_let_loop in crates/engine/tests/integration/batroc_the_leaper_multikicker_counters.rs:139 (the Batroc test under verification); converted theloop { match waiting_for {...} }towhile let WaitingFor::OptionalCostChoice { .. } = ...(semantics preserved). Re-ran clean.cargo test -p phase-engine— clean after in-loop fix (exit 0; 18489 lib + 4486 integration + doc-tests all pass). Initial run had 1 failure: game::engine::stage2_injector_tests::the_cr_603_5_prompt_census_is_pinned... — a pre-existing Windows path-separator bug UNRELATED to Batroc: actual vs expected producer lists had identical line coordinates and counts (5/7/25, total 37), differing only in\(from Path::display() at engine.rs:15144) vs hardcoded/. Normalized the separator with .replace('\', "/") without touching the guarded line-coordinate pins. Re-ran clean.cargo export-cards data --stats --sidecar-dir client/public && cp client/public/card-data.json data/card-data.json— clean. Regenerated 98MB card-data against THIS branch's engine (28154 cards scanned), sidecar written to client/public/card-data.json, copied to data/card-data.json (both 98176031 bytes).cargo coverage— ran clean (exit 0; overall 88.94%, 31713/35657) BUT verification criterion FAILED: 'Batroc the Leaper' is supported:false, gap_count:1. NOTE the earlier apparent exit 101 was a SIGPIPE artifact from aheadclosing the pipe, not a coverage failure; the clean re-run to a file returned 0.cargo semantic-audit— clean (exit 0; 32700 cards audited, 295 with findings). 'Batroc the Leaper' is not in flagged_cards -> 0 findings. semanticAuditClean=true.Scope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
Bug Fixes
Tests