fix(engine): settle cleanup discard triggers - #7120
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes 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)
📝 WalkthroughWalkthroughCleanup discard handling now uses the standard post-action SBA and trigger pipeline before phase advancement. New integration tests cover trigger settlement, batching, delayed triggers, ordering, priority, replacement choices, and normal cleanup progression. ChangesCleanup discard trigger pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupDiscardHandler
participant PostActionPipeline
participant WaitingState
participant PhaseAdvancement
CleanupDiscardHandler->>WaitingState: synchronize provisional priority
CleanupDiscardHandler->>PostActionPipeline: settle discard events
PostActionPipeline->>WaitingState: synchronize resulting waiting state
CleanupDiscardHandler->>PhaseAdvancement: advance when priority is restored and stack is empty
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🧹 Nitpick comments (2)
crates/engine/tests/integration/cleanup_discard_trigger_pipeline.rs (1)
337-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the trigger ordering so the test proves settlement, not only the pause.
The test stops at the
OrderTriggerspause. It never submits the ordering. The half that the fix changes — both triggers reach the stack, and cleanup does not advance until they resolve — stays unverified. A regression that drops the second trigger after ordering still passes this test.Add the ordering submission and assert both Magmakin entries reach the stack, then assert the phase is still
Cleanup.Line 343 (
assert_eq!(artillerists.len(), 2)) asserts a fixture value, not engine behavior. Remove it or replace it with a stack-content assertion.As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline."
🤖 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/cleanup_discard_trigger_pipeline.rs` around lines 337 - 343, Extend the test after the OrderTriggers pause by submitting the trigger ordering through the production pipeline, then assert both Magmakin trigger entries are present on the stack and the phase remains Cleanup. Remove the fixture-only artillerists.len() assertion and replace it with stack-content validation that would fail if the second trigger were dropped.Source: Path instructions
crates/engine/src/game/engine_resolution_choices.rs (1)
4466-4480: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRename the pipeline flags or replace them with a typed options enum.
At this settlement call site, the two positional
falsearguments are not named. Passingfalse, falsedepends on the currentskip_trigger_scan, skip_deferred_trigger_drainorder, which is not obvious fromengine_resolution_choices.rsalone. Add a short inline comment naming them, or pass a typed post-action options struct.🤖 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 4466 - 4480, The settlement call to engine_priority::run_post_action_pipeline_from currently uses ambiguous positional false flags. Add a concise inline comment identifying each argument as skip_trigger_scan and skip_deferred_trigger_drain, or replace the booleans with the existing typed post-action options mechanism if available; preserve both flags as disabled.
🤖 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/cleanup_discard_trigger_pipeline.rs`:
- Around line 414-448: Update the cleanup-discard replacement test around
setup_cleanup_discard to create at least one discard observer, such as a
Magmakin, while preserving the replacement setup. Add a paired positive
reach-guard confirming that the observer is present or otherwise reached the
settlement pipeline, so the stack-empty negative assertion can fail if the early
return in engine_resolution_choices is removed.
---
Nitpick comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 4466-4480: The settlement call to
engine_priority::run_post_action_pipeline_from currently uses ambiguous
positional false flags. Add a concise inline comment identifying each argument
as skip_trigger_scan and skip_deferred_trigger_drain, or replace the booleans
with the existing typed post-action options mechanism if available; preserve
both flags as disabled.
In `@crates/engine/tests/integration/cleanup_discard_trigger_pipeline.rs`:
- Around line 337-343: Extend the test after the OrderTriggers pause by
submitting the trigger ordering through the production pipeline, then assert
both Magmakin trigger entries are present on the stack and the phase remains
Cleanup. Remove the fixture-only artillerists.len() assertion and replace it
with stack-content validation that would fail if the second trigger were
dropped.
🪄 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: 5dd07d89-a2f2-4e3f-b7ca-ab4e538dc335
📒 Files selected for processing (3)
crates/engine/src/game/engine_resolution_choices.rscrates/engine/tests/integration/cleanup_discard_trigger_pipeline.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 0 card(s), 0 signature(s) (baseline: main
|
1 participant
Summary by CodeRabbit
Bug Fixes
Tests