test(engine): Riptide per-opponent target slots are scoped to that opponent (#6565) - #6836
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated per-opponent target fanout resolution to place all preselected targets. Added two integration tests for Riptide Gearhulk target scoping and resulting battlefield and library states. ChangesRiptide Gearhulk targeting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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
🤖 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/tests/integration/riptide_gearhulk_5994.rs`:
- Around line 141-142: Add a multiplayer runtime test case alongside the
existing target-selection cases, using the target players [P1, P2] and target
objects [p1_perm, p2_perm]. Assert that both selected permanents are moved into
their respective owners’ libraries, covering independent acceptance of P2’s
permanent.
- Around line 118-122: The CR reference CR 109.5 in the comments at both the
anchor site (lines 118-122) and the sibling site (lines 156-158) of
crates/engine/tests/integration/riptide_gearhulk_5994.rs does not properly cover
player binding semantics. Replace CR 109.5 with CR 102.2 + CR 608.2c in both
comment locations to accurately reference the rules governing opponent scoping
and target player binding, or remove the CR citations entirely if the more
specific rules are not needed for clarity.
🪄 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: c2258ff2-6043-45af-a214-5351f347d0b8
📒 Files selected for processing (1)
crates/engine/tests/integration/riptide_gearhulk_5994.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the per-opponent controller-scope regression does not exercise the second opponent’s slot.
🔴 Blocker
[MED] The new tests use two-player GameScenario::new() and select only P1 at crates/engine/tests/integration/riptide_gearhulk_5994.rs:125-143 and :161-178. The production fanout creates a separate player/object slot pair for each opponent at crates/engine/src/game/ability_utils.rs:4906-4935; the existing three-player test at riptide_gearhulk_5994.rs:87-115 selects P1 and declines P2. A regression that binds the first opponent correctly but aliases or skips the second therefore passes every test here.
Please add a three-player runtime cast selecting [P1, P2] and [p1_perm, p2_perm], and assert both opponent permanents reach their respective libraries while the caster’s permanent remains on the battlefield. That drives the real pipeline through both independently bound fanout slots.
🟡 Non-blocking
[LOW] The new comments at crates/engine/tests/integration/riptide_gearhulk_5994.rs:118 and :156 cite CR 109.5 for the that player binding. CR 109.5 defines only “you” and “your”; the local rules text at docs/MagicCompRules.txt:2793 is the applicable general instruction to read the whole text and apply English. Please remove the false CR 109.5 citations, or replace them only with a verified, accurately worded CR 608.2c rationale.
Recommendation: request changes for the two-opponent runtime guard; correct the two CR comments in the same update.
|
Maintainer update: the two review findings are addressed on the current head. Rust lint/tests and frontend CI are still pending; the fresh CodeRabbit review is clean. Once the remaining CI checks are green, I will resume with approval and merge-queue enqueue. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new three-player regression exposes an unresolved production defect, so this test-only PR cannot merge until the engine behavior is corrected.
[HIGH] Per-opponent library placement resolves only the first selected opponent target. Evidence: crates/engine/tests/integration/riptide_gearhulk_5994.rs:176-184 selects [P1, P2] and [p1_perm, p2_perm]; current-head CI fails with p2_perm still on the battlefield (Rust shard 1/2 log). The fanout validator preserves both legal object targets at crates/engine/src/game/ability_utils.rs:4988-5001, but PutAtLibraryPosition resolves its printed count as a single global limit at crates/engine/src/game/effects/put_on_top.rs:162 and truncates the selected targets at :315. Why it matters: “for each opponent, put up to one target … that player controls” must apply once for each selected opponent; in a three-player game the second opponent’s selected permanent is silently unaffected. Suggested fix: extend the engine’s per-opponent fanout execution semantics at the target/resolver seam so each selected player/object pair receives its own one-object action, with a runtime regression that covers both selected and declined slots. Do not weaken the new two-opponent regression.
…ponent's permanents (phase-rs#6565) The per-opponent fanout targeting fix (phase-rs#6565, "that player controls" bound to the iterated opponent) had no runtime regression covering the controller scope. The existing riptide_gearhulk_5994.rs tests cover the optional-count behavior (phase-rs#5994) but not that each opponent's slot targets THAT opponent's permanents rather than the caster's. Add two runtime tests driving Riptide's real ETB through the cast pipeline: - `riptide_per_opponent_slot_excludes_the_casters_own_permanents` offers only the caster's own permanent as target intent; it is not a legal target for the opponent's per-opponent slot, so the slot is declined and the permanent stays. Non-vacuous: a broken scope that let the opponent slot target the caster's permanent would move it to the library and flip the assertion. - `riptide_per_opponent_slot_targets_that_opponents_permanent` is the positive half — the opponent's own permanent IS legal and moves to its owner's library, while the caster's permanent is untouched. Refs phase-rs#6565
…sition (phase-rs#6565) A per-opponent target fanout ("for each opponent, put up to one target nonland permanent that player controls into its owner's library third from the top") pre-selects one target PER opponent at stack time via multi_target.max = PlayerCount { Opponent }. The PutAtLibraryPosition effect's count field (Fixed(1)) is the PER-OPPONENT cap, not the total, so put_on_top::resolve wrongly treated the two pre-chosen permanents as "more candidates than count" and issued an interactive EffectZoneChoice { count: 1 } over the already-targeted permanents. That prompt looped forever (selecting one never advanced) and, at best, would have placed only one of the two. CR 601.2c: the number of targets is fixed at targeting; each chosen target is placed. CR 401.4: cards put at the same library position are arranged by their owner. For a per-opponent fanout, expected placement count is the number of pre-chosen targets, so each is routed to its own owner's library (CR 400.7). Fixes the runtime regression covered by riptide_per_opponent_slots_target_each_opponents_permanent (phase-rs#6836).
887a6ee to
c25788f
Compare
|
The 2-opponent test surfaced a real engine bug, so I pushed an engine fix (c25788f) rather than adjusting the test. Root cause: the per-opponent fanout targets correctly at stack time (4 scoped slots; each opponent's permanent chosen for that opponent's slot, per the #6565 fix). But at resolution, Fix: for a per-opponent target fanout the expected placement count is the number of pre-chosen targets, so each is placed into its own owner's library (routed by the move, CR 400.7). Reuses the existing Verified locally: all 4 tests in |
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved. The per-opponent fanout now treats the already-selected one-per-opponent targets as the resolved placement set, and the three-player cast-pipeline regression proves both opponent targets move to their respective owners' libraries. Current-head CI and the parser parse-diff check are green.
2 participants
What this is
The per-opponent fanout targeting fix for Riptide Gearhulk (#6565 � "that player controls" bound to the iterated opponent) had no runtime regression covering the controller scope.
riptide_gearhulk_5994.rscovers the optional-count behavior (#5994), but not that each opponent's slot targets that opponent's permanents rather than the caster's.Tests
Two runtime tests driving Riptide's real ETB through the cast pipeline into
WaitingFor::TriggerTargetSelection:riptide_per_opponent_slot_excludes_the_casters_own_permanents� offers only the caster's own permanent as target intent. It is not a legal target for the opponent's per-opponent slot, so the slot is declined and the permanent stays on the battlefield. Non-vacuous: a broken scope that let the opponent slot target the caster's permanent would move it to the library and flip the assertion.riptide_per_opponent_slot_targets_that_opponents_permanent� the positive half: the opponent's own permanent IS legal and moves to its owner's library third from the top, while the caster's permanent is untouched.cargo fmt,clippy -p phase-engine --tests, and both tests are green.Refs #6565
Summary by CodeRabbit
Bug Fixes
Tests