Fix combat phase stops during auto-pass - #7308
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe client keeps attacker controls active during turn-boundary auto-pass. The engine now exposes priority after combat damage and during EndCombat, including after empty attacker declarations. Tests cover phase stops, inserted phases, combat transitions, and updated cascade timing. ChangesCombat priority and auto-pass
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change restores normal combat priority windows during auto-pass and keeps the relevant combat controls visible; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant advance_after_empty_attackers
participant EndCombat
participant active_player
participant PostCombatMain
advance_after_empty_attackers->>EndCombat: mark EndCombat
EndCombat->>active_player: expose priority
active_player->>EndCombat: pass priority
EndCombat->>PostCombatMain: tear down combat and advance
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 |
![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 3034-3039: The EndCombat transition currently clears combat state
and EndOfCombat effects before the CR 511.1 priority window. In
crates/engine/src/game/turns.rs:3034-3039, keep combat and EndOfCombat state
intact while returning AutoAdvanceStep::waiting for priority, then move combat
removal and EndOfCombat pruning to the transition out of Phase::EndCombat after
all players pass, with a verified CR citation. In
crates/engine/src/game/engine_phase_trigger_regression_tests.rs:170-192, add
production-path assertions that both remain active during EndCombat priority and
are cleared after transitioning to PostCombatMain.
🪄 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: e14ebeb8-a915-45bb-9f7e-d30748e9ae1a
📒 Files selected for processing (7)
client/src/components/board/ActionButton.tsxclient/src/components/board/__tests__/ActionButton.test.tsxcrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/turns.rscrates/engine/tests/integration/issue_3279_song_of_dryads.rscrates/engine/tests/integration/loop_shortcut.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — EndCombat teardown is occurring before the step's priority window.
🔴 Blocker
crates/engine/src/game/turns.rs:3013-3027 clears combat, the combat-attacker restriction, and every EndOfCombat expiry before turns.rs:3034-3039 returns WaitingFor::Priority. That makes the state visible during the EndCombat priority window inconsistent with the rules. Verified CR evidence:
- CR 511.1: “Once it begins, the active player gets priority.”
- CR 511.2: “Effects that last ‘until end of combat’ expire at the end of the combat phase.”
- CR 511.3: “As soon as the end of combat step ends, all creatures, battles, and planeswalkers are removed from combat.”
Move the normal-combat teardown to the transition out of Phase::EndCombat (the phase-transition authority around advance_phase_once), after all players pass. Keep it centralized with the existing expiry cleanup so combat, attacker restrictions, continuous effects, replacement definitions, and pending damage replacements have one rules-correct boundary.
crates/engine/src/game/engine_phase_trigger_regression_tests.rs:170-192 currently proves only that combat is absent after reaching PostCombatMain. Add a production-path regression that proves combat and an EndOfCombat-expiring effect/restriction are still present while priority is offered in EndCombat, then both are gone after the pass loop transitions to PostCombatMain.
Recommendation: request changes; preserve the priority windows, but defer EndCombat teardown until the step actually ends.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the End Combat teardown now runs only when leaving the step, after its priority window. The current head preserves combat state through priority and clears it on the transition to postcombat main.
2 participants
Summary
Fixes combat phase stops during auto-pass. Combat Damage and End Combat now expose their normal priority windows so configured stops can interrupt auto-pass, while the frontend keeps Declare Attackers and Declare Blockers controls visible when an auto-pass session is armed. End Combat state now remains live through its priority window and is torn down only when the step ends.
Files changed
Track
Developer
LLM
Model: GPT-5.6 Luna (via GitHub Copilot; canonical id not exposed)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
Note
Any change to
crates/engine/game logic — parser, effects, resolver, targeting, rules behavior — is expected to go through/engine-implementer.This change follows that engine implementation pipeline, including an independent blocker review and fix round.
CR references
CR 117.3a, CR 508.8, CR 510.4, CR 511.1, CR 511.2, CR 511.3, CR 724.2d, CR 724.2e.
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— passed on the merged head.cargo clippy --all-targets -- -D warnings— passed on the merged head.cargo test -p phase-engine— 18,901 unit tests passed, 4,851 integration tests passed, 0 failed; auxiliary test binaries passed (21 and 9 tests); doc-test harness completed with 7 ignored tests and 0 failures../scripts/gen-card-data.shwithMTGJSON_SKIP_REFRESH=1— completed on the merged head using the complete local MTGJSON cache; 35,798 faces indexed, 31,794 cards supported, 992 existing parser warnings across 3 categories.cargo coverage— 31,794 of 35,798 cards supported (88.815%).cargo semantic-audit— completed for 32,753 cards with the existing baseline of 266 findings; no parser files or card parse behavior changed in this PR.Merged-source frontend protocol/type-check — passed.
Merged-source ESLint — 0 errors and 30 existing warnings.
Merged-source Vitest — 297 test files passed, 3 skipped; 2,698 tests passed, 12 todo.
The initial PR CI failures were infrastructure-only: Rust shard 4 and Card data failed while downloading the
sccachebinary withsocket hang up, before compilation or tests. No source failure was reported. CI is rerunning for the updated head.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=08340f1b64018993a7fce0c9f82e74fa4ae5b368 base=dae3de6c1e95c71c872270510d4f75016dcb8662
Anchored on
Final review-impl
Final review-impl PASS head=08340f1b64018993a7fce0c9f82e74fa4ae5b368
The maintainer blocker was independently validated and fixed: normal End Combat teardown is centralized in
complete_end_combat_teardownand runs whenadvance_phase_onceleaves End Combat, while explicit combat skips reuse the same helper. The regression test proves combat and the attacker restriction remain live during End Combat priority and clear after the pass loop.Claimed parse impact
None.
Scope Expansion
None. The blocker fix remains within combat phase-stop and phase-transition behavior.
Validation Failures
None.
CI Failures
None unresolved. The prior failed CI jobs were external
sccachedownload/bootstrap failures, not code or test failures; the updated head has been pushed for a fresh CI run.Summary by CodeRabbit