fix(parser): scope "that player" to triggering player for Aura/Equipment damage triggers

[hitomi1]

Fixes #4240.

Problem

Sigil of Sleep froze the game (p0 softlock — the "set resolve order" prompt could not be actioned). Its trigger — "Whenever enchanted creature deals damage to a player, return target creature that player controls to its owner's hand" — surfaced two target slots (a Player and a creature) instead of one. The phantom Player slot has no meaningful prompt, so the client hung waiting on it.

Root cause

parse_damage_source_subject (in parser/oracle_trigger.rs) required a leading article, so the article-less Aura/Equipment subjects "enchanted creature" / "equipped creature" failed the strict is_damage_done_trigger_pattern check and fell through to condition_introduces_target_player. That mis-bound the effect's "that player controls" anaphor to ControllerRef::TargetPlayer — whose documented behavior is to auto-surface a companion Player target slot — instead of ControllerRef::TriggeringPlayer (the damaged player identified by the triggering event).

Fix

Recognize the two article-less determiner subjects at the top of parse_damage_source_subject, resolving them to TargetFilter::AttachedTo (which is also the correct valid_source for the source-led trigger path — not Typed(Enchanted/EquippedBy), which would match any attached creature). The DamageDone pattern now matches, so relative_player_scope_for_condition returns TriggeringPlayer and the anaphor binds correctly — a single creature target scoped to the damaged player.

Scope (built for the class, not the card)

Fixes Sigil of Sleep, Hammer of Ruin, Fishing Gear, Whispersteel Dagger. Komainu Battle Armor's compound "this creature or equipped creature" subject is a disjunctive variant left for a follow-up (unchanged here — no regression).

Tests

• Building-block test on relative_player_scope_for_condition asserting both determiner subjects → TriggeringPlayer (covers the class).
• Sigil of Sleep bounce-target test asserting TriggeringPlayer-scoped creature target (was red before the fix).
• Full engine suite green (17530 lib + 3753 integration), clippy -D warnings clean, cargo fmt applied.

CR 303.4b + CR 301.5a + CR 120.3a.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved oracle-trigger parsing for article-less phrases like “enchanted creature” and “equipped creature.”
  • Fixed “that player” anaphor resolution to consistently refer to the player dealt the damage, including for “damage done” triggers.
  • Corrected Sigil of Sleep’s targeting so it affects only eligible creatures controlled by the damaged player (excluding creatures controlled by the Aura’s controller).
• Tests
  • Added unit and integration regression coverage for these trigger parsing and targeting behaviors, including Hammer of Ruin, Quietus Spike/Scytheclaw, and Sword of War and Peace.

[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

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: ecba0db4-3670-4303-9df3-4b7296e69cd2

📥 Commits

Reviewing files that changed from the base of the PR and between 30178ef and bcfac8f.

📒 Files selected for processing (1)

• crates/engine/tests/integration/issue_4240_sigil_of_sleep_target_scope.rs

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

• crates/engine/tests/integration/issue_4240_sigil_of_sleep_target_scope.rs

---

📝 Walkthrough

Walkthrough

The parser now recognizes article-less “enchanted creature” and “equipped creature” damage subjects. Unit and integration tests verify that “that player” resolves to the damaged player across target selection, equipment destruction, life changes, and hand-size calculations.

Changes

Oracle trigger parsing

Layer / File(s)	Summary
Attached-creature damage subject support crates/engine/src/parser/oracle_trigger.rs, crates/engine/src/parser/oracle_trigger_tests.rs	Article-less “enchanted creature” and “equipped creature” subjects parse as attached-creature damage sources. Tests verify triggering-player scope and Sigil of Sleep’s creature target filter.
Damaged-player anaphor runtime coverage crates/engine/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs, crates/engine/tests/integration/main.rs	Runtime tests verify damaged-player references for equipment destruction, life changes, and hand-size calculations.
Sigil of Sleep target-scope regression crates/engine/tests/integration/issue_4240_sigil_of_sleep_target_scope.rs, crates/engine/tests/integration/main.rs	An integration test verifies one legal creature target controlled by the damaged player and confirms it returns to its owner’s hand after resolution.

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 describes the main parser scoping fix for Aura/Equipment damage triggers.
Linked Issues check	✅ Passed	The parser change and Sigil of Sleep regression test address the trigger-stack issue in #4240.
Out of Scope Changes check	✅ Passed	The extra card and runtime tests support the same scoping fix and do not introduce unrelated behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[github-actions]

Parse changes introduced by this PR · 6 card(s), 4 signature(s) (baseline: main 6895ca39a2b1)

🟡 Modified fields (4 signatures)

• 2 cards · 🔄 ability/DealDamage · changed field amount: cards in hand (scoped player) → target zone card count
  • Affected (first 3): Sword of Fire and Ice and War and Peace, Sword of War and Peace
• 2 cards · 🔄 ability/LoseLife · changed field amount: divide(life total (scoped player), 2, rounded up) → divide(life total (target player), 2, rounded up)
  • Affected (first 3): Quietus Spike, Scytheclaw
• 1 card · 🔄 ability/Bounce · changed field target: target player controls creature → triggering player controls creature
  • Affected (first 3): Sigil of Sleep
• 1 card · 🔄 ability/Destroy · changed field target: target player controls Equipment → triggering player controls Equipment
  • Affected (first 3): Hammer of Ruin

[matthewevans]

Requested change

P1 — Explain or constrain the parser blast radius

The fresh coverage-parse-diff shows parser-output changes for four cards outside this PR’s claimed Sigil of Sleep / Hammer of Ruin scope: Sword of Fire and Ice, Sword of War and Peace, Quietus Spike, and Scytheclaw.

Please either narrow the grammar so these changes do not occur, or document why each change is intended and add card-level runtime coverage for the affected behavior. Re-run the parse diff on the updated head.

This is blocking because parse_damage_source_subject is shared damage-trigger grammar, and the current PR does not provide evidence that those additional behavior changes are correct.

[hitomi1]

Thanks — that was the right call to block on. I reproduced the parse diff locally and it matches CI exactly (6 cards, 4 signatures), then wrote runtime coverage that separates the changes into two classes. Short version: 2 of the 6 are behavioral (the fix), and the other 4 are representation-only with provably identical runtime behavior.

Evidence: run the new tests against the fix, then against the fix reverted

New file: crates/engine/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs

Card(s)	Parse-diff signature	Fix reverted	Class
Hammer of Ruin	Destroy.target target→triggering	❌ FAILS — target_slots.len() is 2, not 1	Behavioral — the phantom companion player slot (the #4240 softlock)
Quietus Spike, Scytheclaw	LoseLife.amount scoped→target	✅ passes	Representation-only
Sword of War and Peace (and Sword of Fire and Ice and War and Peace, same printed clause)	DealDamage.amount scoped→target	✅ passes	Representation-only

Why the 4 are inert at runtime: during trigger resolution QuantityContext::scoped_player is already bound to the damaged player (resolve_quantity_for_trigger_check, CR 603.4/102.1), so the pre-fix PlayerScope::ScopedPlayer and the post-fix PlayerScope::Target resolve to the same player. resolve_per_player_scalar reads ScopedPlayer → ctx.scoped_player.unwrap_or(controller) and Target → first player target; in these triggers both land on the damaged player. The parse-diff records the representation change; behavior is unchanged.

I kept those three tests deliberately even though they pass either way, and labelled them invariance tests in the module docs — they are the standing evidence that this shared-grammar edit did not alter what those cards do. Each is discriminating by construction (controller and damaged player are given different life totals / hand sizes, so a controller-bound reading would assert-fail):

• Quietus Spike / Scytheclaw: damaged player 30 life, controller 40 → 30 − 2 combat = 28, halved up = 14 left; a controller-bound reading would halve 40 and leave 8.
• Sword of War and Peace: damaged player's hand 5, controller's hand 2 → 20 − 1 combat − 5 = 14; a controller-bound reading would deal 2 and leave 17. The same test also pins that the life-gain half still counts your hand (+2), i.e. the controller-side reference stayed put.

On narrowing

I looked at narrowing the grammar so the 4 don't move. It isn't reachable without giving up the fix: the same is_damage_done_trigger_pattern recognition that binds "that player controls" to TriggeringPlayer for Sigil/Hammer is what re-renders the quantity-position player-ref for the other 4. Since those 4 are provably behavior-neutral, narrowing would add complexity to suppress a diff entry that has no runtime meaning. Happy to go the other way if you'd still prefer the smaller diff.

Also verified the card Oracle text against Scryfall for all six (the "their life" / "their hand" readings above), per the verify-the-card rule.

Parse diff has been re-run on the updated head — unchanged at 6 cards / 4 signatures.

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs (1)

96-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Wildcard fallback in the WaitingFor match.

other => panic!(...) at Line 104 sidesteps exhaustive matching on WaitingFor, so a newly added variant wouldn't be caught at compile time. It fails loudly at runtime instead, which is acceptable for a test helper, but an exhaustive match with an explicit panic arm per remaining variant would align with the crate's exhaustive-match convention.

As per coding guidelines, "Use exhaustive matches without wildcard fallbacks for known enums so the compiler detects missing variants."

🤖 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/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs`
around lines 96 - 105, Update the WaitingFor match in the trigger-resolution
helper to remove the wildcard other arm and enumerate every remaining WaitingFor
variant with explicit panic arms. Preserve the existing Priority handling and
panic behavior, allowing future enum variants to produce compile-time
exhaustiveness errors.

🤖 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/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs`:
- Around line 96-105: Update the WaitingFor match in the trigger-resolution
helper to remove the wildcard other arm and enumerate every remaining WaitingFor
variant with explicit panic arms. Preserve the existing Priority handling and
panic behavior, allowing future enum variants to produce compile-time
exhaustiveness errors.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 150d7fff-f8ac-44d6-903f-34703a3340e5

📥 Commits

Reviewing files that changed from the base of the PR and between 6aed42b and 30178ef.

📒 Files selected for processing (2)

• crates/engine/tests/integration/issue_4240_damaged_player_anaphor_runtime.rs
• crates/engine/tests/integration/main.rs

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

• crates/engine/tests/integration/main.rs

[hitomi1]

Pushed bcfac8f3e to get CI compiling again. It touches your test from 17a10185, so here's exactly what changed and why — happy to revert any of it if you'd rather take it a different way.

The test couldn't run as written: it failed to compile, and once compiling it didn't reach the window it asserts on. Three minimal changes, all preserving the original intent.

1. The CI error (E0308). The Priority arm evaluated to the ActionResult that .expect(...) unwraps to, but the match is in statement position so it has to be (). The sibling break / panic! arms are ! and coerce to anything, so they never reconciled the type — which is also why the compiler pointed at the panic! arm rather than the arm actually at fault. Wrapped it in a block with a trailing semicolon to discard the value:

WaitingFor::Priority { .. } => {
    runner
        .act(GameAction::PassPriority)
        .expect("passing priority must advance Sigil's trigger");
}

2. It never reached target selection. With only one legal target the engine auto-selects and never opens WaitingFor::TriggerTargetSelection, so the loop passed priority until the damaged player decked out and the test hit GameOver { winner: P0 }. Gave the damaged player a second creature so a genuine choice exists and the window actually opens.

3. Widened the legal-target assertion to both of the damaged player's creatures — the hardcoded single-target expectation was the last failure once the window opened.

The load-bearing assertions are untouched and still pass: exactly one target slot (no phantom companion player slot — the #4240 softlock), and the Aura controller's own creature still excluded from "that player controls". Point 2 only changes the fixture so the assertion is reachable; it doesn't weaken what's being proven.

All five issue_4240 tests now pass locally on the branch (your Sigil test plus the four in issue_4240_damaged_player_anaphor_runtime.rs).

One process note on my side: my earlier "full suite green" claim was run in a checkout that didn't contain your test file, so I never compiled it — CI caught what I should have. I've since been building the branch itself, which is how both defects above surfaced.

[matthewevans]

Maintainer approval: current head bcfac8f is clean at the parser seam; its runtime regression coverage and required CI are green.