fix(parser): bind source-counter-gated rider pronouns to the source (Gemstone Mine #6507) - #6559
fix(parser): bind source-counter-gated rider pronouns to the source (Gemstone Mine #6507)#6559jeffrey701 wants to merge 5 commits into
Conversation
…Gemstone Mine phase-rs#6507) The depletion-land sacrifice rider ("If there are no mining counters on this land, sacrifice it.") parsed to Sacrifice{ParentTarget}. A mana ability has no targets (CR 605.1a), so ParentTarget resolved to an empty set and the sacrifice silently no-op'd — the land never left play. Root cause is a parse-time anaphor mis-binding, not a runtime gap: the chunk-subject threading in the effect-chain parser already binds a bare "it" to SelfRef when the gating condition references the source object, via condition_refs_source_object. That predicate recognized the source-tapped / source-entered / source-attached conditions but not a source-scoped counter threshold (QuantityCheck over CountersOn{Source}), so the counter-gated riders fell through to ParentTarget (and, on typed triggers, to TriggeringSource). Extend condition_refs_source_object with a QuantityCheck arm that returns true when either side reads counters on the source, via a new exhaustive QuantityExpr walker (no wildcard — a future variant must be classified). This single predicate extension drives both existing consumers: the chunk-subject threading now supplies SelfRef, and the ParentTarget rewrite guard now skips these chunks. Bindings become source-correct for the Mercadian depletion lands (Peat Bog, Hickory Woodlot, Remote Farm, Sandstone Needle, Saprazzan Skerry), Gemstone Mine, Tourach's Gate, Daredevil Dragster, Last Light of Durin's Day, ED-E, and the whole source-counter-conditioned rider class (~21 cards). No runtime files change. CR 122.1 + CR 608.2k annotate the new arm. Closes phase-rs#6507 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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe parser now recognizes source-scoped counter references through wrapped quantity expressions, preserving ChangesSource Counter Binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/tests.rs (1)
28634-28709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrapper coverage is partial: only
Ref/Offsetare exercised.
quantity_expr_reads_source_countershas 8 recursive wrapper arms (DivideRounded,Offset,ClampMin,Multiply,Sum,Max,UpTo,Power,Difference), but this test only drives propagation throughOffset. Consider adding a couple more positive cases (e.g.Sum/Difference, which take two sub-expressions and are the likeliest place for a copy-paste slip) to lock in the exhaustive-walk guarantee the doc comment advertises.🤖 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/tests.rs` around lines 28634 - 28709, Add positive test cases in condition_refs_source_object_source_counter_quantity_check covering additional quantity_expr_reads_source_counters wrappers, especially binary Sum and Difference expressions containing a source-scoped CountersOn reference. Keep the assertions focused on propagation through both operands and preserve the existing non-source and wrapper coverage.
🤖 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/tests.rs`:
- Around line 28634-28709: Add positive test cases in
condition_refs_source_object_source_counter_quantity_check covering additional
quantity_expr_reads_source_counters wrappers, especially binary Sum and
Difference expressions containing a source-scoped CountersOn reference. Keep the
assertions focused on propagation through both operands and preserve the
existing non-source and wrapper coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4862da73-1499-4d5a-b7f0-3eb506842b5d
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_tests.rscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR · 17 card(s), 5 signature(s) (baseline: main
|
|
Request changes — the diagnosis, the seam, and the CR work are all correct, but the predicate is one degree too broad and lands a functional regression on Revelation of Power, a card outside the claimed scope. 🔴 Blocker
|
matthewevans
left a comment
There was a problem hiding this comment.
Current head 11b490a remains blocked.
condition_refs_source_object treats every QuantityCheck containing CountersOn { scope: Source } as a source reference. That is over-broad: Revelation of Power's bare "it" refers to its target creature, so this predicate rebinds the conditional flying/lifelink rider to the Instant itself and drops previously working behavior.
Narrow this to an explicit source noun phrase or suppress the path when the chain already carries the chosen-target referent. Add the Revelation of Power regression test and regenerate/reconcile the parse-diff before requesting review again.
… chosen target (phase-rs#6559 review) The phase-rs#6507 predicate that binds a source-counter-gated rider pronoun to SelfRef was one degree too broad: it also fired on Revelation of Power ("Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink"), whose intervening-if mis-scopes the bare "it" to CountersOn{Source}. Binding that grant to the source dropped flying/lifelink onto the one-shot Instant — the card lost its second sentence (engine_regress), and CR 608.2k does not reach it (the source is named by neither a cost nor a trigger condition). Narrow the binding: only rebind the pronoun to the source when NO earlier clause in the chain chose a typed target. Compute one gate at the chunk-subject site and reuse it at both consumers (the chunk-subject binding and the replace_target_with_parent guard): let binds_source_counter_pronoun = condition .is_some_and(condition_refs_source_object) && !chain_has_prior_typed_referent(builder.clauses(), false); chain_has_prior_typed_referent is true for Revelation of Power (its prior "Target creature gets +2/+2" is a Pump over a typed target) and false for every depletion-land / counter rider (whose prior clause is "Add mana" or "put a counter on ~", never a chosen target), so all 21 intended heals keep SelfRef while Revelation of Power's grant returns to ParentTarget. Chosen deliberately over chain_prior_referent_is_chosen_target, whose has_typed_target_widened early-out returns false for a pump-of-a-target. Adds source_counter_gate_over_prior_target_keeps_parent_not_self_ref (pins Revelation of Power's grant to ParentTarget) and extends the predicate unit test to drive the Sum/Difference two-operand walker arms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er' into fix/6507-depletion-sacrifice-rider
|
Thank you — the Revelation of Power catch was exactly right, and the diagnosis ("a mis-scoped bare Blocker — Revelation of Power no longer regressesI took the suppress-when-a-prior-clause-chose-a-typed-target option. At the let binds_source_counter_pronoun = condition
.as_ref()
.is_some_and(condition_refs_source_object)
&& !chain_has_prior_typed_referent(builder.clauses(), false);
I chose
Added Non-blocking items
Full lib + integration suites green; parser-combinator gate (Gate A) passes; the branch is up to date with current |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head production coverage gap.
🟠 Required
The repair for the prior Revelation of Power regression still lacks a production-pipeline regression. crates/engine/src/parser/oracle_effect/mod.rs:29929-29948 changes the runtime target binding and :30725-30743 rewrites the parent target, but Revelation is covered only by parser-shape tests in oracle_tests.rs:22924-22981; the new integration cases exercise Gemstone Mine, Peat Bog, and Last Light. The original defect was visible only after casting, target propagation, and layer application, where the grant could be applied to the Instant rather than the selected creature. Add an integration test that casts Revelation of Power at a countered creature and asserts that creature receives flying and lifelink; make it discriminating against reversion of the new guard.
🟡 Also reconcile before re-review
The sole parse-diff artifact predates this head and still describes Revelation as changing to self; the fresh Card data job is in progress. Let it publish and reconcile the exact affected-card/signature set for this head.
Recommendation: add the end-to-end Revelation regression, then provide current-head parse-diff evidence before re-review.
Summary
Fixes Gemstone Mine (#6507) and the whole source-counter-conditioned rider class: the depletion-land sacrifice rider — "{T}, Remove a mining counter from this land: Add one mana of any color. If there are no mining counters on this land, sacrifice it." — never sacrificed the land after its last counter was removed.
Root cause
Parse-time anaphor mis-binding, not a runtime gap. The rider's sub-ability parsed to
Sacrifice { target: ParentTarget }. A mana ability has no targets (CR 605.1a), soParentTargetresolves to an empty set at resolution (game/effects/sacrifice.rs—effect_object_targets(ParentTarget, [])is empty → the sacrifice silently no-ops and the land survives).The effect-chain parser already binds a bare "it" to
SelfRefwhen the gating condition references the source object, viacondition_refs_source_object. That predicate recognized source-tapped / source-entered / source-attached conditions, but not a source-scoped counter threshold (QuantityCheckoverCountersOn { scope: Source }) — the exact shape these riders produce. So the counter-gated body pronoun fell through toParentTarget(and, on typed triggers, toTriggeringSource).Fix
One additive predicate extension in
crates/engine/src/parser/oracle_effect/mod.rs(+41/-0):QuantityExprwalkerquantity_expr_reads_source_counters(no wildcard arm — a future variant must be classified; mirrorsquantity_expr_uses_recipient);QuantityCheck { lhs, rhs, .. }arm incondition_refs_source_objectreturning true when either side reads counters on the source.This single change drives both existing consumers of the predicate: the chunk-subject threading (mod.rs:28564) now supplies
SelfRef, and theParentTargetrewrite guard (mod.rs:29335) now skips these chunks. No runtime files change; the AST is now what the card says (SelfRef), andsacrifice.rs's existingSelfRefpool resolution + CR 400.7 epoch guard do the rest.Corrects the binding for ~21 cards: the 5 Mercadian depletion lands, Gemstone Mine, Tourach's Gate, Contested Game Ball, Daredevil Dragster, Dawn of a New Age, Evolved Spinoderm (
Sacrifice ParentTarget → SelfRef); Blood Spatter Analysis, Charitable Levy, Decree of Silence, Last Light of Durin's Day, The Heron Moon, ED-E (Sacrifice/PutCounter TriggeringSource → SelfRef); Heirloom Mirror, Ludevic's Test Subject, Replicating Ring, Smoldering Egg (RemoveCounter ParentTarget → SelfRef). Grasping Shadows / Soulcipher Board flipTransform SelfRef → ParentTarget, which is behavior-neutral (the transform effect handler falls back to the source on empty targets).Files changed
crates/engine/src/parser/oracle_effect/mod.rs— the fix (helper + match arm)crates/engine/src/parser/oracle_effect/tests.rs— predicate unit test (Source→true incl. wrapped/Not/And; Target/Recipient scopes→false)crates/engine/src/parser/oracle_tests.rs— 2 parser SHAPE tests (Gemstone Mine, Last Light) with reach-guardscrates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs— 5 runtime testscrates/engine/tests/integration/main.rs— mod lineCR references
CR 122.1(counters) +CR 608.2k(an effect referring to an untargeted object previously referred to by the ability still affects it) — the new predicate arm / helper.CR 605.1a(mana ability requires no target) +CR 605.3b(mana ability resolves immediately) +CR 701.21a(sacrifice) — test annotations.Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: claude-opus-4-8[1m]
Thinking: high
Verification
Required checks ran clean.
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 test -p engine --lib— 17593 passed, 0 failed, 6 ignored (baseline 17590 + 3 new)cargo test -p engine --test integration— 3874 passed, 0 failed, 2 ignored (baseline 3869 + 5 new)cargo fmt --all -- --check— clean./scripts/gen-card-data.sh— regenerated; parse-diff audit of the source-counter rider shape class: 23 cards changed, all either correctness heals or behavior-neutral (Transform empty-target→source fallback), zero regressions.RED/GREEN: tests 1/3/4/5 + both shape tests fail on the pre-fix
ParentTarget/TriggeringSourcebinding and pass after; test 2 is the paired over-trigger negative with positive reach-guards.Gate A
Gate A PASS head=1c74fedb5bef8d76fb45de2c5e22833208179d1d base=6ae8737cdab0fa1ed291cad0f0808473a90f4cf8
Anchored on
crates/engine/src/parser/oracle_trigger.rs:1297— trigger-body parse constructseffect_ctxwithsubject: Some(trigger_subject.clone())(the established subject-threading seam that already binds trigger-borne riders correctly).crates/engine/src/parser/oracle_effect/mod.rs:2876— delayed-trigger body parse threadsinner_ctx.subject = Some(TargetFilter::SelfRef)for the self-referential case — the exactSelfRefsubject-threading this change completes for the counter-gated chunk path (mod.rs:28564).Final review-impl
Final review-impl PASS head=1c74fedb5bef8d76fb45de2c5e22833208179d1d
Claimed parse impact
Validation Failures
None.
CI Failures
None.
Tier: Frontier
Summary by CodeRabbit