Fix True-Name Nemesis chosen-player protection - #7213
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe engine parses ChangesChosen-player protection
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)
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
| // CR 702.16 + CR 109.4: the source is protected against objects | ||
| // controlled by the player chosen for the protected permanent. | ||
| ProtectionTarget::ChosenPlayer => protected | ||
| .chosen_player() | ||
| .is_some_and(|player| source.controller == player), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the ownership leg of protection from the chosen player.
CR 702.16k covers objects controlled by the chosen player and objects owned by that player that another player does not control. The current comparison only checks source.controller, so nonbattlefield sources owned by the chosen player can bypass protection. CR 109.4 also states that objects outside the stack and battlefield have no controller. (media.wizards.com)
crates/engine/src/game/keywords.rs#L545-L549: Match the chosen player against the source owner when the source has no controller, while retaining controller matching for controlled sources.crates/engine/src/types/keywords.rs#L505-L507: Replace the CR 109.4 citation with the applicable CR 702.16k behavior and document the ownership case.
As per path instructions, player-scoped queries on nonbattlefield zones must filter by obj.owner, not controller.
📍 Affects 2 files
crates/engine/src/game/keywords.rs#L545-L549(this comment)crates/engine/src/types/keywords.rs#L505-L507
🤖 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/keywords.rs` around lines 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.
Sources: Path instructions, MCP tools
|
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.
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, or the exact CI-owned alternative is stated below.
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 parse_protection_target_chosen_player— passed:test types::keywords::tests::parse_protection_target_chosen_player ... okcargo test -p phase-engine --test integration issue_5941_true_name_chosen_player_protection— passed:test issue_5941_true_name_chosen_player_protection::true_name_protection_uses_the_protected_objects_chosen_player ... okcargo test -p phase-engine --lib --quiet— passed:18833 passed; 0 failed; 6 ignored; finished in 26.65scargo clippy -p phase-engine --all-targets -- -D warnings— passed.cargo fmt --all— passed.git diff --check— passed.Independent final review-impl —
LGTM.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=b71eec325dc0c7a1a46dbcbd4cf9a1ba5be5973f base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
Final review-impl
Final review-impl PASS head=b71eec325dc0c7a1a46dbcbd4cf9a1ba5be5973f
Claimed parse impact
True-Name Nemesis — expected parser classification changes from the stringly typed
CardType("the chosen player")form toProtectionTarget::ChosenPlayerwhen generated card data is regenerated. The generated card-data artifact was not available in this worktree, so no artifact parse-diff was produced.Scope Expansion
None.
Validation Failures
cargo clippy --all-targets -- -D warnings— blocked before linting becauseopenssl-sys v0.9.116could not find an OpenSSL installation and requiresOPENSSL_DIR.cargo test -p phase-engine— started successfully with many passing tests but exceeded the 120-second safety timeout and was killed whileloop_shortcut::an_evicting_beat_mints_without_growing_the_ringwas still running.CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests