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 2 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOracle lowering now applies 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
| /// 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 be chosen for | ||
| /// more than one of that instruction's target slots (Scheming Symmetry, issue | ||
| /// #6459: "Choose two target players." allowed picking the same player twice). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source and any CR references in the repo docs.
sed -n '2730,2765p' crates/engine/src/parser/oracle.rs
printf '\n---\n'
rg -n '115\.1b|601\.2c|same target can.t be chosen multiple times|DifferentTargetPlayers' docs/MagicCompRules.txt crates/engine/tests/integration/issue_6459_scheming_symmetry.rs crates/engine/src/parser/oracle.rsRepository: phase-rs/phase
Length of output: 3973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '[oracle.rs excerpt]'
sed -n '2738,2758p' crates/engine/src/parser/oracle.rs
echo
echo '[integration test excerpt]'
sed -n '1,140p' crates/engine/tests/integration/issue_6459_scheming_symmetry.rs
echo
echo '[repo CR references]'
rg -n 'CR 115\.1b|CR 601\.2c|DifferentTargetPlayers|same target can.t be chosen multiple times' crates/engine/src/parser/oracle.rs crates/engine/tests/integration/issue_6459_scheming_symmetry.rs docs/MagicCompRules.txtRepository: phase-rs/phase
Length of output: 8377
🌐 Web query:
Magic Comprehensive Rules 115.1b same target can't be chosen multiple times 601.2c choose targets
💡 Result:
Under the Magic: The Gathering Comprehensive Rules, the ability to choose the same object or player as a target multiple times depends on how the word "target" is used in the card's text. The primary rule governing this is that the same target cannot be chosen multiple times for any single instance of the word "target" [1][2]. This is explicitly addressed during the process of casting a spell (or activating/triggering an ability) when targets are chosen [1]. Key takeaways from the rules: 1. Single instance of "target": If a spell or ability uses the word "target" followed by a quantity (e.g., "target two creatures"), you must choose that many distinct objects or players [1][2]. You cannot choose the same object more than once for that specific instance [1][2]. 2. Multiple instances of "target": If a spell or ability uses the word "target" in multiple separate places, you may choose the same object or player once for each instance of the word, provided it meets the targeting criteria for each [1][2]. For example, if a card says "Target creature gains flying. Target creature gains haste," you may choose the same creature for both instances [3][2]. 3. Rule References: Rule 601.2c covers the process of choosing targets when casting a spell, establishing that the same target cannot be chosen multiple times for one instance of the word "target" [1]. Rule 115 (specifically 115.3 in current rulesets) provides the broader definitions for targeting and reinforces that while you cannot repeat a target for a single instance of the word, you can do so across multiple instances [2][4]. Note that some spells may also include additional restrictions, such as the word "another," which would further prohibit choosing the same object even if multiple instances of "target" are present [4].
Citations:
- 1: https://mtg-rules.vercel.app/chapters/601
- 2: https://mtg.fandom.com/wiki/Target
- 3: https://blogs.magicjudges.org/rulestips/2011/07/an-individual-spell-or-ability-can-only-target-a-single-object-once-for-each-use-of-the-word-target-in-a-cards-text/
- 4: https://www.mtgsalvation.com/forums/magic-fundamentals/magic-rulings/803354-can-a-spell-have-the-same-target-twice
Fix the rule citation Use CR 601.2c for the no-duplicate-target restriction; CR 115.1b is the wrong reference here.
🤖 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 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.
Source: Path instructions
| fn ensure_distinct_player_multi_target(def: &mut AbilityDefinition) { | ||
| if def.multi_target.is_none() { | ||
| return; | ||
| } | ||
| // Only player-target requirements are governed by this constraint; an | ||
| // object multi-target ("two target creatures") is kept distinct by the | ||
| // per-slot legal-set exclusion instead. | ||
| if !matches!( | ||
| def.effect.target_filter(), | ||
| Some(TargetFilter::Player | TargetFilter::Opponent) | ||
| ) { | ||
| return; | ||
| } | ||
| if def | ||
| .target_constraints | ||
| .iter() | ||
| .any(|c| matches!(c, TargetSelectionConstraint::DifferentTargetPlayers)) | ||
| { | ||
| return; | ||
| } | ||
| def.target_constraints | ||
| .push(TargetSelectionConstraint::DifferentTargetPlayers); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test coverage limited to one card's shape.
The pass explicitly targets both TargetFilter::Player and TargetFilter::Opponent, and the comment (lines 2755-2756) claims it's also safe for "the 'any number of target players' class", but the only added test (issue_6459_scheming_symmetry.rs) exercises a single "Player, exactly two" instance. Per this repo's engine testing guidance, a building block should be tested across its parameter range rather than through a single card case — an Opponent-filtered multi-target or a variable/"any number" multi-target case is untested here.
As per path instructions: "test the building block and its parameter range rather than a single card case."
🤖 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 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.
Source: Path instructions
|
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.
2 participants
Summary
Choose two target players.(Scheming Symmetry) allowed choosing the same player for both slots. 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 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).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