Fix True-Name Nemesis chosen-player protection - #7213
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe engine parses ChangesChosen-player protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TrueNameNemesis
participant ChooseOption
participant ProtectionMatcher
participant Source
TrueNameNemesis->>ChooseOption: present player choice
ChooseOption-->>TrueNameNemesis: persist chosen player
Source->>ProtectionMatcher: check targeting legality
ProtectionMatcher->>TrueNameNemesis: compare source controller-or-owner with chosen player
ProtectionMatcher-->>Source: allow or reject target
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/src/game/keywords.rs`:
- Around line 545-549: Update ProtectionTarget::ChosenPlayer in
crates/engine/src/game/keywords.rs:545-549 to match the chosen player against
source.owner when source.controller is absent, while retaining controller
matching for controlled sources and using owner for nonbattlefield player-scoped
queries. Update the documentation in crates/engine/src/types/keywords.rs:505-507
to cite CR 702.16k and describe both controller and ownership protection
behavior.
In
`@crates/engine/tests/integration/issue_5941_true_name_chosen_player_protection.rs`:
- Around line 35-41: Update the test setup around the direct
WaitingFor::NamedChoice assignment to enter True-Name Nemesis through the
production enter-the-battlefield path instead of prepopulating post-dispatch
state. Assert that this path produces the expected WaitingFor::NamedChoice, then
submit GameAction::ChooseOption and retain the existing target-legality
assertions.
🪄 Autofix
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: bae2b917-67e3-4060-b068-437d4d432a3e
📒 Files selected for processing (5)
crates/engine/src/game/keywords.rscrates/engine/src/game/static_abilities.rscrates/engine/src/types/keywords.rscrates/engine/tests/integration/issue_5941_true_name_chosen_player_protection.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Held pending current-head CI. I pushed maintainer commit |
|
Held pending current-head validation. I pushed I also confirmed the remaining unresolved CodeRabbit finding is rules-valid: CR 702.16k protects from objects the chosen player controls and objects that player owns but no other player controls. The present |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] ChosenPlayer implements only the controller half of protection from a player. Evidence: crates/engine/src/game/keywords.rs:547-549 compares only source.controller, while CR 702.16k also covers an object the chosen player owns when no other player controls it. This is not theoretical in this engine: crates/engine/src/game/effects/change_zone.rs:1698-1719 and its regression at :4582-4620 document that a stolen permanent retains its former controller after moving to its owner's graveyard, where player-scoped semantics must use owner. Why it matters: a chosen player's off-battlefield source can incorrectly bypass protection, and a stale controller can make an object owned by someone else incorrectly match. Suggested fix: make the ChosenPlayer arm use the engine's owner-vs-controller-by-zone authority (controller for controlled sources; owner where the object has no controller), document CR 702.16k on the variant, and add discriminating tests for both a selected owner's stolen-then-died source and the inverse stale-controller case.
Current-head CI also fails before the claimed production regression reaches its as-enters path: Rust tests (shard 2/4) reports InvalidAction("apply_as_current: no authorized submitter (game over?)") at crates/engine/tests/integration/issue_5941_true_name_chosen_player_protection.rs:46. This is supporting evidence only; it does not replace the rules-correctness fix above.
|
Validated and fixed both current-head blockers in 2ba1c0e. The cast-path regression now stocks the draw step, and CR 702.16k source authority uses controller on controlled-zone objects and owner on owner-zone objects via the shared CR 109.4/108.4a helper. Added opposite stale-controller fixtures; reverting the production fix makes the selected-owner assertion fail. Local gates: focused regression PASS, cargo clippy-strict PASS, cargo test -p phase-engine PASS (4810 passed, 2 ignored), final review-impl PASS, Gate A PASS. Fresh CI is running for this head. |
There was a problem hiding this comment.
Actionable comments posted: 1
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/game/game_object.rs (1)
2949-2959: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve zero final chapter number and gate Saga actions on chapter abilities.
final_chapter_number()must returnSome(0)for an empty Saga andNoneonly for a non-Saga. CR 714.3c and CR 714.4 still require one or more chapter abilities. Updatecrates/engine/src/game/sba.rs:2068-2119andcrates/engine/src/game/turns.rs:2611-2623to check chapter-ability presence separately. Add regression coverage for an empty Saga.🤖 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/game/game_object.rs` around lines 2949 - 2959, Update GameObject::final_chapter_number to return Some(0) for a Saga with no chapter abilities and None only for non-Sagas. In the CR 714.3c and CR 714.4 logic within the saga action and sacrifice handling, check chapter-ability presence separately rather than using final_chapter_number as the Saga-action gate. Add regression coverage verifying empty Sagas retain final chapter number zero and are excluded from those actions.Sources: Path instructions, MCP tools
🤖 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/game/game_object.rs`:
- Around line 1459-1466: Update GameObject::controller_or_owner so ordinary
cards in Zone::Command resolve to self.owner rather than preserving a
potentially stale self.controller; retain self.controller only for explicitly
modeled command-zone exceptions such as emblems, while keeping Battlefield and
Stack behavior unchanged.
---
Outside diff comments:
In `@crates/engine/src/game/game_object.rs`:
- Around line 2949-2959: Update GameObject::final_chapter_number to return
Some(0) for a Saga with no chapter abilities and None only for non-Sagas. In the
CR 714.3c and CR 714.4 logic within the saga action and sacrifice handling,
check chapter-ability presence separately rather than using final_chapter_number
as the Saga-action gate. Add regression coverage verifying empty Sagas retain
final chapter number zero and are excluded from those actions.
🪄 Autofix
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: ad1de6b1-ac74-4f0f-ab0c-f453de662c0c
📒 Files selected for processing (6)
crates/engine/src/game/game_object.rscrates/engine/src/game/keywords.rscrates/engine/src/game/replacement.rscrates/engine/src/types/keywords.rscrates/engine/tests/integration/issue_5941_true_name_chosen_player_protection.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/tests/integration/main.rs
- crates/engine/src/game/keywords.rs
- crates/engine/src/types/keywords.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the current head still assigns an ordinary command-zone card to its stored controller.
🔴 Blocker
[HIGH] controller_or_owner treats every command-zone object as controlled. Evidence: crates/engine/src/game/game_object.rs:1459-1467 returns self.controller for Zone::Command; crates/engine/src/game/zones.rs:1169-1197 changes the zone and incarnation without normalizing that field; and crates/engine/src/game/keywords.rs:545-551 uses the helper for ProtectionTarget::ChosenPlayer. CR 109.4 says, “Only objects on the stack or on the battlefield have a controller,” except for its enumerated cases. CR 702.16k separately protects from objects the chosen player controls and objects that player owns when no other player controls them. Why it matters: a stolen ordinary card sent to the command zone retains a stale controller, so chosen-player protection can attribute it to the wrong player.
Use an exception-aware authority: preserve the controller for the explicitly modeled command-zone exception (the is_emblem path), but use owner for ordinary command-zone cards. Please add a discriminating stale-command regression for both the selected owner's card and the inverse stale-controller case, plus an exception fixture proving an emblem retains its controller. The existing integration regression covers the analogous graveyard fixtures at crates/engine/tests/integration/issue_5941_true_name_chosen_player_protection.rs:104-125, but never reaches Zone::Command.
✅ Clean
The current head’s cast → as-enters → named-choice regression is production-wired, and the head-bound parse artifact reports no card-parse changes.
Recommendation: request changes for the command-zone authority and its two-sided regression coverage; then re-review the new head.
|
Fixed the remaining current-head blocker in 981c479. Ordinary command-zone objects now resolve source authority from owner per CR 109.4/108.4a; only explicitly modeled emblems retain controller authority under CR 109.4c. Added inverse stale-command fixtures and an emblem exception fixture. Focused test, full engine suite, strict clippy, fresh review-impl, and Gate A all pass locally. Current-head CI is queued. |
|
Current head 981c479 is fully validated: CI run 31627339178 is green, current-head parse-diff reports no card-parse changes, and the command-zone blocker is fixed with owner authority for ordinary cards and controller authority only for modeled emblems. The fresh implementation review is clean and Gate A passes for this SHA. The remaining CHANGES_REQUESTED review is bound to 2ba1c0e; please re-review this current head. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for the current head 981c479eb8e8308902a843a131551973f6d36a3e: chosen-player protection now uses owner for ordinary command-zone cards and preserves the emblem exception, with discriminating production-path regressions.
2 participants
Summary
Closes #5941. Fixes the True-Name Nemesis protection misparse by representing “the chosen player” as a typed protection target and resolving it from the protected permanent’s persisted choice, so objects controlled by that player cannot target it.
Files changed
Track
Developer
LLM
Model: GitHub Copilot (via GitHub Copilot; canonical id not exposed)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Verification
Required checks ran clean for the current head; local verification is listed below.
CI run 31627339178 — all required checks passed.
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 phase-engine --test integration issue_5941_true_name_chosen_player_protection -- --nocapture— passed: 1 passed, 0 failed.Command-zone revert discrimination — reverting the command-zone authority failed at the ordinary-command assertion; restoring it passed.
Production-revert discrimination — reverting the
ChosenPlayerauthority failed at the selected-owner assertion; restoring it passed.cargo clippy-strict— passed.cargo test -p phase-engine— passed: 4810 passed, 0 failed, 2 ignored.cargo fmt --all— passed.git diff --check— passed.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=981c479eb8e8308902a843a131551973f6d36a3e base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
Final review-impl
Final review-impl PASS head=981c479eb8e8308902a843a131551973f6d36a3e
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests