Fix Emperor of Bones - #7311
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 change adds recursive ChangesEmperor of Bones resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🔵 Low · up to The PR prevents dependent effects from falling back to Emperor when no creature was exiled, avoiding unintended counter or trigger behavior. It is mergeable with owner awareness because the regression test may not exercise the production target-invalidation path used during zone changes. Possibly related issues
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 |
|
Current-head CI hold for d79c5eb: Rust tests shard 4 failed in |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes
[HIGH] Preserve independent sequential siblings when a forward_result move yields no object. Evidence: crates/engine/src/game/effects/mod.rs:11208 returns from the whole subchain whenever ability.forward_result && forwarded_objects.is_empty(). forward_result is stamped on an instruction when a descendant needs its result; it does not mean every later instruction depends on that result. For example, Yawgmoth's Vile Offering is parsed as an optional graveyard-to-battlefield ChangeZone with forward_result, followed by the independently targeted Destroy, then self-exile. If the controller chooses no first target (or that move cannot occur) but retains a legal destroy target, this return suppresses the destroy and self-exile. Why it matters: CR 608.2b says an ability with at least one remaining legal target resolves normally and other applicable parts can still affect it; CR 608.2c requires its instructions to be followed in written order. Suggested fix: skip only the result-anchored dependent continuation(s), preserving independent sibling instructions, and add a runtime regression that declines/fails the first optional move while destroying a legal second target (and verifies Yawgmoth's Vile Offering is exiled).
![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/effects/mod.rs`:
- Around line 11208-11214: Update the forward-result handling around
ability.forward_result and forwarded_objects so a missing forwarded object
resolves the dependent rider as a no-op without terminating the continuation
chain. Preserve execution of subsequent SubAbilityLink::SequentialSibling
instructions, and add a regression covering this chain shape.
In `@crates/engine/tests/integration/issue_1515_emperor_of_bones.rs`:
- Around line 313-317: The no-result test must also verify that Emperor did not
gain haste. In the test around the delayed-trigger length assertion, add a
direct assertion against Emperor’s haste state while preserving the existing
delayed-trigger counter as the positive reach guard.
🪄 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: 6198db61-785d-4442-95a8-9b93cc7d4644
📒 Files selected for processing (2)
crates/engine/src/game/effects/mod.rscrates/engine/tests/integration/issue_1515_emperor_of_bones.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
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 380-385: Replace the direct zones::move_to_zone call for
graveyard_creature with the production replacement-aware apply or GameAction
path that emits ProposedEvent::ZoneChange before resolution. Preserve the test’s
intended move to Zone::Battlefield while ensuring replacement effects and normal
zone-change handling execute.
🪄 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: c368faa9-8c20-4561-8871-56827d406c0b
📒 Files selected for processing (2)
crates/engine/src/game/effects/mod.rscrates/engine/tests/integration/issue_1515_emperor_of_bones.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/effects/mod.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head is blocked by one resolver regression.
🔴 Blocker
[HIGH] Recursive missing-result detection suppresses an independent sequential instruction. resolve_chain_body returns from the entire chain when ability_chain_refs_parent_target(sub) finds a ParentTarget. That helper recursively traverses later sub_ability and else_ability branches, so it also matches a dependent rider after an independent sibling. This bypasses SubAbilityLink::SequentialSibling, whose contract is the next printed instruction, independent of its parent, and which must resolve even when an optional parent was declined.
For a forward-result move that yields no objects, followed by an independent sequential instruction and then a nested ParentTarget/delayed-trigger rider, the outer return skips the independent instruction. That violates the ordered-instruction rule (CR 608.2c) and contradicts the nearby claim that independent sequential siblings continue through the ordinary chain walker. The previous Yawgmoth-style direct independent tail is preserved, but it does not cover this later-nested dependency boundary.
Please model result dependency per continuation segment: no-op only the dependent continuation while continuing through SequentialSibling tails, and add a runtime regression with an empty forward result, an independent sibling, and a later nested dependent rider.
✅ Confirmed
I rechecked the prior empty-forward failure class and the current direct independent tail; the remaining issue is the new recursive look-ahead across that ordering boundary.
Recommendation: request changes; do not enqueue this shared resolver change until the sibling-order regression is covered.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head still has a sibling-order blocker.
🔴 Blocker
[HIGH] The new loop skips an independent sequential instruction whenever a later descendant is dependent. resolve_chain_body calls ability_chain_refs_parent_target(sibling) before deciding to resolve a SequentialSibling; that helper recursively traverses sub_ability and else_ability (lines 11574–11604). Consequently, an independent action followed by a later nested ParentTarget/delayed rider is skipped wholesale at lines 11222–11224. SequentialSibling is explicitly the next independent printed instruction and must resolve (ability.rs:19342–19346); CR 608.2c requires the instructions in written order.
The added Yawgmoth test only has independent Destroy/self-exile tails (issue_1515_emperor_of_bones.rs:331–405); neither tail carries a later ParentTarget, so it cannot exercise the new recursive branch.
Please traverse the continuation by segment: execute each independent sibling's own effect, suppress only the dependent continuation node, then resume scanning. Add a runtime regression with an empty forward result, an independent sequential action, and a subsequent nested ParentTarget/delayed rider; the independent action must occur while the rider remains a no-op.
✅ Confirmed
The direct Yawgmoth empty-forward regression is now covered and preserves its independent tail. The unresolved case is the later-dependent boundary introduced by this head.
Recommendation: request changes; do not enqueue this shared resolver change until that ordering boundary has discriminating coverage.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head resolves the prior sub-chain case, but still has one missing-result fallback path.
🔴 Blocker
[HIGH] A sequential sibling's dependent else_ability is executed instead of suppressed. The walker enters this path because ability_chain_refs_parent_target correctly recurses through else_ability, but at resolve_chain_body:11222–11234 the per-sibling check looks only at sibling.effect and clears only sub_ability. It then resolves the clone with its else_ability intact. If the sibling's own condition is false, a ParentTarget effect in that else branch runs with no forwarded object and can fall back to the original source.
This is the same ordered-instruction/referent defect through the conditional branch: the independent sibling's own effect should be allowed, while only the dependent branch is a no-op. CR 608.2c requires the written instructions in order; an absent antecedent must not retarget a later rider to the source.
The new synthetic regression covers an independent sibling followed by a dependent sub_ability, but has no conditional else_ability branch (issue_1515_emperor_of_bones.rs:405–495).
Please apply the same segment boundary to both continuation edges: when executing an independent sibling, retain only an else branch that is itself referent-independent (or selectively strip/suppress its dependent branch), and add a false-condition runtime regression with a ParentTarget else rider.
✅ Confirmed
The prior blocker is fixed for the sibling's own effect plus a later sub_ability: the walker now executes independent segments and the new regression asserts that shape. CI, the current no-parse-change artifact, and security scan are green.
Recommendation: request changes; do not enqueue until the conditional dependent branch is handled and covered.
Resolve maintainer-caused import churn while preserving the current resolver behavior and regression coverage. Co-authored-by: traemyn <traemyn@gmail.com>
|
Maintainer port completed for current head This head is held only for its fresh required CI and the current-head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head resolver behavior and regression coverage are complete. The maintainer port is limited to main’s import churn; it preserves the reviewed missing-forward-result segment pruning and its conditional else_ability regression. Current required checks and the current-head parse-diff artifact are green.
2 participants
Summary
Fixes Emperor of Bones so Adapt places counters without self-sacrificing when no creature was exiled, binds Haste and delayed Sacrifice to the returned creature, and preserves independent sequential instructions when an empty
forward_resultmove has later dependent riders.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 fmt --all- exit 0.cargo clippy -p phase-engine --all-targets -- -D warnings- exit 0.cargo test -p phase-engine- 4,855 passed, 0 failed, 2 ignored.cargo test -p phase-engine --test integration issue_1515_emperor_of_bones -- --nocapture- 9 passed, 0 failed.git diff --check- exit 0.Pipeline-reviewed head: 3d5c109
Current branch head: 3d5c109
Pipeline status: current
Current-head review: clean at 3d5c109
Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=3d5c109fb08276f77b8849fd3840da1d00d078de base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
Final review-impl
Final review-impl PASS head=3d5c109fb08276f77b8849fd3840da1d00d078de
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.