Fix Emperor of Bones regression coverage - #7243
Conversation
# Conflicts: # crates/engine/tests/integration/issue_1515_emperor_of_bones.rs
|
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)
📝 WalkthroughWalkthroughThe integration test adds Oracle text for Emperor of Bones and a counter-placement spell. It verifies linked-creature return, Emperor preservation, finality and haste assignment, and delayed sacrifice through the cast pipeline. ChangesEmperor of Bones counter regression
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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/tests/integration/issue_1515_emperor_of_bones.rs`:
- Around line 203-208: Extend the assertions in the test around returned and
state to verify that the returned creature has the required finality counter
using the existing counter accessor. Update the test comment to reference CR
122.1h, ensuring the test fails when enter_with_counters is omitted.
- Around line 29-31: Update the Emperor of Bones fixture text in the affected
test to replace both “this creature” self-references with “Emperor of Bones,”
preserving the authoritative Oracle wording and all other text unchanged.
🪄 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: 00009ad5-b2cd-4daf-830d-0c4bf4c07f7b
📒 Files selected for processing (1)
crates/engine/tests/integration/issue_1515_emperor_of_bones.rs
| Whenever one or more +1/+1 counters are put on this creature, put a creature card exiled with this \ | ||
| creature onto the battlefield under your control with a finality counter on it. It gains haste. \ | ||
| Sacrifice it at the beginning of the next end step."; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the authoritative Oracle wording for both self-references.
The fixture replaces both self-references with this creature. The authoritative Modern Horizons 3 text uses Emperor of Bones in both places. (magic.wizards.com) This test can otherwise exercise a different parser path from the production card text.
Proposed Oracle-text fix
-Whenever one or more +1/+1 counters are put on this creature, put a creature card exiled with this \
-creature onto the battlefield under your control with a finality counter on it. It gains haste. \
+Whenever one or more +1/+1 counters are put on Emperor of Bones, put a creature card exiled with \
+Emperor of Bones onto the battlefield under your control with a finality counter on it. It gains haste. \As per path instructions: “Preserve authoritative Oracle text.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Whenever one or more +1/+1 counters are put on this creature, put a creature card exiled with this \ | |
| creature onto the battlefield under your control with a finality counter on it. It gains haste. \ | |
| Sacrifice it at the beginning of the next end step."; | |
| Whenever one or more +1/+1 counters are put on Emperor of Bones, put a creature card exiled with \ | |
| Emperor of Bones onto the battlefield under your control with a finality counter on it. It gains haste. \ | |
| Sacrifice it at the beginning of the next end step."; |
🤖 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/tests/integration/issue_1515_emperor_of_bones.rs` around lines
29 - 31, Update the Emperor of Bones fixture text in the affected test to
replace both “this creature” self-references with “Emperor of Bones,” preserving
the authoritative Oracle wording and all other text unchanged.
Source: Path instructions
| let state = runner.state(); | ||
| assert_eq!( | ||
| state.objects[&returned].zone, | ||
| Zone::Battlefield, | ||
| "the counter trigger must return the linked creature through apply()" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the finality counter on the returned creature.
The test proves that returned reaches the battlefield, but it does not prove that the return effect adds the required finality counter. The upstream build_dies_return_with_counter_trigger contract explicitly models this with enter_with_counters.
A regression that omits the counter would still pass these assertions. Under CR 122.1h, the counter changes a later graveyard move into exile. (media.wizards.com) Add an assertion using the existing counter accessor, and reference CR 122.1h in the test comment.
As per path instructions: “A test must exercise the failure path the fix prevents.”
🤖 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/tests/integration/issue_1515_emperor_of_bones.rs` around lines
203 - 208, Extend the assertions in the test around returned and state to verify
that the returned creature has the required finality counter using the existing
counter accessor. Update the test comment to reference CR 122.1h, ensuring the
test fails when enter_with_counters is omitted.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head production-path regression coverage is load-bearing.
✅ Clean
crates/engine/tests/integration/issue_1515_emperor_of_bones.rs:181-236drives Emperor's printed trigger from an actual counter spell throughGameRunner::cast(...).resolve(), then distinguishes the returned creature from Emperor for battlefield placement, the finality entry modifier, haste, and the delayed-sacrifice target. That covers the cast/reducer path not exercised by the existing direct-resolver regressions.- CodeRabbit's finality-counter finding was valid and is covered by the maintainer fixup at lines 209-218. Its Oracle-wording finding is refuted: the current official Scryfall Oracle text uses “this creature” for both source references, matching lines 29-30.
Recommendation: merge when the required checks settle.
|
Current head |
2 participants
Summary
Adds a reducer-path regression test for Issue #1515 proving that Emperor of Bones remains on the battlefield while the creature it returns receives haste and the delayed sacrifice rider. The production fix is already present on current upstream main; this PR adds the missing real cast-pipeline coverage against the archived bug behavior.
Files changed
Track
Developer
LLM
Model: GitHub Copilot (canonical id not exposed)
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.The "not used" box is for changes that genuinely fall outside that scope.
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 fmt --all— exit 0.cargo test -p phase-engine --test integration issue_1515_emperor_of_bones -- --nocapture— 4 passed.cargo clippy --all-targets -- -D warnings— exit 0.cargo test -p phase-engine— exit 0.cargo coverage— exit 0; 31,775/35,795 cards supported (88.8%).cargo semantic-audit— exit 0; 32,730 cards audited, 266 existing findings; no Emperor of Bones finding reported.git diff --check upstream/main...HEAD— exit 0.Gate A
Gate A PASS head=74dd287939918ac9cccbb4b1f4889675340e2d9a base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
Final review-impl
Final review-impl PASS head=74dd287939918ac9cccbb4b1f4889675340e2d9a
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Tests
Documentation