fix(parser): bind source-counter-gated rider pronouns to the source (Gemstone Mine #6507)

[jeffrey701]

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), so ParentTarget resolves 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 SelfRef when the gating condition references the source object, via condition_refs_source_object. That predicate recognized source-tapped / source-entered / source-attached conditions, but not a source-scoped counter threshold (QuantityCheck over CountersOn { scope: Source }) — the exact shape these riders produce. So the counter-gated body pronoun fell through to ParentTarget (and, on typed triggers, to TriggeringSource).

Fix

One additive predicate extension in crates/engine/src/parser/oracle_effect/mod.rs (+41/-0):

• a new exhaustive QuantityExpr walker quantity_expr_reads_source_counters (no wildcard arm — a future variant must be classified; mirrors quantity_expr_uses_recipient);
• a QuantityCheck { lhs, rhs, .. } arm in condition_refs_source_object returning 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 the ParentTarget rewrite guard (mod.rs:29335) now skips these chunks. No runtime files change; the AST is now what the card says (SelfRef), and sacrifice.rs's existing SelfRef pool 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 flip Transform 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-guards
• crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs — 5 runtime tests
• crates/engine/tests/integration/main.rs — mod line

CR 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/TriggeringSource binding 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 constructs effect_ctx with subject: 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 threads inner_ctx.subject = Some(TargetFilter::SelfRef) for the self-referential case — the exact SelfRef subject-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

• Gemstone Mine, Peat Bog, Hickory Woodlot, Remote Farm, Sandstone Needle, Saprazzan Skerry, Tourach's Gate, Contested Game Ball, Daredevil Dragster, Dawn of a New Age, Evolved Spinoderm, Blood Spatter Analysis, Charitable Levy, Decree of Silence, Last Light of Durin's Day, The Heron Moon, ED-E Lonesome Eyebot, Heirloom Mirror, Ludevic's Test Subject, Replicating Ring, Smoldering Egg (correctness heals); Grasping Shadows, Soulcipher Board (behavior-neutral Transform relabel).

Validation Failures

None.

CI Failures

None.

---

Tier: Frontier

Summary by CodeRabbit

• Bug Fixes
  • Fixed source-scoped counter threshold evaluation in card conditions, including recursive and wrapped (offset) expressions.
  • Corrected triggered “sacrifice it” behavior to consistently bind and sacrifice the intended source permanent (instead of the triggering object).
• Tests
  • Added unit and oracle parsing regression coverage for the updated source-counter logic.
  • Added new integration test coverage for Gemstone Mine, Peat Bog, and Last Light of Durin’s Day interactions (issue 6507).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5251c336-a3b0-4c64-8eab-43593ee99557

📥 Commits

Reviewing files that changed from the base of the PR and between 1c74fed and 11b490a.

📒 Files selected for processing (4)

• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/tests/integration/main.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_tests.rs

---

📝 Walkthrough

Walkthrough

The parser now recognizes source-scoped counter references through wrapped quantity expressions, preserving SelfRef binding for sacrifice riders. New unit, parser regression, and integration tests cover depletion counters, payment-triggered sacrifices, and typed-subject triggers.

Changes

Source Counter Binding

Layer / File(s)	Summary
Source counter condition detection crates/engine/src/parser/oracle_effect/mod.rs, crates/engine/src/parser/oracle_effect/tests.rs	Quantity expressions recursively detect source-scoped counter reads, and quantity-check classification is tested across wrappers and nested conditions.
Parser binding regressions crates/engine/src/parser/oracle_tests.rs	Gemstone Mine and Last Light of Durin’s Day parsing tests verify source counter conditions and gated sacrifice effects target SelfRef.
Depletion sacrifice scenarios crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs, crates/engine/tests/integration/main.rs	Integration tests cover counter depletion, remaining counters, automatic payment, mana outcomes, and sacrificing the enchantment rather than the triggering Mountain.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: bug

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main parser fix and the Gemstone Mine #6507 bug addressed by the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/src/parser/oracle_effect/tests.rs (1)

28634-28709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrapper coverage is partial: only Ref/Offset are exercised.

quantity_expr_reads_source_counters has 8 recursive wrapper arms (DivideRounded, Offset, ClampMin, Multiply, Sum, Max, UpTo, Power, Difference), but this test only drives propagation through Offset. 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

📥 Commits

Reviewing files that changed from the base of the PR and between e731b73 and 1c74fed.

📒 Files selected for processing (5)

• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/tests/integration/gemstone_mine_depletion_sacrifice_6507.rs
• crates/engine/tests/integration/main.rs

[github-actions]

Parse changes introduced by this PR · 23 card(s), 7 signature(s) (baseline: main 0446b31538b0)

🟡 Modified fields (7 signatures)

• 11 cards · 🔄 ability/Sacrifice · changed field target: parent target → self
  • Affected (first 3): Contested Game Ball, Daredevil Dragster, Dawn of a New Age (+8 more)
• 4 cards · 🔄 ability/RemoveCounter · changed field target: parent target → self
  • Affected (first 3): Heirloom Mirror, Ludevic's Test Subject, Replicating Ring (+1 more)
• 4 cards · 🔄 ability/Sacrifice · changed field target: triggering source → self
  • Affected (first 3): Blood Spatter Analysis, Charitable Levy, Decree of Silence (+1 more)
• 2 cards · 🔄 ability/Transform · changed field target: self → parent target
  • Affected (first 3): Grasping Shadows, Soulcipher Board
• 1 card · 🔄 ability/PutCounter · changed field target: triggering source → self
  • Affected (first 3): ED-E, Lonesome Eyebot
• 1 card · 🔄 ability/grant Flying, grant Lifelink · changed field affects: parent target → self
  • Affected (first 3): Revelation of Power
• 1 card · 🔄 ability/grant Flying, grant Lifelink · changed field target: parent target → ∅
  • Affected (first 3): Revelation of Power

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

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

Revelation of Power loses its conditional grant entirely

crates/engine/src/parser/oracle_effect/mod.rs — the new AbilityCondition::QuantityCheck arm in condition_refs_source_object.

Oracle text, verbatim from Scryfall (Instant, Streets of New Capenna):

Target creature gets +2/+2 until end of turn. If it has a counter on it, it also gains flying and lifelink until end of turn.

Both pronouns anaphor to target creature. The card is an Instant, so it can carry no counters and can gain no flying/lifelink.

The parse-diff sticky for this head reports it twice, and it is absent from ## Claimed parse impact:

- **1 card** · 🔄 ability/grant Flying, grant Lifelink · changed field `affects`: `parent target` → `self`
  - Affected (first 3): Revelation of Power
- **1 card** · 🔄 ability/grant Flying, grant Lifelink · changed field `target`: `parent target` → `∅`
  - Affected (first 3): Revelation of Power

Baseline AST on main (from data/card-data.json) is already correct, and shows exactly why the new arm fires:

"static_abilities": [{
  "affected": { "type": "ParentTarget" },            //  correct: the target creature
  "modifications": [ {"type":"AddKeyword","keyword":"Flying"},
                     {"type":"AddKeyword","keyword":"Lifelink"} ],
  "condition": { "type": "QuantityComparison",
    "lhs": { "type":"Ref", "qty": { "type":"CountersOn",
             "scope": { "type":"Source" } } },       //  pre-existing latent mis-scope
    "comparator": "GE", ... }
}]

The condition's scope: Source here is not a real source reference — it is an unresolved bare "it" that should have lowered to the parent target. That mis-scope was harmless while affected stayed ParentTarget. The new arm keys off precisely this shape, drives chunk_subject → SelfRef (mod.rs:28564), and rebinds the grant to the Instant. The rider becomes a permanent no-op: the card loses its second sentence. This is the engine_regress bucket — a card losing a previously-working handler.

The cited rule also does not reach this case. CR 608.2k, verbatim:

608.2k If an ability's effect refers to a specific untargeted object that has been previously referred to by that ability's cost or trigger condition, it still affects that object even if the object has changed characteristics.

Gemstone Mine qualifies — its cost, "Remove a mining counter from this land", refers to the source. Revelation of Power has neither a cost nor a trigger condition referring to the source; the predicate is reading an intervening-if gate in the effect body, which is a strictly wider surface than 608.2k licenses.

The discriminator you need is already visible in the two cards. A true source gate names its object — "no mining counters on this land", "If Blaster has no +1/+1 counters on it". A false positive gates on a bare pronoun — "If it has a counter on it" — inside a chain whose prior clause already declared a chosen target. Narrow the arm so it fires only when the counter reference came from an explicit source noun phrase (~ / "this land" / the card name), or suppress it when the chunk's chain already carries a chosen-target referent (chain_prior_referent_is_chosen_target / parent_target_available are both in scope at mod.rs:28564). Either gate keeps all 21 intended heals and drops Revelation of Power out of the class.

🟡 Non-blocking

Claimed set does not match the measured set. The body states "23 cards changed, all either correctness heals or behavior-neutral … zero regressions", and the claimed list also has 23 names — but the two sets are not identical. Revelation of Power is measured and unclaimed, so one claimed name is not actually in the diff. The matching totals made the substitution invisible. Worth re-deriving the claim from the artifact rather than by count.

Possible incomplete class coverage. Scanning data/card-data.json for the predicate's shape (source-scoped CountersOn gate + ParentTarget/TriggeringSource binding) also surfaces Hostile Hostel and Blaster, Morale Booster, neither of which appears in the parse diff. Blaster's back face reads "Move X +1/+1 counters from Blaster onto another target artifact. … If Blaster has no +1/+1 counters on it, convert it." — an explicitly-named source gate on a targeted activated ability, where the current ParentTarget binding would convert the targeted artifact instead of Blaster. If a guard is holding these back, the class fix is partial. Confidence: moderate — my scan is a local approximation over a main snapshot, not the CI artifact, so please confirm against the full parse-diff artifact rather than the truncated sticky (the +8 more buckets may hide further unclaimed cards).

CodeRabbit's nitpick is fair. quantity_expr_reads_source_counters has nine recursive arms; the unit test drives propagation through Ref/Offset only. Adding Sum/Difference cases would lock in the exhaustive-walk guarantee the doc comment advertises — the two-operand arms are where a copy-paste slip would hide.

✅ Clean

• Root-cause analysis is correct and precisely stated. ParentTarget does fall to the _ => arm of effect_object_targets (game/effects/mod.rs:251), resolving to the ability's chosen targets — empty for a mana ability per CR 605.1a. The Gemstone Mine diagnosis holds exactly as written.
• Right seam, no new vocabulary. Extending the existing condition_refs_source_object predicate drives both of its existing consumers rather than adding a parallel path, and no new enum variant was introduced.
• The Transform relabel is genuinely behavior-neutral, and I verified it rather than taking the claim. transform_effect::resolve dispatches on ability.targets.as_slice() with [] => ability.source_id (transform_effect.rs:23-30) and never consults the target filter, so Grasping Shadows / Soulcipher Board are unaffected. Confirming this in the body was the right call.
• Every CR citation grep-verifies against docs/MagicCompRules.txt: 122.1 (counters), 608.2k, 605.1a, 605.3b, 701.21a. No invented numbers — this is consistently done well in your PRs.
• The exhaustive QuantityExpr walker has no wildcard arm, so a future variant becomes a compile error instead of a silent false. Correct instinct, and the quantity_expr_uses_recipient mirror is the right precedent to follow.
• Integration test is registered — mod gemstone_mine_depletion_sacrifice_6507; is present in crates/engine/tests/integration/main.rs, so the 5 runtime tests actually run.
• Shape tests carry non-vacuity reach-guards (zero parse warnings, no Effect::Unimplemented anywhere in the parse, asserted ability count) before the shape assertions. This is exactly the guard that keeps a negative assertion from passing for the wrong reason.

Recommendation: narrow the QuantityCheck arm to explicit source noun phrases — or suppress it when the chain already has a chosen-target referent — and add a regression test pinning Revelation of Power's rider to ParentTarget. Then regenerate the parse diff and re-derive the claimed-impact list from the artifact, confirming whether Hostile Hostel and Blaster, Morale Booster should be in or out of the class. The Gemstone Mine fix itself is sound and worth landing once the blast radius is contained.