fix(engine): resume turn order after specified turn for OOS extras (#6416)

[andriypolanski]

Summary

After an out-of-sequence extra turn (e.g. A casts Nexus of Fate / Rise of the Eldrazi during C's turn), play incorrectly resumed from the extra-turn taker (→ B) instead of the player next after the specified turn (→ D).

CR 500.7 inserts extra turns after the specified turn. The engine stored only Vec<PlayerId> (beneficiary), so start_next_turn advanced with next_turn_representative(extra_turn_player) once the queue drained.

This fix mirrors ExtraPhase: each queued entry carries { player, anchor }, and a latched extra_turn_sequence_anchor restores natural order after the specified turn when the LIFO queue empties. projected_turn_order uses the same helper.

Changes

• crates/engine/src/types/game_state.rs

  • ExtraTurn { player, anchor } with legacy serde (PlayerId → { player, anchor: player })
  • extra_turn_sequence_anchor: Option<PlayerId>
  • Totality-guard + PartialEq updated

• crates/engine/src/game/turns.rs

  • enqueue_extra_turn / select_next_turn_after_completion (shared by start_next_turn + projected_turn_order)
  • Unit: LIFO then resume D; Stranglehold/projection fixture updates

• crates/engine/src/game/effects/extra_turn.rs

  • Enqueue with anchor = state.active_player (normalized)

• crates/engine/tests/integration/issue_6416_extra_turn_resume_order.rs

  • 4p OOS: Extra A during C → resume D
  • In-sequence sibling: Extra A on A's turn → resume B

• issue_3323_nexus_of_fate_extra_turn.rs / power_up_keyword.rs / derived_views.rs

  • Shape migration + resume assertion after OOS Nexus

Test plan

• Unit: extra_turns_lifo_then_resume_specified_turn
• Unit: extra_turn_stores_active_player_as_anchor
• Unit: projected_turn_order_begin_turn_replacement_skips_extra_turn_cursor (anchor-aware)
• Unit: stranglehold_skips_extra_turn_not_normal_turn
• Integration: extra_turn_out_of_sequence_resumes_after_specified_turn
• Integration: extra_turn_on_own_turn_resumes_natural_next
• Integration: issue_3323 Nexus suite (incl. resume → P2)

CR annotations verified

• CR 500.7 — extra turns after the specified turn; LIFO
• CR 805.8 — shared-team-turn recipient normalization
• CR 101.4 — APNAP (referenced for multi-grant ordering; unchanged)

Out of scope

• Parser / Rise of the Eldrazi / Molecule Man (already parse correctly)
• Frontend (consumes projected_turn_order)
• linux-schema.json (unrelated Tauri regen — do not include)

Summary by CodeRabbit

• Bug Fixes
  • Improved extra-turn sequencing so play resumes at the correct resumption point when an extra turn is taken out of sequence, including nested cases.
  • Updated extra-turn handling to use a structured entry (beneficiary plus an anchor reference), ensuring consistent consumption order and correct targeted vs shared-team behavior.
  • Preserved backward compatibility when loading older saves that used the previous extra-turn representation.
• Tests
  • Added new integration coverage for Nexus of Fate extra-turn resume order (including out-of-sequence and multi-step progression).
  • Updated existing extra-turn assertions to match the new structured representation and ordering.

[superagent-security]

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Extra turns now carry beneficiary and anchor data. Turn selection centralizes enqueueing, LIFO consumption, and anchored resumption, with compatibility handling and expanded unit and integration coverage.

Changes

Anchored extra-turn sequencing

Layer / File(s)	Summary
Extra-turn state and compatibility contract crates/engine/src/types/game_state.rs	GameState stores structured extra turns, tracks sequence anchors, initializes and compares the new field, and deserializes legacy player-only entries.
Anchored enqueue and turn selection crates/engine/src/game/effects/extra_turn.rs, crates/engine/src/game/turns.rs	Extra-turn effects and turn transitions use centralized helpers for normalized anchored entries, LIFO selection, projected order, and resumption after the specified turn.
Turn-order behavior validation crates/engine/src/game/derived_views.rs, crates/engine/src/game/turns.rs, crates/engine/tests/integration/*	Tests cover structured queue assertions, wire round trips, ordering, team normalization, projected turns, and in-sequence or out-of-sequence Nexus of Fate resumption.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExtraTurnEffect
  participant GameState
  participant TurnSelection
  participant IntegrationTests
  ExtraTurnEffect->>GameState: enqueue ExtraTurn(player, active_player)
  TurnSelection->>GameState: pop queued extra turn
  TurnSelection->>GameState: resume from stored anchor
  TurnSelection->>IntegrationTests: expose next active player and queue state

Loading

Possibly related PRs

• phase-rs/phase#6549: Related turn-order display changes consume downstream engine turn-order metadata.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: resuming turn order after specified turns for out-of-sequence extra turns.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Changes requested — current head b64b9ac4b07c80b24f197bcf99e78d30222459d6

[HIGH] The current head fails required Rust CI. Evidence: Clippy rejects the GameState::default() then active_player reassignment in crates/engine/src/game/effects/extra_turn.rs:175-176; Rust shard 2 reports the new extra_turn_sequence_anchor write family is missing from the CR733 authority matrix. Why it matters: the branch cannot be merged and the CR733 gate is specifically protecting new state-write authority. Suggested fix: initialize GameState with active_player in the struct literal and regenerate/update the canonical, gzipped CR733 authority matrix with the new field and its authority.

[MED] The legacy saved-game decoder is untested. Evidence: ExtraTurnCompat::Legacy in crates/engine/src/types/game_state.rs:13910-13927 is the sole bare-PlayerId migration path; no test deserializes a persisted state with legacy extra_turns. Why it matters: the serialized state contract is changed but the compatibility fallback can silently regress. Suggested fix: add a persisted GameState/PersistedGameState JSON test for a bare entry, asserting anchor == player and the resulting turn selection.

[MED] Nested extra turns do not test the latch branch that preserves the outer anchor. Evidence: crates/engine/src/game/turns.rs:675-687 only sets extra_turn_sequence_anchor when absent, while current tests create all extras during C's original turn. Why it matters: an extra granted during A's extra turn must still resume after original C once the sequence drains. Suggested fix: add a production-pipeline C → extra A → (during A) extra B → D case that fails if the is_none() latch guard is removed.

[matthewevans]

Changes requested — current head 01b90274ea9c9a8f671d99c65a9c87361e9f1413

The Clippy initializer issue from the prior review is fixed. The remaining blockers are below; the required Rust jobs are still pending.

[HIGH] The CR733 authority matrix still omits the new extra_turn_sequence_anchor write family. Evidence: a fresh scripts/cr733_mutation_census.py run finds reachable writes at crates/engine/src/game/turns.rs:679 and :683, but crates/engine/tests/fixtures/cr733/authority_matrix.json.gz contains no row for that field. Why it matters: the current head will repeat the prior Rust shard-2 failure and leaves a new game-state write outside the CR733 authority contract. Suggested fix: regenerate/update the canonical gzipped authority matrix with exactly one mapped row for extra_turn_sequence_anchor and its intended authority.

[MED] The legacy saved-game decoder remains untested. Evidence: ExtraTurnCompat::Legacy in crates/engine/src/types/game_state.rs:13915-13927 is still the sole bare-PlayerId migration path, and the current diff adds no test that deserializes persisted extra_turns in the legacy shape. Why it matters: a serialized state compatibility path can regress while normal new-format tests stay green. Suggested fix: deserialize a legacy GameState or PersistedGameState value with a bare extra-turn entry, assert anchor == player, then drive turn selection to prove the preserved in-sequence behavior.

[MED] The nested extra-turn anchor latch is still not covered through the production pipeline. Evidence: crates/engine/src/game/turns.rs:675-687 retains the is_none() latch guard, while the new/current tests only grant extras during the original C turn. Why it matters: an extra granted during A's extra turn must retain the outer C anchor when the queue drains. Suggested fix: add a C → extra A → (during A) extra B → D scenario that fails if the latch guard is removed.

Evidence freshness: the parse-diff sticky comment reports no_changes but was last updated at 21:31:33Z, before this head was committed at 21:55:14Z. The only new-head delta is non-parser extra_turn.rs, so that result is consistent with the diff, but it is not current-head parse-diff evidence; refresh it with the next CI/current-head update.

[matthewevans]

Changes requested — current head 605f5bb6757086fe728552782cc44aa941fb088a

This head includes the one permitted maintainer merge of current main to address the parse-diff baseline-pending condition. The merge is clean, but it does not resolve the existing substantive blockers.

[HIGH] The CR733 authority matrix still omits the new extra_turn_sequence_anchor write family. Evidence: the current fixture crates/engine/tests/fixtures/cr733/authority_matrix.json.gz has no row for the field, while crates/engine/src/game/turns.rs:678-683 writes it in the live turn-selection path. The prior current-head CI run failed Rust shard 2 with fresh CR733 write-family fields missing from the authority matrix: ["extra_turn_sequence_anchor"]. Why it matters: the new state write remains outside the CR733 authority contract and fails required CI. Suggested fix: regenerate/update the canonical gzipped authority matrix with exactly one appropriate authority row for this field.

[MED] The legacy saved-game decoder still lacks a regression test. Evidence: ExtraTurnCompat::Legacy in crates/engine/src/types/game_state.rs:13915-13927 remains the sole bare-PlayerId migration path, and no current test deserializes a legacy persisted extra_turns entry then exercises turn selection. Why it matters: compatibility can regress independently of new-format behavior. Suggested fix: deserialize a legacy GameState or PersistedGameState, assert anchor == player, and prove the following turn selection.

[MED] The nested outer-anchor latch remains untested through the production pipeline. Evidence: crates/engine/src/game/turns.rs:678-683 preserves the first anchor with is_none(), but the current tests still grant every queued extra during original C. Why it matters: an extra granted during A's extra turn must continue the original C sequence and resume at D after B. Suggested fix: add C → extra A → (during A) extra B → D coverage that fails if the latch guard is removed.

Evidence status: the parse-diff sticky comment is currently Baseline pending for the pre-update main snapshot. This merge is the handler's single baseline-pending recovery attempt; wait for the new CI publication rather than creating another merge-main churn commit.

[matthewevans]

Changes requested — current head 3cccc913ee516bdff8813a9472c35aca0cc80e5b

[MED] The nested-anchor regression bypasses the production resolver. Evidence: crates/engine/src/game/turns.rs:7748-7783 queues extra turns directly via enqueue_extra_turn; it does not exercise crates/engine/src/game/effects/extra_turn.rs:21-40 through GameRunner, unlike crates/engine/tests/integration/issue_6416_extra_turn_resume_order.rs:55-112. Why it matters: a stale/latching anchor forwarded by the resolver can still yield wrong nested C→A→B→D order while the direct test passes. Suggested fix: add a GameScenario test resolving an extra turn during C, then another during A, asserting B then D.

The prior CR733 authority-matrix and legacy serde blockers were independently confirmed fixed. The existing bug type label remains correct. Do not enqueue until this production-pipeline regression is covered.

[matthewevans]

Current-head maintainer review is clean. The production-path C→A→B→D regression now proves the nested anchor/latch resume order; the current parser artifact has no changes. The Contributor trust ACTION_REQUIRED check is not a protected required status check.