fix(engine): Gemstone Mine-class conditional sacrifice binds to source, not an unbound ParentTarget - #6797
Conversation
…e, not an unbound ParentTarget
An anaphoric-target rewrite in the chain parser unconditionally bound a
following clause's bare "it"/"that creature" to ParentTarget whenever any
prior clause existed in the chain, even when that prior clause established
no target at all (e.g. a mana ability's untargeted "Add one mana of any
color"). For Gemstone Mine's "{T}, Remove a mining counter: Add one mana of
any color. If there are no mining counters on this land, sacrifice it.",
this clobbered the sacrifice's target into a ParentTarget that resolves to
nothing at runtime, so the land was never sacrificed after its last counter
was removed (phase-rs#6507).
Gate the rewrite on the prior clause actually exposing a target concept, and
rebind a sacrifice imperative's unbound ParentTarget default to the ability's
own source when no such antecedent exists — covering the whole class of
targetless activated abilities with a trailing conditional self-sacrifice,
not just this card.
Co-Authored-By: Claude Sonnet 5 <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 (2)
📝 WalkthroughWalkthroughThe oracle effect parser refines conditional sacrifice target resolution while preserving created-token and outer referent bindings. Regression coverage validates Gemstone Mine parsing, mana production, counter depletion, and last-counter sacrifice behavior. ChangesOracle anaphora resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file 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: 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_effect/mod.rs`:
- Around line 30719-30724: The ParentTarget-to-SelfRef rewrite in the oracle
effect parser must not discard an existing created/tracked antecedent merely
because no target filter is present; update the logic around the
Effect::Sacrifice handling to preserve non-target anchors using composable
antecedent handling before rebinding. Add a regression case in
crates/engine/src/parser/oracle_tests.rs:8217-8259 covering a targetless
object-producing “if you do, sacrifice it” flow. Both sites require changes.
🪄 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: 3eb12e6d-2e78-49d9-82ef-de0bb1eb8e3a
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_tests.rs
Parse changes introduced by this PR · 6 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The SelfRef fallback can discard a real non-target antecedent. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:29888-29904 establishes if_you_do_anchor and parent_target_available as antecedent authority, including created/tracked “if you do” objects and inherited parent context. The new fallback at :30719-30724 tests only whether the immediately prior effect has target_filter().is_none() (:30688-30691) before replacing ParentTarget with SelfRef. Why it matters: a targetless object-producing clause can correctly establish a tracked/created referent while having no target filter, so the rewrite would sacrifice the source rather than that object. Suggested fix: guard the fallback with the existing non-target antecedent authority (including if_you_do_anchor/parent_target_available) and add a discriminating targetless object-producing if you do, sacrifice it regression.
[MED] The Gemstone Mine coverage is parser-shape only and does not prove the runtime fix. Evidence: crates/engine/src/parser/oracle_tests.rs:8226-8258 asserts an Effect::Mana AST node, a conditional sub-ability, and TargetFilter::SelfRef; it never activates the ability or resolves the sacrifice. Why it matters: it can pass while cost payment, conditional evaluation, or the SelfRef sacrifice resolution path still fails, leaving the reported gameplay bug untested. Suggested fix: add an integration pipeline test that activates the land with mining counters remaining and with the final counter removed, asserting mana is produced in both states and the land is sacrificed only after the last counter.
[MED] The parser-wide blast radius has not been reconciled with the current parse-diff artifact. Evidence: this parser change modifies anaphoric binding in crates/engine/src/parser/oracle_effect/mod.rs; the current <!-- coverage-parse-diff --> sticky reports 59 changed cards and 49 signatures, whereas the PR describes only the Gemstone Mine depletion-land class. Why it matters: the same rewrite can alter targets across unrelated conditional chains, and the broad changed surface is presently unexplained. Suggested fix: regenerate/attach the complete post-change artifact for the current head, enumerate and justify every gained/changed card, and add regressions for any intended sibling classes before re-review.
… non-target referents; add integration coverage Review follow-up on phase-rs#6797: - HIGH: the new "no antecedent" fallback compared only `target_filter().is_none()` on the prior clause, which also fires for a targetless clause that DOES establish a real referent through a different channel — a token/copy publisher (Effect::Populate has no target field but publishes a created-token referent via chain_prior_referent_is_created_token) or a GenericEffect grant surfaced only through its static's `affected` field (if_you_do_object_anchor). Excluding both from the guard prevents the sacrifice-target rewrite from firing on those chains, so a conditional "sacrifice it" after Populate keeps binding to the populated token (LastCreated) instead of being misrouted to the ability's own source. Added a regression (populate_conditional_sacrifice_keeps_created_token_not_self_ref) that fails against the prior guard and passes against this one. - MED: added a runtime integration test pair (gemstone_mine_survives_activation_with_counters_remaining, gemstone_mine_sacrifices_itself_on_last_counter_removed) that activates the real mana+conditional-sacrifice ability shape through resolve_mana_ability/resolve_mana_ability_sub_chain — proving mana production and the SelfRef sacrifice resolution path both work end-to-end, not just that the parser emits the right AST node. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
30672-30706: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the unverified
CR 122.1citation.
CR 122.1defines counters; it does not describe anaphoric target resolution or theParentTarget→SelfRefrewrite. The current rules document instead describesCR 608.2cas following instructions in order and applying the rules of English. (media.wizards.com)Remove
CR 122.1or replace it only with a verified rule whose text directly supports this implementation.As per path instructions, every rules-touching code path requires a verified CR citation whose rule body describes the implementation.
Also applies to: 30722-30740
🤖 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/mod.rs` around lines 30672 - 30706, Remove the unverified “CR 122.1” citation from the comments documenting the ParentTarget-to-SelfRef rewrite near prior_clause_has_no_target_concept and the sibling fixup around the corresponding later block. Retain CR 608.2c only unless a verified rule directly describing anaphoric target resolution and this rewrite is available; do not change the implementation logic.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/src/game/mana_abilities.rs (1)
9960-10032: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing Gemstone Mine fixture instead of duplicating it.
make_gemstone_mine_with_sacrifice_sub_abilityrepeats the object, counter, and cost setup frommake_gemstone_mine. Share one builder and parameterize the initial counter count and conditional sub-ability so the two fixtures cannot drift.As per coding guidelines, existing building blocks must be reused and helpers must not be duplicated.
🤖 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/game/mana_abilities.rs` around lines 9960 - 10032, Refactor make_gemstone_mine_with_sacrifice_sub_ability to reuse the existing make_gemstone_mine builder rather than duplicating object, counter, and mana-cost setup. Parameterize the shared builder with the initial mining counter count and optional conditional sacrifice sub-ability, preserving the current behavior for both fixtures and preventing their configurations from diverging.Source: Coding guidelines
🤖 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_effect/mod.rs`:
- Around line 30701-30706: Update the prior-clause lookup used to compute
prior_clause_has_no_target_concept so it skips ClauseDisposition::Continue
absorbed markers and selects the nearest non-absorbed semantic effect, matching
the lookback logic later in this function. Preserve typed referents so a valid
ParentTarget is not misclassified and rewritten as SelfRef.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 30672-30706: Remove the unverified “CR 122.1” citation from the
comments documenting the ParentTarget-to-SelfRef rewrite near
prior_clause_has_no_target_concept and the sibling fixup around the
corresponding later block. Retain CR 608.2c only unless a verified rule directly
describing anaphoric target resolution and this rewrite is available; do not
change the implementation logic.
---
Nitpick comments:
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 9960-10032: Refactor make_gemstone_mine_with_sacrifice_sub_ability
to reuse the existing make_gemstone_mine builder rather than duplicating object,
counter, and mana-cost setup. Parameterize the shared builder with the initial
mining counter count and optional conditional sacrifice sub-ability, preserving
the current behavior for both fixtures and preventing their configurations from
diverging.
🪄 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: 6aa6a34c-8a5c-4421-95d8-bdf5343c5b7e
📒 Files selected for processing (3)
crates/engine/src/game/mana_abilities.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
I re-reviewed the live head a43be74b86aa8d8dd9de3e9129897b75f399b26c. The new mana-ability tests close the prior runtime-coverage gap, but the shared anaphora fallback and its current-head evidence are not ready to merge.
[HIGH] The SelfRef fallback can discard an enclosing Otherwise antecedent. Evidence: oracle_effect/mod.rs:28539-28555 deliberately carries the outer referent into a recursively parsed else branch through ctx.parent_target_available; :29894-29904 turns that into parent_target_available. The new predicate and rewrite at :30701-30739 do not consult it: an else branch whose immediately preceding internal clause is targetless can therefore rewrite a valid outer ParentTarget to SelfRef. Why it matters: the resulting conditional sacrifice acts on the source instead of the object selected before the paired conditional. Suggested fix: make the fallback depend on the established parent_target_available authority (and not merely the local prior clause’s missing target_filter); add a discriminating nested-Otherwise regression with an outer typed target, a targetless inner clause, and a conditional sacrifice it that must remain ParentTarget.
[MED] The antecedent lookback includes absorbed Continue markers. Evidence: oracle_effect/mod.rs:30701-30706 uses builder.clauses().last(), while nearby semantic lookbacks intentionally select the nearest non-ClauseDisposition::Continue clause (for example :30950-30953). Why it matters: an absorbed rider after a target-bearing clause can make a valid antecedent look targetless and trigger the erroneous SelfRef rewrite. Suggested fix: use the reverse non-Continue semantic-clause lookback consistently, with a regression covering a target-bearing antecedent followed by an absorbed continuation and a conditional pronoun consumer.
[MED] The new CR annotations do not support the behavior they describe. Evidence: the two new comments at oracle_effect/mod.rs:30672 and :30722 cite CR 122.1, which defines counters, not pronoun/antecedent binding. CR 608.2c is the relevant verified support for applying card instructions in written order and according to the rules of English. Why it matters: an unrelated CR citation falsely presents parser behavior as rules-verified. Suggested fix: remove CR 122.1 from both comments and retain/use only a verified, behavior-relevant annotation (CR 608.2c with an accurate description).
[MED] The parse-diff evidence is stale and its scope is not reconciled. Evidence: the <!-- coverage-parse-diff --> sticky was posted at 2026-07-29T23:18:59Z, before the current head’s follow-up commit, and reports 59 cards / 49 signatures. That is far beyond the stated six depletion-land target changes (Gemstone Mine, Hickory Woodlot, Peat Bog, and siblings) and includes unrelated targets, grants, zone changes, and Feather. Why it matters: this shared parser rewrite can silently alter unrelated conditional chains; the current artifact cannot establish that the intended surface is bounded. Suggested fix: regenerate the full parse-diff artifact for this exact head, reconcile every affected card/signature against the claimed scope, and add focused regressions for each intentionally changed sibling class before requesting re-review.
…e only, revert the shared Kicker-clause gate Review follow-up on phase-rs#6797 (MED: blast-radius concern). Narrowing the shared "Kicker clause" rewrite's gate (used by `replace_target_with_parent` for every targetable effect kind, not just Sacrifice) was unnecessary and wrong: a full coverage-parse-diff against main's fd53e6a showed it flipped 59 cards across 49 signatures, including a genuine regression in Feather, the Redeemed (its ExileResolvingSpell return-at/return-to rider dropped entirely) and target-binding churn across PutCounter, Pump, grant-Flying/DoubleStrike/FirstStrike/Trample/Haste/ Indestructible, ChangeZone, DealDamage, Destroy, and GainControl on unrelated cards (Managorger Phoenix, Skizzik, Searing Blaze, Savage Punch, ED-E, and more) — none of which are the reported bug's class. The Kicker-clause gate is restored to its original `!builder.is_empty()`. The actual fix is the standalone Sacrifice-specific fixup added below it, which only rebinds `Effect::Sacrifice`'s unbound `ParentTarget` default to `SelfRef` — that alone was already sufficient to fix Gemstone Mine; touching the shared gate never contributed to the fix and only caused collateral damage. Re-verified with the same parse-diff pipeline: this narrowed change now touches exactly 6 cards / 1 signature (`ability/Sacrifice`, target `parent target` → `self`) — Gemstone Mine and the five genuine Homelands depletion lands (Hickory Woodlot, Peat Bog, Remote Farm, Sandstone Needle, Saprazzan Skerry), all sharing the identical "{T}, Remove a depletion counter: Add mana. If there are no depletion counters on this land, sacrifice it." shape. Zero unrelated cards changed; the full engine test suite (18015 tests) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all three findings. HIGH — SelfRef fallback could discard a real non-target antecedent. Confirmed: the guard only checked MED — parser-shape-only coverage. Added a real activation-pipeline test pair in MED — blast radius vs. the parse-diff artifact. This was the important one. I regenerated the parse-diff locally ( Root cause: I had unnecessarily narrowed the shared "Kicker clause" rewrite's gate ( Re-ran the same parse-diff pipeline against the narrowed fix: 6 cards / 1 signature, all 🤖 Generated with Claude Code |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head still has an anaphora correctness defect and incomplete current-base evidence.
🔴 Blocker
[HIGH] The Sacrifice-only SelfRef fallback can still discard an enclosing Otherwise antecedent. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:28539-28555 explicitly seeds ctx.parent_target_available for an else branch, and :29894-29904 establishes that authority. The new predicate at :30717-30728 only examines local antecedents and can rebind a valid inherited ParentTarget to SelfRef. Why it matters: a nested else branch can sacrifice the ability source instead of the object selected before the paired conditional. Suggested fix: make the fallback respect established parent_target_available, and add a nested-Otherwise regression with an outer typed target, a targetless local clause, and a conditional sacrifice it that remains ParentTarget.
🟡 Non-blocking
[MED] The predicate uses a syntactic last clause rather than the semantic predecessor. Evidence: oracle_effect/mod.rs:30717-30722 calls .last(), while the nearby semantic lookback at :30935-30951 reverses past ClauseDisposition::Continue. Why it matters: an absorbed continuation can hide a target-bearing antecedent and spuriously trigger the SelfRef rewrite. Suggested fix: use the same reverse non-Continue lookback and add a regression for a target-bearing clause followed by an absorbed continuation and pronoun consumer.
[MED] CR 122.1 does not support pronoun/antecedent parsing. Evidence: oracle_effect/mod.rs:30694 cites it, but CR 122.1 defines a counter as “a marker placed on an object or player”; it does not concern anaphora. The verified relevant text is CR 608.2c: the controller follows instructions “in the order written” and applies the rules of English. Why it matters: the annotation falsely claims rules support for this parser behavior. Suggested fix: remove CR 122.1 and retain only a verified, accurate CR 608.2c annotation.
[MED] The parse-diff proof is not yet current-base public evidence. Evidence: the latest CI artifact for this head is bounded to six depletion lands and one Sacrifice signature, but the public <!-- coverage-parse-diff --> sticky still names baseline main bbb176eb756d, whereas current main has advanced. Why it matters: this shared parser seam needs a current public card-level artifact before its bounded blast radius can be approved. Suggested fix: refresh the sticky for the current base and reconcile the six-card set before requesting re-review.
Recommendation: request changes. Address the inherited-authority and semantic-lookback regressions, correct the CR annotation, and refresh the public parse-diff evidence on the resulting head.
…ailable; drop mismatched CR citation Review round 2 follow-up on phase-rs#6797. - HIGH: the no-antecedent guard checked only the immediately-preceding clause's target_filter(), which cannot see a referent established BEFORE a paired conditional when the current clause is inside a recursively-parsed `Otherwise` else-branch (its own `clauses` start empty). Now defers first to `parent_target_available` (already computed above, and already carrying `ctx.parent_target_available` across exactly this recursive-else-branch boundary for the same reason — see the "Otherwise" handler's own comment). Added conditional_sacrifice_defers_to_seeded_parent_target_available, which drives parse_effect_chain_ir directly with parent_target_available seeded (mirroring what a real outer Otherwise referent seeds) and fails against the old guard. - MED: the lookback now uses the nearest non-`ClauseDisposition::Continue` clause instead of a bare `.last()`, matching the existing lookback idiom `if_you_do_object_anchor` already uses for the same reason — an absorbed rider carries no independent referent and must not be mistaken for a targetless antecedent. - MED: dropped the CR 122.1 citation from the Sacrifice-fixup comment. CR 122.1 defines counters; it does not support pronoun/antecedent-binding behavior. CR 608.2c (order-of-instructions + English-rules anaphora) is the citation this code actually implements, and is now the sole one. - MED: regenerated the parse-diff against this exact head (oracle-gen + coverage-report --all + coverage-parse-diff, same base fd53e6a used throughout this PR's review). Unchanged: 6 cards / 1 signature (ability/Sacrifice target parent target -> self — Gemstone Mine and the five genuine Homelands depletion lands). The two code fixes above are purely defensive additions gated on conditions none of those six cards' parse trees trigger, so the verified scope is stable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all four findings from this round. HIGH — SelfRef fallback could discard an enclosing Otherwise antecedent. Confirmed. The guard checked only the immediately-preceding clause's Getting a natural-language regression for this took a few wrong turns — Oracle-text constructions I tried either routed the trailing conditional through a different "shared tail after if/otherwise" duplication path, or lost the condition entirely in a "then if"/"and if" mid-sentence join. Rather than fight the chunker further, I drove MED — antecedent lookback includes absorbed Continue markers. Fixed — the lookback now uses the nearest non- MED — CR 122.1 doesn't support this behavior. Correct, dropped it. CR 122.1 defines counters; it doesn't govern pronoun/antecedent binding. CR 608.2c (order-of-instructions + English-rules anaphora) is what this code actually implements and is now the sole citation. MED — stale/unreconciled parse-diff. Regenerated the full local pipeline ( Full engine suite (18016 tests, excluding one pre-existing flaky call-count test unrelated to this change and confirmed independent) is green; 🤖 Generated with Claude Code |
|
Hold — current-head parser evidence pending. For head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 1d6d21c. The current parse-diff artifact is bounded to the six depletion lands (one Sacrifice target signature), and required checks are green.
2 participants
Summary
parse_effect_chain_irbound a conditional clause's bareit/that creaturetoParentTargetwhenever any prior clause existed in the chain — even when that prior clause established no target at all (a mana ability's untargeted "Add one mana of any color" has no target concept). For Gemstone Mine's"{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.", this clobbered the sacrifice's target into an unboundParentTargetthat resolves to nothing at runtime, so the land was never sacrificed.crates/engine/src/parser/oracle_effect/mod.rs):target_filter().is_some()) instead of merely!builder.is_empty().ParentTargetdefault to the ability's own source (SelfRef) — the only objectitcan plausibly name in that shape.crates/engine/src/parser/oracle_tests.rs) that parses Gemstone Mine's actual Oracle text end-to-end and asserts the sacrifice sub-ability binds toTargetFilter::SelfRef.Test plan
gemstone_mine_conditional_sacrifice_binds_to_self_refpasses.cargo test -p phase-engine --lib(18000 tests) passes, including the pre-existing anaphoric-binding regression tests this change touches (kicker_instead_chain_produces_correct_condition, Jhoira of the Ghitu suspend tests, Managorger Phoenix graveyard-return test, Dalkovan Encampment delayed-trigger test,delayed_trigger_at_beginning_of_next_end_stepsnapshot).cargo fmt --allclean.cargo clippy -p phase-engine --lib -- -D warningsclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests