fix(parser): retain target for counter instead overrides

[keloide]

Summary

Fix the optional-payment form of a conditional instead override: after “you may pay … If you do, X”, a later “Y instead” now replaces the paid continuation X, rather than the payment itself. This completes the Emiel the Blessed path while retaining the existing target/event-context rebinding work for #6677.

Closes #6677.

Files changed

• crates/engine/src/parser/oracle_effect/assembly.rs
• crates/engine/src/parser/oracle_effect/lower.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/tests/integration/issue_6677_wakandan_royal_guard.rs
• crates/engine/tests/integration/main.rs

Track

Developer

LLM

Model: gpt-5-6
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

CR references

• CR 608.2c — later text may modify an earlier instruction; the paid continuation is the instruction modified here.
• CR 603.2 — Emiel’s entering-creature triggered ability.
• CR 122.1 — counter placement.

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 — passed.

• git diff --check — passed.

• ./scripts/check-skill-doc.sh — passed.

• ./scripts/check-parser-combinators.sh — Gate G PASS; Gate A PASS below.

• cargo clippy --all-targets -- -D warnings — infrastructure-inconclusive: the runner completed engine compilation without a Rust diagnostic but dropped the final Cargo status.

• cargo test -p phase-engine --test integration issue_6677_wakandan_royal_guard::emiel_the_blessed_doubles_counters_on_the_entering_unicorn -- --exact --nocapture — infrastructure-inconclusive: the runner terminated before the integration binary linked or ran; no Rust diagnostic.

• ./scripts/gen-card-data.sh — infrastructure-inconclusive: the runner terminated after refreshing MTGJSON inputs and before invoking the generator; no tracked output changed.

• cargo coverage / cargo semantic-audit — CI-owned: generated data/card-data.json is absent locally, so these audits cannot run before card-data generation completes.

Gate A

Gate A PASS head=453876adc9287a4b0602aee22807236b2b838adf base=02760f58841457847f535a6f54aa480ae8e22af5

Anchored on

• crates/engine/src/parser/oracle_effect/lower.rs:2388 — existing counter-override antecedent rebinding authority.
• crates/engine/src/game/effects/mod.rs:2693 — existing OptionalEffectPerformed continuation resolution authority.

Final review-impl

Final review-impl PASS head=453876adc9287a4b0602aee22807236b2b838adf

Claimed parse impact

Pending current-head coverage parse-diff. The prior artifact predates this commit and covered only Elder Cathar and Wakandan Royal Guard; this repair is expected to add Emiel the Blessed’s paid-continuation shape.

Scope Expansion

None.

Validation Failures

• Local Cargo test and lint commands cannot yield a terminal status in this runner after compilation; no source diagnostic was emitted.
• Card-data generation was externally stopped before the generator, leaving its required audit inputs unavailable.

CI Failures

The prior current-head CI failure in Emiel’s regression is addressed by this commit. Current-head CI is pending.

Summary by CodeRabbit

• Bug Fixes

  • Fixed conditional counter effects so replacement counters remain attached to the originally selected creature.
  • Corrected counter distribution for restricted targets, including Heroes and non-Hero creatures.
  • Preserved trigger-event references when applying conditional effects to entering creatures.
  • Fixed optional payment effects so conditional replacements apply after payment choices without disrupting the payment flow.

• Tests

  • Added parser and integration coverage for conditional counter overrides and related targeting scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

The parser now preserves selected targets across conditional PutCounter overrides and optional-payment continuations. Parser and integration tests cover target matching, counter quantities, and trigger-event references.

Changes

Conditional counter target reuse

Layer / File(s)	Summary
Assemble optional-payment replacements crates/engine/src/parser/oracle_effect/assembly.rs	Instead clauses under optional payments now attach to the paid continuation and preserve target, anaphor, and fallback bindings.
Lower typed counter overrides crates/engine/src/parser/oracle_effect/lower.rs, crates/engine/src/parser/oracle_effect/tests.rs	Typed PutCounter antecedents reuse ParentTarget. Supported parent and triggering references remain available. Parser tests verify successful lowering and target preservation.
Validate counter target resolution crates/engine/tests/integration/issue_6677_wakandan_royal_guard.rs, crates/engine/tests/integration/main.rs	Integration tests verify counter placement for Hero, non-Hero, Unicorn, and unrelated creature targets. The regression module is registered in the engine suite.

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

Possibly related PRs

• phase-rs/phase#6310: Both changes handle ParentTarget for scoped counter effects.
• phase-rs/phase#6528: Both changes update context-aware target binding in Oracle parsing.
• phase-rs/phase#6748: Both changes modify cross-line Instead handling.

Suggested labels: bug

Suggested reviewers: matthewevans, andriypolanski

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The parser changes and regression tests address issue #6677 by keeping counters on the selected creature.
Out of Scope Changes check	✅ Passed	The parser changes and related regression tests support the target-retention fix and do not introduce unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the parser fix that preserves targets for counter Instead overrides.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[matthewevans]

Changes requested — the current implementation still misses one member of the stated replacement class.

🔴 Blocker

[MED] The new rewrite only rebinds an instead-counter body when its immediate PutCounter antecedent has TargetFilter::Typed; it returns false for every other antecedent target. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:2406-2419. The same stated “it … instead” class includes Emiel the Blessed: resolve_it_pronoun maps any non-self trigger subject to TriggeringSource (crates/engine/src/parser/oracle_effect/mod.rs:221-238), and Emiel’s “another creature you control enters” is that case. The lowering guard then returns false for TriggeringSource, so the override’s bare it remains SelfRef and puts counters on Emiel rather than the entering Unicorn. The linked #6677 lists Emiel as the same root cause, while tests only cover typed Wakandan/Elder forms (tests.rs:19091-19132; issue_6677_wakandan_royal_guard.rs:14-103).

Recommendation: preserve valid already-bound object antecedents, including ParentTarget/TriggeringSource where applicable, and add an Emiel runtime regression with an entering Unicorn and distinct source/other creature.

[github-actions]

Parse changes introduced by this PR · 3 card(s), 6 signature(s) (baseline: main 5a3a42a79bf0)

🟡 Modified fields (6 signatures)

• 2 cards · 🔄 ability/PutCounter · changed field target: self → parent target
  • Affected (first 3): Elder Cathar, Wakandan Royal Guard
• 1 card · 🔄 ability/PutCounter · changed field conditional: instead if (target is Unicorn) → previous effect outcome
  • Affected (first 3): Emiel the Blessed
• 1 card · 🔄 ability/PutCounter · changed field conditional: previous effect outcome → instead if (target is Unicorn)
  • Affected (first 3): Emiel the Blessed
• 1 card · 🔄 ability/PutCounter · changed field counter: 1 P1P1 → 2 P1P1
  • Affected (first 3): Emiel the Blessed
• 1 card · 🔄 ability/PutCounter · changed field counter: 2 P1P1 → 1 P1P1
  • Affected (first 3): Emiel the Blessed
• 1 card · 🔄 ability/PutCounter · changed field target: self → triggering source
  • Affected (first 3): Emiel the Blessed

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

[matthewevans]

Changes requested — the current-head Emiel regression fails in CI.

🔴 Blocker

[MED] The new runtime regression does not establish the required behavior: issue_6677_wakandan_royal_guard.rs:140 expects the entering Unicorn to have two +1/+1 counters, but the CI run observed left: 0, right: 2. The terminal job is Rust tests (shard 2/2). The change therefore does not yet prove or deliver the Emiel trigger path it adds.

Recommendation: Trace the accepted optional-payment trigger through its counter/"instead" branches, correct the unresolved binding or execution path, and update the regression so it passes with a reach guard for the optional paid path.

Evidence: current-head CI output and the linked regression test. Confidence: high. I have not inferred a root cause beyond the failing production-path assertion.

[matthewevans]

Maintainer hold — current-head verification is pending.

I merged the branch cleanly with the current main and pushed current head 09b43387dd224044565295d4e414a70e4d083dd9. The implementation review found the prior target-binding and Emiel paid-continuation blockers resolved at this head's parent, but approval/enqueue must wait for current-head evidence:

• required Rust, frontend, card-data, WASM, and Tauri checks are pending;
• the parse-diff artifact predates this merge and must refresh for the current head;
• the PR body’s Gate A and final review-impl hashes still name 453876adc9287a4b0602aee22807236b2b838adf.

No action is required from the contributor. This is a maintainer verification hold; I will not approve or enable auto-merge until the updated checks and artifact are live.

[matthewevans]

Maintainer sign-off: current-head CI, parse-diff, and implementation review are clean.