fix(engine): preserve beginning combat priority - #7080
Conversation
|
Warning Review limit reached
Next review available in: 25 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)
📝 WalkthroughWalkthroughThe engine now initializes combat before trigger processing and always grants active-player priority during ChangesBeginCombat priority flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GameRunner
participant PhaseInterpreter
participant ActivePlayer
participant DeclareAttackers
GameRunner->>PhaseInterpreter: enter BeginCombat
PhaseInterpreter->>PhaseInterpreter: initialize combat state
PhaseInterpreter->>ActivePlayer: request priority
GameRunner->>ActivePlayer: pass priority twice
ActivePlayer->>DeclareAttackers: submit empty attacker declaration
DeclareAttackers-->>PhaseInterpreter: advance to PostCombatMain
Possibly related PRs
🚥 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 |
![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
🤖 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/turns.rs`:
- Around line 2859-2877: Update the auto_advance_skips_combat_phases test to
reflect the new BeginCombat priority window: after the first auto_advance call,
submit the active player’s priority response, then submit the empty attacker
declaration before asserting Phase::PostCombatMain. Preserve the test’s existing
empty-combat scenario and use the established turn/priority submission helpers.
🪄 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: cb16744f-8266-4800-9ddc-7dd6217a03e9
📒 Files selected for processing (4)
crates/engine/src/game/engine_auto_pass_decision_tests.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/turns.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: 3
🤖 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/scenario.rs`:
- Around line 1318-1324: Update the implicit pass block in the scenario action
handling around GameAction::DeclareAttackers to require
self.state.stack.is_empty() in addition to the existing BeginCombat and Priority
conditions, so hidden passes occur only with an empty beginning-of-combat stack.
- Around line 1318-1324: Update the BeginCombat handling around
GameAction::DeclareAttackers to use the engine’s topology-aware priority-pass
sequence instead of applying exactly two PassPriority actions; otherwise
restrict this shorthand to two-player scenarios. Preserve successful
DeclareAttackers setup for multiplayer states and add a regression test covering
a three-player combat.
- Around line 1322-1325: Update the implicit-pass handling in GameRunner::act to
retain each apply_as_current result, accumulate their emitted events in order,
and prepend them to the ActionResult returned for the explicit action. Preserve
error propagation and ensure events from both GameAction::PassPriority calls,
including phase or stack events, are included.
🪄 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: ac0f94db-e128-43c9-a2fb-d67be70e1be0
📒 Files selected for processing (3)
crates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/scenario.rscrates/engine/src/game/turns.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/game/turns.rs
- crates/engine/src/game/engine_phase_trigger_regression_tests.rs
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_3279_song_of_dryads.rs`:
- Around line 97-98: Update the integration test around the duplicated
runner.pass_both_players() calls to first run an unenchanted Obuun control
through the same priority passes and assert it reaches
WaitingFor::TriggerTargetSelection or WaitingFor::OrderTriggers. Keep the Song
of the Dryads scenario’s existing assertion that no trigger prompt remains,
using the control assertion as its 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: 577ce2e3-6ef3-438f-93d1-0ec79e7e870e
📒 Files selected for processing (8)
crates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/scenario.rscrates/engine/src/game/turns.rscrates/engine/tests/integration/engine_invariants.rscrates/engine/tests/integration/export_runtime_canaries.rscrates/engine/tests/integration/issue_3279_song_of_dryads.rscrates/engine/tests/integration/json_smoke_test.rscrates/engine/tests/integration/loop_shortcut.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/game/scenario.rs
- crates/engine/src/game/turns.rs
- crates/engine/src/game/engine_phase_trigger_regression_tests.rs
1 participant
Summary
Preserves the rules-required Beginning of Combat priority window before attackers are declared, including phase stops, trigger prompts, extra combat, and controlled-combat priority semantics.
Validation failures
The engine-implementer baseline projection could not complete because the detached base revision has unrelated engine compilation failures (zone-pipeline and exhaustive-match errors). Formatting, diff checks, and pre-commit gates passed; CI/merge queue will validate this branch.
Pipeline-reviewed head: 9077f8c46a64a17c5a6be066200ede445caa36c9
Current branch head: 20f479f54052d427cac0d2223783899db08b678a
Pipeline status: historical — shipped despite baseline measurement CANNOT_ANSWER
Current-head review: none
Summary by CodeRabbit
New Features
Bug Fixes