fix(parser): Final Fortune loses at the extra turn's end step, not this one (#4231)

[rykerwilliams]

Closes #4231.

The bug

Final Fortune:

Take an extra turn after this one. At the beginning of that turn's end step, you lose the game.

This reproduces on current main. Cast on your own turn, the delayed loss fires at this turn's end step, so the game ends (GameOver { winner: P1 }) before the extra turn is ever taken. Cast at instant speed on an opponent's turn, it happened to work, because that end step isn't the caster's.

Root cause

Both temporal recognizers in crates/engine/src/parser/oracle_effect/lower.rs map "at the beginning of that turn's end step" to AtNextPhaseForPlayer { phase: End, gate: TurnGate::None, binding: Controller }:

• the prefix form, in strip_temporal_prefix;
• the suffix form, in the strip_temporal_suffix table.

Their comments called this "identical to the 'your next end step' arm". It isn't. "Your next end step" may be the current turn's. "That turn" is the extra turn from the first sentence, which CR 500.7 adds after the current turn, so the current turn's end step must never fire it. With no turn floor, the matcher fired at the first end step where the caster was the active player.

The fix

Both arms now use the existing TurnGate::AfterCreationTurn, which is already the floor for the Kav Landseeker "the end step on your next turn" arm next to them. It is stamped to After(creation_turn) in effects::delayed_trigger::resolve, and the AtNextPhaseForPlayer matcher in game/triggers.rs skips every matching phase up to and including that turn.

It is a parser-only change: no new types, fields, or variants, and no runtime change.

Class

Scryfall lists four printed cards with exactly this sentence after an extra turn: Final Fortune, Last Chance, Warrior's Oath, Chance for Glory. Alchemist's Gambit carries the same sentence inside its cleave brackets. Oracle en-Vec also says "that turn's end step", but its anaphor is "that player's next turn". The parse-diff below will show whether it reaches this arm.

Tests

crates/engine/tests/integration/issue_4231_final_fortune_loses_at_extra_turns_end_step.rs drives the real cast and priority loop (GameRunner::cast(..).resolve(), then PassPriority through each step). It uses Final Fortune's verbatim Oracle text.

Reach-guards run before any timing assertion: exactly one extra turn is queued and exactly one delayed trigger is scheduled.

• final_fortune_cast_on_your_turn_loses_at_the_extra_turns_end_step_not_this_one: the caster must still be in the game once the casting turn is over. The next turn must be theirs (turn_number + 1), and they must lose, with P1 winning, at that turn's end step.
• final_fortune_cast_on_an_opponents_turn_loses_at_the_extra_turns_end_step: the instant-speed sibling. It already passed before the fix and pins that the gate doesn't delay the loss past the caster's extra turn.

The existing unit test that_turns_end_step_temporal_resolves_to_controller_next_end_step now expects AfterCreationTurn from both recognizers.

Revert-proof: with lower.rs reverted to upstream/main, the own-turn test fails with Final Fortune's caster must still be in the game after the turn Final Fortune was cast in; waiting_for = GameOver { winner: Some(PlayerId(1)) }.

Verification

All results are on the committed head e33d78a4a:

• Targeted integration: 127 passed, 0 failed. This covers both new tests plus the delayed-trigger, extra-turn (Nexus of Fate, Taking an extra turn out of sequence play doesn't go back. — I took a turn out of sequence using [[Rise of Eldrazi]] an… #6416 resume order), Kav Landseeker, and Stranglehold suites.
• Targeted lib: 129 passed, 0 failed. This includes the updated that_turns_end_step_temporal_resolves_to_controller_next_end_step test and extra_turn_then_lose_parses_delayed_lose_the_game.
• cargo test -p phase-engine --lib: 21381 passed, 0 failed (8 ignored).
• Full integration suite: CI's Rust tests job passed on e33d78a4a. Locally, two full runs were cut off by the 580s cap on a loaded machine, and a four-shard run completed two shards: 3571 passed, 0 failed (1743 + 1828). The other two shards (3484 tests) hit the cap while a concurrent build held the CPU. I'm not claiming a local full pass; CI's is the complete one.
• Parse-diff: no card-parse changes, as expected for a gate-field change inside an existing delayed-trigger condition. The coverage status of the four cards is unchanged; only when the loss fires changes.
• cargo fmt --all is clean. Clippy was not run locally; CI's lint job owns it.
• CR citations were checked against docs/MagicCompRules.txt: 500.7, 603.7a, 104.3e, 614.10a.

Residuals, stated rather than hidden

Both of these are equally wrong on main today; this PR neither causes nor fixes them. Fixing either properly needs a delayed trigger bound to a specific extra turn, and extra turns carry no identity today.

• A skipped extra turn (Stranglehold, via the BeginTurn replacement): the ruling and CR 614.10a say you don't lose, but the loss now waits for your next natural turn's end step.
• A second extra turn for the same player granted later in the same turn: per CR 500.7 the most recent is taken first, so the loss fires at the end of that earlier-taken turn, one turn early.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 7bbf53c7-a946-4786-9d62-5877897e918b

📥 Commits

Reviewing files that changed from the base of the PR and between 9d919c6 and e33d78a.

📒 Files selected for processing (3)

• crates/engine/src/parser/oracle_effect/lower.rs
• crates/engine/tests/integration/issue_4231_final_fortune_loses_at_extra_turns_end_step.rs
• crates/engine/tests/integration/main.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The parser now delays “that turn’s end step” triggers until after the creation turn. New integration tests verify Final Fortune resolves its loss at the granted extra turn’s end step.

Changes

Final Fortune timing

Layer / File(s)	Summary
Delayed trigger gate crates/engine/src/parser/oracle_effect/lower.rs	Both temporal parser forms now use TurnGate::AfterCreationTurn. The unit test expects the updated gate.
Integration test harness crates/engine/tests/integration/issue_4231_final_fortune_loses_at_extra_turns_end_step.rs	The new test defines Final Fortune, builds the game state, advances priority, and checks survival and game-over results.
Extra-turn timing scenarios crates/engine/tests/integration/issue_4231_final_fortune_loses_at_extra_turns_end_step.rs, crates/engine/tests/integration/main.rs	Tests cover casting on the caster’s turn and on an opponent’s turn. The integration module is registered.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to e33d7

The delayed-loss timing fix is covered for casts during either player’s turn, with no concrete unresolved merge risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue #4231. Both strip_temporal_suffix and strip_temporal_prefix now use TurnGate::AfterCreationTurn for “that turn’s end step”. This skips the casting turn and schedules th…
Out of Scope Changes check	✅ Passed	The changed parser logic, unit-fixture update, integration tests, and test-module registration directly support issue #4231. No unrelated runtime types, fields, variants, or behavior are introduced.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: Final Fortune now causes the loss trigger at the extra turn's end step instead of the casting turn's end step.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 too large.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[matthewevans]

Changes requested

This fixes the immediate-turn timing, but the representation is still not rules-correct for that specific extra turn.

1. lower.rs:6853-6866 and lower.rs:7044-7061 select Controller + AfterCreationTurn, but delayed_trigger.rs:235-265 persists only After(turn_number) and triggers.rs:13175-13192 matches any later controller end step. It must bind the delayed loss to the actual enqueued extra turn, or leave this form unimplemented until that provenance exists. In particular, add runtime coverage for a skipped extra turn and a reordered/LIFO extra-turn queue; the present tests cover only one ordinary extra turn.

2. The generic syntax arms have no antecedent discriminator but force Controller. Oracle en-Vec uses the same phrase after "that player's next turn," where the referent is the target player's turn rather than the controller's extra turn. Carry the antecedent authority through lowering, or reject/preserve the non-extra-turn form, with a discriminating parser/runtime test.

Required CI and the exact-head parse-diff receipt are still pending/missing. Those are merge gates after the correctness fixes, not the basis of these requested changes.

[github-actions]

Generated for head e33d78a4aa81d0eb1dd4d79afe309ab790856019.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[rykerwilliams]

Both review points are correct, and I'm handing the design decision back rather than pushing a fix, because the rules-correct version turns out to reach the wire protocol. Everything below is measured; file:line references are at e33d78a4a unless noted.

Why "bind it to the actual enqueued extra turn" is not a small change

ExtraTurn { player, anchor } (types/game_state.rs:21623) carries no identity, and select_next_turn_after_completion (turns.rs:1168-1190) pops the entry and returns only (PlayerId, bool) — the entry is dropped. So nothing downstream can name the turn that was just granted. Making the delayed loss name it requires, at minimum:

1. An id on ExtraTurn, an allocator, a per-resolution ledger so the CreateDelayedTrigger clause can read what the ExtraTurn clause just minted, and current_extra_turn stamped in start_next_turn after both skip paths (turns.rs:1416-1419 counter, turns.rs:1428-1431 BeginTurn Prevented).
2. A turn selector on TurnGate. I ran the add-engine-variant gate: a fourth sibling is refused (it would silently turn a floor enum into a mixed floor/selector), and the approved shape is a symbolic parse-time value rewritten at creation into a concrete ExactExtraTurn(id), mirroring AfterCreationTurn → After(turn_number).
3. A CR 614.10a purge when the granted turn is skipped.
4. Antecedent discrimination in lowering, which cannot live in lower.rs: strip_temporal_prefix/strip_temporal_suffix are both (&str) -> (&str, Option<DelayedTriggerCondition>) with no chain context. It belongs at the two parse_effect_chain_ir seams (oracle_effect/mod.rs:38068, :38367), where builder.clauses() exposes each ClauseIr.parsed.effect.

The blocker that makes this your call, not mine

The id is wire-visible. #[serde(from = "ExtraTurnCompat")] governs deserialization only; ExtraTurn still derives Serialize over its own fields, and GameState.extra_turns has #[serde(default)] but no skip_serializing_if (types/game_state.rs:18339). Both fixtures/adapter-contract/game_started.json and state_update.json carry extra_turns, so any game with a queued extra turn emits a new key.

And compatibility here is exact-match: server-core/src/protocol.rs:32 sets MIN_SUPPORTED_PROTOCOL = PROTOCOL_VERSION, with the same pattern client-side. A bump therefore evicts every deployed full-game peer (plus a legacy lobby cohort), and scripts/check-protocol-version.mjs pins six surfaces plus test titles that must move in lockstep. Paying that cost to correct the timing of four or five printed cards is a judgement call that belongs to maintainers.

Other registration surfaces this touches (found in review, listed so they aren't rediscovered)

• State equality, not just serde: normalize_for_loop (types/game_state.rs:25587) zeroes every monotonic allocator for CR 104.4b loop comparison — a new allocator and a per-resolution ledger must be normalized there or identical positions stop comparing equal. Also _gamestate_partition_is_total (:26594) and the hand-written PartialEq (:27064).
• The second TurnGate surface: WheneverEventExpiry::UntilControllersNextTurn { after } (types/ability.rs:5155) shares the enum, and turns.rs:1800-1812 expires it with a matches! on After(..) only — new values are a silent non-match, no compile error.
• Coverage text: coverage.rs:2316-2328 keys on binding, not gate, so it would keep printing "at your next end step" for a gate naming a specific turn.
• projected_turn_order (turns.rs:1230-1318) re-implements both skip paths on a clone; a purge added to the live paths must be deliberately omitted there.
• GameEvent::ExtraTurnCreated is the natural carrier for the id, and that decision should precede a protocol bump rather than follow it.

A measured scope gap that the CR 614.10a purge would inherit

Stranglehold's "If an opponent would begin an extra turn" loses its subject in the engine: oracle_replacement.rs:13345 maps the phrase to ReplacementCondition::OnlyExtraTurn with no controller scope, replacement.rs:5286's begin_turn_matcher ignores source and state, and replacement.rs:6638 tests only is_extra_turn: true. So it currently skips anyone's extra turn, including its own controller's. A skip-driven purge would give that pre-existing bug a second consumer; it probably wants fixing first, and I'm happy to do that separately if useful.

A correction to my own record

I earlier measured "Chance for Glory lowers to zero abilities" and would have used it to shrink the class. That was a dead instrument, not a finding — the committed test extra_turn_then_lose_parses_delayed_lose_the_game (parser/oracle_effect/lower.rs:13678) parses that card's character-identical text and asserts both the ExtraTurn and the delayed LoseTheGame. My probe reported zero abilities and zero Unimplemented, which in this codebase means the parser was never reached. The class stands at five pending a correct re-probe, and Alchemist's Gambit needs probing on its verbatim text, since its clause sits inside cleave brackets that nothing in crates/ strips.

Where that leaves this PR

The commit here is a strict improvement — it stops the loss firing at the casting turn's end step — but it is not the representation you asked for, and it is wrong in the two ways you named: a skipped extra turn still kills the caster, and a LIFO-reordered queue fires it a turn early. Oracle en-Vec is confirmed as your second point: it already produces a controller-bound AtNextPhaseForPlayer while its "During that player's next turn" clause is Unimplemented.

Three ways forward, and I'll take whichever you pick:

1. Land this as an interim timing fix with the two residuals documented in the issue, and treat the specific-turn binding as separate work.
2. Leave the phrase unimplemented until turn provenance exists — your stated alternative. Coverage-honest, but it moves these cards from wrongly-timed to unsupported and contradicts the shipped changelog entry at client/public/changelog.json:1451.
3. Build the full provenance, protocol bump included, once you've decided the eviction is acceptable.

I have the full design, the gate verdict, and a verification matrix (skipped turn, LIFO reorder, zero-referent grant, non-controller grant, legacy-save decode, both serialized TurnGate surfaces) ready to go if you choose 3. Happy to close this PR instead if you prefer 2.

[matthewevans]

I re-read the follow-up on this head. The requested changes remain: the delayed-loss trigger must bind the specific granted extra turn and be discarded when that turn is skipped, and the generic temporal anaphor cannot force the controller without an antecedent authority. No code changed to resolve either issue, so this remains blocked.

[rykerwilliams]

Understood, and agreed on both points — I'm not disputing the requirements, and I've deliberately not pushed a partial fix.

To unblock it I need one decision from you, because it isn't mine to make:

Is a PROTOCOL_VERSION bump acceptable here? Binding the loss to the specific granted turn requires an identity on ExtraTurn, which is serialized (#[serde(from = ...)] governs decode only, and GameState.extra_turns has no skip_serializing_if) and rides state_update / game_started. Because MIN_SUPPORTED_PROTOCOL == PROTOCOL_VERSION (server-core/src/protocol.rs:32, mirrored client-side), bumping evicts every deployed full-game peer — to correct the timing of four or five printed cards.

• Yes, bump it → I implement the full provenance: identity + selector + CR 614.10a purge on both skip paths + the antecedent rule at the two parse_effect_chain_ir seams, with regressions for a skipped extra turn, a LIFO-reordered queue, a zero-referent grant, a non-controller grant, legacy-save decode, and both serialized TurnGate surfaces. The design and gate verdict are already done.
• No → I close this PR and lower the phrase to Effect::unimplemented until turn provenance exists, which is coverage-honest but moves these cards from wrongly-timed to unsupported and contradicts the shipped changelog entry at client/public/changelog.json:1451.

Either answer unblocks me immediately; I just don't want to spend a peer-evicting protocol bump on your behalf without you choosing it.

[matthewevans]

I’ve captured the protocol-version tradeoff. The current PR remains blocked; no protocol bump or coverage rollback is authorized in this sweep. Please leave the branch unchanged pending an explicit maintainer decision on the supported-wire compatibility cost.