fix(parser): scope each-player phase-trigger anaphors to the phase player (Citadel of Pain #6508) - #6564
Conversation
…ayer (Citadel of Pain phase-rs#6508) "At the beginning of each player's end step, ~ deals X damage to that player, where X is the number of untapped lands they control." counted the SOURCE's controller's untapped lands instead of the phase player's, so on an opponent's end step Citadel dealt the controller's count (often 0) rather than the opponent's. Root cause is a parse-time anaphor mis-binding. The trigger already 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 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). 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. Part A: carry the ScopedPlayer anaphor context into the CDA-quantity delegate via the existing for_each_anaphor_context + parse_cda_quantity_with_context. 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 read the phase player. Part B: lower_trigger_ir had TargetPlayer and SourceChosenPlayer rewrite passes but no ScopedPlayer branch, so possessive quantities (TargetZoneCardCount{Hand}, LifeTotal{Target}) in these triggers stayed target-marker refs that resolve to 0 at runtime with no player target — Iron Maiden always dealt 0, Rackling always dealt max, Havoc Festival lost 0 life. Add the missing ScopedPlayer branch, reusing the identical rewrite_event_player_quantity_refs_to_scoped the TargetPlayer branch already calls (rewrites only Target/TargetZoneCardCount, never Controller, so mixed-anaphor Dark Suspicions keeps its -HandSize{Controller} operand). Behavior-fixed: Citadel of Pain, Iron Maiden, Viseling, Rackling, Storm World, Wheel of Torture, Dark Suspicions, Dreamborn Muse, Price of Knowledge, Havoc Festival. Parser-only; no runtime files change. Closes phase-rs#6508 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe parser now captures player scope alongside ChangesScoped player binding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant ParseContext
participant ClauseBuilder
participant WhereXLowering
participant TriggerLowering
OracleText->>ParseContext: parse each-player and where-X text
ParseContext->>ClauseBuilder: derive and store where_x_scope
ClauseBuilder->>WhereXLowering: provide where-X expression and scope
WhereXLowering->>TriggerLowering: resolve scoped quantities and filters
TriggerLowering-->>OracleText: produce phase-scoped effects
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
needs-maintainer
Parse changes introduced by this PR · 10 card(s), 8 signature(s) (baseline: main
|
|
Part B is clean and at the right seam — Part A is not. Reviewed at head 📊 Parse-diff verdict: claimed 14, measured 13Sticky comment 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 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 Unexplained #2 — the 3 "label-only" cards are target-player anaphors (Scryfall-verified):
The correct referent for all three is 🔴 Blockers[HIGH]
The sibling interpreter already does this correctly: Suggested fix: add [MED] The "runtime-identical" justification cites the wrong resolver.
Confidence: high that the comment is wrong about its own code path; medium that the divergence is reachable ( [MED] Fixture path-divergence. All 4 runtime tests ( [MED] Claimed parse impact (14) ≠ measured (13). Reconcile Inscription of Abundance. ✅ Clean — verified
🟡 Non-blocking
Recommendationrequest-changes, contributor round-trip — not a maintainer fixup. Threading
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: |
…mp (#6568) `release: v0.35.2` (21a53d5) bumped `client/src-tauri/Cargo.toml` to 0.35.2 via cargo-release's `pre-release-replacements`, but `client/src-tauri/` is a separate cargo workspace with its own lockfile that cargo-release does not manage. The lock still recorded `phase-tauri 0.35.1`, so the `tauri-check` CI job's `cargo check --locked --manifest-path client/src-tauri/Cargo.toml` refused to reconcile the mismatch and exited 101. That reds the required `Rust (fmt, clippy, test, coverage-gate)` aggregator (which `needs: [... tauri-check]`) on every open PR whose merge ref includes the release commit, blocking the merge queue repo-wide. Evidence: PR #6561 tauri-check PASSED at 20:43:00Z; the release commit landed at 20:49:54Z; PRs #6564 (20:56:21Z) and #6563 (21:15:11Z) both FAILED with the identical --locked error. None of the three touched any Cargo manifest. Follow-up (not in this change): cargo-release should keep the nested lockfile in sync so the next release does not re-break it. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…mp (phase-rs#6568) `release: v0.35.2` (21a53d5) bumped `client/src-tauri/Cargo.toml` to 0.35.2 via cargo-release's `pre-release-replacements`, but `client/src-tauri/` is a separate cargo workspace with its own lockfile that cargo-release does not manage. The lock still recorded `phase-tauri 0.35.1`, so the `tauri-check` CI job's `cargo check --locked --manifest-path client/src-tauri/Cargo.toml` refused to reconcile the mismatch and exited 101. That reds the required `Rust (fmt, clippy, test, coverage-gate)` aggregator (which `needs: [... tauri-check]`) on every open PR whose merge ref includes the release commit, blocking the merge queue repo-wide. Evidence: PR phase-rs#6561 tauri-check PASSED at 20:43:00Z; the release commit landed at 20:49:54Z; PRs phase-rs#6564 (20:56:21Z) and phase-rs#6563 (21:15:11Z) both FAILED with the identical --locked error. None of the three touched any Cargo manifest. Follow-up (not in this change): cargo-release should keep the nested lockfile in sync so the next release does not re-break it. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
matthewevans
removed
the
needs-maintainer
github-actions
Bot
added
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head cb03323 remains blocked.
parse_where_x_quantity_expression still creates a default ParseContext and unconditionally installs ScopedPlayer instead of receiving and threading the caller's context. Consequently target-player anaphors such as Revelation of Power's sibling class are rebound to ScopedPlayer rather than TargetPlayer.
Add the context-aware entry point, thread the existing effect context through its callers, retain the default wrapper only for genuinely context-free callers, and cover a target-player where-X case before requesting review again.
…nstead of hardcoding ScopedPlayer (phase-rs#6564 review) The phase-rs#6508 Part A install of a where-X anaphor scope hardcoded ScopedPlayer in parse_where_x_quantity_expression, so a targeted spell's "that player controls" / "they control" where-X was rebound to ScopedPlayer instead of the caller's actual scope. Make the interpreter context-aware: - parse_where_x_quantity_expression_with_context(text, ctx) is the real entry point; the CDA arm binds the anaphor to ctx.relative_player_scope when a scope is stamped, and takes the exact pre-phase-rs#6508 parse_cda_quantity path (legacy caster-relative You) when it is not — never a hardcoded ScopedPlayer. parse_where_x_quantity_expression(text) is a thin default wrapper for the genuinely context-free boundary/shape probes. - The where-X string is stripped to the IR and interpreted at the context-free assembly walk, so the parse-time scope is captured into a new ClauseIr.where_x_scope, rebuilt into a ParseContext at the top of apply_where_x_ability_expression, and threaded through the apply_/bind_ where_x_* chain (including the recursive "N plus/minus <where-x>" self-call). Inline ctx-bearing callers (token.rs, imperative.rs) call the _with_context entry point directly. Each-player / each-opponent phase triggers (Citadel of Pain, Iron Maiden, Rackling, Dark Suspicions, Havoc Festival, Price of Knowledge) thread their ScopedPlayer scope through and keep the correct binding. Targeted spells are no longer rebound to ScopedPlayer; with no stamped scope they retain the legacy You binding, byte-identical to main. A spell's "that player" is frequently cross-clause (Curious Herd) or shared across sibling sub-effects (Pact of the Serpent's "draws X and loses X"), so auto-deriving TargetPlayer from a single clause's own target diverged the two X operands (CR 107.3i single-value-of-X). That target carry-forward is a separate change; the context-aware entry point already binds TargetPlayer when a caller supplies that scope (covered by where_x_that_player_controls_binds_caller_scope_not_scoped_player). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Done at head What changed
Result:
Why I did not auto-derive
|
![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 14125-14132: Remove the stale truncated first documentation line
above the helper, leaving the subsequent CR 109.5 + CR 608.2c paragraph as the
sole documentation for the relative player scope behavior.
🪄 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: 6d7a70f7-6abf-4326-ba9a-7adf4dd05e88
📒 Files selected for processing (7)
crates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/token.rscrates/engine/src/parser/oracle_ir/effect_chain.rs
| /// CR 109.5 + CR 608.2c: True when this clause's own effect targets a PLAYER, so a | ||
| /// CR 109.5 + CR 608.2c: Capture the player scope that a trailing where-X anaphor | ||
| /// ("they control" / "that player controls") must bind to for this clause. The | ||
| /// scope is exactly the `relative_player_scope` a trigger / for-each / fanout | ||
| /// setter stamped onto the parse context — `ScopedPlayer` for Citadel of Pain's | ||
| /// each-player phase count, `TargetPlayer` for a per-opponent fanout iterand, etc. | ||
| /// `None` leaves the legacy caster-relative (`You`) binding untouched, exactly as | ||
| /// on `main`; the context-aware entry point rebinds only when a scope is present. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale first doc line contradicts the helper's behavior.
Line 14125 is a truncated leftover from the pre-2a97ab924 predicate version ("True when this clause's own effect targets a PLAYER, so a") and is followed immediately by a second CR 109.5 + CR 608.2c: sentence that is the actual doc. Since the paragraph below explicitly documents that target-derivation was removed, the dangling line is misleading.
📝 Proposed doc fix
-/// CR 109.5 + CR 608.2c: True when this clause's own effect targets a PLAYER, so a
/// CR 109.5 + CR 608.2c: Capture the player scope that a trailing where-X anaphor
/// ("they control" / "that player controls") must bind to for this clause. The📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// CR 109.5 + CR 608.2c: True when this clause's own effect targets a PLAYER, so a | |
| /// CR 109.5 + CR 608.2c: Capture the player scope that a trailing where-X anaphor | |
| /// ("they control" / "that player controls") must bind to for this clause. The | |
| /// scope is exactly the `relative_player_scope` a trigger / for-each / fanout | |
| /// setter stamped onto the parse context — `ScopedPlayer` for Citadel of Pain's | |
| /// each-player phase count, `TargetPlayer` for a per-opponent fanout iterand, etc. | |
| /// `None` leaves the legacy caster-relative (`You`) binding untouched, exactly as | |
| /// on `main`; the context-aware entry point rebinds only when a scope is present. | |
| /// CR 109.5 + CR 608.2c: Capture the player scope that a trailing where-X anaphor | |
| /// ("they control" / "that player controls") must bind to for this clause. The | |
| /// scope is exactly the `relative_player_scope` a trigger / for-each / fanout | |
| /// setter stamped onto the parse context — `ScopedPlayer` for Citadel of Pain's | |
| /// each-player phase count, `TargetPlayer` for a per-opponent fanout iterand, etc. | |
| /// `None` leaves the legacy caster-relative (`You`) binding untouched, exactly as | |
| /// on `main`; the context-aware entry point rebinds only when a scope is present. |
🤖 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 14125 - 14132,
Remove the stale truncated first documentation line above the helper, leaving
the subsequent CR 109.5 + CR 608.2c paragraph as the sole documentation for the
relative player scope behavior.
Co-authored-by: Jeffrey <158072326+jeffrey701@users.noreply.github.com>
Remove the truncated duplicate doc line above the complete player-scope documentation. Co-authored-by: Jeffrey <158072326+jeffrey701@users.noreply.github.com>
|
Maintainer update for current head |
|
Maintainer CI fixup is pushed at current head |
2 participants
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) viarelative_player_scope_for_condition— which is why the DealDamage recipient parsed correctly. But thewhere X is … they controlcount is stripped as a raw string and interpreted later, at assembly time, through the context-freeparse_cda_quantity— sorelative_player_scopewasNoneand "they control" fell to the legacyunwrap_or(ControllerRef::You), binding the count to the source's controller. The siblingfor-eachinterpreter in the same function already defaults this anaphor toScopedPlayer; the where-X CDA arm simply never carried the context.A second, live gap in the same trigger family:
lower_trigger_irhad rewrite passes forTargetPlayerandSourceChosenPlayerscopes but noScopedPlayerbranch, 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)
oracle_effect/lower.rs,parse_where_x_quantity_expression): carry theScopedPlayeranaphor context into the CDA-quantity delegate via the existingfor_each_anaphor_context+parse_cda_quantity_with_context, exactly as the sibling for-each interpreter does.ScopedPlayerdegrades 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.oracle_trigger.rs,lower_trigger_ir): add the missingSome(ControllerRef::ScopedPlayer)rewrite branch, reusing the identicalrewrite_event_player_quantity_refs_to_scopedtheTargetPlayerbranch already calls — converting the target-marker possessives toHandSize{ScopedPlayer}/LifeTotal{ScopedPlayer}. It rewrites onlyPlayerScope::TargetandTargetZoneCardCount, neverController, so a mixed-anaphor card like Dark Suspicions keeps its-HandSize{Controller}operand intact.for_each_anaphor_contextwidened topub(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 Bcrates/engine/src/parser/oracle_quantity.rs—pub(crate)visibilitycrates/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 testscrates/engine/tests/integration/main.rs— mod lineCR 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 amountObjectCount{controller: ScopedPlayer}; Iron Maiden/ViselingOffset{HandSize{ScopedPlayer}, −4}; Rackling/Storm World/Wheel of TortureClampMin{N − HandSize{ScopedPlayer}}; Dreamborn Muse/Price of KnowledgeHandSize{ScopedPlayer}; Havoc FestivalDivideRounded{LifeTotal{ScopedPlayer}, 2}; Dark SuspicionsSum[HandSize{ScopedPlayer}, −HandSize{Controller}](controller operand intact); The Rack staysSourceChosenPlayer, Copper Tablet staysFixed, 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
TargetZoneCardCountresolves 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 toScopedPlayer; Part A adopts the same helper at the where-X CDA arm.crates/engine/src/parser/oracle_trigger.rs:1629— the existingTargetPlayer/SourceChosenPlayerrewrite branches inlower_trigger_ircallingrewrite_event_player_quantity_refs_to_scoped/rewrite_player_quantity_refs_to_source_chosen; Part B adds the missingScopedPlayerrung 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
Validation Failures
None.
CI Failures
None.
Tier: Frontier
Summary by CodeRabbit
Bug Fixes
where-Xplayer-scope binding in per-player triggers so phrases like “they/their/that player” resolve to the scoped (phase) player instead of an incorrect default.Tests
#6508, including Citadel of Pain each-player end step and Iron Maiden upkeep damage scenarios.