fix(engine): retain batch delivery on reveal continuation cleanup - #7275
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (1)
📝 WalkthroughWalkthroughThe resolution stack now removes an active ability continuation directly or beneath an adjacent batch-delivery child. Game-state exposes this behavior, and accepted or declined reveal choices use the new cleanup path. ChangesContinuation cleanup
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 docstrings
🧪 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 |
a635584 to
f9323a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/engine_resolution_choices.rs (1)
3705-3712: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoute
CastFromZonecontinuation consumption through the batch-aware helper.
open_private_zone_cast_selectioncan place the continuation below an activeBatchDelivery. The declined and selectedCastFromZonebranches still calltake_active_ability_continuation().expect(...), which can panic whenBatchDeliveryowns the stack top.Attachdoes not use this insertion path.🤖 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/engine_resolution_choices.rs` around lines 3705 - 3712, Update both selected and declined CastFromZone branches in open_private_zone_cast_selection to consume the continuation through clear_active_ability_continuation_or_batch_delivery_child instead of take_active_ability_continuation. Preserve the existing expect-based failure handling while supporting continuations nested beneath an active BatchDelivery.
🧹 Nitpick comments (1)
crates/engine/src/types/resolution.rs (1)
3998-4054: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a BatchDelivery whose parent is not an AbilityContinuation.
The new test covers two cases: the direct batch-child relation succeeds, and an unrelated top frame (
ChangeZone, notBatchDelivery) is rejected. It does not cover the case whereBatchDeliveryowns the stack top but the frame directly below it is something other thanAbilityContinuation(for exampleChangeZonebelowBatchDelivery). The implementation's own doc comment states that "missing or unrelated parent relationships return no continuation," so this path needs its own assertion that the stack is left unmodified andOk(None)is returned.Based on learnings, CLAUDE.md requires focused unit tests for "stack ordering, frame preservation, direct continuation removal, and rejection of unrelated/non-adjacent frames." The unrelated-parent-below-BatchDelivery case is not yet exercised.
🧪 Proposed additional test case
let mut unrelated_child = ResolutionStack::default(); unrelated_child.push_inner(continuation_frame(2)); unrelated_child.push_inner(change_zone_frame(2)); assert!( matches!( unrelated_child.take_active_ability_continuation_or_batch_delivery_child(), Err(ResolutionStackError::UnexpectedTop { expected: FrameKind::AbilityContinuation, actual: FrameKind::ChangeZone, }) ), "the helper does not search through an arbitrary child frame" ); + + let mut unrelated_parent = ResolutionStack::default(); + unrelated_parent.push_inner(change_zone_frame(3)); + unrelated_parent.push_inner(batch_delivery_frame(3)); + let before = unrelated_parent.clone(); + assert_eq!( + unrelated_parent.take_active_ability_continuation_or_batch_delivery_child(), + Ok(None), + "a BatchDelivery child with a non-continuation parent has nothing to consume" + ); + assert_eq!( + unrelated_parent, before, + "an unrelated parent leaves the stack untouched" + ); }🤖 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/types/resolution.rs` around lines 3998 - 4054, Extend take_continuation_preserves_an_active_batch_delivery_child with a case where BatchDelivery is on top of a ChangeZone frame rather than an AbilityContinuation. Assert take_active_ability_continuation_or_batch_delivery_child returns Ok(None), preserves both frames and their ordering, and does not remove or mutate the stack.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 3705-3712: Update both selected and declined CastFromZone branches
in open_private_zone_cast_selection to consume the continuation through
clear_active_ability_continuation_or_batch_delivery_child instead of
take_active_ability_continuation. Preserve the existing expect-based failure
handling while supporting continuations nested beneath an active BatchDelivery.
---
Nitpick comments:
In `@crates/engine/src/types/resolution.rs`:
- Around line 3998-4054: Extend
take_continuation_preserves_an_active_batch_delivery_child with a case where
BatchDelivery is on top of a ChangeZone frame rather than an
AbilityContinuation. Assert
take_active_ability_continuation_or_batch_delivery_child returns Ok(None),
preserves both frames and their ordering, and does not remove or mutate the
stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 709a558c-5a4e-4a51-8aab-b59893986f93
📒 Files selected for processing (3)
crates/engine/src/game/engine_resolution_choices.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolution.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Summary by CodeRabbit