fix(engine): scope "target opponent whose turn it is" to the active opponent (#6496) - #6689
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR changes ChangesActive-player relation targeting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant AbilityTargeting
participant GameState
participant PlayerRelation
OracleParser->>AbilityTargeting: emit ActivePlayer { relation }
AbilityTargeting->>GameState: read active_player
AbilityTargeting->>PlayerRelation: validate relation against source controller
PlayerRelation-->>AbilityTargeting: legal target result
AbilityTargeting-->>GameState: apply target selection or revalidate on resolution
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the ActivePlayer parameterization changes a persisted enum wire shape without legacy decoding.
🔴 Blocker
[HIGH] Existing bare "ActivePlayer" JSON no longer deserializes. ControllerRef::ActivePlayer is now a struct variant with relation in crates/engine/src/types/ability.rs:3663-3687; its prior unit-variant wire representation was the bare enum tag. Although the intended All semantics are unchanged, persisted card data using the legacy form breaks at the serialization boundary.
Add a compatibility deserializer mapping legacy ActivePlayer to ActivePlayer { relation: PlayerRelation::All }, with a regression that loads the old wire shape.
🟡 Evidence requested
This changes oracle_target and parser-effect code, but the required coverage parse-diff sticky artifact is absent. Please provide a fresh artifact or reconcile the parser-impact evidence before approval.
![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_nom/target.rs`:
- Line 822: Update the terminator guard in the relative-clause parser around
terminated(tag("turn it is"), ...) to accept EOF, whitespace, and explicit
terminal separators comma, semicolon, and period. Add regression cases covering
terminal target phrases ending with each supported separator, including “target
opponent whose turn it is.”
🪄 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: 4038515a-f988-45c4-9925-e7bf938b316a
📒 Files selected for processing (27)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/copy_spell.rscrates/engine/src/game/effects/sacrifice.rscrates/engine/src/game/filter.rscrates/engine/src/game/players.rscrates/engine/src/game/quantity.rscrates/engine/src/game/replacement.rscrates/engine/src/game/sba.rscrates/engine/src/game/static_abilities.rscrates/engine/src/game/targeting.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/parser/oracle_nom/filter.rscrates/engine/src/parser/oracle_nom/target.rscrates/engine/src/parser/oracle_static/static_helpers.rscrates/engine/src/parser/oracle_target.rscrates/engine/src/types/ability.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/beamtown_bullies_active_opponent_target.rscrates/engine/tests/integration/coerced_attack_punisher.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/nettling_imp_continuity_target.rscrates/mtgish-import/src/convert/player_effect.rs
Parse changes introduced by this PR · 1 card(s), 3 signature(s) (baseline: main
|
ControllerRef::You misclassified as relative, blocking activation
#6690
…pponent (phase-rs#6496) The Beamtown Bullies' activated ability reads "Target opponent whose turn it is puts target nonlegendary creature card from your graveyard onto the battlefield under their control." The "whose turn it is" relative clause was silently dropped: `parse_target_with_syntax` matched `tag("opponent")` and returned immediately, leaving the clause on the remainder, which `parse_subject_application` then discarded. Any opponent was a legal target, including on the controller's own turn. Rather than add a `ControllerRef::ActiveOpponent` sibling, this parameterizes the existing variant: ControllerRef::ActivePlayer { relation: PlayerRelation } Zero new enum variants. This is what the in-code LEGALITY-SCOPE PAIR directive at `types/ability.rs` prescribes -- a third legality-scoped sibling crosses the /add-engine-variant Stage-2 threshold and must be refactored onto the existing `PlayerRelation` axis instead. All 48 pre-existing sites take `relation: PlayerRelation::All`, for which `matches_relation` is unconditionally true, so behaviour is preserved. Targeting rides the existing candidate loop in `find_legal_targets_with_context` rather than adding a branch, so the phasing / left-the-game / hexproof / shroud / protection guards compose for free. `relation: Opponent` yields no candidate on the controller's own turn, making the activation illegal to announce (CR 601.2c, imported into activation by CR 602.2b; CR 602.2), and the read is live so CR 608.2b re-checks it on resolution. Team correctness routes through `players::matches_relation` (CR 102.3), so an active teammate in Two-Headed Giant is not "the opponent whose turn it is". Parser: new `parse_active_player_turn_clause` combinator in `oracle_nom/target.rs`, wired into both player head nouns behind a guarded fallback, so every existing "target opponent"/"target player" card is byte-identical. The card remains UNSUPPORTED and is not claimed as fixed. Two further gaps block it, both pre-existing and out of scope here: * `ability_utils::relative_controller_kind` classifies the chained ChangeZone's `ControllerRef::You` ("from YOUR graveyard") as a relative controller, so the graveyard slot is enumerated against the target opponent's graveyard and activation fails with "No legal targets available". Pinned by a known-gap regression test. * The "under their control" controller binding is unparsed. An `Effect::unimplemented("enters_under_their_control", ...)` marker records it so coverage reports `supported: false, gap_count: 1` instead of silently claiming the card. Without that marker, fixing the first gap alone would reanimate the creature under the ABILITY CONTROLLER's control, inverting the card. Also corrects two CR mis-citations touched by this change: `CR 800.4a` -> `CR 104.5 + CR 102.1` for the left-the-game targeting guard at both sites in `targeting.rs` (800.4a governs the departing player's objects, not target legality), and drops an overstatement of `CR 702.26b` to players (it is permanent-scoped; the CR has no player-phasing rule). Verified in an isolated worktree (not Tilt-watched): clippy -D warnings clean; 17,773 lib + 4,076 integration tests passing; parser-combinator gate PASS; cargo coverage exit 0. `ordering_parity_sweep` fails identically on the pristine base commit and is pre-existing on main. Review follow-ups (PR phase-rs#6689): * Serde back-compat. `ControllerRef::ActivePlayer` used to be a UNIT variant, so its externally-tagged wire form was the bare string `"ActivePlayer"`. Regenerating in-repo fixtures cannot reach data persisted OUTSIDE the repo (saved game states, reconnect snapshots, external card-data.json consumers), which still carry that form and would now fail to deserialize outright. `ControllerRef` therefore gets a hand-written `Deserialize` that accepts the legacy bare tag as `relation: PlayerRelation::All` -- exactly the pre-parameterization meaning, since `matches_relation` is unconditionally true for `All`. `Serialize` stays DERIVED, so nothing written out changes. The canonical path delegates to a derived mirror enum carrying all 13 variants verbatim; two tripwires keep it from drifting -- a wildcard-free exhaustive match that fails to COMPILE when a variant is added, and a per-variant serialize/deserialize round trip. Follows the `QuantityExpr` legacy bare-integer and `ChoiceType` legacy unit-variant deserializers in the same module. * Terminal separators. `parse_active_player_turn_clause`'s boundary accepted only EOF or a space, so the sentence-final form "target opponent whose turn it is." failed the arm and left the relative clause unparsed. The guard is now the single `one_of(" ,;.")` char class -- word boundary, comma, semicolon, period -- still under `peek`, so every separator byte survives on the remainder for the callers' `&text[lower.len() - rest.len()..]` offset arithmetic in `oracle_target.rs`. `" whose turn it isn't"` and Oath of Ghouls' `" whose graveyard ..."` are still rejected. Re-verified in the isolated worktree: cargo fmt clean; clippy -D warnings clean (exit 0); 17,798 lib + 4,097 integration tests passing (exit 0); parser combinator gate PASS against the base commit. coverage-parse-diff against the published main baseline attributes exactly 1 card / 3 signatures to this PR (The Beamtown Bullies), with no other parse change across 35,516 faces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTBTG756AfwmkoPwEty1np
ee87f11 to
7e636c2
Compare
Parse changes introduced by this PR · 1 card(s), 3 signature(s) (baseline: main
|
|
@matthewevans — both points addressed in 🔴 Blocker — legacy
|
| Gate | Exit | Result |
|---|---|---|
cargo fmt --all -- --check |
0 | clean |
cargo clippy --all-targets -- -D warnings |
0 | no warnings |
cargo test -p engine --lib |
0 | 17798 passed / 0 failed |
cargo test -p engine --test integration |
0 | 4097 passed / 0 failed |
check-parser-combinators.sh HEAD~1 |
0 | Gate A + Gate G PASS |
coverage-report data/ |
0 | 35,516 faces decoded |
The integration suite loads tests/fixtures/integration_cards.json, so its passing is direct evidence the fixture still decodes under the new Deserialize.
FORGE_TEST_FULL_DB=1 ordering_parity_sweep remains excluded — it fails identically on pristine base commit 7be8d002c with its own generated corpus, i.e. pre-existing on main.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] State-aware player-target matching still rejects every active-player scope. Evidence: filter.rs:6272-6283 sends player_matches_target_filter_in_state through _with, while filter.rs:6324-6337 hard-codes ControllerRef::ActivePlayer { .. } => false; targeting.rs:975-995 uses that supposedly state-aware matcher while checking resolved player targets. Why it matters: an active opponent can be legal at announcement through find_legal_targets, yet fail the later player-target match/revalidation path; the sole target is then removed and the ability fizzles instead of using the live state.active_player as required by CR 608.2b. Suggested fix: make the in-state matcher handle ActivePlayer { relation } at this authority (player_id == state.active_player plus players::active_player_satisfies_relation(state, source_controller, relation)), preserving the state-free wrapper's fail-closed behavior. Add a direct resolution-validation regression: an already-chosen active opponent remains legal and resolves while they are active, then is removed/fizzles after the active player changes; include a direct matcher assertion so this authority cannot silently regress again.
…pponent (phase-rs#6496) The Beamtown Bullies' activated ability reads "Target opponent whose turn it is puts target nonlegendary creature card from your graveyard onto the battlefield under their control." The "whose turn it is" relative clause was silently dropped: `parse_target_with_syntax` matched `tag("opponent")` and returned immediately, leaving the clause on the remainder, which `parse_subject_application` then discarded. Any opponent was a legal target, including on the controller's own turn. Rather than add a `ControllerRef::ActiveOpponent` sibling, this parameterizes the existing variant: ControllerRef::ActivePlayer { relation: PlayerRelation } Zero new enum variants. This is what the in-code LEGALITY-SCOPE PAIR directive at `types/ability.rs` prescribes -- a third legality-scoped sibling crosses the /add-engine-variant Stage-2 threshold and must be refactored onto the existing `PlayerRelation` axis instead. All 48 pre-existing sites take `relation: PlayerRelation::All`, for which `matches_relation` is unconditionally true, so behaviour is preserved. Targeting rides the existing candidate loop in `find_legal_targets_with_context` rather than adding a branch, so the phasing / left-the-game / hexproof / shroud / protection guards compose for free. `relation: Opponent` yields no candidate on the controller's own turn, making the activation illegal to announce (CR 601.2c, imported into activation by CR 602.2b; CR 602.2), and the read is live so CR 608.2b re-checks it on resolution. Team correctness routes through `players::matches_relation` (CR 102.3), so an active teammate in Two-Headed Giant is not "the opponent whose turn it is". Parser: new `parse_active_player_turn_clause` combinator in `oracle_nom/target.rs`, wired into both player head nouns behind a guarded fallback, so every existing "target opponent"/"target player" card is byte-identical. The card remains UNSUPPORTED and is not claimed as fixed. Two further gaps block it, both pre-existing and out of scope here: * `ability_utils::relative_controller_kind` classifies the chained ChangeZone's `ControllerRef::You` ("from YOUR graveyard") as a relative controller, so the graveyard slot is enumerated against the target opponent's graveyard and activation fails with "No legal targets available". Pinned by a known-gap regression test. * The "under their control" controller binding is unparsed. An `Effect::unimplemented("enters_under_their_control", ...)` marker records it so coverage reports `supported: false, gap_count: 1` instead of silently claiming the card. Without that marker, fixing the first gap alone would reanimate the creature under the ABILITY CONTROLLER's control, inverting the card. Also corrects two CR mis-citations touched by this change: `CR 800.4a` -> `CR 104.5 + CR 102.1` for the left-the-game targeting guard at both sites in `targeting.rs` (800.4a governs the departing player's objects, not target legality), and drops an overstatement of `CR 702.26b` to players (it is permanent-scoped; the CR has no player-phasing rule). Verified in an isolated worktree (not Tilt-watched): clippy -D warnings clean; 17,773 lib + 4,076 integration tests passing; parser-combinator gate PASS; cargo coverage exit 0. `ordering_parity_sweep` fails identically on the pristine base commit and is pre-existing on main. Review follow-ups (PR phase-rs#6689): * Serde back-compat. `ControllerRef::ActivePlayer` used to be a UNIT variant, so its externally-tagged wire form was the bare string `"ActivePlayer"`. Regenerating in-repo fixtures cannot reach data persisted OUTSIDE the repo (saved game states, reconnect snapshots, external card-data.json consumers), which still carry that form and would now fail to deserialize outright. `ControllerRef` therefore gets a hand-written `Deserialize` that accepts the legacy bare tag as `relation: PlayerRelation::All` -- exactly the pre-parameterization meaning, since `matches_relation` is unconditionally true for `All`. `Serialize` stays DERIVED, so nothing written out changes. The canonical path delegates to a derived mirror enum carrying all 13 variants verbatim; two tripwires keep it from drifting -- a wildcard-free exhaustive match that fails to COMPILE when a variant is added, and a per-variant serialize/deserialize round trip. Follows the `QuantityExpr` legacy bare-integer and `ChoiceType` legacy unit-variant deserializers in the same module. * Terminal separators. `parse_active_player_turn_clause`'s boundary accepted only EOF or a space, so the sentence-final form "target opponent whose turn it is." failed the arm and left the relative clause unparsed. The guard is now the single `one_of(" ,;.")` char class -- word boundary, comma, semicolon, period -- still under `peek`, so every separator byte survives on the remainder for the callers' `&text[lower.len() - rest.len()..]` offset arithmetic in `oracle_target.rs`. `" whose turn it isn't"` and Oath of Ghouls' `" whose graveyard ..."` are still rejected. Re-verified in the isolated worktree: cargo fmt clean; clippy -D warnings clean (exit 0); 17,798 lib + 4,097 integration tests passing (exit 0); parser combinator gate PASS against the base commit. coverage-parse-diff against the published main baseline attributes exactly 1 card / 3 signatures to this PR (The Beamtown Bullies), with no other parse change across 35,516 faces. * Player-target revalidation. `filter::player_matches_target_filter_with` hard-coded the new `ControllerRef::ActivePlayer { relation }` arm to `false`, with a comment claiming no caller reached it. That claim was wrong: `targeting::target_ref_matches_resolved_filter` routes every announced `TargetRef::Player` back through `player_matches_target_filter_in_state` on resolution (CR 608.2b), and `stack_spell_entry_matches_filter` (CR 115.9b/c) and the Aura host restriction reach it too -- so a still-legal active opponent failed its own re-check. The helper now takes a second closure alongside the existing `is_opponent` one, `active_player_ok`, which the in-state entry point supplies from `state` (candidate == `state.active_player`, narrowed through the single authority `players::active_player_satisfies_relation`, CR 102.1 / CR 102.3) and the state-free `player_matches_target_filter` supplies as `|_, _| false`, preserving its fail-closed contract. The false comment is gone. Three new tests: an end-to-end CR 608.2b resolution pin (target stays active -> resolves; turn passes -> fizzles), a revert-failing direct assertion on both matcher entry points, and a 2HG teammate case -- each negative paired with a positive reach-guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTBTG756AfwmkoPwEty1np
7e636c2 to
ce336ca
Compare
|
@matthewevans — confirmed and fixed in You're right, and this one was introduced by this PR: The fix
I took your suggested closure shape over threading Callers of the private helper — all three, all in One correction to the finding's blast radiusI revert-checked the tests — restored the
So the fizzle path itself was already correct: Fixed as prescribed regardless, since a wrong answer from that authority is a real defect. Flagging it because the test doc says the same thing: I did not want the end-to-end test reading as stronger coverage of this fix than it is. Tests (in the existing integration file, no new top-level binary)
Verification (real exit codes, each redirected to its own log)
No card-data regeneration needed — this fix touches neither the parser nor any serialized shape, so the posted parse-diff artifact still stands. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — shared-team turns are rules-incorrect.
[HIGH] ActivePlayer narrows to the engine's single representative instead of the active team. Evidence: players.rs:204 evaluates relation only against state.active_player; targeting.rs:257 then permits only player.id == state.active_player; and GameState stores that as one PlayerId (game_state.rs:11029). In a shared-team-turn game, however, each member of the active team is an active player (CR 805.4/805.4a), and an ability referring to an active player must select the specific one its controller chooses (CR 805.9). Thus, in 2HG with P2 and P3 on the active opposing team, this PR exposes only the representative P2 and makes the equally legal P3 impossible to target.
Please make the active-player authority enumerate/select the active-team candidates at the target-choice seam and thread that same authority through the resolution-side matcher, instead of treating state.active_player as the complete set. Add a production-pipeline 2HG test proving both active opposing-team players are offered/choosable and their chosen target remains legal on resolution. The current 2HG test only swaps the single representative between a teammate and an opposing player, so it cannot detect this omission.
This reverts commit 1ebbf65.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — manual quality gate remains unmet
The current implementation is at the correct targeting/filter seam, and the earlier state-aware matcher defect is fixed at this head. The parse evidence also matches the claimed scope: one affected card, The Beamtown Bullies, with the three expected signatures.
However, the strict quality gate requires a production-pipeline test that demonstrably fails when the production matcher change is reverted. The existing active_opponent_target_is_revalidated_against_the_live_active_player scenario drives apply(), but the author’s documented revert check confirms it still passes with the matcher arm reverted: target legality there is independently decided by find_legal_targets. The only demonstrated revert-failing coverage is the direct player_matches_target_filter_in_state assertion.
Please add a focused scenario-runner / apply() regression that reaches a production caller whose result actually depends on player_matches_target_filter_in_state, and include the revert result. Until then this is a test-discrimination gap rather than a correctness finding, but it blocks the quality label and merge-queue approval under the maintainer quality bar.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Active-player targeting is rules-incorrect in shared-team-turn games. Evidence: crates/engine/src/game/players.rs:204-214 evaluates the relation only against the singleton state.active_player; crates/engine/src/game/targeting.rs:257-264 and crates/engine/src/game/filter.rs:6309-6319 then require the candidate to equal that one representative. In Two-Headed Giant, CR 805.4a makes the whole team active and CR 805.9 requires the ability controller to choose one specific active player. With P0/P1 versus P2/P3 and state.active_player == P2, P3 is an equally legal active opponent but is never offered and cannot survive revalidation. The current 2HG test at crates/engine/src/game/targeting.rs:3847-3868 tests only the inactive teammate/P2 representative distinction, so it cannot detect P3 being omitted. Why it matters: “target opponent whose turn it is” rejects a legal target and the new ActivePlayer { relation } readers all encode the same singleton assumption. Suggested fix: model active-team candidate membership at the shared authority, enumerate and revalidate every active-team member satisfying the relation, add a real 2HG announcement-and-resolution test choosing each opposing teammate, and audit scalar active-player reads for the CR 805.9 choice/binding requirement.
[MED] The current head still lacks a discriminating production-pipeline regression for the state-aware matcher. Evidence: crates/engine/tests/integration/beamtown_bullies_active_opponent_target.rs:664-672 explicitly says its apply() scenario is not the regression pin for player_matches_target_filter_in_state; the sole current pin is the direct helper test at :759-. Commit cc4e481 reverted the only cast-pipeline test that exercised a production caller of that matcher. Why it matters: the direct test catches the helper in isolation but does not meet the required proof that a real caller fails if its production behavior regresses. Suggested fix: restore or replace it with a focused apply()/scenario regression that enters a real matcher-dependent caller and demonstrably fails when the active-player matcher arm is reverted.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Active-player targeting remains rules-incorrect in shared-team-turn games. Evidence: players.rs:204-214 evaluates only the singleton state.active_player; targeting.rs:257-264 and filter.rs:6309-6319 then require that same singleton. The new 2HG test proves P2 but never tests P3, the active opponent's teammate. Under CR 805.4a, the team whose turn it is is the active team, and CR 805.9 requires the ability controller to choose a specific active player. With P0/P1 versus P2/P3 and state.active_player == P2, P3 is an equally legal active opponent but is neither offered nor able to survive revalidation. Why it matters: this rejects a legal target for the new active-player scope, and the same singleton assumption is threaded through the other ControllerRef::ActivePlayer readers. Suggested fix: centralize active-team membership at the authority, enumerate and revalidate each active-team member that satisfies the relation, bind the CR 805.9 choice where a scalar consumer needs one, and add a real 2HG announcement-and-resolution test selecting P2 and P3.
[MED] The required discriminating production-pipeline matcher test is still absent. Evidence: beamtown_bullies_active_opponent_target.rs:664-672 explicitly says its apply() test is not the regression pin for player_matches_target_filter_in_state; the direct helper assertion remains the only pin. Commit cc4e481 reverted the cast-pipeline test, and this head does not replace it. Why it matters: the manual quality gate requires a runtime production caller whose result fails when the matcher change is reverted. Suggested fix: add a focused scenario / apply() regression through a production caller that depends on the state-aware matcher and include the revert result.
The parse-diff evidence is still unavailable at this head (Baseline pending); the PR already contains one maintainer merge for that churn, so I am not adding another merge-main commit solely to retry it. Security/intake were clean, the final diff has no workflow/instruction changes, and CI's aggregate Rust gate is still in progress; neither changes the blockers above.
…ns (phase-rs#6496) CR 102.1 + CR 805.4a: under the shared team turns option every member of the team whose turn it is is an active player, but every ControllerRef::ActivePlayer { relation } read compared candidates against the single stored representative (state.active_player). In 2HG with P2/P3 as the active opposing team, "target opponent whose turn it is" offered only P2 and made the equally legal P3 impossible to target (CR 805.9: the ability's controller chooses WHICH active player it refers to). - players.rs: active-player identity is now owned by four single authorities: is_active_player (CR 805.4a membership), active_players (representative-first enumeration), active_player_candidate_matches (candidate + relation, evaluated against the CANDIDATE per CR 805.9; replaces active_player_satisfies_relation), and choose_active_player (the CR 805.9 deterministic stand-in for scalar reads). - targeting/filter/static_abilities/ability_utils/quantity: every candidate-shaped ActivePlayer read routes through the candidate authority; the two scalar reads (filter::controller_ref_player, the sacrifice scope) route through choose_active_player. - tests: new 2HG announcement-and-resolution pipeline test proving both active-team members are offered, individually choosable, and resolve (fails under the singleton revert); extended the 2HG matcher and enumeration tests with membership positives/negatives; restored the reverted cast-pipeline cost-reducer regression for player_matches_target_filter_in_state with a corrected reach-guard (sole-candidate announcement auto-binds, so the stack target is asserted instead of a TargetSelection prompt) — it demonstrably fails when the matcher arm is reverted to fail-closed. Re-applies matthewevans' 1ebbf65 (reverted in cc4e481) with the reach-guard fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125MPN7zw3bbEXdeFsn2sJT
|
@matthewevans — both remaining findings addressed in 🔴 HIGH — active-player targeting in shared-team-turn gamesYou're right: every The fix — membership at the shared authority
All eleven candidate read sites (targeting enumeration, both matcher entry points, filter props, static restrictions, ability_utils enumeration, quantity counts) now route through the candidate authority, so the announce seam and the resolution seam cannot drift: the target-choice prompt is the CR 805.9 choice, and revalidation accepts exactly the same set. Production-pipeline 2HG test
The existing narrow 2HG tests were extended rather than replaced: the unit enumeration test now pins Revert evidence: with 🟡 MED — discriminating production-pipeline regression for the matcherYour Revert evidence: with the in-state Verification (worktree, direct cargo — Tilt does not watch it)
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Scalar ActivePlayer reads replace the required controller choice with a deterministic teammate. Evidence: players::choose_active_player explicitly selects the first qualifying active-team member (crates/engine/src/game/players.rs:256-275), and scalar callers use it for controller resolution and sacrifice scope (crates/engine/src/game/filter.rs:978-989; crates/engine/src/game/effects/sacrifice.rs:111-121). CR 805.9 requires the ability controller to choose which specific active player an ability refers to when its effect is applied. Why it matters: in shared-team turns, an effect that searches a referenced player library, puts a card under that player control, or makes that player sacrifice chooses P2 solely because P2 is the representative or first seat; it must permit choosing P3 when both are active. Suggested fix: model and bind that CR 805.9 player choice at effect application, threading the bound player through scalar consumers, rather than using a deterministic choose_active_player; add a 2HG production-pipeline regression that exercises a scalar consumer with distinct P2/P3 outcomes.
3 participants
Partially addresses #6496 (The Beamtown Bullies). #6496 should stay open — this fixes one of its three gaps; the card still does not activate.
The bug
The
whose turn it isrelative clause was silently dropped.parse_target_with_syntaxmatchedtag("opponent")and returned immediately, leaving the clause on the remainder, whichparse_subject_applicationthen discarded. Any opponent was a legal target, including on the controller's own turn.The fix — parameterize, don't proliferate
Rather than add a
ControllerRef::ActiveOpponentsibling, this parameterizes the existing variant:Zero new enum variants. This is what the in-code LEGALITY-SCOPE PAIR directive at
types/ability.rsprescribes — a third legality-scoped sibling crosses the/add-engine-variantStage-2 threshold and must be refactored onto the existingPlayerRelationaxis instead. All 48 pre-existing sites takerelation: PlayerRelation::All, for whichmatches_relationis unconditionallytrue, so behaviour is preserved.An alternative
TargetFilter::PlayerMatching { player: Box<PlayerFilter> }bridge was designed and rejected: it delivers the same one card, needs 2 new variants, and introduces ~16 silentmatches!classifier sites with no compile error. The parameterization also discharges three correctness risks structurally — reusing the existing candidate loop infind_legal_targets_with_contextmeans no guard re-implementation, no layering inversion, and no unboundedPlayerFiltersurface at the targeting boundary.Behaviour
players::matches_relation(CR 102.3) — an active teammate in Two-Headed Giant is not "the opponent whose turn it is"The card is NOT claimed as fixed
Two further gaps block it, both pre-existing and out of scope:
ability_utils::relative_controller_kindclassifies the chainedChangeZone'sControllerRef::You("from your graveyard") as a relative controller, so the graveyard slot is enumerated against the target opponent's graveyard and activation fails withActionNotAllowed("No legal targets available"). Proven pre-existing: the same text with the relative clause removed fails identically. Pinned by a known-gap regression test."under their control"binding is unparsed. AnEffect::unimplemented("enters_under_their_control", …)marker records it, so coverage reportssupported: false, gap_count: 1rather than silently claiming the card.Without marker 2, fixing gap 1 alone would reanimate the creature under the ability controller's control — inverting the card. The marker node must be deleted in the same change that unblocks activation, since it currently makes the
ChangeZone'ssub_linkaContinuationStep.Also corrected
CR 800.4a→CR 104.5 + CR 102.1for the left-the-game targeting guard, at both sites intargeting.rs(800.4a governs the departing player's objects, not target legality)CR 702.26bto players — it is permanent-scoped; the CR has no player-phasing ruleVerification
Run directly in an isolated worktree (not Tilt-watched), with real exit codes:
cargo clippy --all-targets -- -D warningscargo test -p engine --libcargo test -p engine --test integrationcheck-parser-combinators.sh HEAD~1cargo coverageSerde note:
ControllerRefis externally tagged, so"ActivePlayer"→{"ActivePlayer":{"relation":{"type":"All"}}}is a wire-shape change.tests/fixtures/integration_cards.jsonis regenerated; no legacy bare-string reader remains. No frontend change —TargetFilterisRecord<string, unknown>client-side.FORGE_TEST_FULL_DB=1 ordering_parity_sweepfails with 18 unexplained diffs — pre-existing on main, verified by running it on pristine base commit7be8d002cwith its own generated corpus, where it fails identically. Not caused by this change.The final re-verification after rebasing onto current
mainwas still running when this was pushed; CI is the check on that.🤖 Generated with Claude Code
https://claude.ai/code/session_01KTBTG756AfwmkoPwEty1np
Summary by CodeRabbit