fix(parser): scope each-player phase-trigger anaphors to the phase player (Citadel of Pain #6508)

[jeffrey701]

Summary

Fixes Citadel of Pain (#6508) and the each-player/each-opponent phase-trigger anaphor class: "At the beginning of each player's end step, this enchantment deals X damage to that player, where X is the number of untapped lands they control." On an opponent's end step Citadel counted the controller's untapped lands instead of the phase player's — so opponents visibly never took the right amount (frequently 0), while the controller took its own count on its own end step.

Root cause

Parse-time anaphor mis-binding. The each-player phase trigger correctly binds "that player" anaphors to the phase's active player (ControllerRef::ScopedPlayer) via relative_player_scope_for_condition — which is why the DealDamage recipient parsed correctly. But the where X is … they control count is stripped as a raw string and interpreted later, at assembly time, through the context-free parse_cda_quantity — so relative_player_scope was None and "they control" fell to the legacy unwrap_or(ControllerRef::You), binding the count to the source's controller. The sibling for-each interpreter in the same function already defaults this anaphor to ScopedPlayer; the where-X CDA arm simply never carried the context.

A second, live gap in the same trigger family: lower_trigger_ir had rewrite passes for TargetPlayer and SourceChosenPlayer scopes but no ScopedPlayer branch, so possessive quantities in these triggers (TargetZoneCardCount{Hand}, LifeTotal{Target}) stayed target-marker refs that resolve to 0 at runtime when the trigger has no player target — Iron Maiden always dealt 0, Rackling always dealt max, Havoc Festival lost 0 life, etc.

Fix (parser-only, +73/-2 across 3 files)

• Part A (oracle_effect/lower.rs, parse_where_x_quantity_expression): carry the ScopedPlayer anaphor context into the CDA-quantity delegate via the existing for_each_anaphor_context + parse_cda_quantity_with_context, exactly as the sibling for-each interpreter does. ScopedPlayer degrades to the source's controller at runtime when no scope is stamped, so spell where-X reads are unchanged; only each-player/each-opponent phase triggers now read the phase player.
• Part B (oracle_trigger.rs, lower_trigger_ir): add the missing Some(ControllerRef::ScopedPlayer) rewrite branch, reusing the identical rewrite_event_player_quantity_refs_to_scoped the TargetPlayer branch already calls — converting the target-marker possessives to HandSize{ScopedPlayer} / LifeTotal{ScopedPlayer}. It rewrites only PlayerScope::Target and TargetZoneCardCount, never Controller, so a mixed-anaphor card like Dark Suspicions keeps its -HandSize{Controller} operand intact.
• Supporting: for_each_anaphor_context widened to pub(crate).

Behavior-fixed cards: Citadel of Pain (#6508), Iron Maiden, Viseling, Rackling, Storm World, Wheel of Torture, Dark Suspicions, Dreamborn Muse, Price of Knowledge, Havoc Festival. (Noetic Scales shares the family but embeds its count in a filter's PtComparison, which the quantity visitor doesn't descend into — documented residual, separate follow-up.)

Files changed

• crates/engine/src/parser/oracle_effect/lower.rs — Part A (+ where-X unit test)
• crates/engine/src/parser/oracle_trigger.rs — Part B
• crates/engine/src/parser/oracle_quantity.rs — pub(crate) visibility
• crates/engine/src/parser/oracle_trigger_tests.rs — 4 parser SHAPE tests (Citadel, Iron Maiden, Dark Suspicions multi-authority, Havoc Festival, + you-control negative w/ reach-guard)
• crates/engine/tests/integration/citadel_of_pain_each_player_end_step_6508.rs — 4 runtime tests
• crates/engine/tests/integration/main.rs — mod line

CR references

• CR 513.1 (end step), CR 603.2b (phase trigger fires), CR 102.1 (active player = "that player"), CR 109.5 (why "you control" stays You), CR 608.2c (anaphor binding), CR 503.1a (Iron Maiden upkeep test), CR 110.5 (tapped/untapped status).

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 — 17596 passed, 0 failed, 6 ignored (baseline 17590 + 6 new parser tests)

• cargo test -p engine --test integration — 3873 passed, 0 failed, 2 ignored (baseline 3869 + 4 new)

• cargo fmt --all -- --check — clean

• ./scripts/gen-card-data.sh — regenerated; parse-diff audit of the family confirms every expected card flipped to the phase-player form and nothing else changed: Citadel amount ObjectCount{controller: ScopedPlayer}; Iron Maiden/Viseling Offset{HandSize{ScopedPlayer}, −4}; Rackling/Storm World/Wheel of Torture ClampMin{N − HandSize{ScopedPlayer}}; Dreamborn Muse/Price of Knowledge HandSize{ScopedPlayer}; Havoc Festival DivideRounded{LifeTotal{ScopedPlayer}, 2}; Dark Suspicions Sum[HandSize{ScopedPlayer}, −HandSize{Controller}] (controller operand intact); The Rack stays SourceChosenPlayer, Copper Tablet stays Fixed, Ancient Runes unchanged.

• RED/GREEN: T1 (P0=1/P1=3 untapped → asserts P1 −3; pre-fix −1), T2 (P1 lands tapped → 0; pre-fix −2), T4 (Iron Maiden hand 7−4 → −3; pre-fix TargetZoneCardCount resolves 0 → 0), plus all shape tests fail on revert. The you-control negative carries a positive reach-guard.

Gate A

Gate A PASS head=03c3aa6ac6c8ca090fbc05ecfefb047298db2600 base=6ae8737cdab0fa1ed291cad0f0808473a90f4cf8

Anchored on

• crates/engine/src/parser/oracle_quantity.rs:3337 — for_each_anaphor_context + parse_cda_quantity_with_context (oracle_quantity.rs:781): the sibling for-each interpreter that already defaults the third-person "they control" anaphor to ScopedPlayer; Part A adopts the same helper at the where-X CDA arm.
• crates/engine/src/parser/oracle_trigger.rs:1629 — the existing TargetPlayer/SourceChosenPlayer rewrite branches in lower_trigger_ir calling rewrite_event_player_quantity_refs_to_scoped / rewrite_player_quantity_refs_to_source_chosen; Part B adds the missing ScopedPlayer rung to that same ladder (oracle_trigger.rs:1648) reusing the same rewrite fn.

Final review-impl

Final review-impl PASS head=03c3aa6ac6c8ca090fbc05ecfefb047298db2600

Claimed parse impact

• Citadel of Pain, Iron Maiden, Viseling, Rackling, Storm World, Wheel of Torture, Dark Suspicions, Dreamborn Muse, Price of Knowledge, Havoc Festival (behavior-fixed); Curious Herd, Jovial Evil, Pact of the Serpent, Inscription of Abundance (label-only ScopedPlayer normalization, runtime-identical for spells).

Validation Failures

None.

CI Failures

None.

---

Tier: Frontier

Summary by CodeRabbit

• Bug Fixes

  • Fixed “they/their” references in per-player triggers and where-X expressions so they correctly refer to the player whose phase is being processed.
  • Preserved “you/your” references as referring to the ability’s controller.
  • Corrected effects involving controlled permanents, hand size, life totals, and damage calculations.

• Tests

  • Added coverage for per-player end-step and upkeep interactions, including Citadel of Pain and Iron Maiden scenarios.

[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: 279b69e2-cd45-43e4-8be6-8d23b92b7f01

📥 Commits

Reviewing files that changed from the base of the PR and between 21a53d5 and 03c3aa6.

📒 Files selected for processing (6)

• crates/engine/src/parser/oracle_effect/lower.rs
• crates/engine/src/parser/oracle_quantity.rs
• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/tests/integration/citadel_of_pain_each_player_end_step_6508.rs
• crates/engine/tests/integration/main.rs

---

📝 Walkthrough

Walkthrough

The parser now supplies scoped-player context for where-X quantities, and trigger lowering rewrites player references for each-player phases. Regression coverage validates controller, hand, life, and damage bindings through parser and integration tests.

Changes

Scoped player binding

Layer / File(s)	Summary
Where-X parser context crates/engine/src/parser/oracle_effect/lower.rs, crates/engine/src/parser/oracle_quantity.rs	Where-X CDA quantities use a ScopedPlayer anaphor context, with crate-visible helper access and direct parser coverage.
Phase-scoped trigger lowering crates/engine/src/parser/oracle_trigger.rs, crates/engine/src/parser/oracle_trigger_tests.rs	Each-player trigger lowering rewrites scoped player references and tests controller, hand, life, and negative you control bindings.
Gameplay regression scenarios crates/engine/tests/integration/citadel_of_pain_each_player_end_step_6508.rs, crates/engine/tests/integration/main.rs	Integration tests cover scoped-player damage and hand-count behavior for Citadel of Pain and Iron Maiden.

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 accurately summarizes the main parser fix and the Citadel of Pain regression it targets.
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.}

[matthewevans]

Parse changes introduced by this PR · 13 card(s), 12 signature(s) (baseline: main 21a53d50cbcb)

🟡 Modified fields (12 signatures)

• 2 cards · 🔄 ability/DealDamage · changed field amount: max(-1*target zone card count+3, 0) → max(-1*cards in hand (scoped player)+3, 0)
  • Affected (first 3): Rackling, Wheel of Torture
• 2 cards · 🔄 ability/DealDamage · changed field amount: target zone card count+-4 → cards in hand (scoped player)+-4
  • Affected (first 3): Iron Maiden, Viseling
• 1 card · 🔄 ability/DealDamage · changed field amount: # of untapped you control land → # of untapped scoped player controls land
  • Affected (first 3): Citadel of Pain
• 1 card · 🔄 ability/DealDamage · changed field amount: 2*# of white you control creature → 2*# of white scoped player controls creature
  • Affected (first 3): Jovial Evil
• 1 card · 🔄 ability/DealDamage · changed field amount: max(-1*target zone card count+4, 0) → max(-1*cards in hand (scoped player)+4, 0)
  • Affected (first 3): Storm World
• 1 card · 🔄 ability/DealDamage · changed field amount: target zone card count → cards in hand (scoped player)
  • Affected (first 3): Price of Knowledge
• 1 card · 🔄 ability/Draw · changed field count: # of chosen creature type you control creature → # of chosen creature type scoped player controls creature
  • Affected (first 3): Pact of the Serpent
• 1 card · 🔄 ability/LoseLife · changed field amount: # of chosen creature type you control creature → # of chosen creature type scoped player controls creature
  • Affected (first 3): Pact of the Serpent
• 1 card · 🔄 ability/LoseLife · changed field amount: (target zone card count + -1*cards in hand (you)) → (cards in hand (scoped player) + -1*cards in hand (you))
  • Affected (first 3): Dark Suspicions
• 1 card · 🔄 ability/LoseLife · changed field amount: divide(life total (target player), 2, rounded up) → divide(life total (scoped player), 2, rounded up)
  • Affected (first 3): Havoc Festival
• 1 card · 🔄 ability/Mill · changed field count: target zone card count → cards in hand (scoped player)
  • Affected (first 3): Dreamborn Muse
• 1 card · 🔄 ability/Token · changed field token: # of you control artifact× +3/+3 Green Beast (Creature Beast) → # of scoped player controls artifact× +3/+3 Green Beast (Creature Beast)
  • Affected (first 3): Curious Herd

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

[matthewevans]

Part B is clean and at the right seam — Part A is not. parse_where_x_quantity_expression hard-codes ScopedPlayer instead of threading the caller's ParseContext, and the new parse-diff proves the cost: three cards labelled "runtime-identical" are actually target-player anaphors moved from one wrong referent to another.

Reviewed at head 03c3aa6a (unchanged since the last look). Thank you for the parse-diff — it's exactly the evidence that made this reviewable, and it's what turned a predicted harm into a measured one.

📊 Parse-diff verdict: claimed 14, measured 13

Sticky comment 5063481753 (baseline 21a53d50cbcb): 13 cards / 12 signatures, all "Modified fields", none gained, none lost.

All 10 claimed behavior-fixed cards check out — each Oracle text verified against Scryfall: Citadel of Pain, Iron Maiden, Viseling, Rackling, Wheel of Torture, Storm World, Dark Suspicions, Dreamborn Muse, Price of Knowledge, Havoc Festival. Dark Suspicions correctly retains its -1*cards in hand (you) controller operand.

Unexplained #1 — Inscription of Abundance is claimed but absent from the measurement. Its clause is an aggregate ("where X is the greatest power among creatures they control"), not an ObjectCount, so it plausibly never reaches the touched arm. The sticky's trailing "2 card(s) had Oracle-text changes (errata/reprint) — excluded" bucket is unnamed, so this can't be reconciled from the sticky alone. Please account for it.

Unexplained #2 — the 3 "label-only" cards are target-player anaphors (Scryfall-verified):

Card	Oracle	Change
Jovial Evil	"deals X damage to target opponent, where X is twice the number of white creatures that player controls"	2*# of white you control creature → 2*# of white scoped player controls creature
Curious Herd	"Choose target opponent. You create X … where X is the number of artifacts that player controls"	# of you control artifact → # of scoped player controls artifact
Pact of the Serpent	"Target player draws X cards and loses X life, where X is the number of creatures they control…"	same shift, 2 signatures (Draw + LoseLife)

The correct referent for all three is ControllerRef::TargetPlayer. Calling this "runtime-identical for spells" understates it — it swaps one wrong binding for a different wrong binding.

🔴 Blockers

[HIGH] parse_where_x_quantity_expression hard-codes ScopedPlayer rather than threading ParseContext.

crates/engine/src/parser/oracle_effect/lower.rs:8145-8161 injects for_each_anaphor_context(&ParseContext::default(), &ControllerRef::ScopedPlayer) unconditionally, across ~30 non-test call sites (mana.rs:1071; token.rs:118/683/727; imperative.rs:8429/9958; counter.rs:239; 32 refs in lower.rs).

The sibling interpreter already does this correctly: crates/engine/src/parser/oracle_quantity.rs:3337 parse_for_each_clause_with_context takes &ParseContext and threads whichever scope the caller holds. And the pre-existing test for_each_they_control_threads_target_player (oracle_quantity.rs:6258, Burden of Greed) proves TargetPlayer binding already works today. The machinery to bind those three cards correctly exists; this change routes around it.

Suggested fix: add parse_where_x_quantity_expression_with_context(expr, ctx), thread effect_ctx from the where-X binding path, and leave the context-free signature delegating with ParseContext::default().

[MED] The "runtime-identical" justification cites the wrong resolver.

lower.rs:8149-8151 cites scoped_player_or_controller. Two functions share that name. The one the comment describes — crates/engine/src/game/quantity.rs:3842 — is ability.scoped_player.unwrap_or(controller), a clean degrade. But this change's actual output for Citadel of Pain is ObjectCount { filter: Typed { controller: Some(ScopedPlayer) } }, whose runtime authority is crates/engine/src/game/filter.rs:1154 → filter.rs:780, and that path inserts an extra rung .or_else(|| triggering_event_player(state)) before source_controller, reading state.current_trigger_event / the DETECTION_TRIGGER_EVENT TLS (quantity.rs:1491).

Confidence: high that the comment is wrong about its own code path; medium that the divergence is reachable (current_trigger_event is save/restore-scoped at game/engine.rs:7915/7921 and game/triggers.rs:152/168, but game/combat.rs:13571 assigns AttackersDeclared with no paired restore in that block). Either fix the citation and prove the spell claim, or drop the claim.

[MED] Fixture path-divergence. All 4 runtime tests (tests/integration/citadel_of_pain_each_player_end_step_6508.rs) and all 4 shape tests (oracle_trigger_tests.rs) are phase triggers, so ability.scoped_player is always Some. The None arm at filter.rs:780 — the arm all three spell cards take in production — has zero fixtures.

[MED] Claimed parse impact (14) ≠ measured (13). Reconcile Inscription of Abundance.

✅ Clean — verified

• Part B is at the right seam and needs no changes. parser/oracle_trigger.rs:1639-1652 adds the missing ScopedPlayer rung to the existing TargetPlayer/SourceChosenPlayer ladder in lower_trigger_ir and reuses the pre-existing rewrite_event_player_quantity_refs_to_scoped (oracle_effect/mod.rs:24608). Verified at mod.rs:24619-24639 that it rewrites only PlayerScope::Target and TargetZoneCardCount, never Controller — so Dark Suspicions survives, which the parse diff confirms. The Rack mutual-exclusion argument holds.
• The class is real — 10 Scryfall-verified cards, well above the 3-card bar.
• Nom mandate satisfied — no find()/split_once()/contains()/starts_with() parsing dispatch, no verbatim Oracle string match anywhere in the diff.
• All 9 CR numbers grep-verified in docs/MagicCompRules.txt, and each describes the code it annotates: 109.5, 608.2c, 603.2b, 102.1, 513.1, 503.1a, 110.5, 107.3i, 115.1.
• Runtime tests genuinely discriminate. T1 is asymmetric (P0=1/P1=3; revert flips −3→−1), T2 (P1 all tapped; revert flips 0→−2), T4 (Part B, 7−4=3 vs pre-fix 0). They drive GameScenario/GameRunner → advance_to_end_step → advance_until_stack_empty and assert life deltas rather than AST shape. IRON_MAIDEN_ORACLE matches Scryfall verbatim.

🟡 Non-blocking

• The PR body checks - [x] Required checks ran clean. while the required aggregator is red. The root cause is maintainer-side, so this isn't misreporting — but please keep the template honest.
• The triage packet reports anchors: [] despite a well-formed "## Anchored on" section with two valid anchors; worth a look at why it isn't being picked up.

Recommendation

request-changes, contributor round-trip — not a maintainer fixup. Threading ParseContext through ~30 call sites is a design change, not a small local correction. Please:

1. Thread the caller's ParseContext through parse_where_x_quantity_expression (this is the same fix flagged previously, now backed by three named misbound cards).
2. Fix the filter.rs:780 citation, and either prove or drop the "runtime-identical for spells" claim.
3. Add a fixture that reaches the None arm.
4. Reconcile claimed 14 vs measured 13.

Part B is clean — keep it as is.

Your CI red is not your fault and cannot be fixed by rebasing, because main's tip is the broken commit: release: v0.35.2 bumped client/src-tauri/Cargo.toml without updating the separate client/src-tauri/Cargo.lock, redding Tauri compile check and through it the required Rust aggregator on every open PR. #6568 fixes it on main.