Fix Toxrill, the Corrosive — bind a counter anaphor to the recipient in per-recipient statics - #6335
Conversation
… statics
CR 611.3a: a continuous effect from a static ability "isn't locked in; it
applies at any given moment to whatever its text indicates", so a bare
anaphoric pronoun ("it" / "them" / "him" / "her") in a per-recipient
continuous static names the object RECEIVING the effect, not the source.
`parse_counter_object_scope` (oracle_nom/quantity.rs) has no view of the
enclosing static, so it bound every counter anaphor to `ObjectScope::Source`.
Toxrill, the Corrosive therefore scaled "-1/-1 for each slime counter on them"
off Toxrill's own (always empty) slime pile, and the penalty never applied.
Rebinding happens once in `lower_static_ir` — the existing post-parse lowering
funnel that already runs `populate_active_zones_from_condition` — so anthem,
type_change and grammar builders all inherit it from a single authority
instead of each re-deriving the referent.
This is the quantity-axis twin of `StaticCondition::RecipientHasCounters`
(documented in types/ability.rs as "the recipient analog of `HasCounters`"),
so both axes now agree on what the pronoun in "...counters on it" refers to.
No new enum variant: `ObjectScope::Recipient` already existed and the layer
evaluator already resolves it per affected object — only the recipient-varying
classifier in `quantity_expr_uses_recipient` was missing its `CountersOn` arm.
The rebind is gated on the PRONOUN FORM, not merely on the affected set. The
much larger Door of Destinies / Joraga Warcaller / Lion Sash class writes
"...for each charge counter on ~", naming the source outright, where `Source`
is already correct; a blanket rebind would have broken 32 cards to fix 5.
Parse impact (regenerated card-data, whole 35k corpus): exactly 5 cards move
to a recipient-scoped counter read, and no other card's scope changes.
Toxrill, the Corrosive - Clamavus - Thelon of Havenwood
Luxior, Giada's Gift - Spark Rupture
Closes phase-rs#5929
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughContinuous static counter anaphors are parsed as deferred references, bound to source or affected recipients during static lowering, and resolved with recipient-aware quantity evaluation. Integration tests cover recipient-scoped, explicit source-scoped, and mixed counter references. ChangesCounter anaphor binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant StaticLowering
participant QuantityResolver
participant GameState
OracleParser->>StaticLowering: preserve and bind counter anaphor
StaticLowering->>QuantityResolver: evaluate scoped quantity
QuantityResolver->>GameState: read counters for resolved object
GameState-->>QuantityResolver: counter value
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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_static/shared.rs`:
- Around line 816-822: Update the logic around description_binds_counter_anaphor
and rebind_source_counters_to_recipient so it carries the recipient/anaphoric
scope on the parsed quantity and lowers only the counter operand associated with
that scope, rather than traversing every dynamic quantity in def.modifications.
Preserve explicit ~ counter reads unchanged when a static mixes them with
pronoun-backed reads, and add a regression test covering the mixed-scope case.
🪄 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: 3e4d79f9-3659-4d01-81d4-cf3998b89829
📒 Files selected for processing (5)
crates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/shared.rscrates/engine/tests/integration/counter_anaphor_binds_to_recipient.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
There was a problem hiding this comment.
Blocking: counter-source provenance is lost before static lowering
parse_counter_object_scope lowers it/them and ~ to the same ObjectScope::Source. The new static lowering then scans the whole description for any recipient pronoun and rewrites every CountersOn { object: Source, .. } in every dynamic modification. A static that reads counters on both ~ and on its recipient will therefore rebind the explicit source read to the recipient.
This needs per-quantity anaphoric provenance carried through parsing/lowering, rather than a description-wide post-pass. Please add a mixed source/recipient runtime regression; the Toxrill and Joraga cases do not cover that combination.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…escription Addresses the review on phase-rs#6335: the first commit inferred the referent by scanning the static's whole description, so a static reading counters on BOTH `~` and its recipient would rebind the explicit source read too. The referent is now recorded per quantity at the parse site and bound at lowering. CR 608.2k: `parse_for_each_counters_on_source` (oracle_nom/quantity.rs) parsed the object with `parse_source_self_ref`, which accepts `~` AND "it" and collapsed both to `ObjectScope::Source`. That collapse is where the provenance was lost. The object is now dispatched: an explicit self-reference records `Source`, a bare objective pronoun records the deferred `ObjectScope::Anaphoric` — the variant the engine already documents for exactly this purpose and which `rebind_anaphoric_object_scope` already lists `CountersOn` under. `lower_static_ir` then binds each deferred read from the one place that knows the antecedent, walking both the continuous modifications and the `UnlessPay` condition tree (CR 118.12 — a combat tax carries its per-counter magnitude there, not in a modification). A self-referential subject binds back to `Source`, so Myr Prototype's AST is unchanged and its test needed no edit. Every other entry point settles an unbound anaphor back to `Source` at its own module boundary, so carrying provenance costs nothing outside the statics that consume it. Verified absolutely on the regenerated 35k corpus: ZERO cards leave an unbound `Anaphoric`, and the scope distribution is Source 495 / Recipient 8 / Target 6 — every counter read outside the fixed cards is byte-identical. Scope reduced honestly: Spark Rupture is no longer claimed. Its "equal to the number of loyalty counters on it" text reaches a different combinator whose provenance change would have churned 10 unrelated cards, so that path is left exactly as it was and the card stays a known gap. Adds the mixed source/recipient runtime regression the review asked for: one static reading charge counters on `~` (+3/+0 from the source) and +1/+1 counters on "it" (+0/+4 from the recipient) must produce 9/10 on a 2/2 ally — a single collapsed referent cannot produce that pair. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — the blocking finding was correct, and the description-wide post-pass is gone. Where the provenance was actually lost. Not
Binding happens at lowering, per quantity. Mixed-scope regression added, as requested. One static reads charge counters on Runtime: a 2/2 ally with four +1/+1 counters, under a source holding three charge counters, resolves to 9/10 (+3/+0 from the source, +0/+4 from the recipient). A single collapsed referent cannot produce that pair. The Toxrill test still fails on revert with Blast radius held to zero outside the fix. Every non-static entry settles an unbound anaphor back to So the only cards whose counter scope changes are the four fixed ones — Toxrill, Clamavus, Thelon of Havenwood, Luxior, Giada's Gift. Coverage total is identical (31507/35482, 88.7971%). I checked this because an earlier revision did leak Scope reduced honestly: Spark Rupture is no longer claimed. Its "power and toughness each equal to the number of loyalty counters on it" reaches the other combinator, and giving that one provenance churned 10 unrelated cards; that path is left exactly as it was and the card stays a known gap rather than riding along on unrelated churn. Verification on |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_quantity.rs (1)
3943-3995: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd back a regression test for the plain (non-deferred) "counter on it" settle path.
The old test list covered
parse_for_each_clause("counter on it")settling toCountersOn{Source}. The new tests only exercise the deferred entry point (parse_for_each_clause_deferred), so nothing pins thatparse_for_each_clause/parse_for_each_clause_with_contextstill callsettle_deferred_counter_anaphor_refand collapse the pronoun back toSourcefor ordinary (non-static) callers.✅ Suggested addition
#[test] fn for_each_singular_counter_on_self() { ... } + + /// Regression: the PLAIN (non-deferred) entry point must still settle the + /// bare pronoun back to Source for every non-static caller. + #[test] + fn for_each_counter_on_pronoun_settles_to_source_via_plain_entry() { + let qty = parse_for_each_clause("counter on it"); + assert!( + matches!( + qty, + Some(QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: None, + }) + ), + "plain entry must settle Anaphoric -> Source, got {qty:?}" + ); + }🤖 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 3943 - 3995, Add a regression test alongside for_each_any_counter_on_pronoun_defers that calls parse_for_each_clause("counter on it") and asserts it settles to QuantityRef::CountersOn with ObjectScope::Source and no counter type, preserving the ordinary caller path through settle_deferred_counter_anaphor_ref while keeping the deferred entry-point test unchanged.
🤖 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_quantity.rs`:
- Around line 3943-3995: Add a regression test alongside
for_each_any_counter_on_pronoun_defers that calls parse_for_each_clause("counter
on it") and asserts it settles to QuantityRef::CountersOn with
ObjectScope::Source and no counter type, preserving the ordinary caller path
through settle_deferred_counter_anaphor_ref while keeping the deferred
entry-point test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03c04e88-c90e-4696-88b5-cc5b962012cb
📒 Files selected for processing (6)
crates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/src/parser/oracle_quantity.rscrates/engine/src/parser/oracle_static/anthem.rscrates/engine/src/parser/oracle_static/shared.rscrates/engine/tests/integration/counter_anaphor_binds_to_recipient.rs
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head ee6c968: counter anaphor provenance is now bound per quantity at static lowering; the mixed-scope runtime regression and the plain non-static settlement regression cover the two relevant boundaries.
…in per-recipient statics (phase-rs#6335) * fix(parser): bind a counter anaphor to the recipient in per-recipient statics CR 611.3a: a continuous effect from a static ability "isn't locked in; it applies at any given moment to whatever its text indicates", so a bare anaphoric pronoun ("it" / "them" / "him" / "her") in a per-recipient continuous static names the object RECEIVING the effect, not the source. `parse_counter_object_scope` (oracle_nom/quantity.rs) has no view of the enclosing static, so it bound every counter anaphor to `ObjectScope::Source`. Toxrill, the Corrosive therefore scaled "-1/-1 for each slime counter on them" off Toxrill's own (always empty) slime pile, and the penalty never applied. Rebinding happens once in `lower_static_ir` — the existing post-parse lowering funnel that already runs `populate_active_zones_from_condition` — so anthem, type_change and grammar builders all inherit it from a single authority instead of each re-deriving the referent. This is the quantity-axis twin of `StaticCondition::RecipientHasCounters` (documented in types/ability.rs as "the recipient analog of `HasCounters`"), so both axes now agree on what the pronoun in "...counters on it" refers to. No new enum variant: `ObjectScope::Recipient` already existed and the layer evaluator already resolves it per affected object — only the recipient-varying classifier in `quantity_expr_uses_recipient` was missing its `CountersOn` arm. The rebind is gated on the PRONOUN FORM, not merely on the affected set. The much larger Door of Destinies / Joraga Warcaller / Lion Sash class writes "...for each charge counter on ~", naming the source outright, where `Source` is already correct; a blanket rebind would have broken 32 cards to fix 5. Parse impact (regenerated card-data, whole 35k corpus): exactly 5 cards move to a recipient-scoped counter read, and no other card's scope changes. Toxrill, the Corrosive - Clamavus - Thelon of Havenwood Luxior, Giada's Gift - Spark Rupture Closes phase-rs#5929 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(parser): carry counter-anaphor provenance per quantity, not per description Addresses the review on phase-rs#6335: the first commit inferred the referent by scanning the static's whole description, so a static reading counters on BOTH `~` and its recipient would rebind the explicit source read too. The referent is now recorded per quantity at the parse site and bound at lowering. CR 608.2k: `parse_for_each_counters_on_source` (oracle_nom/quantity.rs) parsed the object with `parse_source_self_ref`, which accepts `~` AND "it" and collapsed both to `ObjectScope::Source`. That collapse is where the provenance was lost. The object is now dispatched: an explicit self-reference records `Source`, a bare objective pronoun records the deferred `ObjectScope::Anaphoric` — the variant the engine already documents for exactly this purpose and which `rebind_anaphoric_object_scope` already lists `CountersOn` under. `lower_static_ir` then binds each deferred read from the one place that knows the antecedent, walking both the continuous modifications and the `UnlessPay` condition tree (CR 118.12 — a combat tax carries its per-counter magnitude there, not in a modification). A self-referential subject binds back to `Source`, so Myr Prototype's AST is unchanged and its test needed no edit. Every other entry point settles an unbound anaphor back to `Source` at its own module boundary, so carrying provenance costs nothing outside the statics that consume it. Verified absolutely on the regenerated 35k corpus: ZERO cards leave an unbound `Anaphoric`, and the scope distribution is Source 495 / Recipient 8 / Target 6 — every counter read outside the fixed cards is byte-identical. Scope reduced honestly: Spark Rupture is no longer claimed. Its "equal to the number of loyalty counters on it" text reaches a different combinator whose provenance change would have churned 10 unrelated cards, so that path is left exactly as it was and the card stays a known gap. Adds the mixed source/recipient runtime regression the review asked for: one static reading charge counters on `~` (+3/+0 from the source) and +1/+1 counters on "it" (+0/+4 from the recipient) must produce 9/10 on a 2/2 ally — a single collapsed referent cannot produce that pair. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(PR-6335): cover plain counter anaphor settling --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Summary
Fixes the counter-anaphor scope misparse for Toxrill, the Corrosive and its class.
CR 611.3a: a continuous effect from a static ability "isn't 'locked in'; it applies at any given moment to whatever its text indicates." So a bare anaphoric pronoun (
it/them/him/her) in a per-recipient continuous static names the object receiving the effect, not the ability's source.parse_counter_object_scope(oracle_nom/quantity.rs:1203) has no view of the enclosing static, so it bound every counter anaphor toObjectScope::Source. Toxrill therefore scaled "-1/-1 for each slime counter on them" off Toxrill's own (always empty) slime pile, and the penalty never applied — the exact symptom reported.The rebind is gated on the pronoun form, not merely on the affected set. The much larger Door of Destinies / Joraga Warcaller / Lion Sash class writes "…for each charge counter on ~", naming the source outright, where
Sourceis already correct — a blanket rebind would have broken 32 cards to fix 5. A second test pins that class.No new enum variant:
ObjectScope::Recipientalready existed and the layer evaluator already resolves it per affected object (layers.rs:6078). Only the recipient-varying classifier inquantity_expr_uses_recipientwas missing itsCountersOnarm. This is the quantity-axis twin ofStaticCondition::RecipientHasCounters, documented intypes/ability.rs:7329as "the recipient analog ofHasCounters" — both axes now agree on what the pronoun in "…counters on it" refers to.Closes #5929
Files changed
crates/engine/src/parser/oracle_static/shared.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/game/quantity.rscrates/engine/tests/integration/counter_anaphor_binds_to_recipient.rscrates/engine/tests/integration/main.rsCR references
CR 611.3a— a static's continuous effect applies to whatever its text indicates, so the pronoun rebinds to the current recipientCR 613.4c— layer 7c, the effects/counters that modify power and/or toughnessCR 122.1— a counter is a marker placed on an objectImplementation method (required)
Method: not-applicable — single-seam parser lowering fix; planned and implemented in-session against a regenerated 35k-card corpus rather than via the multi-agent pipeline.
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
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— cleancargo clippy -p engine --all-targets --features proptest -- -D warnings— exit 0, no warningscargo test -p engine --lib—ok. 17490 passed; 0 failed; 6 ignoredcargo test -p engine(integration) —ok. 3727 passed; 0 failed; 2 ignored./scripts/gen-card-data.sh— exit 0, 35452 cards, 31507/35482 supported (88.80%)cargo semantic-audit— exit 0; zero findings for any of the 5 changed cardscargo coverage— all 5 changed cardssupported=true, gap_count=0; control class (Joraga Warcaller, Door of Destinies, Lion Sash, Hancock, Earthen Goo, Intrepid Adversary) alsosupported=true, gap_count=0Revert-proof: commenting out the single
bind_counter_anaphor_to_recipientcall and re-running the new integration test fails atcounter_anaphor_binds_to_recipient.rs:56withleft: (1, 1) right: (2, 2)— the source-scoped read applied Toxrill's own 3 slime counters uniformly to every affected creature. The test drivesGameScenario→evaluate_layers→derive_display_state, i.e. the production layer pipeline, not a parser-shape assertion.Gate A
Gate A PASS head=d39c97130e68af2902e45cd6d67df34c3266717f base=cf3381e5a8ca97351a573eac5e510de892d78dc6
Anchored on
crates/engine/src/parser/oracle_static/mod.rs:223—populate_active_zones_from_condition, the existing post-parse lowering transform inlower_static_irthat this one is registered beside, so every static builder inherits it from one authoritycrates/engine/src/parser/oracle_static/shared.rs:461— existing// CR 611.3a: bind the combat state to the recipient, not the sourcerecipient-binding in this same module, same CR, same rebind directioncrates/engine/src/game/quantity.rs:711—continuous_modification_dynamic_quantity, whose exhaustive no-wildcard arm list the new_mutmirror follows verbatimFinal review-impl
Final review-impl PASS head=d39c97130e68af2902e45cd6d67df34c3266717f
Self-review found and fixed one defect before this head:
affected_names_the_sourceoriginally citedCR 604.2for "an absent filter is self-scoped". CR 604.2 governs how long a static's continuous effect is active, not the default affected set — the citation did not describe the code, so it was replaced with a plain statement of the engine'sOption<TargetFilter>representation. Two anaphora helpers were also retargeted fromCR 608.2k(which scopes to cost/trigger-condition referents) toCR 611.3a, matching the convention already used atshared.rs:461.Claimed parse impact
Regenerated the full corpus and audited every
CountersOnscope in every static. Before this change zero cards parsed to a recipient-scoped counter read; after, exactly these 5 do, and no other card's scope changes:Corpus-wide scope distribution after the change:
Source90,Recipient10 (5 cards × power+toughness). The 32-card explicit-~class was asserted unchanged.Validation Failures
None.
CI Failures
None.
Tier: Frontier
Summary by CodeRabbit
Bug Fixes
Tests