fix(parser): resolve bare 'they may' pronoun subject as optional TriggeringPlayer

[claytonlin1110]

Wandering Archaic's "Whenever an opponent casts an instant or sorcery
spell, they may pay {2}. If they don't, you may copy that spell."
never worked: the payer resolved to the ability's Controller (the
Wandering Archaic player) instead of the casting opponent, and the
PayCost step was mandatory instead of optional, so the "if they
don't" copy branch could never fire correctly.

parse_subject_application already recognized "that player may pay"
(Smothering Tithe, Mind Whip) as an optional-modal subject phrase, but
the equivalent bare-pronoun phrasing "they may pay" had no matching
arm — only the exact string "they" (without a trailing "may") was
accepted, so "they may" fell through with no match and the payer/
optionality defaulted to Controller/false. Add a "they may" branch
that reuses the existing resolve_they_pronoun dispatch and threads
through is_optional, mirroring the "that player may" handling.

Fixes #6477.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

Summary by CodeRabbit

• Bug Fixes

  • Fixed Oracle parsing and resolution for “they may …” instructions, including correct optionality and target selection.
  • Corrected defending-player references in triggered effects and related pronoun resolution.
  • Ensured optional mana payments are offered to the correct player, with follow-up copy effects occurring only when payment is declined.

• Tests

  • Added coverage for “they may …” trigger scenarios and defending-player targeting.
  • Added integration tests for Wandering Archaic’s optional payment and conditional copy effect.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The parser recognizes “they may” as an optional subject application and resolves defending-player scope. Tests cover player binding across oracle effects and validate Wandering Archaic payment, copying, mana, and damage outcomes.

Changes

Oracle subject resolution

Layer / File(s)	Summary
Subject parsing and scope resolution crates/engine/src/parser/oracle_effect/subject.rs, crates/engine/src/parser/oracle_trigger.rs	Subject parsing accepts “they may”. Trigger parsing assigns defending-player scope for later pronoun resolution.
Pronoun binding test coverage crates/engine/src/parser/oracle_trigger_tests.rs	Parser tests cover optionality and player binding for payment, discard, draw, reveal, damage, and target-filter effects.
Wandering Archaic payment and copy flow crates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rs, crates/engine/tests/integration/main.rs	Integration tests construct the game, resolve Shock, validate optional payment prompts, and verify copy outcomes for both payment choices. The suite is registered in the integration harness.

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

Sequence Diagram(s)

sequenceDiagram
  participant P1
  participant GameRunner
  participant P0
  GameRunner->>P1: Offer optional {2} payment
  P1->>GameRunner: Accept or decline payment
  GameRunner->>P0: Offer copy choice when payment is declined
  P0->>GameRunner: Accept copy
  GameRunner->>GameRunner: Resolve original spell and optional copy

Loading

Possibly related PRs

• phase-rs/phase#6564: Related anaphor scoping changes for trigger contexts.

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 identifies the parser fix for bare “they may” optional pronoun handling.
Linked Issues check	✅ Passed	The changes satisfy issue #6477 by resolving the casting opponent as payer, preserving optionality, and testing both payment outcomes.
Out of Scope Changes check	✅ Passed	The parser updates and regression tests directly support issue #6477 and related pronoun-resolution behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

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_trigger_tests.rs`:
- Around line 21891-21896: Correct the documentation comment above the related
oracle trigger test to remove the player-resolution attribution to CR 608.2k.
Retain CR 608.2d for the optional payment choice, and narrow any remaining CR
608.2k reference to its applicable “that spell” object reference.

🪄 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: a63a7e80-ebbe-4b49-ac72-950a06aba531

📥 Commits

Reviewing files that changed from the base of the PR and between 6c79e41 and a308d98.

📒 Files selected for processing (2)

• crates/engine/src/parser/oracle_effect/subject.rs
• crates/engine/src/parser/oracle_trigger_tests.rs

[matthewevans]

Blocked — the parser seam is appropriate, but this head needs one rules-evidence correction and a production-path regression before it can merge.

🔴 Blocker

• crates/engine/src/parser/oracle_trigger_tests.rs:21891 attributes resolving the player pronoun they to CR 608.2k. That rule instead begins, “If an ability’s effect refers to a specific untargeted object,” so it does not support player-pronoun resolution. Keep CR 608.2d only for the optional choice, and either remove the 608.2k citation from this test or restrict it to the that spell object reference. This confirms the unresolved CodeRabbit finding on the current head.

• crates/engine/src/parser/oracle_trigger_tests.rs:21898 only calls parse_trigger_line and inspects the lowered definition. It proves the parser shape, but not the reported behavior: the opponent must receive the optional payment choice during real trigger resolution, declining it must run the conditional CopySpell, and paying must suppress that branch. Add a registered integration scenario using Wandering Archaic and an opponent-cast instant or sorcery; exercise both decline and pay paths. The existing integration copy/payment suites are the appropriate production-level test seam.

✅ Clean

The all_consuming(alt(...)) subject arm in crates/engine/src/parser/oracle_effect/subject.rs:2845 is at the existing subject-application authority, preserves the prior resolve_they_pronoun dispatch, and makes the new may axis explicit. The focused parser assertions would fail on removal of that new arm.

Recommendation: request changes — correct the CR annotation and add the two-outcome runtime regression, then request a fresh review.

[github-actions]

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

🟡 Modified fields (5 signatures)

• 2 cards · 🔄 ability/Discard · changed field target: any target → parent target
  • Affected (first 3): Mishra's Command, Undercity Plunder
• 1 card · 🔄 ability/DamageAll · changed field filter: without Flying you control creature → without Flying defending player controls creature
  • Affected (first 3): Siege Dragon
• 1 card · 🔄 ability/Draw · changed field target: any target → triggering player
  • Affected (first 3): Tarnation
• 1 card · 🔄 ability/Draw · changed field target: parent target → defending player
  • Affected (first 3): Elder Brain
• 1 card · 🔄 ability/RevealHand · changed field player: any target → defending player
  • Affected (first 3): Smart Ass

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

[matthewevans]

Blocked — this current head has an unexplained parser blast radius outside the claimed Wandering Archaic / #6477 scope.

🔴 Blocker

• crates/engine/src/parser/oracle_effect/subject.rs:2845 changes subject parsing beyond the intended they may behavior. The current-head parse-diff reports four unrelated modified fields: Mishra's Command and Undercity Plunder (Discard.target, any target → parent target), Tarnation (Draw.target, any target → triggering player), and Smart Ass (RevealHand.player, any target → parent target). Evidence: current parse-diff artifact. Explain and add focused tests for each changed parsing class, or narrow the change so it affects only the intended they may behavior.

Recommendation: request changes — resolve this parser blast radius together with the existing CR-annotation and runtime-regression blockers, then request a fresh review.

[coderabbitai]

Actionable comments posted: 2

🤖 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/tests/integration/issue_6477_wandering_archaic_optional_payment.rs`:
- Around line 164-165: Update the comment in the test around the
payment-decision assertion to cite CR 608.2d only for the payment choice
belonging to the casting opponent, not for resolving the bare “they” reference
to P1. Remove the citation or attribution from the payer-resolution claim while
preserving the regression context.
- Around line 132-135: Update the WaitingFor::OptionalEffectChoice handling in
the helper so the paid path fails immediately on any unexpected subsequent copy
prompt instead of dispatching DecideOptionalEffect with accept: false. Preserve
the existing behavior for expected game states, but ensure the flow after P1
pays cannot silently decline the copy and still pass with only 2 damage.

🪄 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: a31f0af5-ba50-480f-b29c-bf7f74918f33

📥 Commits

Reviewing files that changed from the base of the PR and between a308d98 and af96799.

📒 Files selected for processing (3)

• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rs
• crates/engine/tests/integration/main.rs

[matthewevans]

Blocked — this current head contains a non-discriminating paid-path regression and two unresolved evidence defects.

🔴 Blocker

• crates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rs:124-150,281-291: drive_to_idle silently sends accept: false for every later WaitingFor::OptionalEffectChoice. The paid-path test calls it after accepting the payment, so a regression that incorrectly offers the copy is declined by the helper and still reaches the expected 2 damage. Make the paid path fail on a subsequent optional-effect prompt (or give the helper an explicit allowed-prompt contract) so the assertion distinguishes payment suppressing the copy.

• crates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rs:164-165: CR 608.2d governs how an effect's offered choices are made; it does not establish that bare they identifies P1. Keep it only for the optional choice, and remove the payer-attribution claim or support that parsing assertion without that citation. The checked rule text starts, "If an effect ... offers any choices ... the player announces these while applying the effect."

• crates/engine/src/parser/oracle_effect/subject.rs:2845: the current parse-diff artifact reports four unrelated changed cards/fields (Mishra's Command, Undercity Plunder, Tarnation, and Smart Ass) outside the stated Wandering Archaic scope. Explain and cover each affected parsing class, or narrow the arm so it changes only the intended they may form. Evidence: #6823 (comment)

✅ Clean

The all_consuming(alt(...)) they may parser arm remains at the existing subject-application authority and preserves the existing resolve_they_pronoun dispatch.

Recommendation: request changes — make the paid-path test fail on an unexpected copy prompt, correct the CR attribution, and resolve the measured parser blast radius before re-review.

[coderabbitai]

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_trigger_tests.rs`:
- Around line 25906-25912: Update the Effect::RevealHand assertion in the oracle
trigger test to compare target against the exact TargetFilter representing the
preceding “defending player” referent, replacing the weaker TargetFilter::Any
inequality check while preserving the existing match guard.

🪄 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: e054d0df-447e-4846-add0-e7f4bded6a0f

📥 Commits

Reviewing files that changed from the base of the PR and between af96799 and 656677c.

📒 Files selected for processing (2)

• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/tests/integration/issue_6477_wandering_archaic_optional_payment.rs

[matthewevans]

Blocked — the parser still lowers “defending player” through the generic parent-target fallback instead of the combat-relative player it names.

🔴 Blocker

• crates/engine/src/parser/oracle_trigger.rs:1323-1329 establishes a relative_player_scope, but crates/engine/src/parser/oracle_effect/subject.rs:3177-3202 has no DefendingPlayer arm and falls through to TargetFilter::ParentTarget. The added test at oracle_trigger_tests.rs:25906-25918 only asserts that the result is not Any; it therefore accepts that incorrect fallback. The current parse-diff corroborates the defect: Smart Ass changes from any target to parent target, while its Oracle text says “If defending player has no cards … they may reveal their hand.” CR 506.2 identifies the defending player in combat ("the nonactive player is the defending player" in two-player combat). Suggested fix: carry DefendingPlayer through the trigger-relative context at the trigger authority and assert the exact TargetFilter::DefendingPlayer in a production parser test, with a sibling non-defending relative-player case to protect the scope routing.

Recommendation: request changes — preserve the named combat-relative player through lowering and replace the permissive non-Any assertion with an exact scope assertion.

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/src/parser/oracle_trigger.rs (1)

980-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse scan_at_word_boundaries instead of a manual word-boundary loop.

effect_body_introduces_defending_player reimplements the word-boundary walk that scan_at_word_boundaries already provides in this same file (see scan_for_phase, which calls super::oracle_nom::primitives::scan_at_word_boundaries(text, parse_phase_keyword)). Since the result here is only a boolean, scan_at_word_boundaries(effect_lower, parse_if_defending_player).is_some() gives the same behavior with one shared implementation instead of a second hand-rolled scan loop.

The integration at lines 1362-1369 is otherwise correct: it only assigns ControllerRef::DefendingPlayer when relative_player_scope_for_condition returned None, so it cannot override a scope the head condition already established.

♻️ Proposed refactor

 fn effect_body_introduces_defending_player(effect_lower: &str) -> bool {
-    let mut remaining = effect_lower;
-    while !remaining.is_empty() {
-        if parse_if_defending_player(remaining).is_ok() {
-            return true;
-        }
-        remaining = match remaining.find(' ') {
-            Some(i) => remaining[i + 1..].trim_start(),
-            None => "",
-        };
-    }
-    false
+    nom_primitives::scan_at_word_boundaries(effect_lower, parse_if_defending_player).is_some()
 }

As per CLAUDE.md's parser guidance to "reuse existing pronoun resolution, typed filters, controller scopes, and optionality representations" and to build on "existing parser building blocks."

Also applies to: 1362-1369

🤖 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_trigger.rs` around lines 980 - 1012, Replace
the manual word-boundary loop in effect_body_introduces_defending_player with
scan_at_word_boundaries(effect_lower, parse_if_defending_player).is_some(),
reusing the existing parser primitive while preserving the current boolean
behavior and the surrounding ControllerRef::DefendingPlayer integration.

Source: Path instructions

🤖 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_trigger.rs`:
- Around line 980-1012: Replace the manual word-boundary loop in
effect_body_introduces_defending_player with
scan_at_word_boundaries(effect_lower, parse_if_defending_player).is_some(),
reusing the existing parser primitive while preserving the current boolean
behavior and the surrounding ControllerRef::DefendingPlayer integration.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e1be567a-af9d-4778-90ed-0b5969051119

📥 Commits

Reviewing files that changed from the base of the PR and between 656677c and f8ebe7c.

📒 Files selected for processing (3)

• crates/engine/src/parser/oracle_effect/subject.rs
• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/parser/oracle_trigger_tests.rs

[matthewevans]

Blocked — the current parser scope fix needs a production Oracle regression for the newly changed Elder Brain route.

🔴 Blocker

[MED] Elder Brain’s attacker-to-defending-player chain is untested. Evidence: the current parse-diff changes Elder Brain’s Draw.target from parent target to defending player; crates/engine/src/parser/oracle_trigger.rs:1174-1184 stamps ControllerRef::DefendingPlayer, and crates/engine/src/parser/oracle_effect/subject.rs:3157-3169 resolves that new scope. The added parser coverage at crates/engine/src/parser/oracle_trigger_tests.rs:25891-26002 covers Smart Ass, a synthetic TargetPlayer sibling, and Siege Dragon, but not Elder Brain’s attacker → defending-player route. Why it matters: the card-level scope change can regress while all current tests stay green. Suggested fix: add a full Oracle parser regression for Elder Brain asserting the chained ExileHand then Draw ability targets TargetFilter::DefendingPlayer.

🟡 Non-blocking

[LOW] effect_body_introduces_defending_player duplicates the shared word-boundary scanner. Evidence: crates/engine/src/parser/oracle_trigger.rs:999-1010 reproduces the loop in crates/engine/src/parser/oracle_nom/primitives.rs:1008-1021; the current CodeRabbit scanner comment identifies the same live finding. Why it matters: duplicated parser traversal can drift. Suggested fix: use scan_at_word_boundaries(effect_lower, parse_if_defending_player).is_some() in a follow-up or alongside the blocker fix.

Recommendation: request changes — add the Elder Brain production-parser regression, then request a new review on the updated head.

[coderabbitai]

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_trigger_tests.rs`:
- Around line 26005-26019: Update the Elder Brain test comment describing the
defending-player mapping to include the verified annotation “CR 508.5” and state
that the defending player for an ability of an attacking creature is the player
that creature attacks. Keep the existing explanation of
condition_introduces_defending_player and resolve_they_pronoun unchanged.

🪄 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: f20a7db2-2301-490e-bcd6-b98d6388de0a

📥 Commits

Reviewing files that changed from the base of the PR and between f8ebe7c and 9858be9.

📒 Files selected for processing (2)

• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/parser/oracle_trigger_tests.rs

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

• crates/engine/src/parser/oracle_trigger.rs

[matthewevans]

Maintainer hold on current head ef56fff: the verified CR 508.5 Elder Brain commentary fix is pushed, the CodeRabbit thread is resolved, and the branch is current with main. Fresh CI remains in progress (Rust lint/tests and card-data); approval and merge-queue enrollment will follow only after those current-head checks pass.

[matthewevans]

Approved: current head ef56fff is rules-correct at the existing parser authorities; the refreshed parse-diff is the six reviewed and covered scope changes, all feedback threads are resolved, and current-head required checks are green.