fix(engine): settle resumed change zone members - #7268
Conversation
|
Warning Review limit reached
Next review available in: 4 seconds 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 (4)
📝 WalkthroughWalkthroughThe change centralizes terminal completion resolution for paused zone-change deliveries. Explicit outcomes take precedence over inferred outcomes. Resumed deliveries always record the resolved completion, and pipeline processing uses the shared classifier. ChangesZone-change completion resolution
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 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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
c0c60b5 to
07c0785
Compare
07c0785 to
e9e204d
Compare
![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/types/game_state.rs`:
- Around line 3449-3483: The completion classifier in
terminal_completion_after_resume and completion_from_delivery_events needs a
verified CR annotation. Add a concise “CR 400.7” comment documenting the
new-object boundary for zone changes and stating that the retained delivery
event slice must be used because current object state cannot identify this
delivery; do not alter the classification logic.
- Around line 22748-22775: Extend the focused integration tests under the
existing integration module registration to exercise the real replacement-pause
resume flow through drain_pending_change_zone_iteration, rather than
constructing PendingZoneChangeDelivery directly. Cover all three resumed
outcomes—Moved, Prevented, and Remained—while preserving the existing terminal
outcome assertions and using the actual retained-event and replacement-pause
path.
🪄 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: f28abc73-406b-40f9-aaa7-703dfe0846bc
📒 Files selected for processing (4)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rs
| #[test] | ||
| fn paused_zone_change_delivery_derives_a_terminal_outcome_after_resume() { | ||
| let member = ObjectIncarnationRef::of(ObjectId(71), 4); | ||
| let mut delivery = PendingZoneChangeDelivery::new( | ||
| member, | ||
| ProposedEvent::zone_change( | ||
| member.object_id, | ||
| Zone::Battlefield, | ||
| Zone::Graveyard, | ||
| Some(ObjectId(72)), | ||
| ), | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| delivery.terminal_completion_after_resume(), | ||
| ZoneMoveCompletion::Remained, | ||
| "an answered pause with no original-incarnation event completed without moving" | ||
| ); | ||
|
|
||
| delivery | ||
| .record_terminal_completion(ZoneMoveCompletion::Prevented) | ||
| .expect("the explicit replacement outcome is recorded once"); | ||
| assert_eq!( | ||
| delivery.terminal_completion_after_resume(), | ||
| ZoneMoveCompletion::Prevented, | ||
| "an explicit replacement outcome remains authoritative over slice inference" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add an end-to-end integration test for resumed completion.
This test constructs PendingZoneChangeDelivery directly. It does not execute replacement pause, retained-event capture, and drain_pending_change_zone_iteration. Add an integration test that validates Moved, Prevented, and Remained through the real resume path.
As per path instructions, “Add or extend focused engine tests in crates/engine/tests/integration/ with the existing integration module registration.”
🤖 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/game_state.rs` around lines 22748 - 22775, Extend the
focused integration tests under the existing integration module registration to
exercise the real replacement-pause resume flow through
drain_pending_change_zone_iteration, rather than constructing
PendingZoneChangeDelivery directly. Cover all three resumed outcomes—Moved,
Prevented, and Remained—while preserving the existing terminal outcome
assertions and using the actual retained-event and replacement-pause path.
Source: Path instructions
…e-resumed-change-zone-members # Conflicts: # crates/engine/src/game/engine.rs
|
Current-head review follow-up for
No further change is warranted on this head. |
1 participant
Summary by CodeRabbit