Fix Emperor of Bones - #7200
Conversation
📝 WalkthroughWalkthrough
ChangesReplacement waiting-state handling
Estimated code review effort: 3 (Moderate) | ~20 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: 1
🤖 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/replacement.rs`:
- Around line 1006-1016: Annotate the park_waiting_for function’s waiting-choice
preservation logic with a verified “CR 614.12a” reference and concise
description that the required choice occurs before the permanent enters the
battlefield. Place the annotation directly at the relevant logic without
changing its behavior.
🪄 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: 4cd1585e-0069-42ec-a338-c6fc101bd1df
📒 Files selected for processing (2)
crates/engine/src/game/replacement.rscrates/engine/tests/integration/issue_1515_emperor_of_bones.rs
| /// as-enters effect already surfaced its own interactive prompt. Leave that prompt | ||
| /// in place so the entry choice completes before the surrounding ability resumes. | ||
| pub fn park_waiting_for(state: &mut GameState, player: PlayerId) { | ||
| if matches!(state.waiting_for, WaitingFor::EffectZoneChoice { .. }) { | ||
| if matches!( | ||
| state.waiting_for, | ||
| WaitingFor::EffectZoneChoice { .. } | ||
| | WaitingFor::CopyTargetChoice { .. } | ||
| | WaitingFor::ChooseOneOfBranch { .. } | ||
| | WaitingFor::NamedChoice { .. } | ||
| | WaitingFor::ReturnAsAuraTarget { .. } | ||
| ) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the required rules annotation.
park_waiting_for changes how the engine preserves choices while a permanent enters the battlefield. Add a verified CR 614.12a: ... annotation at this logic. CR 614.12a requires the choice before the permanent enters the battlefield. (media.wizards.com)
As per coding guidelines, “verify the relevant CR section before completion, and annotate rules-related code with a verified CR number and description.” As per path instructions, rules-touching code without that annotation is a finding.
🤖 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/replacement.rs` around lines 1006 - 1016, Annotate the
park_waiting_for function’s waiting-choice preservation logic with a verified
“CR 614.12a” reference and concise description that the required choice occurs
before the permanent enters the battlefield. Place the annotation directly at
the relevant logic without changing its behavior.
Sources: Coding guidelines, Path instructions
|
Generated for head 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.
Request changes — the Emperor/Peacekeeper regression is covered, but the prompt-preservation fix remains partial.
🔴 Blocker
crates/engine/src/game/replacement.rs:1008-1018hard-codes fiveWaitingForvariants.engine_replacement.rs:2429-2436dispatches a post-replacement continuation as a general resolved/template effect, whileengine_resolution_choices.rs:652-710is the existing authority that classifies the wider resolution-owned prompt family (includingSearchChoice,ChooseFromZoneChoice,PayAmountChoice, and others). A downstream member of that family will still be overwritten byReplacementChoice; the new regression atcrates/engine/tests/integration/issue_1515_emperor_of_bones.rs:159-256exercises onlyNamedChoice, one of the newly listed variants. Reuse the central classifier here, retaining only genuinely external prompt families as explicit exceptions, and add a production-path regression for one representative previously unlisted prompt so this cannot regress into another hand-maintained list.
✅ Clean
- The current-head CodeRabbit annotation request is already satisfied:
replacement.rs:1005-1007carries the CR 614.12a annotation at the preserved-prompt seam. I found no additional current-head finding in its review.
Recommendation: replace the hand-maintained subset with the shared resolution-choice authority, cover the omitted class, then request re-review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/issue_1515_emperor_of_bones.rs`:
- Around line 263-275: Rewrite the regression test around the scenario runner so
it creates the pending replacement through the normal zone-move or scenario
pipeline instead of assigning runner.state_mut().waiting_for directly or calling
park_waiting_for. Submit the resulting action via runner.act(...), exercising
replacement handling through apply, move_object, replace_event, or the relevant
GameAction path. Assert that SearchChoice remains active during replacement and
that the replacement continuation resumes afterward.
🪄 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: b1fe99b6-cac4-44e3-afb5-4cff24686965
📒 Files selected for processing (4)
crates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/replacement.rscrates/engine/src/game/zone_pipeline.rscrates/engine/tests/integration/issue_1515_emperor_of_bones.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the shared resolution-choice authority fixes the prior hand-maintained-list blocker, but the new representative regression is not exercising the production pause/resume path.
🔴 Blocker
-
crates/engine/tests/integration/issue_1515_emperor_of_bones.rs:259-287constructsWaitingFor::SearchChoicedirectly and callsreplacement::park_waiting_fordirectly. This proves only the helper's immediate classification; it does not entereffects/change_zone.rs:755-769/1100-1142, create a pending replacement, submit the choice throughGameRunner::act, or prove that the paused continuation resumes. Consequently it cannot distinguish the intended fix from a future caller that bypasses the parking authority or preserves a stale consumed prompt. Replace it with a scenario/runtime regression that reaches a realpark_waiting_forcall site with a previously unlisted resolution-owned prompt, acts through that prompt, and asserts the replacement/outer continuation completes. IfSearchChoicecannot occur at one of those call sites, use a reachable omitted variant (or narrow the classifier to the demonstrably reachable class). -
The required parse-diff evidence is not current-head evidence: the only
<!-- coverage-parse-diff -->comment is bound to3eb0982ab1c78aa36163f1d0978ba81beb0f475c, while this review is for64cce71026ed6de413d27b4528656eee8f894063. Please obtain the current-head artifact before the next review; it is the required scope/coverage evidence for an engine PR.
✅ Clean
crates/engine/src/game/replacement.rs:1008-1016now delegates resolution-owned prompts toengine_resolution_choices::handles, the central authority atengine_resolution_choices.rs:652-727; the only retained explicit exceptions are the external copy/aura prompt families.crates/engine/src/game/engine_resolution_choices.rs:3964clears the consumedSearchPartitionChoicebefore its zone move can surface a replacement choice. The existing annotation is accurate: CR 614.12a states, "If a replacement effect that modifies how a permanent enters the battlefield requires a choice, that choice is made before the permanent enters the battlefield."
Recommendation: add the discriminating production-path regression and refresh the current-head parse-diff artifact, then request re-review.
|
Correction to my current-head review: the existing parse-diff artifact was refreshed in place and is now bound to |
|
Implemented the requested review fix in commit Focused Emperor tests: 3 passed. Full engine suite: 4771 passed, 0 failed, 2 ignored; doc-tests clean. Gate A: PASS at @matthewevans please re-review. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head re-review is clean.
The new regression reaches the real Emperor return/replacement path, preserves the previously unlisted PayAmountChoice, answers it through GameRunner::act, and verifies the outer continuation finishes. Exact-head CI and the parse-diff artifact are green.
2 participants
Summary
Fixes Emperor of Bones so an as-enters choice on the returned creature is not overwritten by replacement parking. Anointed Peacekeeper now completes both choices before Emperor's haste and delayed-sacrifice riders resume.
Files changed
Track
Developer
LLM
Model: gpt-5.6-sol
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
Note
Any change to
crates/engine/game logic — parser, effects, resolver,targeting, rules behavior — is expected to go through
/engine-implementer.CR references
Verification
Available checks ran clean; exact-head evidence is recorded below.
Gate A output below is bound to the current committed head.
Review tooling ran against the exact current head; its proposed precedence change was rejected by executable evidence from the established lifecycle suite, with no code change required.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all -- --check— PASScargo clippy-strict— PASS using the user-local pkgconf/OpenSSL development prefixRUST_MIN_STACK=33554432 cargo test -p phase-engine— PASS at the exact committed head: 4771 passed; 0 failed; 2 ignored; doc-tests 0 failedRUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration issue_1515_emperor_of_bones -- --nocapture— PASS (3 passed)RUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration cost_zone_pipeline -- --nocapture— PASS (106 passed)RUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration cast_during_resolution_pipeline -- --nocapture— PASS (11 passed)RUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration issue_3245_abhorrent_oculus_manifest_dread -- --nocapture— PASS (2 passed)RUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration issue_3311_manifest_dread_land -- --nocapture— PASS (2 passed)RUST_MIN_STACK=33554432 cargo test -p phase-engine --test integration surveil_rest_pile_redirect_continuation -- --nocapture— PASS (2 passed)./scripts/gen-card-data.sh— PASScargo coverage— PASS (31775/35795 cards supported; 88.8%)cargo semantic-audit— completed (32730 cards audited; 266 existing findings)./scripts/check-parser-combinators.sh— PASS (Gate G and Gate A)Gate A
Gate A PASS head=a9a20eaa3671b15e101b90f51fe2ab87443b619a base=97591656218103d8e8c7315725b24cfe64645dd4
Review disposition
The production-path regression now runs Emperor's return through the real zone-move replacement pipeline. It reaches the shared parking boundary with a previously unlisted resolution-owned
PayAmountChoice, verifies that prompt remains active, answers it withGameRunner::act, and verifies the returned permanent and delayed Emperor rider complete.The rejected broad routing of the three fresh replacement-choice branches remains intentionally unchanged: the established lifecycle suite produced 14 regressions for that proposal, while the current direct-assignment contract remains green.
Summary by CodeRabbit
Bug Fixes
Tests