fix(engine): anchor "after this phase" to the resolving phase (#7240)

[mike-theDude]

Closes #7240

Model: claude-opus-5[1m]
Tier: Frontier
Thinking: High

The bug

Overpowering Attack cast in a postcombat main untapped its creatures but added no combat phase. The parser selected Effect::AdditionalPhase's after anchor by scanning for the literal "after this main phase", so the bare "after this phase" grammar fell through to Phase::EndCombat — an anchor already in the past when the sorcery resolves in a postcombat main. advance_phase_once never matched it and the entry was cleared at end of turn.

CR 500.8 inserts an extra phase directly after the specified phase; CR 608.2c binds "this phase" at resolution. The parser now emits a resolution-time sentinel for every "after this [main|combat] phase" form, and the resolver maps it via last_step_of_phase(state.phase).

Why the diff is larger than a parser patch

Fixing the anchor exposed that advance_phase_once replaced the natural successor with the inserted phase rather than inserting before it. That was invisible because anchor-successor and inserted-terminal-successor coincide for every anchor previously in use, and diverge only for main-phase anchors — exactly this issue's class:

anchor	next_phase(anchor)	next_phase(terminal of inserted)
EndCombat	PostCombatMain	PostCombatMain	coincide
Upkeep / End / Untap	—	—	coincide
PreCombatMain	BeginCombat	PostCombatMain	diverge
PostCombatMain	End	PostCombatMain	diverge

Evidence this was already shipping broken, not hypothetical:

• Full Throttle already took the PreCombatMain path and already lost its natural combat. The bundle_anchor EndCombat re-anchor existed only to paper over this.
• Relentless Assault cast in a precombat main: the extra combat ate the natural combat, and the follow-up-main entry was never consumed (rposition(anchor == PostCombatMain) never matches anchor == PreCombatMain) — orphaned, then cleared at the turn boundary.

GameState.extra_phase_resume is therefore generalized from inserted beginning phases only to all inserted phases (Vec<Phase> → Vec<ExtraPhaseResume { anchor, inserted }>), so the turn resumes at the anchor's own successor.

Defects fixed that this change would otherwise have introduced

Each was found by review and is pinned by a revert-failing test.

1. CR 500.8 — the "after this main phase" class must create nothing outside a main phase. Gatherer is explicit (Fury of the Horde: "If it's somehow not a main phase when Fury of the Horde resolves, all it does is untap all creatures that attacked that turn. No new phases are created."; Relentless Assault: "creates an additional combat and main phase only if it resolves during a main phase."). The old code satisfied this by accident — the sentinel stayed PreCombatMain, a dead anchor. A typed ThisPhaseQualifier { Unqualified, Main, Combat } now carries the qualifier the combinator was discarding, and this_phase_anchor_gate gates the Main class with AbilityCondition::CurrentPhaseIs { [PreCombatMain, PostCombatMain] }. The gate is clause-scoped, so the untap still resolves, per the rulings. Bare CurrentPhaseIs without IsYourTurn, because Relentless Assault's other ruling confirms casting in an opponent's main phase works.

2. The turn-boundary wrap could be skipped by a live insertion. state.phase == Cleanup && next == Untap misses when an insertion is consumed leaving Cleanup, and misses again when the unwind resumes. start_next_turn would not run: no turn increment, no active-player rotation, no reset of the per-turn ledger. Now keyed on "no extra phase consumed and no resume owed" rather than on Phase values. Not reachable with printed cards — every instant in the bare-anchor class is combat-restricted (ActionNotAllowed("Casting restriction not satisfied: DuringCombat")) and Great Train Heist's combat phase gate is false in an end step — but the resolver does produce anchor: Cleanup, so the guard is hardened and pinned.

3. A bundle created inside an already-inserted phase ran out of order. Aggravated Assault re-activated in the additional main phase its first activation created produced C1, Main, C2, C3, Main, Main instead of C1, Main, C2, Main, C3, Main. The resume walk now threads an unwind boundary through popped anchors so each insertion keeps its own frame. Both stacks always drained, so this was pure mis-ordering — invisible to any count-based assertion.

4. Serde compat. extra_phase_resume's element type changed and PhaseEntryOutcome::Paused is a durable save point, so a session persisted mid-insertion would fail to restore. An ExtraPhaseResumeCompat untagged shim accepts the legacy bare-Phase payload. All committed fixtures record []; the risk was live sessions across a deploy.

Card class

48 cards. 36 bare "after this phase" / "after this combat phase" (Aurelia, Godo, Combat Celebrant, Najeela, Scourge of the Throne, …) — all resolve inside a combat step, where last_step_of_phase is EndCombat, bit-identical to the previous default. 10 "after this main phase" (Relentless Assault, Full Throttle, Aggravated Assault, …) — now correctly gated. 1 "after this combat phase" (Raphael, Tag Team Tough). The Group C cards this issue is about — Overpowering Attack, Moraug, All-Out Assault, Grim Reaper's Sprint, Sokenzan, Valor's Reach — are fixed.

after flips from EndCombat to the sentinel for 47 cards in card-data.json; only Bear with Set's Mechanic, Save Point, Swinging Ship, Throat Wolf and World at War keep the legacy default.

Behavior changes worth maintainer attention

• Full Throttle now grants 3 combat phases, not 2. CR-correct: "two additional combat phases" inserted per CR 500.8 plus the turn's own. The prior test asserting 2 encoded the swallowed natural combat.
• World at War now enters an extra postcombat main after its inserted one (CR 505.1a). Its CR 505.1b ordinal anchor remains wrong — out of scope.
• "First combat phase of the turn" gating shifts when the insert comes from a precombat main, since the inserted combat now runs first (Genji Glove, Hexplate Wallbreaker, Karlach, Finest Hour, Raiyuu, Raph & Leo, Balthier and Fran). CR-correct, pinned by a test.

Anchored on

• crates/engine/src/parser/oracle_effect/imperative.rs:7624 — existing nom_primitives::scan_at_word_boundaries word-boundary scan feeding a composed combinator; the new parse_this_phase_anchor dispatch mirrors it.
• crates/engine/src/parser/oracle_effect/imperative.rs:9797 — existing scan_at_word_boundaries use for parse_additional_phase_count, the sibling axis of the same producer.
• crates/engine/src/game/effects/additional_phase.rs:54 — the inserted-beginning-phase branch already resolving its anchor as last_step_of_phase(state.phase); the new sentinel branch is the same pattern generalized.
• crates/engine/src/parser/oracle_effect/imperative.rs:1056 — existing unreachable!() convention in the same lowering function, followed for the GatedEffect arm.

Gate A

Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=ef5e8b01456820584640ed987c5a7cf2fe8f99d6 base=20963d6fbfee2ce05e94d5d52d958c39485dd39e

Verification

All run directly in the worktree (Tilt does not watch it), against the committed head:

Check	Result
cargo fmt --all -- --check	exit 0
cargo clippy --all-targets -- -D warnings	clean
cargo nextest run -p phase-engine --lib	19200 passed
cargo nextest run -p phase-engine --test integration	5047 passed
cargo nextest run --workspace --no-fail-fast	27569 run, 27568 passed, 1 failed
./scripts/check-parser-combinators.sh origin/main	Gate G + Gate A PASS
./scripts/check-interaction-bindings.sh	exit 0
./scripts/check-prelowered-ratchet.sh	Gate P PASS
cargo coverage	88.9% (31472/35397)

The single workspace failure is mtgish-import::manifest_coverage every_list_field_is_in_ordering_manifest, which is pre-existing on origin/main — the fields it names (ResolvedAbility::selected_target_incarnations, SpellContext::parent_target_iteration_members) exist untouched at the base commit and this diff contains no mtgish-import change.

Every fix in this PR was revert-verified: the fix was reverted locally, the named test confirmed red, then restored.

Disclosures

• The final review-impl gate (AI-CONTRIBUTOR §5) was not satisfied. Two independent review passes were run against an earlier revision of this diff and both produced findings that were addressed (they are the source of defects 1–4 above). Two further review passes over the final revision did not complete — one was killed to break a cargo-fingerprint deadlock, the other lost its API connection. The final revision is therefore self-verified and gate-verified but not independently reviewed, and is opened as a draft for that reason.
• crates/engine/tests/fixtures/integration_cards.json.gz was patched surgically, not regenerated. A full gen-test-fixture.py run in this environment changed 150 entries and dropped 5 cards other tests reference, because the only local AtomicCards.json is older than the vintage the committed fixture was built from. The applied delta is exactly 15 cards (12 after flips + 3 new condition gates), byte-canonical gzip -9 -n, with zero card additions or removals. Please regenerate on a machine with current MTGJSON before merge.
• python3 scripts/gen-test-fixture.py --check fails in this worktree with 26 uncovered cards; verified pre-existing by restoring the committed fixture and re-running (identical failure). It is not wired into CI.
• No coverage A/B baseline: the pre-change export was overwritten during regeneration. After is 88.9%. No regression is expected — CurrentPhaseIs is classified Handled, the after change is a value change, and the condition arms remove swallowed-clause warnings (verified: Sokenzan, Valor's Reach and Great Train Heist each now show parse_warnings: 0).

Out of scope (documented, unchanged)

• CR 500.10a is over-applied at additional_phase.rs — it drops any extra phase when controller != active_player, but CR 500.10a restricts only the "you get" wording, so Take the Bait and Illusionist's Gambit are wrongly dropped on an opponent's turn. Behavior identical before and after this change.
• CR 505.1b ordinal anchors (World at War, Swinging Ship) keep the legacy default; fixing them needs an ordinal-anchor concept.
• The "additional upkeep step" / "additional end step" producers still hard-code their anchors and were left out of this unification; a KNOWN GAP comment records that Obeka's entries anchor in the past and never fire.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VeUwZuMcyVaPL1BA5xQUgz

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2a3a885b-3f81-4cde-be03-bab4a6c6354a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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]

Hold — the conflicted branch cannot be safely ported at ef5e8b01456820584640ed987c5a7cf2fe8f99d6.

Merging current main produces one conflict in crates/engine/tests/fixtures/integration_cards.json.gz. Both sides changed this generated fixture: this branch contains the new additional-phase parser output (for example, Combat Celebrant and Full Throttle), while current main contains later generated-card updates. Choosing either binary would silently discard valid generated data. The PR is also still a draft, so its current CodeRabbit run was skipped.

No maintainer push was made. The next step is an isolated maintainer port that regenerates the fixture from the merged current source, followed by fresh CI, a SHA-bound parse-diff artifact, and a new implementation review after the PR is marked ready for review.

[matthewevans]

Hold — maintainer-owned port required for exact head ef5e8b01456820584640ed987c5a7cf2fe8f99d6.

An isolated merge of current main shows maintainer-caused staleness across the shared additional-phase surface: both sides changed coverage.rs, additional_phase.rs, turns.rs, parser condition/imperative/IR files, ability.rs, game_state.rs, and the integration module registry. Git textually combines those source edits, but the generated crates/engine/tests/fixtures/integration_cards.json.gz conflicts as a binary: taking either side would discard valid generated data.

No contributor rebase or implementation review is requested. This PR remains a draft, and CodeRabbit skipped the draft head. A maintainer must port the source and regenerate the fixture through the project’s approved, verification-capable workflow; the resulting new head then needs fresh required CI, a SHA-bound parse-diff artifact, CodeRabbit review, and implementation review before it can leave hold.