fix(engine): require distinct players for a multi-target player requirement#6572
fix(engine): require distinct players for a multi-target player requirement#6572rsnetworkinginc wants to merge 3 commits into
Conversation
…rement
CR 115.1b + CR 601.2c: a single instruction that chooses two or more target
players requires them to be different — the same player can't fill more than
one of the instruction's target slots. Scheming Symmetry ("Choose two target
players.", issue phase-rs#6459) let the same player be chosen for both slots.
Modal cards attach DifferentTargetPlayers from "each mode must target a
different player"; a plain multi-target player requirement had no equivalent,
so the runtime distinctness authority (validate_target_constraints) never saw a
constraint to enforce. A post-lowering pass now attaches
TargetSelectionConstraint::DifferentTargetPlayers to every non-modal
multi-target Player/Opponent requirement, reusing the existing enforcement.
Closes phase-rs#6459
📝 WalkthroughWalkthroughThe change extends per-target-instance distinctness checks from objects to all target references, updates related documentation, and adds an integration test verifying Scheming Symmetry rejects duplicate player targets while accepting distinct players. ChangesDistinct player targeting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle.rs (1)
2884-2901: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPlacement-pin ordering comment not updated for the new pass.
The preceding "PLACEMENT PIN" comment (lines 2884-2891) explicitly enumerates the ordered sequence of passes and states "Order is behavior-load-bearing"-style guarantees, but it doesn't mention
ensure_distinct_player_multi_targets, inserted at line 2901 betweenreconcile_host_bound_phase_outsandapply_linked_choice_persisted_player. Given how heavily this file relies on this comment as the single source of truth for safe reordering, a future refactor could move this call unaware of any ordering constraint it has (or doesn't have).📝 Suggested doc update
// PLACEMENT PIN: the two enters-choice relations run first, then the // within-item `reconcile_host_bound_phase_outs` chain repair (NOT a document - // relation — it belongs to unit 7), then the persisted-player relation, then - // the swallow audit, then the two enters/attack relations — reproducing the + // relation — it belongs to unit 7), then `ensure_distinct_player_multi_targets` + // (a post-lowering normalization, not a document relation), then the + // persisted-player relation, then the swallow audit, then the two + // enters/attack relations — reproducing the // exact order the five standalone passes ran in (choose-counter → self-chosen // type → host-bound → persisted-player → swallow → etb-exile → punisher).🤖 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.rs` around lines 2884 - 2901, Update the PLACEMENT PIN comment above the linked-choice passes to include ensure_distinct_player_multi_targets in its enumerated execution order between reconcile_host_bound_phase_outs and the persisted-player relation. Clarify its ordering constraint or lack thereof consistently with the existing behavior-load-bearing guidance, without changing the pass implementations.
🤖 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.rs`:
- Around line 2769-2791: Expand test coverage for
ensure_distinct_player_multi_target beyond the existing fixed-count Player case:
add cases for TargetFilter::Opponent and variable/“any number of target players”
multi-target definitions, verifying distinct-target enforcement in each. Keep
the existing card scenario, and exercise the helper’s supported parameter range
directly or through representative parser fixtures.
- Around line 2743-2746: Update the rule citation in the comment above the
target-player validation to reference CR 601.2c only, removing the incorrect CR
115.1b reference while preserving the explanation of the no-duplicate-target
restriction.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 2884-2901: Update the PLACEMENT PIN comment above the
linked-choice passes to include ensure_distinct_player_multi_targets in its
enumerated execution order between reconcile_host_bound_phase_outs and the
persisted-player relation. Clarify its ordering constraint or lack thereof
consistently with the existing behavior-load-bearing guidance, without changing
the pass implementations.
🪄 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: 8aca0558-46f7-4a7d-8472-f3d85ebce795
📒 Files selected for processing (3)
crates/engine/src/parser/oracle.rscrates/engine/tests/integration/issue_6459_scheming_symmetry.rscrates/engine/tests/integration/main.rs
|
Right seam, right class, and a genuinely discriminating runtime test — this reuses the existing distinctness authority instead of adding enforcement, which is exactly the shape I want. Two things to fix, one of them a wrong CR citation. Reviewed at head 🔴 BlockerCR 115.1b is the wrong rule, and it is cited in the load-bearing doc comment. CR 601.2c is correct and is the only citation you need. Verified verbatim at
If you want a companion citation for "what a target is" generally, that's CR 115.1 ( Worth noting the second sentence of 601.2c is also the precise justification for why this pass is scoped to a single 🟡 Non-blocking1. The gate reads only the primary effect's filter, so the pass can silently no-op. 2. One card is not a class. The doc comment claims two classes (Scheming Symmetry and "any number of target players"), but both tests are Scheming Symmetry. Given this is a blanket post-lowering pass applied to every non-modal ability and trigger, a second card from the claimed class — ideally one with a variable target count, where the "no-op below two players" reasoning actually gets exercised — would turn the class claim into evidence. Right now the "constraint is a no-op for a selection of fewer than two players" assertion in the doc is reasoning, not a tested property. ✅ Clean
Recommendation: fix the CR citation (blocker), answer #1, and this is ready to land. The architecture is the part that's hard to get right and you got it right — I'd rather have this pattern with a corrected annotation than any amount of additional machinery. |
|
Correcting my previous comment: I called this the right seam, and it isn't. There's a better fix twenty lines away in a file this PR doesn't touch — and my CR guidance was also wrong. The CR citation — I sent you to the wrong rule tooI said CR 115.1b was wrong (it is — it's the Aura rule, CR 115.3 (
This repo already standardises on The seam — the existing CR 115.3 authority drops players, and that is this bug
// CR 601.2c + CR 115.3: within one instance of "target", the same object
// can't be chosen twice. …
let already_in_instance: HashSet<ObjectId> = prior_specs.iter().zip(selected_slots)
.filter(|(prior, _)| prior.instance == spec.instance)
.filter_map(|(_, sel)| match sel {
Some(TargetRef::Object(id)) => Some(*id),
_ => None, // <-- TargetRef::Player discarded here
})
.collect();
legal.retain(|t| !matches!(t, TargetRef::Object(id) if already_in_instance.contains(id)));It is narrowed to Four reasons that beats the parser-lowering pass:
There's also a live asymmetry worth knowing: the stateless And it's not a selection-only fix. Both paths route through Blast radius is 21 cards, not oneMeasured against The class is correct — this is not build-for-the-card, and I want to be clear about that. It's just undeclared, and worth enumerating in the description. Related: eleven of those 23 are Also non-blocking
What stands from my last commentThe runtime test is genuinely discriminating and I'd keep it exactly as written — real Oracle text is verbatim-correct against Scryfall, no new enum variant, no new Revised recommendation: move the fix to |
Parse changes introduced by this PR✓ No card-parse changes detected. |
Resolves the deterministic crates/engine/tests/integration/main.rs mod-line conflict by keeping both test modules in rustfmt-sorted order. Maintainer-side port: main advanced under this PR (phase-rs#6498 landed a sibling integration test).
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the behavior is sound, but the new CR annotation is not.
🔴 Blocker
[MED] Correct the duplicate-target citation at crates/engine/src/parser/oracle.rs:2743. CR 115.1b concerns Aura targeting; CR 601.2c is the rule that says the same target cannot be chosen multiple times for one instance of “target.” Cite CR 601.2c alone here and in matching new comments/tests.
Recommendation: request changes; make the local CR-citation correction and re-request review.
Relocate the issue phase-rs#6459 fix from a parser-lowering pass to the existing per-instance distinctness authority in legal_targets_for_selected_slot (CR 601.2c + CR 115.3): `already_in_instance` was narrowed to HashSet<ObjectId>, so TargetRef::Player fell through and the same player could fill two slots of one instance of "target" (Scheming Symmetry, "Choose two target players."). Widening it to HashSet<TargetRef> keeps objects AND players distinct within an instance while still permitting reuse across separate instances, and covers both the selection and validation paths, which route through the same helper. - drop the ensure_distinct_player_multi_targets parser pass and its parse-level test: the runtime authority now enforces the rule with no card-data churn, so the parse-diff is empty - correct the CR citation from CR 115.1b (the Aura rule) to the repo's standard CR 601.2c + CR 115.3 pair at every site - fix the TargetInstanceId doc, which restated CR 115.3 as "mutually distinct objects" — the rule covers objects or players - document TargetSelectionConstraint::DifferentTargetPlayers (modal path) with its CR reference The runtime regression test (real CastSpell -> ChooseTarget in a 3-player game: same player rejected, slot stays open, different player completes) is unchanged and passes against the relocated fix.
|
Done as recommended — the fix is relocated to the instance-aware distinctness authority, and thanks for the correction on both the seam and the rule.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/tests/integration/issue_6459_scheming_symmetry.rs (2)
80-112: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd coverage for reuse across separate target instances.
This test only proves distinctness within one target instance. An implementation that incorrectly carries the duplicate set into a later target instance would still pass. Add an end-to-end case where the same player is legally selected in two separate target instances and both selections complete.
As per path instructions, tests should exercise the building block’s parameter range; the PR objective also requires reuse across separate instances.
🤖 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_6459_scheming_symmetry.rs` around lines 80 - 112, Extend the integration test around the existing target-selection flow to create and complete a second target instance after the first resolves, selecting the same player in both instances. Assert both selections succeed and each instance completes, demonstrating duplicate tracking is scoped per target instance rather than carried across instances.Source: Path instructions
80-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the duplicate-rejection assertion prove it reaches the second slot.
The guard before
reselect_same.is_err()only proves that target selection is active; it does not prove that the runner advanced to the second slot or that this is an otherwise valid player-target path. An unrelated validation failure could therefore make this regression pass. Assert the active slot and a valid alternative candidate such asP2before attempting to reselectP1, then retain the existing post-error state assertion.As per path instructions, negative assertions require a positive reach-guard proving the input reached the code under test.
🤖 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_6459_scheming_symmetry.rs` around lines 80 - 104, Strengthen the precondition in the target-selection test before reselect_same.is_err(): inspect WaitingFor::TargetSelection and assert it represents the second target slot with P2 as a valid alternative candidate, rather than only confirming target selection is active. Then keep the existing duplicate-P1 rejection and post-error waiting-state assertions unchanged.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.
Outside diff comments:
In `@crates/engine/tests/integration/issue_6459_scheming_symmetry.rs`:
- Around line 80-112: Extend the integration test around the existing
target-selection flow to create and complete a second target instance after the
first resolves, selecting the same player in both instances. Assert both
selections succeed and each instance completes, demonstrating duplicate tracking
is scoped per target instance rather than carried across instances.
- Around line 80-104: Strengthen the precondition in the target-selection test
before reselect_same.is_err(): inspect WaitingFor::TargetSelection and assert it
represents the second target slot with P2 as a valid alternative candidate,
rather than only confirming target selection is active. Then keep the existing
duplicate-P1 rejection and post-error waiting-state assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a382d493-9b7f-4318-92e4-5ba7e638fcdd
📒 Files selected for processing (3)
crates/engine/src/game/ability_utils.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/issue_6459_scheming_symmetry.rs
|
Maintainer acknowledgement: rechecked current head b2f8472 after the latest follow-up. This remains blocked on a maintainer merge of current main and fresh CI; the authorized fork-push GH_TOKEN needed to publish that merge is unavailable. No further author action is requested. |
|
Maintainer handling update: current-head review confirmed the implementation seam and runtime behavior, but CodeRabbit correctly identified that the new negative assertion needs a second-slot/alternative-target reach guard. This is a maintainer-fixup-sized test-only edit; no contributor action is requested. The authorized handler cannot publish it because the fork-push credential ( |
2 participants
Summary
Choose two target players.(Scheming Symmetry) allowed choosing the same player for both slots. CR 601.2c + CR 115.3: the same target — object or player — can't be chosen multiple times for any one instance of the word "target", so a single instruction that chooses two or more target players requires them to be different — the same player can't fill more than one of that instruction's target slots.Root cause
Modal cards get distinctness from the sentence "each mode must target a different player" (
ModalSelectionConstraint::DifferentTargetPlayers→TargetSelectionConstraint::DifferentTargetPlayers). A plain multi-target player requirement (amulti_targetoverTargetFilter::Player/Opponent) had no constraint attached, so the runtime distinctness authorityvalidate_target_constraints— which already knows how to reject duplicate player targets — never received a constraint to enforce.Fix
A post-lowering pass (
ensure_distinct_player_multi_targets, next to the existingreconcile_host_bound_phase_outspass) attachesTargetSelectionConstraint::DifferentTargetPlayersto every non-modal ability/trigger whosemulti_targetrequirement targetsPlayer/Opponentand doesn't already carry it. No new enforcement code — it reuses the existing shared authority. The constraint is a no-op for a selection of fewer than two players, so attaching it whenever the requirement can select multiple players is always safe. Object multi-targets ("two target creatures") are unaffected (their distinctness is handled separately).Update (post-review)
Per review, the fix now lives at the instance-aware distinctness authority in
game/ability_utils.rs(legal_targets_for_selected_slot):already_in_instanceis widened fromHashSet<ObjectId>toHashSet<TargetRef>, so within one instance of "target" both objects and players stay distinct (CR 601.2c + CR 115.3), while separate instances may still reuse the same object or player. The parser-lowering pass and its parse-level test are dropped, restoringlower_oracle_iruntouched — the parse-diff for this head is empty. The relocated fix covers the ~21-card multi-target-player class enumerated in review with zero card-data churn, and both the selection and validation paths route through this helper.Tests
scheming_symmetry_parses_distinct_player_constraint— the lowered ability carriesDifferentTargetPlayers.scheming_symmetry_rejects_choosing_the_same_player_twice— end-to-end in a 3-player game: after choosing P1 for slot 1,ChooseTarget(P1)for slot 2 is rejected and the slot stays open, while choosing a different player (P2) is accepted and the spell leaves target selection.target_constraints == []; runtime: the same player is accepted), pass with it.Parity (isolated
CARGO_TARGET_DIR, on9613bcc22)cargo fmt --all -- --check: cleancargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings: cleancargo test -p engine --lib: 17590 passed / 0 failedcargo test -p engine --test integration: 3878 passed / 0 failedCloses #6459
Summary by CodeRabbit
Bug Fixes
Documentation