Partial: Batroc the Leaper - #7000
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
needs-maintainer
📝 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
| /// CR 608.2c + CR 608.2k: these pronouns are anaphors that bind to an object | ||
| /// established earlier in the same ability (its trigger, cost, or an earlier | ||
| /// clause) rather than a newly-parsed target. Gendered singulars (`him`/`her`) | ||
| /// appear on named creatures that self-reference (e.g. Batroc the Leaper's | ||
| /// "a +1/+1 counter on him"); `them` covers a distributive subject or a | ||
| /// gender-neutral singular. The engine never inspects which pronoun was used — | ||
| /// it only needs the anaphoric binding — so the whole set maps to one referent. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the incorrect Comprehensive Rules citation.
CR 608.2c requires following instructions in written order. It does not define anaphoric object binding. CR 608.2k covers a specific untargeted object previously referred to by an ability’s cost or trigger condition. It does not support the full claim about any earlier clause and pronoun mapping. (media.wizards.com)
Narrow the comment to the verified rule behavior, or cite a verified rule that supports the exact claim.
As per path instructions, rules-touching engine code must use a verified CR citation whose rule body matches the implementation.
🤖 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_nom/primitives.rs` around lines 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.
Source: Path instructions
| pub fn parse_object_recipient_pronoun(input: &str) -> OracleResult<'_, &str> { | ||
| alt((tag("it"), tag("them"), tag("him"), tag("her"))).parse(input) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-token prefixes in the shared pronoun parser.
tag("it") succeeds on item and itself, leaving the suffix for the caller. parse_counter_object_scope calls this parser without a boundary guard, so a non-pronoun prefix can enter ObjectScope::Source. Enforce a word boundary here or at every caller. Add regression cases for item and itself.
As per path instructions, parser code must preserve full-clause consumption so unsupported tails are not silently swallowed.
🤖 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_nom/primitives.rs` around lines 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.
Source: Path instructions
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
|
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