diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index d360d59cc1..c430fa71c8 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -3740,7 +3740,20 @@ fn effect_details(effect: &Effect) -> Vec<(String, String)> { } => { d.push(("player".into(), fmt_target(target))); d.push(("phase".into(), format!("{phase:?}"))); - d.push(("after".into(), format!("{after:?}"))); + // CR 608.2c: `after: PreCombatMain` is the "after this phase" + // resolution-time sentinel, never a literal precombat-main anchor + // (see `types::ability::Effect::AdditionalPhase`'s `after` doc). + // Render what it means — otherwise the audit output claims + // "after: PreCombatMain" for the 47 cards whose insertion point is + // simply the phase the effect resolves in. + d.push(( + "after".into(), + if matches!(after, crate::types::phase::Phase::PreCombatMain) { + "the phase this resolves in".into() + } else { + format!("{after:?}") + }, + )); if !followed_by.is_empty() { d.push(("followed by".into(), format!("{followed_by:?}"))); } diff --git a/crates/engine/src/game/effects/additional_phase.rs b/crates/engine/src/game/effects/additional_phase.rs index 39199a4a61..b9c1765f10 100644 --- a/crates/engine/src/game/effects/additional_phase.rs +++ b/crates/engine/src/game/effects/additional_phase.rs @@ -85,13 +85,27 @@ pub fn resolve( } }; - // CR 500.8 (Full Throttle): "After this main phase, there are N additional - // combat phases" anchors to whichever main phase the spell resolves in. - // The parser emits `after: PreCombatMain` as a sentinel for this wording. - let after = if after == Phase::PreCombatMain - && matches!(state.phase, Phase::PreCombatMain | Phase::PostCombatMain) - { - state.phase + // CR 608.2c + CR 500.8: `Phase::PreCombatMain` in `after` is the parser's + // sentinel for "after this [main|combat] phase" — the anchor is the LAST STEP + // of the phase this effect is resolving in, resolved here because only the + // resolver can see `state.phase`. CR 608.2c ("read the whole text and apply + // the rules of English") is the authority for "this phase" denoting the + // resolving phase; CR 500.8 then adds the insertion directly after it. + // Mirrors the beginning-phase branch above. Combat-resolved sources (Aurelia, + // Godo, Combat Celebrant, Najeela, Scourge of the Throne, Great Train Heist, + // …) yield `EndCombat`, identical to the pre-sentinel default. A literal + // `after` (Upkeep, End, EndCombat) passes through untouched. + // + // This remap is deliberately class-agnostic. The "after this MAIN phase" + // wording additionally creates nothing at all outside a main phase (Gatherer, + // Fury of the Horde: "No new phases are created."), but that precondition is + // information only the PARSER holds — it is attached to the clause as an + // `AbilityCondition::CurrentPhaseIs` gate by + // `oracle_effect::imperative::this_phase_anchor_gate`, so this effect never + // resolves at all in that case. Re-deriving it here is impossible: `after` + // carries no qualifier. + let after = if after == Phase::PreCombatMain { + crate::game::turns::last_step_of_phase(state.phase) } else { after }; @@ -168,22 +182,22 @@ pub fn resolve( // the bundle `count` times so each scheduled occurrence still fires // its own anchor → primary → follow_up sequence. // - // When `count > 1` inserts multiple combat phases after a main-phase - // anchor, only the first bundle may anchor to that main phase — the - // turn never returns there. Chain subsequent combat bundles to - // `EndCombat` so each extra combat is reachable (Full Throttle). - // Repeating the same phase/step (Obeka upkeep) keeps the original anchor. - for i in 0..count { - let bundle_anchor = if i == 0 || phase == after { - after - } else if phase == Phase::BeginCombat { - Phase::EndCombat - } else { - after - }; + // CR 500.8: every bundle anchors at the SAME insertion point. + // `advance_phase_once` chains them through the resume frame ("another phase + // queued after the same anchor runs next"), so Full Throttle's two combats + // run back to back BEFORE the turn resumes at the anchor's natural + // successor, per "the most recently created phase will occur first". The + // former `EndCombat` re-anchor for `i > 0` is removed: it made the second + // combat reachable only by consuming the natural combat's slot, which lost a + // whole combat phase. CR 500.8 is the authority — an extra phase is ADDED + // directly after the specified phase, so it never displaces a phase the turn + // already has (CR 505.1a corroborates by contemplating "a turn in which an + // effect has caused an additional combat phase and an additional main phase + // to be created", but it only classifies which main is precombat). + for _ in 0..count { for &follow_up in followed_by.iter().rev() { state.extra_phases.push(ExtraPhase { - anchor: bundle_anchor, + anchor: after, phase: follow_up, attacker_restriction: None, attacker_restriction_source: None, @@ -200,7 +214,7 @@ pub fn resolve( None }; state.extra_phases.push(ExtraPhase { - anchor: bundle_anchor, + anchor: after, phase, attacker_restriction_source: if restriction.is_some() { Some(ability.source_id) @@ -332,6 +346,10 @@ mod tests { } } + /// CR 500.8 + CR 608.2c: the `after: PreCombatMain` sentinel resolves to the + /// LAST STEP of the phase the effect resolves in — here the postcombat main + /// phase. CR 500.8: every bundle of a `count > 1` effect anchors at that same + /// insertion point, so both extra combats run before the turn resumes. #[test] fn additional_phase_after_this_main_phase_uses_active_main_as_anchor() { let mut state = GameState { @@ -356,22 +374,27 @@ mod tests { state.extra_phases, vec![ ep(Phase::PostCombatMain, Phase::BeginCombat), - ep(Phase::EndCombat, Phase::BeginCombat), + ep(Phase::PostCombatMain, Phase::BeginCombat), ] ); } + /// CR 500.8 + CR 506.1: the Group B wording ("after this phase", resolving + /// during combat — Aurelia, Port Razer, Najeela, …) reaches the resolver as + /// the `PreCombatMain` sentinel and must resolve to an `EndCombat` anchor, + /// exactly as the pre-sentinel literal default did. #[test] fn additional_phase_pushes_begin_combat() { let mut state = GameState { active_player: PlayerId(0), + phase: Phase::DeclareAttackers, ..Default::default() }; let mut events = Vec::new(); let ability = make_ability( TargetFilter::Controller, Phase::BeginCombat, - Phase::EndCombat, + Phase::PreCombatMain, vec![], PlayerId(0), ); @@ -390,13 +413,14 @@ mod tests { fn additional_phase_with_main_pushes_both() { let mut state = GameState { active_player: PlayerId(0), + phase: Phase::DeclareAttackers, ..Default::default() }; let mut events = Vec::new(); let ability = make_ability( TargetFilter::Controller, Phase::BeginCombat, - Phase::EndCombat, + Phase::PreCombatMain, vec![Phase::PostCombatMain], PlayerId(0), ); @@ -419,6 +443,7 @@ mod tests { fn cr_500_8_lifo_ordering() { let mut state = GameState { active_player: PlayerId(0), + phase: Phase::DeclareAttackers, ..Default::default() }; let mut events = Vec::new(); @@ -427,7 +452,7 @@ mod tests { let ability1 = make_ability( TargetFilter::Controller, Phase::BeginCombat, - Phase::EndCombat, + Phase::PreCombatMain, vec![], PlayerId(0), ); @@ -437,7 +462,7 @@ mod tests { let ability2 = make_ability( TargetFilter::Controller, Phase::BeginCombat, - Phase::EndCombat, + Phase::PreCombatMain, vec![], PlayerId(0), ); @@ -465,13 +490,14 @@ mod tests { // Active player is 1, but controller is 0 let mut state = GameState { active_player: PlayerId(1), + phase: Phase::DeclareAttackers, ..Default::default() }; let mut events = Vec::new(); let ability = make_ability( TargetFilter::Controller, Phase::BeginCombat, - Phase::EndCombat, + Phase::PreCombatMain, vec![], PlayerId(0), ); @@ -552,11 +578,16 @@ mod tests { ); } - /// CR 500.8 (Full Throttle): count>1 combat bundles after a main-phase - /// anchor must chain through EndCombat — the turn never returns to the - /// main phase between inserted combats. + /// CR 500.8 (Full Throttle): every bundle of a `count > 1` effect anchors at + /// the SAME insertion point — the phase the effect resolved in. The former + /// `EndCombat` re-anchor for `i > 0` was wrong: the second combat was then + /// inserted after the FIRST inserted combat's end-of-combat step, which is + /// where the turn's own natural combat phase would otherwise resume, so the + /// extra combat consumed the natural one (CR 500.8 — an extra phase is ADDED + /// directly after the specified phase, so it never displaces one the turn + /// already has). #[test] - fn additional_combat_count_chains_after_end_combat() { + fn additional_combat_count_anchors_every_bundle_at_the_insertion_point() { let mut state = GameState { active_player: PlayerId(0), phase: Phase::PreCombatMain, @@ -578,7 +609,7 @@ mod tests { state.extra_phases, vec![ ep(Phase::PreCombatMain, Phase::BeginCombat), - ep(Phase::EndCombat, Phase::BeginCombat), + ep(Phase::PreCombatMain, Phase::BeginCombat), ] ); } @@ -612,8 +643,11 @@ mod tests { assert_eq!(state.phase, Phase::Untap, "inserted beginning phase starts"); assert_eq!( state.extra_phase_resume, - vec![Phase::PostCombatMain], - "resume anchor recorded" + vec![crate::types::game_state::ExtraPhaseResume { + anchor: Phase::PostCombatMain, + inserted: Phase::Untap, + }], + "resume frame records the anchor and the inserted beginning phase" ); advance_phase(&mut state, &mut events); @@ -681,6 +715,14 @@ mod tests { assert!(state.extra_phases.is_empty()); } + /// CR 500.8 (Full Throttle, cast in a precombat main phase): an extra phase is + /// ADDED directly after the specified phase, so "there are two additional + /// combat phases" grants two combats ADDITIONAL to + /// the turn's own combat phase, so the turn runs three. The old assertion + /// (two combats, then straight to the postcombat main) encoded the swallowed + /// natural combat: the second bundle was anchored at the first inserted + /// combat's `EndCombat`, which is exactly where the turn would otherwise have + /// resumed at its natural `BeginCombat`. #[test] fn additional_combat_count_advances_through_both_extra_phases() { use crate::game::turns::advance_phase; @@ -701,21 +743,33 @@ mod tests { ); resolve(&mut state, &ability, &mut events).unwrap(); - advance_phase(&mut state, &mut events); - assert_eq!(state.phase, Phase::BeginCombat, "first extra combat"); - - while state.phase != Phase::EndCombat { + let mut sequence = Vec::new(); + for _ in 0..24 { advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::End { + break; + } } - advance_phase(&mut state, &mut events); - assert_eq!(state.phase, Phase::BeginCombat, "second extra combat"); - while state.phase != Phase::EndCombat { - advance_phase(&mut state, &mut events); - } - advance_phase(&mut state, &mut events); - assert_eq!(state.phase, Phase::PostCombatMain); + let combat = || { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] + }; + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // first inserted combat + expected.extend(combat()); // second inserted combat + expected.extend(combat()); // the turn's own natural combat + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(sequence, expected); assert!(state.extra_phases.is_empty()); + assert!(state.extra_phase_resume.is_empty()); } /// CR 501.1 + CR 500.8: "additional beginning phase after this phase" @@ -850,4 +904,180 @@ mod tests { .expect("tracked set published at resolution"); assert_eq!(members, &vec![ObjectId(11), ObjectId(22)]); } + + /// CR 500.8 + CR 506.1 + CR 608.2c: the "after this phase" sentinel resolving + /// anywhere inside the combat phase anchors at that phase's LAST step, so + /// the inserted combat begins only once the current combat is over. This is + /// the building-block proof for the whole "after this phase" class that + /// resolves during combat (Aurelia, Godo, Combat Celebrant, Najeela, Port + /// Razer, Scourge of the Throne, Hellkite Charger, … — 30+ cards): reverting + /// the sentinel remap makes every row anchor at `PreCombatMain` and the + /// extra combat becomes unreachable. + #[test] + fn current_phase_sentinel_resolves_to_end_combat_from_every_combat_step() { + for resolving_in in [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] { + let mut state = GameState { + active_player: PlayerId(0), + phase: resolving_in, + ..Default::default() + }; + let mut events = Vec::new(); + let ability = make_ability( + TargetFilter::Controller, + Phase::BeginCombat, + Phase::PreCombatMain, + vec![], + PlayerId(0), + ); + + resolve(&mut state, &ability, &mut events).unwrap(); + + assert_eq!( + state.extra_phases, + vec![ep(Phase::EndCombat, Phase::BeginCombat)], + "resolving in {resolving_in:?} must anchor the extra combat at end of combat", + ); + } + } + + /// CR 500.8 + CR 608.2c: the same building block across EVERY phase and step + /// of a turn — the sentinel always resolves to the LAST STEP of the phase the + /// effect is resolving in (CR 501.1 beginning phase → draw step; CR 505.1 + /// main phases have no steps; CR 506.1 combat → end of combat; CR 512.1 + /// ending phase → cleanup step). + /// + /// The `End` / `Cleanup` rows are the ones that matter for the turn + /// boundary: they produce a `Cleanup` anchor, i.e. a phase inserted directly + /// after the cleanup step. `turns::advance_phase_once` must still start the + /// next turn once that insertion is exhausted — see + /// `turns::tests::cleanup_anchored_insertion_still_ends_the_turn`. + #[test] + fn current_phase_sentinel_resolves_to_the_last_step_of_every_phase() { + for (resolving_in, expected_anchor) in [ + (Phase::Untap, Phase::Draw), + (Phase::Upkeep, Phase::Draw), + (Phase::Draw, Phase::Draw), + (Phase::PreCombatMain, Phase::PreCombatMain), + (Phase::BeginCombat, Phase::EndCombat), + (Phase::DeclareAttackers, Phase::EndCombat), + (Phase::DeclareBlockers, Phase::EndCombat), + (Phase::CombatDamage, Phase::EndCombat), + (Phase::EndCombat, Phase::EndCombat), + (Phase::PostCombatMain, Phase::PostCombatMain), + (Phase::End, Phase::Cleanup), + (Phase::Cleanup, Phase::Cleanup), + ] { + let mut state = GameState { + active_player: PlayerId(0), + phase: resolving_in, + ..Default::default() + }; + let mut events = Vec::new(); + let ability = make_ability( + TargetFilter::Controller, + Phase::BeginCombat, + Phase::PreCombatMain, + vec![], + PlayerId(0), + ); + + resolve(&mut state, &ability, &mut events).unwrap(); + + assert_eq!( + state.extra_phases, + vec![ep(expected_anchor, Phase::BeginCombat)], + "resolving in {resolving_in:?} must anchor at {expected_anchor:?}", + ); + } + } + + /// CR 500.8 ("if multiple extra phases are created after the same phase, the + /// most recently created phase will occur first"): two effects + /// resolving in the SAME postcombat main phase — one with a follow-up main + /// phase (Relentless Assault) and one without (Port Razer's wording) — both + /// anchor at that main phase, and the more recent bundle runs first. The + /// discriminating property is that neither bundle's follow-up main is + /// consumed by the other bundle's combat: the follow-up main is anchored at + /// the shared insertion point, not at the inserted combat's end. + #[test] + fn two_main_anchored_bundles_interleave_most_recent_first() { + use crate::game::turns::advance_phase; + + let mut state = GameState { + active_player: PlayerId(0), + phase: Phase::PostCombatMain, + ..Default::default() + }; + let mut events = Vec::new(); + + // First resolution: extra combat followed by an extra main phase. + resolve( + &mut state, + &make_ability( + TargetFilter::Controller, + Phase::BeginCombat, + Phase::PreCombatMain, + vec![Phase::PostCombatMain], + PlayerId(0), + ), + &mut events, + ) + .unwrap(); + // Second resolution: a bare extra combat (most recent → runs first). + resolve( + &mut state, + &make_ability( + TargetFilter::Controller, + Phase::BeginCombat, + Phase::PreCombatMain, + vec![], + PlayerId(0), + ), + &mut events, + ) + .unwrap(); + + assert_eq!( + state.extra_phases, + vec![ + ep(Phase::PostCombatMain, Phase::PostCombatMain), + ep(Phase::PostCombatMain, Phase::BeginCombat), + ep(Phase::PostCombatMain, Phase::BeginCombat), + ], + "every entry anchors at the shared insertion point", + ); + + let mut sequence = Vec::new(); + for _ in 0..24 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::End { + break; + } + } + + let combat = || { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] + }; + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // bare bundle (most recent) first + expected.extend(combat()); // first bundle's combat + expected.push(Phase::PostCombatMain); // first bundle's follow-up main + expected.push(Phase::End); // resume after the anchor + assert_eq!(sequence, expected); + assert!(state.extra_phases.is_empty()); + assert!(state.extra_phase_resume.is_empty()); + } } diff --git a/crates/engine/src/game/turns.rs b/crates/engine/src/game/turns.rs index 21bb1096a0..ee0ce8e518 100644 --- a/crates/engine/src/game/turns.rs +++ b/crates/engine/src/game/turns.rs @@ -10,8 +10,8 @@ use crate::types::counter::CounterType; use crate::types::events::GameEvent; use crate::types::format::GameFormat; use crate::types::game_state::{ - AutoPassMode, ExtraPhase, ExtraTurn, GameState, LoopCollapseAxis, PayableResource, - PendingCounterAddition, PendingEffectResolved, TurnBoundary, WaitingFor, + AutoPassMode, ExtraPhase, ExtraPhaseResume, ExtraTurn, GameState, LoopCollapseAxis, + PayableResource, PendingCounterAddition, PendingEffectResolved, TurnBoundary, WaitingFor, }; use crate::types::identifiers::ObjectId; use crate::types::phase::Phase; @@ -55,7 +55,8 @@ pub(crate) fn last_step_of_phase(phase: Phase) -> Phase { match phase { // CR 501.1: beginning phase = untap, upkeep, draw. Phase::Untap | Phase::Upkeep | Phase::Draw => Phase::Draw, - // CR 505.1: each main phase is a single step. + // CR 500.1 + CR 505.1: a main phase has no steps, so it is its own last + // "step" for anchoring purposes. Phase::PreCombatMain => Phase::PreCombatMain, // CR 506.1: combat phase = begin, declare attackers/blockers, damage, end. Phase::BeginCombat @@ -69,6 +70,56 @@ pub(crate) fn last_step_of_phase(phase: Phase) -> Phase { } } +/// CR 500.8 + CR 500.10: The step whose end finishes an *inserted* phase, given +/// the phase value the insertion was scheduled with. This is NOT +/// [`last_step_of_phase`]: that answers "which step ends the phase CONTAINING +/// this one" for a phase in natural progression, whereas an inserted entry only +/// runs the steps the insertion actually creates. +/// +/// The match is deliberately exhaustive with no wildcard — a new `Phase` variant +/// must be a compile error here, not silently treated as a one-step insert. +pub(crate) fn inserted_phase_terminal_step(inserted: Phase) -> Phase { + match inserted { + // CR 501.1: `Untap` is the inserted-BEGINNING-PHASE marker (no other + // producer emits it), so the whole untap → upkeep → draw phase runs and + // the draw step ends it. + Phase::Untap => Phase::Draw, + // CR 500.10: an inserted STEP ("there is an additional upkeep step" — + // Paradox Haze; "there is an additional end step" — Y'shtola Rhul) + // creates the phase that normally contains that step, and "any other + // steps that phase would normally have are skipped". So the inserted + // step both begins and ends the insertion. + // + // KNOWN GAP (pre-existing, deliberately NOT closed here): the parser + // arms for those two wordings (`oracle_effect/imperative.rs`, + // "additional upkeep step" / "additional end step") hard-code + // `after: Upkeep` / `after: End` and were left out of the "after this + // phase" sentinel unification. Paradox Haze works because its trigger + // resolves at upkeep, but Obeka, Splitter of Seconds ("there are that + // many additional upkeep steps after this phase") resolves in the + // combat damage step, so her entries are anchored in the PAST and never + // fire. Obeka is therefore NOT covered by this function's callers; do + // not cite her as a witness for the inserted-step path. + Phase::Upkeep => Phase::Upkeep, + Phase::Draw => Phase::Draw, + Phase::DeclareAttackers => Phase::DeclareAttackers, + Phase::DeclareBlockers => Phase::DeclareBlockers, + Phase::CombatDamage => Phase::CombatDamage, + Phase::EndCombat => Phase::EndCombat, + Phase::End => Phase::End, + Phase::Cleanup => Phase::Cleanup, + // CR 506.1: `BeginCombat` schedules a whole additional COMBAT PHASE + // (Aurelia, Moraug, Relentless Assault); its five steps run in order and + // the end-of-combat step ends it. + Phase::BeginCombat => Phase::EndCombat, + // CR 500.1 + CR 505.1: a main phase has NO steps (only the beginning, + // combat and ending phases are broken into steps), so an inserted main + // phase begins and ends as that one phase. + Phase::PreCombatMain => Phase::PreCombatMain, + Phase::PostCombatMain => Phase::PostCombatMain, + } +} + /// CR 500.5: Advance through phase/step successors until one phase entry has /// been committed. A skipped successor is still a distinct one-hop transition: /// the loop, rather than recursive re-entry, advances past it. @@ -136,47 +187,114 @@ pub(in crate::game) fn advance_phase_once( let leaving = state.phase; let removed: Option; let next: Phase; - if leaving == Phase::Draw && !state.extra_phase_resume.is_empty() { - // CR 501.1: an inserted beginning phase's draw step is ending. - let anchor = *state.extra_phase_resume.last().unwrap(); - if let Some(i) = state - .extra_phases - .iter() - .rposition(|ep| ep.anchor == anchor && ep.phase == Phase::Untap) - { - // CR 500.8: another beginning phase was queued after the same phase — - // run it next (the resume anchor stays on the stack). The anchor phase - // is never re-entered, so its beginning-of-phase triggers (Temple's - // postcombat-main trigger) do not re-fire. - state.extra_phases.remove(i); - removed = None; - next = Phase::Untap; - } else { - // CR 500.8: no more queued beginning phases — resume the turn after - // "this phase" (the anchor's natural successor). - state.extra_phase_resume.pop(); - removed = None; - next = next_phase(anchor); + + // CR 500.8: an entry anchored at the phase we are LEAVING is inserted + // directly after it and takes precedence — this is the innermost, most + // recent insertion point. `rposition` keeps "the most recently created phase + // will occur first". + let taken = state + .extra_phases + .iter() + .rposition(|ep| ep.anchor == leaving) + .map(|i| state.extra_phases.remove(i)); + + if let Some(ep) = taken { + next = ep.phase; + match state.extra_phase_resume.last_mut() { + // CR 500.8: continuing the bundle already anchored here reuses its + // frame (two Aurelia-style combats, Obeka's N upkeeps, two Temple + // beginning phases) rather than nesting a redundant return point — + // the turn still resumes exactly once, at this anchor's natural + // successor. + Some(frame) if frame.anchor == leaving => frame.inserted = ep.phase, + // CR 500.8: any other insertion point gets its OWN frame, pushed on + // top of whatever is already running. In particular, when the frame + // below is EXHAUSTED (this step is the one that ends its insertion) + // its return point is still owed, and the new insertion is anchored + // *here*, not at the outer frame's anchor — so the outer frame must + // be kept intact rather than having the new insertion written over + // it. Overwriting it (the previous "inherit the exhausted frame" + // behavior) makes every remaining entry anchored at `leaving` + // unreachable: the unwind below looks for siblings at the frame's + // anchor, and after the overwrite no frame carries `leaving` any + // more. Reproducer: Aggravated Assault activated in the precombat + // main and again in the additional main phase that activation + // created — the second bundle's follow-up main was deferred past the + // turn's natural combat instead of running directly after its own + // combat. The stranding hazard the overwrite was protecting against + // is handled by threading the unwind boundary through popped + // anchors; see the `boundary` walk below. + _ => state.extra_phase_resume.push(ExtraPhaseResume { + anchor: leaving, + inserted: ep.phase, + }), } + removed = Some(ep); } else { - let taken = state - .extra_phases - .iter() - .rposition(|ep| ep.anchor == leaving) - .map(|i| state.extra_phases.remove(i)); - next = taken - .as_ref() - .map(|ep| ep.phase) - .unwrap_or_else(|| next_phase(leaving)); - // CR 501.1: entering a freshly-inserted beginning phase — remember where - // to resume once its draw step ends. (No other producer emits `phase: - // Untap`, so this uniquely identifies an inserted beginning phase.) - if let Some(ep) = &taken { - if ep.phase == Phase::Untap { - state.extra_phase_resume.push(ep.anchor); + // CR 500.8: nothing is queued after the phase we are leaving. If we are + // inside one or more inserted phases whose terminal step this is, unwind + // them: an insertion is "directly after" its anchor, so once it (and + // every insertion nested inside it that ends at the same step) is + // exhausted, the turn continues from the OUTERMOST exhausted anchor's + // natural successor — never from the inserted phase's own default + // successor. + // + // The unwind (rather than a single top-frame inspection) is what keeps + // the natural combat phase alive when a combat-triggered extra combat + // (Port Razer, Combat Celebrant, Scourge of the Throne) nests inside a + // main-phase-anchored extra combat (Moraug, Relentless Assault): both + // frames end at the same `EndCombat`, and only the outer one knows the + // turn still owes its natural combat. + // + // `boundary` is the step at which the insertion the walk is currently + // looking at came to an end. It starts as the step we are leaving, and + // after each frame is popped it becomes THAT frame's anchor — because a + // frame anchored at `A` was only ever pushed while leaving `A`, so the + // frame directly beneath it was, at that moment, either exhausted + // (its terminal step was `A`) or still running (its terminal step was + // not). Comparing the outer frame's terminal step against `A` therefore + // asks exactly "was this frame already exhausted when the one above it + // was pushed?", which is what lets the walk keep unwinding through a + // frame whose insertion ended at an EARLIER step than the innermost one + // (Aggravated Assault's main-anchored bundle underneath World at War's + // `EndCombat`-anchored bundle: the outer insertion ended at + // `PostCombatMain`, the inner at `EndCombat`). Without that threading + // the walk stops at the outer frame and its anchor's natural successor + // — the turn's own combat phase — is never reached. + let mut resumed_anchor: Option = None; + let mut chained: Option = None; + let mut boundary = leaving; + while let Some(frame) = state.extra_phase_resume.last().copied() { + if inserted_phase_terminal_step(frame.inserted) != boundary { + break; } - } - removed = taken; + // CR 500.8: another phase queued after the SAME anchor runs next + // ("the most recently created phase will occur first"), before the + // turn resumes. The anchor phase itself is never re-entered, so its + // beginning-of-phase triggers do not re-fire. + if let Some(i) = state + .extra_phases + .iter() + .rposition(|ep| ep.anchor == frame.anchor) + { + let ep = state.extra_phases.remove(i); + if let Some(top) = state.extra_phase_resume.last_mut() { + top.inserted = ep.phase; + } + chained = Some(ep); + break; + } + state.extra_phase_resume.pop(); + resumed_anchor = Some(frame.anchor); + boundary = frame.anchor; + } + next = match (&chained, resumed_anchor) { + (Some(ep), _) => ep.phase, + // CR 500.8: resume after the outermost exhausted insertion point. + (None, Some(anchor)) => next_phase(anchor), + (None, None) => next_phase(leaving), + }; + removed = chained; } // CR 511.3: End Combat teardown happens when the step ends, after its @@ -185,10 +303,33 @@ pub(in crate::game) fn advance_phase_once( complete_end_combat_teardown(state); } - // If wrapping from Cleanup to Untap, start next turn. Turn-level skip - // replacements (CR 614.10) are handled inside `start_next_turn` — the - // per-phase pipeline below runs only for within-turn phase advances. - if state.phase == Phase::Cleanup && next == Phase::Untap { + // CR 500.1 + CR 500.8: the turn is over exactly when the cleanup step's + // NATURAL successor is reached — `next_phase(Cleanup)` is the only source of + // `Phase::Untap` that is not an inserted entry, so `next == Untap` together + // with "no extra phase was consumed on this hop" is the precise predicate. + // Turn-level skip replacements (CR 614.10) are handled inside + // `start_next_turn`; the per-phase pipeline below runs only for within-turn + // phase advances. + // + // The phase we are LEAVING is not a reliable witness in either direction: + // * A phase inserted directly after the cleanup step makes the turn resume + // from that insertion's TERMINAL step, so `state.phase` is (say) + // `EndCombat` and the old `state.phase == Cleanup` test missed the + // boundary entirely: no `start_next_turn`, so no turn increment, no + // active-player rotation, and every per-turn ledger left stale — a free + // extra turn. Any bare "after this phase" effect resolving in the end or + // cleanup step produces exactly that anchor (`last_step_of_phase` maps + // both `End` and `Cleanup` to `Cleanup`; CR 514.3 permits a cleanup-step + // cast). No printed card reaches it today — every instant in that class + // carries a "cast only during combat" restriction the caster enforces — + // but this change routes 36 more cards through the sentinel, so the guard + // is keyed on the machinery rather than on a phase value that only + // happens to be right for the anchors in use. + // * Conversely, an inserted BEGINNING phase anchored at the cleanup step + // produces `next == Untap` from a real entry while leaving `Cleanup`; the + // old test wrapped the turn and `start_next_turn` silently ate the + // insertion. `removed.is_none()` is what rules that out. + if next == Phase::Untap && removed.is_none() { start_next_turn(state, events); } else { // CR 614.1b + CR 614.10 + CR 500.11: Route phase/step starts through the @@ -205,6 +346,21 @@ pub(in crate::game) fn advance_phase_once( // as though it didn't exist." Advance `state.phase` past the skipped // phase so the next loop iteration computes the phase AFTER it, then // let the outer advance loop compute the phase AFTER it. + // + // CR 500.8 + CR 500.11: the resume frame created (or continued) just + // above is deliberately LEFT IN PLACE. The frame does not record + // "an inserted phase is running"; it records where the turn continues + // once this insertion point is exhausted, and skipping the inserted + // phase does not repay that debt. Dropping it here would resume the + // turn at the skipped phase's own default successor — exactly the + // defect this change removes (a skipped inserted combat scheduled in + // a postcombat main would fall into a second postcombat main; a + // skipped inserted untap step would send a Temple of Atropos turn to + // the precombat main phase instead of the end step, which is also the + // behavior the pre-change code had, since it never popped here + // either). The frame is consumed by the unwind above when the + // insertion's terminal step ends, and unconditionally cleared at + // every turn boundary. state.phase = next; return AdvancePhaseOnce::Skipped; } @@ -3120,14 +3276,21 @@ fn auto_advance_once(state: &mut GameState, events: &mut Vec) -> Auto // CR 614.10a + CR 614.1b: Other "skip your draw step" effects // (replacements or static abilities) also remove the whole step. // CR 103.8a: only the STARTING player's FIRST (natural) draw step - // is skipped. An inserted beginning phase's draw step - // (`extra_phase_resume` non-empty) is not that first draw and must - // not be skipped (Temple of Atropos as the turn-1 starting plane). + // is skipped. An inserted beginning phase's draw step is not that + // first draw and must not be skipped (Temple of Atropos as the + // turn-1 starting plane). Test for an inserted BEGINNING phase + // specifically (CR 501.1: `inserted == Untap` is the beginning-phase + // marker) — the resume stack also carries inserted combat, main and + // step insertions, which never coexist with a natural draw step but + // must not be allowed to answer this question by accident. // `should_skip_step_now` (continuous "skip your draw step" effects, // CR 614.10a) is intentionally NOT exempted — those skip every draw. if (state.turn_number == 1 && first_player_skips_first_draw(state) - && state.extra_phase_resume.is_empty()) + && !state + .extra_phase_resume + .iter() + .any(|frame| frame.inserted == Phase::Untap)) || should_skip_step_now(state, Phase::Draw) { let _ = advance_phase_once(state, events); @@ -4819,12 +4982,20 @@ mod tests { assert!(state.extra_phases.is_empty()); } - /// CR 500.8: World at War / Combat Celebrant exert variant — additional - /// combat phase followed by additional main phase. Both push with - /// anchor = EndCombat; LIFO ordering (`rposition` from the end) - /// consumes BeginCombat (most recent push) on the FIRST EndCombat - /// transition, then PostCombatMain on the SECOND EndCombat transition - /// (after the extra combat finishes). + /// CR 500.8 + CR 505.1a: World at War — an additional combat phase followed + /// by an additional main phase, both anchored at `EndCombat`. LIFO ordering + /// (`rposition` from the end) consumes BeginCombat (most recent push) on the + /// FIRST EndCombat transition, then PostCombatMain on the SECOND EndCombat + /// transition (after the extra combat finishes). + /// + /// The turn then resumes at the ANCHOR's natural successor + /// (`next_phase(EndCombat) == PostCombatMain`), so the turn's own postcombat + /// main phase still happens after the inserted one. That is CR 505.1a + /// verbatim: "[this] is also true of a turn in which an effect has caused an + /// additional combat phase and an additional main phase to be created" — the + /// inserted main is *additional to* the natural postcombat main, and the + /// natural one was previously swallowed because the turn resumed at the + /// inserted main phase's own default successor instead. #[test] fn cr_500_8_with_main_phase_lifo_anchor_ordering() { use crate::types::game_state::ExtraPhase; @@ -4870,11 +5041,16 @@ mod tests { Phase::EndCombat, // Second EndCombat consumes the remaining push: PostCombatMain. Phase::PostCombatMain, + // CR 500.8: the turn's OWN postcombat main phase still occurs — + // the turn resumes at `next_phase(EndCombat)`, the anchor's + // natural successor. + Phase::PostCombatMain, // Natural successor — no entries left. Phase::End, ] ); assert!(state.extra_phases.is_empty()); + assert!(state.extra_phase_resume.is_empty()); } /// CR 500.8: Multiple extra combats stacked with the same anchor are @@ -4914,6 +5090,754 @@ mod tests { assert_eq!(state.extra_phases.len(), 1); } + /// CR 500.8: an extra combat inserted after the PRECOMBAT MAIN phase is + /// inserted *directly after that phase* — the turn's own combat phase still + /// follows it. Before the resume frame existed, the turn resumed at the + /// inserted combat's own successor (`next_phase(EndCombat)`), so the natural + /// combat phase was silently swallowed (Moraug, Overpowering Attack, + /// Relentless Assault cast precombat). + #[test] + fn cr_500_8_main_anchored_extra_combat_precedes_the_natural_combat() { + use crate::types::game_state::ExtraPhase; + + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.extra_phases.push(ExtraPhase { + anchor: Phase::PreCombatMain, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..16 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::End { + break; + } + } + + assert_eq!( + sequence, + vec![ + // The inserted combat runs first (CR 500.8: directly after the + // precombat main phase). + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + // …then the turn resumes at the ANCHOR's natural successor: its + // own combat phase. + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + Phase::PostCombatMain, + Phase::End, + ] + ); + assert!(state.extra_phase_resume.is_empty()); + } + + /// CR 500.8 (the nested-insertion case): an extra combat triggered *inside* + /// a main-anchored extra combat (Port Razer / Combat Celebrant / Scourge of + /// the Throne connecting during a Moraug combat) is created directly after + /// the step that ENDS the outer insertion, so the outer frame is exhausted + /// while its return point is still owed. `advance_phase_once` pushes a frame + /// for the new insertion point on top of it; the turn must still resume at + /// the OUTERMOST anchor's successor and run its natural combat phase. + /// Resuming at the innermost `next_phase(EndCombat)` loses it. + /// + /// Both insertions here end at the SAME step (`EndCombat`). The + /// differing-terminal-step case — where the unwind has to keep walking past + /// a frame whose insertion ended earlier — is + /// `cr_500_8_unwind_walks_past_a_frame_whose_insertion_ended_at_an_earlier_step`. + #[test] + fn cr_500_8_nested_extra_combat_unwinds_to_the_outermost_anchor() { + use crate::types::game_state::ExtraPhase; + + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.extra_phases.push(ExtraPhase { + anchor: Phase::PreCombatMain, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + let mut injected = false; + for _ in 0..24 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + // During the FIRST inserted combat, a combat-damage trigger schedules + // another combat after "this phase" (anchor = EndCombat). + if !injected && state.phase == Phase::CombatDamage { + injected = true; + state.extra_phases.push(ExtraPhase { + anchor: Phase::EndCombat, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + } + if state.phase == Phase::End { + break; + } + } + + let combat = || { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] + }; + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // main-anchored insert + expected.extend(combat()); // nested insert, anchored at its EndCombat + expected.extend(combat()); // the turn's own natural combat — still owed + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(sequence, expected); + assert!( + state.extra_phase_resume.is_empty(), + "every return point is repaid by the end of the turn" + ); + } + + /// CR 500.8 — the differing-terminal-step regression. Aggravated Assault + /// (`{3}{R}{R}: … After this main phase, there is an additional combat phase + /// followed by an additional main phase.`) activated in the precombat main + /// anchors both of its entries there; World at War (`After the second main + /// phase this turn, there's an additional combat phase followed by an + /// additional main phase.` — a CR 505.1b ordinal anchor, so it keeps the + /// legacy `EndCombat` default) cast in the same main phase anchors both of + /// its entries at `EndCombat`. + /// + /// At the first inserted combat's end-of-combat step, World at War's combat + /// is created directly after a step that ENDS the outer, main-anchored + /// insertion. The frame pushed for it ends at `EndCombat`, but the outer, + /// main-anchored frame ends at `PostCombatMain` — an EARLIER step in the + /// walk's order. If the unwind compared every frame's terminal step against + /// the step being left, it would pop the inner frame when World at War's + /// follow-up main ends and then stop, because the outer frame's terminal + /// step (`EndCombat`) no longer matches. The turn then ran two combats and + /// two postcombat mains, and both `extra_phases` and `extra_phase_resume` + /// were left permanently non-empty — Aggravated Assault's own follow-up main + /// was never consumed. + /// + /// Threading the unwind boundary through each popped frame's anchor repays + /// every bundle instead: three combat phases and three postcombat main + /// phases, both stacks drained. + #[test] + fn cr_500_8_unwind_walks_past_a_frame_whose_insertion_ended_at_an_earlier_step() { + use crate::types::game_state::ExtraPhase; + + let entry = |anchor: Phase, phase: Phase| ExtraPhase { + anchor, + phase, + attacker_restriction: None, + attacker_restriction_source: None, + }; + + let mut state = setup(); + state.phase = Phase::PreCombatMain; + // Aggravated Assault resolves first: follow-up main pushed before the + // combat so the LIFO scan takes the combat first (CR 500.8). + state + .extra_phases + .push(entry(Phase::PreCombatMain, Phase::PostCombatMain)); + state + .extra_phases + .push(entry(Phase::PreCombatMain, Phase::BeginCombat)); + // World at War resolves second, in the same main phase. + state + .extra_phases + .push(entry(Phase::EndCombat, Phase::PostCombatMain)); + state + .extra_phases + .push(entry(Phase::EndCombat, Phase::BeginCombat)); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..40 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::End { + break; + } + } + + let combat = || { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] + }; + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // Aggravated Assault's combat + expected.extend(combat()); // World at War's combat, after its EndCombat + expected.push(Phase::PostCombatMain); // World at War's follow-up main + expected.push(Phase::PostCombatMain); // Aggravated Assault's follow-up main + expected.extend(combat()); // the turn's OWN combat phase — still owed + expected.push(Phase::PostCombatMain); // the turn's own postcombat main + expected.push(Phase::End); + assert_eq!( + sequence, expected, + "CR 500.8: every bundle is inserted directly after its own anchor and \ + none of them displaces a phase the turn already has", + ); + assert_eq!( + sequence + .iter() + .filter(|p| **p == Phase::BeginCombat) + .count(), + 3, + "two additional combat phases plus the turn's own", + ); + assert!( + state.extra_phases.is_empty(), + "no entry may be stranded: {:?}", + state.extra_phases, + ); + assert!( + state.extra_phase_resume.is_empty(), + "no return point may be stranded: {:?}", + state.extra_phase_resume, + ); + } + + /// The five steps of one combat phase (CR 506.1), in order. Shared by the + /// insertion-ordering tests below so an expected phase sequence reads as the + /// list of phases the turn is supposed to contain. + fn combat_steps() -> [Phase; 5] { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] + } + + fn extra_phase_entry(anchor: Phase, phase: Phase) -> crate::types::game_state::ExtraPhase { + crate::types::game_state::ExtraPhase { + anchor, + phase, + attacker_restriction: None, + attacker_restriction_source: None, + } + } + + /// Drive `advance_phase` to the end step (or the given hop budget), pushing a + /// re-activation bundle each time an additional main phase is entered, up to + /// `activations` times. Models Aggravated Assault being activated again in + /// the very main phase its previous activation created: "After this main + /// phase" anchors the new bundle at `last_step_of_phase(PostCombatMain) == + /// PostCombatMain` (CR 500.1 + CR 505.1 — a main phase has no steps). + fn run_reactivating_in_each_additional_main( + state: &mut GameState, + mut activations: usize, + ) -> Vec { + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..120 { + advance_phase(state, &mut events); + sequence.push(state.phase); + if activations > 0 && state.phase == Phase::PostCombatMain { + activations -= 1; + // Follow-up main pushed first so the LIFO scan (CR 500.8: "the + // most recently created phase will occur first") takes the + // combat phase first. + state.extra_phases.push(extra_phase_entry( + Phase::PostCombatMain, + Phase::PostCombatMain, + )); + state + .extra_phases + .push(extra_phase_entry(Phase::PostCombatMain, Phase::BeginCombat)); + } + if state.phase == Phase::End { + break; + } + } + sequence + } + + /// CR 500.8 — Aggravated Assault's primary line. `{3}{R}{R}: Untap all + /// creatures you control. After this main phase, there is an additional + /// combat phase followed by an additional main phase.` activated in the + /// natural precombat main, then activated AGAIN during the additional main + /// phase the first activation created. + /// + /// CR 500.8 is positional: each bundle is added "directly after the specified + /// phase", and the second activation's specified phase is the ADDITIONAL MAIN + /// it was activated in — not the precombat main. So the second combat and its + /// follow-up main run back to back, before the turn's own combat phase, which + /// is still owed from the first bundle's anchor. + /// + /// Regression: the second bundle's follow-up main was deferred past the + /// natural combat phase (`… C2, natural combat, natural main, extra main …`). + /// Both stacks still drained, so this was pure mis-ORDERING: the second + /// activation's insertion point was written over the first activation's + /// resume frame, leaving no frame carrying `PostCombatMain` for the unwind's + /// sibling scan to find, so the follow-up main was only picked up much later + /// by the natural postcombat main's own anchor scan. + #[test] + fn cr_500_8_reactivation_inside_an_additional_main_runs_directly_after_its_own_combat() { + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.active_player = PlayerId(0); + state.extra_phases.push(extra_phase_entry( + Phase::PreCombatMain, + Phase::PostCombatMain, + )); + state + .extra_phases + .push(extra_phase_entry(Phase::PreCombatMain, Phase::BeginCombat)); + + let sequence = run_reactivating_in_each_additional_main(&mut state, 1); + + let mut expected: Vec = Vec::new(); + expected.extend(combat_steps()); // first activation's combat + expected.push(Phase::PostCombatMain); // first activation's main (re-activated here) + expected.extend(combat_steps()); // second activation's combat + expected.push(Phase::PostCombatMain); // second activation's main + expected.extend(combat_steps()); // the turn's OWN combat phase — still owed + expected.push(Phase::PostCombatMain); // the turn's own postcombat main + expected.push(Phase::End); + assert_eq!( + sequence, expected, + "CR 500.8: the second bundle is inserted directly after the main phase \ + it was created in, not after the first bundle's anchor", + ); + assert!( + state.extra_phases.is_empty(), + "no entry may be stranded: {:?}", + state.extra_phases, + ); + assert!( + state.extra_phase_resume.is_empty(), + "no return point may be stranded: {:?}", + state.extra_phase_resume, + ); + } + + /// CR 500.8 — the same Aggravated Assault line taken to three activations, + /// each in the additional main phase the previous one created. The insertion + /// points nest three deep, and every one of them must be repaid in order: + /// four combat phases and four postcombat main phases (three additional plus + /// the turn's own of each), with the natural pair LAST. + #[test] + fn cr_500_8_three_chained_reactivations_each_run_directly_after_their_own_combat() { + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.active_player = PlayerId(0); + state.extra_phases.push(extra_phase_entry( + Phase::PreCombatMain, + Phase::PostCombatMain, + )); + state + .extra_phases + .push(extra_phase_entry(Phase::PreCombatMain, Phase::BeginCombat)); + + let sequence = run_reactivating_in_each_additional_main(&mut state, 2); + + let mut expected: Vec = Vec::new(); + for _ in 0..4 { + expected.extend(combat_steps()); + expected.push(Phase::PostCombatMain); + } + expected.push(Phase::End); + assert_eq!( + sequence, expected, + "CR 500.8: three additional combat/main bundles run back to back, then \ + the turn's own combat phase and postcombat main", + ); + assert!(state.extra_phases.is_empty(), "{:?}", state.extra_phases); + assert!( + state.extra_phase_resume.is_empty(), + "{:?}", + state.extra_phase_resume, + ); + } + + /// CR 500.8 + CR 501.1 — a bundle created INSIDE an inserted beginning phase. + /// Fixture: a beginning phase inserted after the postcombat main (Temple of + /// Atropos / Sphinx of the Second Sun shape, CR 501.1 — `Phase::Untap` is the + /// inserted-beginning-phase marker), and during its DRAW step an "after this + /// phase" combat + main bundle is created. `last_step_of_phase(Draw) == Draw` + /// (CR 501.1: the draw step ends the beginning phase), so the bundle is + /// anchored there and runs directly after the inserted beginning phase. + /// + /// The two insertions end at DIFFERENT steps — the beginning phase at `Draw`, + /// the bundle at `PostCombatMain` — which is the shape that strands the outer + /// return point if the unwind only ever compares against the step being left. + /// The turn must still resume at `next_phase(PostCombatMain)`, the END step: + /// the postcombat main phase this whole insertion was anchored to has already + /// happened. + #[test] + fn cr_500_8_bundle_created_inside_an_inserted_beginning_phase_resumes_at_the_outer_anchor() { + let mut state = setup(); + state.phase = Phase::PostCombatMain; + state.active_player = PlayerId(0); + state + .extra_phases + .push(extra_phase_entry(Phase::PostCombatMain, Phase::Untap)); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + let mut injected = false; + for _ in 0..60 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if !injected && state.phase == Phase::Draw { + injected = true; + state + .extra_phases + .push(extra_phase_entry(Phase::Draw, Phase::PostCombatMain)); + state + .extra_phases + .push(extra_phase_entry(Phase::Draw, Phase::BeginCombat)); + } + if state.phase == Phase::End { + break; + } + } + + let mut expected = vec![Phase::Untap, Phase::Upkeep, Phase::Draw]; + expected.extend(combat_steps()); + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!( + sequence, expected, + "CR 500.8: the bundle runs directly after the inserted beginning phase, \ + then the turn resumes after the postcombat main the beginning phase \ + was itself inserted after", + ); + assert!(state.extra_phases.is_empty(), "{:?}", state.extra_phases); + assert!( + state.extra_phase_resume.is_empty(), + "{:?}", + state.extra_phase_resume, + ); + } + + /// CR 500.8 — three levels of nesting through the SAME terminal step. A + /// main-anchored extra combat (Moraug / Relentless Assault), a second combat + /// created at its end-of-combat step, and a third created at that one's + /// (Port Razer / Combat Celebrant / Scourge of the Throne connecting in each + /// inserted combat). Every level unwinds to the outermost anchor, so the + /// turn's own combat phase is the fourth and is still followed by its own + /// postcombat main. + #[test] + fn cr_500_8_three_levels_of_nested_extra_combats_all_unwind_to_the_outermost_anchor() { + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.active_player = PlayerId(0); + state + .extra_phases + .push(extra_phase_entry(Phase::PreCombatMain, Phase::BeginCombat)); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + let mut injections = 0; + for _ in 0..60 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::CombatDamage && injections < 2 { + injections += 1; + state + .extra_phases + .push(extra_phase_entry(Phase::EndCombat, Phase::BeginCombat)); + } + if state.phase == Phase::End { + break; + } + } + + let mut expected: Vec = Vec::new(); + for _ in 0..4 { + expected.extend(combat_steps()); + } + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!( + sequence, expected, + "CR 500.8: three inserted combat phases, then the turn's own", + ); + assert!(state.extra_phases.is_empty(), "{:?}", state.extra_phases); + assert!( + state.extra_phase_resume.is_empty(), + "{:?}", + state.extra_phase_resume, + ); + } + + /// CR 500.11 + CR 614.10: a skip replacement over an INSERTED phase. The + /// `ReplacementResult::Prevented` arm of `advance_phase_once` deliberately + /// leaves the resume frame in place: the frame records where the turn + /// CONTINUES once this insertion point is exhausted, and "proceeding past the + /// phase as though it didn't exist" does not repay that debt. + /// + /// Fixture: a main-anchored extra combat phase (Moraug / Overpowering Attack + /// shape) on a turn bound by a turn-scoped combat skip (False Peace / Empty + /// City Ruse, CR 614.10a). Every combat step — inserted and natural — is + /// prevented, so the turn goes straight from the precombat main phase to the + /// postcombat main phase and the end step, and no return point is left owed. + /// + /// Dropping the frame in the `Prevented` arm instead would resume the turn at + /// the SKIPPED phase's own default successor, i.e. at a second postcombat + /// main phase for this fixture. + #[test] + fn skipped_inserted_phase_keeps_its_return_point_and_repays_it() { + use crate::types::game_state::ExtraPhase; + + let mut state = setup(); + state.phase = Phase::PreCombatMain; + state.active_player = PlayerId(0); + state.combat_phase_skip_next_turn[0].active = true; + state.extra_phases.push(ExtraPhase { + anchor: Phase::PreCombatMain, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..24 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::End { + break; + } + } + + assert_eq!( + sequence, + vec![Phase::PostCombatMain, Phase::End], + "CR 500.11: every combat step of the turn is proceeded past as though \ + it didn't exist — including the inserted one — and the turn resumes \ + ONCE at the anchor's natural successor", + ); + assert!(state.extra_phases.is_empty()); + assert!( + state.extra_phase_resume.is_empty(), + "the return point created for the skipped insertion is still repaid", + ); + } + + /// CR 500.11 + CR 500.8 — the discriminating pair for the test above, which + /// cannot tell the two behaviors apart: there, the anchor's natural successor + /// and the skipped insertion's own default successor both lead to the + /// postcombat main phase. + /// + /// Here they diverge. A combat phase inserted after the CLEANUP step (CR + /// 512.1: `last_step_of_phase(End) == Cleanup`, the anchor + /// `additional_phase::resolve` produces for a bare "after this phase" effect + /// resolving in the end or cleanup step) is skipped by a turn-scoped combat + /// skip (CR 614.10a). Keeping the return point resumes the turn at + /// `next_phase(Cleanup)` and the turn ends. Dropping it in the `Prevented` + /// arm resumes at the skipped combat's OWN default successor — a postcombat + /// main phase after the cleanup step, and a turn that never ends. + #[test] + fn a_skipped_insertion_resumes_at_its_anchor_not_at_its_own_successor() { + let mut state = setup(); + state.phase = Phase::End; + state.turn_number = 2; + state.active_player = PlayerId(0); + state.combat_phase_skip_next_turn[0].active = true; + state + .extra_phases + .push(extra_phase_entry(Phase::Cleanup, Phase::BeginCombat)); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..24 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::Untap { + break; + } + } + + assert_eq!( + sequence, + vec![Phase::Cleanup, Phase::Untap], + "CR 500.11: the inserted combat phase is proceeded past as though it \ + didn't exist, and the turn resumes at the cleanup step's natural \ + successor — NOT at the skipped combat's own successor", + ); + assert_eq!( + state.turn_number, 3, + "CR 500.1: the turn boundary still fires", + ); + assert!(state.extra_phases.is_empty()); + assert!(state.extra_phase_resume.is_empty()); + } + + /// CR 500.8 + CR 500.1: an extra phase anchored at the CLEANUP step — what + /// `additional_phase::resolve` produces for any bare "after this phase" + /// effect resolving in the end step or the cleanup step (CR 512.1: + /// `last_step_of_phase(End) == Cleanup`; CR 514.3 allows a cleanup-step cast) + /// — must run, and the turn must still END afterwards. + /// + /// Regression: the wrap was keyed on `state.phase == Cleanup && next == + /// Untap`. Leaving `Cleanup`, the insertion made `next == BeginCombat`, so + /// the guard missed; when the inserted combat ended, the unwind computed + /// `next_phase(Cleanup) == Untap` but `state.phase` was `EndCombat`, so the + /// guard missed AGAIN. `start_next_turn` never ran: no turn increment, no + /// active-player rotation, and every per-turn ledger left stale — the active + /// player got a free extra turn. + #[test] + fn cleanup_anchored_insertion_still_ends_the_turn() { + use crate::types::game_state::ExtraPhase; + + let mut state = setup(); + state.phase = Phase::End; + state.turn_number = 2; + state.active_player = PlayerId(0); + state.extra_phases.push(ExtraPhase { + anchor: Phase::Cleanup, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let mut events = Vec::new(); + let mut sequence = Vec::new(); + for _ in 0..16 { + advance_phase(&mut state, &mut events); + sequence.push(state.phase); + if state.phase == Phase::Untap { + break; + } + } + + assert_eq!( + sequence, + vec![ + Phase::Cleanup, + // CR 500.8: the insertion runs directly after the cleanup step. + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + // …and then the turn is over. + Phase::Untap, + ], + ); + assert_eq!( + state.turn_number, 3, + "CR 500.1: the turn boundary must still fire", + ); + assert_eq!( + state.active_player, + PlayerId(1), + "CR 102.1: the active player must still rotate", + ); + assert!(state.extra_phases.is_empty()); + assert!(state.extra_phase_resume.is_empty()); + } + + /// CR 500.8 (the other half of the same guard): a BEGINNING phase anchored at + /// the cleanup step produces `next == Untap` from a real entry, not from + /// `next_phase(Cleanup)`. The turn must NOT wrap — the insertion has to run. + /// The old `state.phase == Cleanup && next == Untap` test could not tell the + /// two apart, wrapped the turn, and `start_next_turn` silently cleared the + /// entry. + #[test] + fn cleanup_anchored_beginning_phase_is_not_eaten_by_the_turn_boundary() { + use crate::types::game_state::ExtraPhase; + + let mut state = setup(); + state.phase = Phase::Cleanup; + state.turn_number = 2; + state.active_player = PlayerId(0); + state.extra_phases.push(ExtraPhase { + anchor: Phase::Cleanup, + phase: Phase::Untap, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let mut events = Vec::new(); + advance_phase(&mut state, &mut events); + + assert_eq!( + state.phase, + Phase::Untap, + "the inserted beginning phase runs" + ); + assert_eq!( + state.turn_number, 2, + "the turn has not ended — this untap step belongs to the insertion", + ); + assert_eq!(state.active_player, PlayerId(0)); + assert_eq!( + state.extra_phase_resume, + vec![ExtraPhaseResume { + anchor: Phase::Cleanup, + inserted: Phase::Untap, + }], + "the return point is owed", + ); + } + + /// CR 500.10 + CR 501.1 + CR 505.1 + CR 506.1: exhaustive inserted-phase → + /// terminal-step mapping. An inserted STEP ends with itself (CR 500.10: the + /// phase's other steps are skipped); the `Untap` beginning-phase marker ends + /// at the draw step; an inserted combat phase ends at end of combat. + #[test] + fn inserted_phase_terminal_step_maps_each_insertion_to_its_final_step() { + assert_eq!(inserted_phase_terminal_step(Phase::Untap), Phase::Draw); + assert_eq!(inserted_phase_terminal_step(Phase::Upkeep), Phase::Upkeep); + assert_eq!(inserted_phase_terminal_step(Phase::Draw), Phase::Draw); + assert_eq!( + inserted_phase_terminal_step(Phase::PreCombatMain), + Phase::PreCombatMain + ); + assert_eq!( + inserted_phase_terminal_step(Phase::BeginCombat), + Phase::EndCombat + ); + assert_eq!( + inserted_phase_terminal_step(Phase::DeclareAttackers), + Phase::DeclareAttackers + ); + assert_eq!( + inserted_phase_terminal_step(Phase::DeclareBlockers), + Phase::DeclareBlockers + ); + assert_eq!( + inserted_phase_terminal_step(Phase::CombatDamage), + Phase::CombatDamage + ); + assert_eq!( + inserted_phase_terminal_step(Phase::EndCombat), + Phase::EndCombat + ); + assert_eq!( + inserted_phase_terminal_step(Phase::PostCombatMain), + Phase::PostCombatMain + ); + assert_eq!(inserted_phase_terminal_step(Phase::End), Phase::End); + assert_eq!(inserted_phase_terminal_step(Phase::Cleanup), Phase::Cleanup); + } + /// Negative test — extra-turn / extra-step mechanics that did NOT use /// `extra_phases` are unaffected by the typing change. `extra_turns` is /// a separate LIFO stack consumed by `start_next_turn`. @@ -7695,15 +8619,19 @@ mod tests { /// CR 103.8a: the turn-1 draw skip applies only to the starting player's /// FIRST (natural) draw step. An inserted beginning phase's draw step - /// (`extra_phase_resume` non-empty) must still perform the turn-based draw, - /// even on turn 1 in a 2-player game (Temple of Atropos as the starting plane). + /// (a resume frame whose `inserted` is the CR 501.1 beginning-phase marker) + /// must still perform the turn-based draw, even on turn 1 in a 2-player game + /// (Temple of Atropos as the starting plane). #[test] fn inserted_beginning_phase_draw_not_skipped_on_first_turn() { let mut state = setup(); // 2-player, turn_number = 1 state.phase = Phase::Draw; state.active_player = PlayerId(0); // Simulate being inside an inserted beginning phase. - state.extra_phase_resume = vec![Phase::PostCombatMain]; + state.extra_phase_resume = vec![ExtraPhaseResume { + anchor: Phase::PostCombatMain, + inserted: Phase::Untap, + }]; let id = create_object( &mut state, @@ -7723,6 +8651,39 @@ mod tests { assert!(!state.players[0].library.contains(&id)); } + /// CR 103.8a (the discriminating pair for the test above): the exemption is + /// keyed on an inserted BEGINNING phase, not on "the resume stack is + /// non-empty". A frame for any other insertion (here an inserted combat + /// phase) must NOT exempt the starting player's first natural draw step. + /// Reverting the predicate to `extra_phase_resume.is_empty()` flips this. + #[test] + fn non_beginning_phase_resume_frame_does_not_exempt_the_first_turn_draw_skip() { + let mut state = setup(); // 2-player, turn_number = 1 + state.phase = Phase::Draw; + state.active_player = PlayerId(0); + state.extra_phase_resume = vec![ExtraPhaseResume { + anchor: Phase::PostCombatMain, + inserted: Phase::BeginCombat, + }]; + + let id = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Card".to_string(), + Zone::Library, + ); + + let mut events = Vec::new(); + auto_advance(&mut state, &mut events); + + assert!( + state.players[0].library.contains(&id), + "CR 103.8a: the starting player's first natural draw is still skipped", + ); + assert!(!state.players[0].hand.contains(&id)); + } + #[test] fn skip_draw_step_static_prevents_draw() { use crate::types::statics::StaticMode; diff --git a/crates/engine/src/parser/oracle_effect/conditions.rs b/crates/engine/src/parser/oracle_effect/conditions.rs index 7015f1dc1d..8d142eb19a 100644 --- a/crates/engine/src/parser/oracle_effect/conditions.rs +++ b/crates/engine/src/parser/oracle_effect/conditions.rs @@ -6,7 +6,7 @@ use nom::bytes::complete::{tag, take_until}; use nom::character::complete::char; use nom::character::complete::multispace0; use nom::combinator::{all_consuming, map, opt, peek, value}; -use nom::sequence::{preceded, terminated}; +use nom::sequence::{pair, preceded, terminated}; use nom::Parser; use super::super::oracle_nom::bridge::{nom_on_lower, nom_parse_lower}; @@ -3834,6 +3834,20 @@ fn parse_phase_name_set( ), value(vec![Phase::PreCombatMain], tag("precombat main phase")), value(vec![Phase::PostCombatMain], tag("postcombat main phase")), + // CR 506.1: the combat phase is its five steps — "it's your combat + // phase" (Great Train Heist) is true during any of them. Ordinal + // refinements ("the first combat phase this turn", CR 505.1b) are not + // phase NAMES and deliberately stay unparsed here. + value( + vec![ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ], + tag("combat phase"), + ), value(vec![Phase::Upkeep], tag("upkeep")), value(vec![Phase::Draw], tag("draw step")), value(vec![Phase::BeginCombat], tag("beginning of combat step")), @@ -3847,37 +3861,72 @@ fn parse_phase_name_set( .parse(input) } -/// CR 505.1 + CR 102.1 + CR 608.2c: "it is[n't] your [phase/step]" — the +/// CR 102.1: which player's phase the determiner names. A typed axis rather +/// than a bool: the possessive and the indefinite article select genuinely +/// different rules checks, and the type says which. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PhasePossessive { + /// "your " — CR 102.1: additionally requires that the controller is + /// the active player. + Yours, + /// "a " — whichever phase of the turn is current, whoever the active + /// player is. + Any, +} + +/// CR 505.1 + CR 102.1 + CR 608.2c: "it is[n't] {your|a} [phase/step]" — the /// resolution-time current-phase gate (CR 608.2c: read the whole text when the -/// ability resolves). The "your [phase]" possessive decomposes into two +/// ability resolves). The possessive "your [phase]" decomposes into two /// orthogonal checks: `CurrentPhaseIs { phases }` (the live phase, via /// `parse_phase_name_set`) AND `IsYourTurn` (CR 102.1: the active player is the -/// controller — "your" phase means a phase of your turn). The polarity prefix -/// selects negation, wrapping the conjunction in `Not`. NON-`all_consuming`: -/// the dispatcher wraps with `all_consuming` so the whole clause must be -/// consumed, which (together with the expletive "it") rules out any anaphoric -/// mis-binding. +/// controller — "your" phase means a phase of your turn). The indefinite +/// article does NOT add that check: "a main phase" (Sokenzan, Valor's Reach) is +/// satisfied by whichever main phase of the turn is current, whoever the active +/// player is — those planes' chaos abilities can trigger on any player's turn. +/// The polarity prefix selects negation, wrapping the whole condition in `Not`. +/// NON-`all_consuming`: the dispatcher wraps with `all_consuming` so the whole +/// clause must be consumed, which (together with the expletive "it") rules out +/// any anaphoric mis-binding. fn parse_current_phase_condition( input: &str, ) -> super::super::oracle_nom::error::OracleResult<'_, AbilityCondition> { - let (rest, negated) = alt(( - value( - true, - alt(( - tag::<_, _, OracleError<'_>>("it isn't your "), - tag("it is not your "), - tag("it's not your "), - )), + // Compose the two axes (polarity × determiner) instead of enumerating their + // product: each polarity form is followed by the shared determiner axis. + let determiner = || { + alt(( + value( + PhasePossessive::Yours, + tag::<_, _, OracleError<'_>>("your "), + ), + value(PhasePossessive::Any, tag("a ")), + )) + }; + let (rest, (negated, possessive)) = alt(( + pair( + value( + true, + alt(( + tag::<_, _, OracleError<'_>>("it isn't "), + tag("it is not "), + tag("it's not "), + )), + ), + determiner(), + ), + pair( + value(false, alt((tag("it is "), tag("it's ")))), + determiner(), ), - value(false, alt((tag("it is your "), tag("it's your ")))), )) .parse(input)?; let (rest, phases) = parse_phase_name_set(rest)?; - let condition = AbilityCondition::And { - conditions: vec![ - AbilityCondition::CurrentPhaseIs { phases }, - AbilityCondition::IsYourTurn, - ], + let current_phase_is = AbilityCondition::CurrentPhaseIs { phases }; + let condition = match possessive { + // CR 102.1: "your" phase — the controller must also be the active player. + PhasePossessive::Yours => AbilityCondition::And { + conditions: vec![current_phase_is, AbilityCondition::IsYourTurn], + }, + PhasePossessive::Any => current_phase_is, }; Ok((rest, maybe_negate(condition, negated))) } @@ -7932,12 +7981,74 @@ mod tests { Some(your_phase(vec![Phase::End])), ); + // CR 505.1 + CR 102.1: the indefinite article is a DIFFERENT check — + // "a main phase" (the planes Sokenzan and Valor's Reach: "Whenever chaos + // ensues, untap all creatures that attacked this turn. If it's a main + // phase, there is an additional combat phase after this phase, followed + // by an additional main phase.") is satisfied by whichever main phase is + // current, on anybody's turn, so it must NOT carry `IsYourTurn`. + // + // Before this arm existed the shape returned `None`, so + // `strip_leading_general_conditional` dropped the head, the clause was + // flagged as a swallowed conditional, and NO `AdditionalPhase` was + // emitted at all — those planes got no extra combat phase rather than an + // ungated one. This is coverage work (it makes the class parse), not a + // prerequisite of the anchor fix. + let any_main = || AbilityCondition::CurrentPhaseIs { + phases: vec![Phase::PreCombatMain, Phase::PostCombatMain], + }; + assert_eq!(parse("it's a main phase"), Some(any_main())); + assert_eq!(parse("it is a main phase"), Some(any_main())); + assert_eq!( + parse("it isn't a main phase"), + Some(AbilityCondition::Not { + condition: Box::new(any_main()), + }), + ); + // The discriminating assertion for the two determiner arms: the article + // arm must not swallow the possessive arm (and vice versa). + assert_ne!( + parse("it's your main phase"), + parse("it's a main phase"), + "CR 102.1: 'your main phase' additionally requires the active-player check", + ); + + // CR 506.1: "combat phase" names all five combat steps. Great Train + // Heist's "If it's your combat phase, there is an additional combat phase + // after this phase" had no phase-name arm before this fix, so the whole + // conditional head failed to lower and the clause was swallowed — the + // Spree mode produced no extra combat phase at all. With the arm in + // place the mode both parses AND carries its CR 506.1 gate, which is what + // keeps it from granting an extra combat phase when the mode is chosen in + // a main phase. + assert_eq!( + parse("it's your combat phase"), + Some(your_phase(vec![ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ])), + ); + // Negative: an unrelated condition must NOT be captured by this arm. assert_ne!( parse("it is your turn"), Some(main()), "bare 'your turn' (no phase name) must not match the current-phase arm", ); + // The new indefinite-article axis must not swallow an unrelated "it's a + // " clause: `parse_phase_name_set` still has to match a phase + // NAME. ("it's a creature" is claimed by the revealed-card-type arm, so + // the assertion is on the shape, not on `None`.) + assert!( + !matches!( + parse("it's a creature"), + Some(AbilityCondition::CurrentPhaseIs { .. }) + ), + "the determiner arm must not capture a non-phase noun phrase", + ); } /// CR 120.10: both voices of the excess-damage condition route through the diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 8c6c7f8ee8..4af5caa390 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -9843,18 +9843,107 @@ fn parse_additional_phase_count(lower: &str) -> QuantityExpr { terminated(alt((event_bound, literal)), tag(" additional")).parse(input) } - let mut remaining = lower; - while !remaining.is_empty() { - if let Ok((_rest, qty)) = count_combinator(remaining) { - return qty; - } - // Advance to the next word boundary so the combinator stays anchored - // to candidate quantifier positions. - remaining = remaining - .find(' ') - .map_or("", |i| remaining[i + 1..].trim_start()); + // The scan is the shared `scan_at_word_boundaries` building block: try the + // composed combinator at each word boundary, first match wins. + nom_primitives::scan_at_word_boundaries(lower, count_combinator) + .unwrap_or(QuantityExpr::Fixed { value: 1 }) +} + +/// CR 500.8 + CR 608.2c: which phase type an "after this … phase" clause names. +/// +/// A typed axis rather than a discarded `opt()`: the qualifier is the ONLY +/// information distinguishing two different rules classes that share one anchor +/// value, so throwing it away merges them at zero information gain and leaves +/// the resolver unable to re-derive which one it holds. +/// +/// Census over `data/mtgjson/AtomicCards.json`, restricted to faces that also +/// contain "additional combat phase" (the only wording that reaches this +/// production): 36 unqualified, 10 main-qualified, 1 combat-qualified. The bare +/// phrase "after this phase" appears on 42 card faces in total; the other six +/// are beginning-phase and upkeep-step insertions claimed by the sibling arms. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ThisPhaseQualifier { + /// "after this phase" — no phase type is named, so the clause applies in + /// whichever phase the effect resolves in (36 cards: Aurelia, Godo, + /// Overpowering Attack, Take the Bait, Moraug, Najeela, …). + Unqualified, + /// "after this **main** phase" (10 card faces: Aggravated Assault, Drench + /// the Soil in Their Blood, Full Throttle, Fury of the Horde, Last Night + /// Together, Relentless Assault, the Resurgence face of Response // + /// Resurgence, Seize the Day, Waves of Aggression, Wyll of the Blade Pact). + Main, + /// "after this **combat** phase" (1 card: Raphael, Tag Team Tough). + Combat, +} + +/// CR 500.8 + CR 608.2c: "after this [main|combat] phase" — the insertion point +/// is the phase the effect RESOLVES in, not a phase named on the card. One +/// production covers all three surface forms; the optional qualifier is a single +/// `alt` axis (Overpowering Attack "after this phase", Relentless Assault "after +/// this main phase", Raphael, Tag Team Tough "after this combat phase"). Ordinal +/// anchors ("after the first combat phase this turn" — Swinging Ship; "after the +/// second main phase this turn" — World at War, CR 505.1b) deliberately do NOT +/// match: they name a specific phase of the turn rather than the resolving one. +fn parse_this_phase_anchor(input: &str) -> OracleResult<'_, ThisPhaseQualifier> { + map( + ( + tag::<_, _, OracleError<'_>>("after this "), + opt(alt(( + value(ThisPhaseQualifier::Main, tag("main ")), + value(ThisPhaseQualifier::Combat, tag("combat ")), + ))), + tag("phase"), + ), + |(_, qualifier, _)| qualifier.unwrap_or(ThisPhaseQualifier::Unqualified), + ) + .parse(input) +} + +/// CR 608.2c + CR 500.8: the implicit precondition carried by a qualified +/// "after this **main** phase" clause. +/// +/// CR 500.8 adds an extra phase "directly after the specified phase". When the +/// clause names a phase TYPE and the effect resolves outside that type, there is +/// no specified phase to add anything after, so nothing is created — CR 608.2c +/// ("read the whole text and apply the rules of English"). Gatherer states this +/// verbatim for the whole class: +/// +/// * Fury of the Horde / Waves of Aggression — "If it's somehow not a main +/// phase when [this] 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." +/// * Full Throttle — "If you somehow cast this spell when it's not a main +/// phase, the second ability still takes effect, but there are no additional +/// combat phases this turn." +/// +/// This is reachable: Vedalken Orrery / Leyline of Anticipation let these +/// resolve mid-combat or in the end step. +/// +/// The gate is the BARE `CurrentPhaseIs` — deliberately not +/// `And([CurrentPhaseIs, IsYourTurn])`. Relentless Assault's other ruling +/// confirms an opponent's main phase works: "If you manage to cast this during a +/// main phase of your opponent's turn, that opponent's creatures will untap and +/// that opponent will be able to attack again." That makes the implicit +/// precondition converge on exactly the representation the explicit +/// "If it's a main phase, …" grammar already produces (`conditions.rs`, +/// `PhasePossessive::Any`). +/// +/// The gate attaches to the additional-phase clause ONLY, never to the whole +/// ability: the rulings are explicit that the untap still happens. +/// +/// `Combat` intentionally carries no gate. Its single card (Raphael, Tag Team +/// Tough) triggers on combat damage, so the gate could never be false, and no +/// ruling establishes the reading for that wording — adding one would be an +/// unwitnessed behavior change. The mapping lives here so a future producer only +/// has to fill in this arm. +fn this_phase_anchor_gate(qualifier: ThisPhaseQualifier) -> Option { + match qualifier { + ThisPhaseQualifier::Main => Some(AbilityCondition::CurrentPhaseIs { + phases: vec![Phase::PreCombatMain, Phase::PostCombatMain], + }), + ThisPhaseQualifier::Unqualified | ThisPhaseQualifier::Combat => None, } - QuantityExpr::Fixed { value: 1 } } /// CR 701.4a: Recognize a "behold a [quality]" effect leaf. "Behold a [quality]" @@ -10127,16 +10216,26 @@ pub(super) fn parse_imperative_family_ast( if nom_primitives::scan_contains(lower, "additional combat phase") { let with_main = nom_primitives::scan_contains(lower, "followed by an additional main phase"); - // CR 500.8 (Full Throttle): "After this main phase, there are N additional - // combat phases" anchors to whichever main phase the spell resolves in. - // `PreCombatMain` is a resolution-time sentinel remapped in - // `effects/additional_phase.rs` when the active phase is a main phase. - let after = if nom_primitives::scan_contains(lower, "after this main phase") { + // CR 500.8 + CR 608.2c: "after this phase" anchors to whichever phase the + // effect resolves in. `Phase::PreCombatMain` is a resolution-time + // SENTINEL that `game/effects/additional_phase.rs` remaps via + // `last_step_of_phase(state.phase)`; it never means the literal precombat + // main phase. Wording with no "after this …" clause (World at War, + // Swinging Ship — CR 505.1b ordinal anchors) keeps the legacy + // end-of-combat default. + // + // The qualifier is carried, not discarded: a "main"-qualified clause + // additionally gates the scheduling on the resolving phase actually being + // a main phase (see `this_phase_anchor_gate` for the CR + Gatherer + // authority). The gate rides on THIS clause only, so a Group A card's + // untap still happens outside a main phase. + let qualifier = nom_primitives::scan_at_word_boundaries(lower, parse_this_phase_anchor); + let after = if qualifier.is_some() { Phase::PreCombatMain } else { Phase::EndCombat }; - return Some(ImperativeFamilyAst::GainKeyword(Effect::AdditionalPhase { + let effect = Effect::AdditionalPhase { target: TargetFilter::Controller, phase: Phase::BeginCombat, after, @@ -10147,7 +10246,14 @@ pub(super) fn parse_imperative_family_ast( }, count: parse_additional_phase_count(lower), attacker_restriction: None, - })); + }; + return Some(match qualifier.and_then(this_phase_anchor_gate) { + Some(condition) => ImperativeFamilyAst::GatedEffect { + effect: Box::new(effect), + condition: Box::new(condition), + }, + None => ImperativeFamilyAst::GainKeyword(effect), + }); } if nom_primitives::scan_contains(lower, "additional upkeep step") { return Some(ImperativeFamilyAst::GainKeyword(Effect::AdditionalPhase { @@ -12887,6 +12993,14 @@ pub(super) fn lower_imperative_family_ast(ast: ImperativeFamilyAst) -> ParsedEff ))); clause } + // CR 608.2c: the gate belongs to THIS clause, not to the ability — a + // sibling clause in the same chain ("Untap all creatures that attacked + // this turn.") must still resolve when the gate is false. + ImperativeFamilyAst::GatedEffect { effect, condition } => { + let mut clause = parsed_clause(*effect); + clause.condition = Some(*condition); + clause + } // All other arms produce a bare Effect with no sub_ability chain. other => parsed_clause(lower_imperative_family_effect(other)), } @@ -13134,6 +13248,16 @@ fn lower_imperative_family_effect(ast: ImperativeFamilyAst) -> Effect { // CR 701.4a: Behold a [quality] — reveal-or-choose keyword action. ImperativeFamilyAst::Behold(filter) => Effect::Behold { filter }, ImperativeFamilyAst::GainKeyword(effect) => effect, + // The gate is carried by the CLAUSE, not the effect, so this node cannot + // be lowered to a bare `Effect` without silently DROPPING a rules-bearing + // condition. Panicking (as `Recruit`/`Assimilate` already do for the same + // reason) keeps that impossible: production always goes through + // `lower_imperative_family_ast`, and any probe that reaches for the bare + // effect is told to do the same instead of quietly testing an ungated + // shape. + ImperativeFamilyAst::GatedEffect { .. } => { + unreachable!("GatedEffect lowering carries a clause-scoped condition") + } ImperativeFamilyAst::LoseKeyword(effect) => effect, ImperativeFamilyAst::LoseTheGame => Effect::LoseTheGame { target: None }, ImperativeFamilyAst::WinTheGame => Effect::WinTheGame { target: None }, @@ -17836,6 +17960,11 @@ mod tests { } } + /// CR 500.8 + CR 608.2c (Full Throttle's verbatim first line): the + /// main-qualified anchor binds the current-phase sentinel AND its implicit + /// precondition. Routed through `lower_imperative_family_ast` — the + /// production lowering — because the gate rides on the clause, not the + /// effect. #[test] fn parse_additional_phase_after_this_main_phase_anchors_to_main() { let text = "After this main phase, there are two additional combat phases."; @@ -17845,10 +17974,10 @@ mod tests { result.is_some(), "Should parse main-phase-anchored additional combats" ); - let effect = lower_imperative_family_effect(result.unwrap()); + let clause = lower_imperative_family_ast(result.unwrap()); assert!( matches!( - effect, + clause.effect, Effect::AdditionalPhase { phase: Phase::BeginCombat, after: Phase::PreCombatMain, @@ -17857,10 +17986,24 @@ mod tests { .. } if followed_by.is_empty() ), - "Expected AdditionalPhase anchored to main phase with count 2, got {effect:?}" + "Expected AdditionalPhase anchored to main phase with count 2, got {:?}", + clause.effect + ); + assert_eq!( + clause.condition, + Some(AbilityCondition::CurrentPhaseIs { + phases: vec![Phase::PreCombatMain, Phase::PostCombatMain], + }), + "CR 608.2c: \"after this MAIN phase\" creates nothing outside a main phase", ); } + /// CR 500.8 + CR 608.2c: bare "after this phase" anchors to the phase the + /// effect RESOLVES in, emitted as the `PreCombatMain` resolution-time + /// sentinel (remapped by `additional_phase::resolve` via + /// `last_step_of_phase(state.phase)`). Before the fix this fell through to a + /// hard-coded `EndCombat`, so a spell resolving in a main phase scheduled its + /// extra combat after a combat phase that had already ended (or never came). #[test] fn parse_additional_phase_phase() { let text = "there is an additional combat phase after this phase"; @@ -17873,15 +18016,113 @@ mod tests { effect, Effect::AdditionalPhase { phase: Phase::BeginCombat, - after: Phase::EndCombat, + after: Phase::PreCombatMain, ref followed_by, .. } if followed_by.is_empty() ), - "Expected AdditionalPhase without main phase, got {effect:?}" + "Expected AdditionalPhase with the current-phase sentinel, got {effect:?}" ); } + /// CR 500.8 + CR 608.2c: all three real surface forms of the "after this … + /// phase" anchor reach the same current-phase sentinel — one production, one + /// optional qualifier axis — but the QUALIFIER is not discarded: naming a + /// phase type is an implicit precondition on the resolving phase, carried as + /// a clause-scoped `CurrentPhaseIs` gate. Verbatim Oracle clauses: + /// * Overpowering Attack / Aurelia — "after this phase" (no gate) + /// * Relentless Assault / Full Throttle — "after this main phase" (gated) + /// * Raphael, Tag Team Tough — "after this combat phase" (no gate today — + /// see `this_phase_anchor_gate`) + /// + /// Routed through `lower_imperative_family_ast`, the PRODUCTION lowering + /// (`oracle_effect::mod.rs`'s clause loop calls exactly this), because the + /// gate lives on the clause, not on the effect. This is the discriminating + /// test for `this_phase_anchor_gate`: returning `None` from its `Main` arm + /// turns the middle row red. + #[test] + fn parse_this_phase_anchor_covers_bare_main_and_combat_qualifiers() { + let main_phase_gate = AbilityCondition::CurrentPhaseIs { + phases: vec![Phase::PreCombatMain, Phase::PostCombatMain], + }; + for (text, expected_condition) in [ + ("there is an additional combat phase after this phase", None), + ( + "after this main phase, there is an additional combat phase", + Some(main_phase_gate.clone()), + ), + ( + "after this combat phase, there is an additional combat phase", + None, + ), + ] { + let lower = text.to_lowercase(); + let clause = lower_imperative_family_ast( + parse_imperative_family_ast(text, &lower, &mut ParseContext::default()) + .unwrap_or_else(|| panic!("{text:?} should parse as AdditionalPhase")), + ); + assert!( + matches!( + clause.effect, + Effect::AdditionalPhase { + phase: Phase::BeginCombat, + after: Phase::PreCombatMain, + .. + } + ), + "{text:?} must bind the current-phase sentinel, got {:?}", + clause.effect + ); + assert_eq!( + clause.condition, expected_condition, + "{text:?}: CR 608.2c gate mismatch — a phase-type-qualified anchor \ + must carry its implicit precondition, and an unqualified one must not", + ); + } + } + + /// CR 505.1b: ordinal anchors name a SPECIFIC phase of the turn ("the first + /// combat phase this turn" — Swinging Ship; "the second main phase this turn" + /// — World at War), not the phase the effect resolves in, so they must NOT + /// take the current-phase sentinel. Paired positive reach-guard: each input + /// must first produce a real `AdditionalPhase { phase: BeginCombat }`, so the + /// negative cannot pass merely because the line failed to parse. + #[test] + fn ordinal_phase_anchors_do_not_take_the_current_phase_sentinel() { + for text in [ + "after the first combat phase this turn, there's an additional combat phase", + "after the second main phase this turn, there's an additional combat phase followed by an additional main phase", + ] { + let lower = text.to_lowercase(); + let effect = lower_imperative_family_effect( + parse_imperative_family_ast(text, &lower, &mut ParseContext::default()) + .unwrap_or_else(|| panic!("{text:?} should parse as AdditionalPhase")), + ); + // Reach guard: a real additional-combat effect, not Unimplemented. + assert!( + matches!( + effect, + Effect::AdditionalPhase { + phase: Phase::BeginCombat, + .. + } + ), + "{text:?} must still parse as an additional combat phase, got {effect:?}" + ); + // The discriminating assertion: no current-phase sentinel. + assert!( + matches!( + effect, + Effect::AdditionalPhase { + after: Phase::EndCombat, + .. + } + ), + "{text:?} is a CR 505.1b ordinal anchor and must keep the legacy default, got {effect:?}" + ); + } + } + #[test] fn parse_pay_any_amount_of_mana_as_variable_mana_cost() { let text = "pay any amount of mana"; @@ -17973,6 +18214,10 @@ mod tests { } } + /// No "after this …" clause at all: the wording keeps the legacy + /// end-of-combat default (CR 500.8). This is the no-anchor-clause arm of the + /// `after` decision, complementing + /// `parse_this_phase_anchor_covers_bare_main_and_combat_qualifiers`. #[test] fn parse_additional_phase_with_main_phase() { let text = "there is an additional combat phase followed by an additional main phase"; diff --git a/crates/engine/src/parser/oracle_ir/ast.rs b/crates/engine/src/parser/oracle_ir/ast.rs index de77f5b30c..c59cafd01a 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -828,6 +828,19 @@ pub(crate) enum ImperativeFamilyAst { /// CR 701.56a: Time travel — add or remove time counters. TimeTravel, GainKeyword(Effect), + /// CR 608.2c: an effect leaf that carries its OWN resolution-time gate, + /// derived from the instruction's own grammar rather than from a leading + /// "If …, " head. Lowers to the effect plus + /// [`ParsedEffectClause::condition`], so the gate scopes to this clause + /// alone and sibling clauses in the same chain are unaffected. + /// + /// Producer: the "after this **main** phase" additional-phase grammar, whose + /// named phase type is an implicit precondition on the phase the effect + /// resolves in (`imperative::this_phase_anchor_gate`). + GatedEffect { + effect: Box, + condition: Box, + }, LoseKeyword(Effect), /// CR 104.3a: "[target player] lose(s) the game" LoseTheGame, diff --git a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_ir.snap b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_ir.snap index 0ae4b554e3..09566c5dd7 100644 --- a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_ir.snap +++ b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_ir.snap @@ -58,7 +58,7 @@ expression: "&ir" "type": "Controller" }, "phase": "BeginCombat", - "after": "EndCombat", + "after": "PreCombatMain", "followed_by": [], "count": { "type": "Fixed", diff --git a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_lowered.snap b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_lowered.snap index 31b185cf17..ffd79b1078 100644 --- a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_lowered.snap +++ b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__combat_celebrant_lowered.snap @@ -39,7 +39,7 @@ expression: "&lowered" "type": "Controller" }, "phase": "BeginCombat", - "after": "EndCombat", + "after": "PreCombatMain", "followed_by": [], "count": { "type": "Fixed", diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 3e2a12a736..482c7e8742 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -15137,6 +15137,27 @@ pub enum Effect { #[serde(default = "default_target_filter_controller")] target: TargetFilter, phase: Phase, + /// CR 500.8: the phase the insertion is added directly after. + /// + /// `after: Phase::PreCombatMain` is a RESOLUTION-TIME SENTINEL meaning + /// "the phase this effect resolves in" (CR 608.2c), remapped by + /// `additional_phase::resolve` via `turns::last_step_of_phase(state.phase)`. + /// It NEVER means a literal precombat-main anchor, and no producer may + /// emit it as one. Any new producer of a literal precombat-main anchor + /// must introduce a typed `PhaseAnchor` first. + /// + /// The sentinel is deliberately class-agnostic and therefore carries NO + /// precondition of its own. The "after this **main** phase" wording (10 + /// cards: Relentless Assault, Fury of the Horde, Full Throttle, …) has + /// one — it creates NOTHING when the effect resolves outside a main + /// phase (Gatherer, Fury of the Horde: "If it's somehow not a main phase + /// when [this] resolves, all it does is untap all creatures that attacked + /// that turn. No new phases are created.") — and that precondition is + /// carried as an `AbilityCondition::CurrentPhaseIs` gate attached to the + /// clause by `oracle_effect::imperative::this_phase_anchor_gate`, NOT by + /// this field. A producer that emits the sentinel for a phase-type-named + /// wording MUST attach that gate too, or the effect will wrongly schedule + /// a phase outside the named phase type. after: Phase, #[serde(default)] followed_by: Vec, diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index 7d86a36bb6..61674e8141 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -16287,15 +16287,27 @@ declare_game_state! { #[serde(default)] pub extra_phases: Vec, - /// CR 500.8 + CR 501.1: LIFO stack of anchor phases for inserted beginning - /// phases (Temple of Atropos, Sphinx/Shadow of the Second Sun, Cyclonus) - /// currently in progress. When such a phase's draw step ends, the turn - /// resumes at the anchor's natural successor (or runs the next queued - /// beginning phase for the same anchor) rather than at the draw step's - /// default successor. Empty outside inserted beginning phases. - /// `#[serde(default)]` so saved games load unchanged. + /// CR 500.8: LIFO stack of return points for the inserted phases currently + /// in progress — beginning phases (Temple of Atropos, Sphinx/Shadow of the + /// Second Sun, Cyclonus), combat phases (Aurelia, Moraug, Relentless + /// Assault), main phases (the "followed by an additional main phase" rider) + /// and inserted steps (Paradox Haze's extra upkeep) alike. When an inserted + /// phase's terminal step ends (`turns::inserted_phase_terminal_step`), the + /// turn either runs the next phase queued after the same anchor or resumes + /// at the anchor's natural successor — never at the inserted phase's own + /// default successor, which is what silently swallowed the natural combat + /// phase / duplicated the postcombat main phase before. Empty outside + /// inserted phases; cleared at every turn boundary. + /// + /// `#[serde(default)]` so saved games without the field load unchanged. The + /// element type changed from a bare `Phase` to [`ExtraPhaseResume`]; a + /// payload carrying the old element shape (e.g. `["PostCombatMain"]`) is + /// migrated at the load boundary by [`ExtraPhaseResumeCompat`]. Every + /// committed serialized `GameState` (`crates/engine/tests/**/*.json.gz`) + /// records `"extra_phase_resume":[]`, and no client / server / ts-rs consumer + /// reads the field. #[serde(default)] - pub extra_phase_resume: Vec, + pub extra_phase_resume: Vec, /// CR 103.1: The current turn-order direction. Durable — persists across /// turns until an effect reverses it again. Default `Normal` is the game's @@ -19156,6 +19168,65 @@ pub struct ExtraPhase { pub attacker_restriction_source: Option, } +/// CR 500.8: one in-progress inserted phase. `anchor` is the phase the insert +/// was added directly after — the turn resumes at `next_phase(anchor)` once the +/// bundle anchored there is exhausted. `inserted` is the phase currently +/// running, from which `turns::inserted_phase_terminal_step` derives the step +/// that ends it. +/// +/// Storing `inserted` rather than a denormalized terminal step keeps the +/// primitive fact in state and the CR 500.10 step-vs-phase derivation in one +/// function. +/// +/// `Deserialize` goes through [`ExtraPhaseResumeCompat`] so a session persisted +/// before this type existed still loads — see that type for the migration. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(from = "ExtraPhaseResumeCompat")] +pub struct ExtraPhaseResume { + /// The phase this insert was added directly after (CR 500.8). + pub anchor: Phase, + /// The inserted phase currently in progress. + pub inserted: Phase, +} + +/// Load-boundary compatibility shape for [`ExtraPhaseResume`]. +/// +/// `extra_phase_resume` used to be a `Vec` holding only the ANCHOR of an +/// inserted BEGINNING phase (`Temple of Atropos`, `Sphinx of the Second Sun`, +/// `Cyclonus`), because that was the only insertion kind that needed a return +/// point. `#[serde(default)]` on the field covers a payload that omits it +/// entirely, but NOT one that carries the old element shape — and such payloads +/// are real: `advance_phase_once` can return `PhaseEntryOutcome::Paused` inside +/// an inserted beginning phase (an untap-choice prompt under Winter Orb during +/// a Temple of Atropos insert), and a `Paused` phase transition is a durable +/// save point. Such a session serializes `"extra_phase_resume":["PostCombatMain"]` +/// and would otherwise fail to deserialize across a deploy. +/// +/// The legacy element is mapped to what it meant: an inserted beginning phase +/// (CR 501.1 — `Phase::Untap` is the beginning-phase marker) anchored at the +/// recorded phase. +#[derive(Debug, Clone, Copy, Deserialize)] +#[serde(untagged)] +pub enum ExtraPhaseResumeCompat { + /// The current shape. + Current { anchor: Phase, inserted: Phase }, + /// Pre-`ExtraPhaseResume` saves: a bare anchor phase. + LegacyBeginningPhaseAnchor(Phase), +} + +impl From for ExtraPhaseResume { + fn from(compat: ExtraPhaseResumeCompat) -> Self { + match compat { + ExtraPhaseResumeCompat::Current { anchor, inserted } => Self { anchor, inserted }, + // CR 501.1: the old stack only ever recorded inserted beginning phases. + ExtraPhaseResumeCompat::LegacyBeginningPhaseAnchor(anchor) => Self { + anchor, + inserted: Phase::Untap, + }, + } + } +} + // Pin `GameState: Send + Sync` at compile time. Blocks accidental imports of // `im-rc` (the single-threaded variant of `im`, which is !Send/!Sync) and // catches any future field addition that violates thread-safety. @@ -28012,6 +28083,52 @@ mod tests { /// via `#[serde(alias)]` + `#[serde(default)]`. The `UntilStackEmpty` arm is /// asserted unchanged as a positive reach-guard proving the alias captured /// the right tag and did not disturb the sibling variant. + /// CR 500.8 + CR 501.1: `extra_phase_resume` used to be a `Vec` of + /// inserted-BEGINNING-phase anchors. A session persisted while + /// `advance_phase_once` was `Paused` inside such an insertion (an untap + /// choice under Winter Orb during a Temple of Atropos insert) is a durable + /// save point carrying `["PostCombatMain"]`, and `#[serde(default)]` does not + /// help — the field is present, only its element shape changed. The compat + /// shim maps the legacy element to what it meant. + #[test] + fn extra_phase_resume_legacy_bare_phase_element_deserializes() { + // Legacy element shape. + assert_eq!( + serde_json::from_str::>(r#"["PostCombatMain"]"#).unwrap(), + vec![ExtraPhaseResume { + anchor: Phase::PostCombatMain, + inserted: Phase::Untap, + }], + ); + // Positive reach-guard: the current shape still round-trips, so the + // untagged shim did not swallow it. + let current = ExtraPhaseResume { + anchor: Phase::PreCombatMain, + inserted: Phase::BeginCombat, + }; + let encoded = serde_json::to_string(¤t).unwrap(); + assert_eq!( + encoded, + r#"{"anchor":"PreCombatMain","inserted":"BeginCombat"}"# + ); + assert_eq!( + serde_json::from_str::(&encoded).unwrap(), + current, + ); + // Whole-`GameState` load boundary: a legacy payload restores. + let mut raw = serde_json::to_value(GameState::default()).unwrap(); + raw["extra_phase_resume"] = serde_json::json!(["PostCombatMain"]); + let restored = serde_json::from_value::(raw) + .expect("a legacy extra_phase_resume payload must still load"); + assert_eq!( + restored.extra_phase_resume, + vec![ExtraPhaseResume { + anchor: Phase::PostCombatMain, + inserted: Phase::Untap, + }], + ); + } + #[test] fn auto_pass_mode_legacy_eot_deserializes() { assert_eq!( diff --git a/crates/engine/tests/fixtures/integration_cards.json.gz b/crates/engine/tests/fixtures/integration_cards.json.gz index 81f01c376d..8b93c6655d 100644 Binary files a/crates/engine/tests/fixtures/integration_cards.json.gz and b/crates/engine/tests/fixtures/integration_cards.json.gz differ diff --git a/crates/engine/tests/integration/incredible_hulk_enrage_attacking.rs b/crates/engine/tests/integration/incredible_hulk_enrage_attacking.rs index 845505cd88..00022a80c6 100644 --- a/crates/engine/tests/integration/incredible_hulk_enrage_attacking.rs +++ b/crates/engine/tests/integration/incredible_hulk_enrage_attacking.rs @@ -92,6 +92,12 @@ fn run_enrage(attacking: bool) -> EnrageOutcome { // CR 500.10a: the additional-combat-phase guard only adds the phase to the // controller's own turn — make Hulk's controller the active player. runner.state_mut().active_player = P0; + // CR 500.8 + CR 608.2c: "there is an additional combat phase after this phase" + // anchors to the phase the trigger RESOLVES in, so the fixture must sit in the + // phase this trigger actually resolves in — the combat damage step, where an + // attacking creature is dealt damage. Held constant across both cases, so + // attacking-status remains the sole variable. + runner.state_mut().phase = Phase::CombatDamage; // Pre-tap Hulk so the chained "untap him" rider has an observable to flip. // CR 508.1f: a declared attacker is normally tapped; for the not-attacking diff --git a/crates/engine/tests/integration/issue_7240_additional_phase_anchor.rs b/crates/engine/tests/integration/issue_7240_additional_phase_anchor.rs new file mode 100644 index 0000000000..8afc57f487 --- /dev/null +++ b/crates/engine/tests/integration/issue_7240_additional_phase_anchor.rs @@ -0,0 +1,698 @@ +//! Issue #7240 — "after this phase" must anchor to the phase the effect +//! RESOLVES in, and the turn must resume at that anchor's natural successor. +//! +//! https://github.com/phase-rs/phase/issues/7240 +//! +//! Three defects, one root cause: +//! +//! 1. The parser only recognized the "after this **main** phase" wording, so +//! every bare "after this phase" card fell through to a hard-coded +//! `Phase::EndCombat` anchor. Cast in a main phase, the scheduled entry was +//! anchored at a combat phase that had already ended (postcombat) or that +//! would consume it (precombat), so the extra combat phase never happened. +//! 2. `advance_phase_once` REPLACED the natural successor with the inserted +//! phase instead of inserting before it. With every anchor previously in use +//! (EndCombat/Upkeep/End/Untap) the anchor's successor and the inserted +//! phase's own successor coincide, so the defect was invisible — for +//! main-phase anchors they diverge, swallowing the turn's own combat phase +//! (precombat) or duplicating its main phase (postcombat). +//! 3. `count > 1` re-anchored every bundle after the first at `EndCombat`, +//! which is exactly the slot the turn's natural combat would resume in. +//! +//! CR references: +//! - CR 500.8: extra phases are added directly after the specified phase; if +//! several are created after the same phase, the most recent occurs first. +//! This is also the authority for "additional" meaning ADDED, never +//! substituted for a phase the turn already has. +//! - CR 608.2c: "read the whole text and apply the rules of English" — the +//! authority for "this phase" denoting the phase the effect resolves in, and +//! for a phase-type-qualified "this MAIN phase" carrying an implicit +//! precondition on that resolving phase. +//! - CR 505.1a: classifies which main phase of such a turn is the precombat +//! one; it corroborates but does not establish the "additional" reading. +//! - CR 506.1: a combat phase is its five steps, in order. + +use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::types::actions::GameAction; +use engine::types::game_state::ExtraPhase; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; + +// --------------------------------------------------------------------------- +// Verbatim Oracle text (MTGJSON `AtomicCards.json`). +// --------------------------------------------------------------------------- + +const OVERPOWERING_ATTACK: &str = "Freerunning {2}{R} (You may cast this spell for its freerunning cost if you dealt combat damage to a player this turn with an Assassin or commander.)\nUntap all creatures you control that attacked this turn. If it's your main phase, there is an additional combat phase after this phase, followed by an additional main phase."; + +const RELENTLESS_ASSAULT: &str = "Untap all creatures that attacked this turn. After this main phase, there is an additional combat phase followed by an additional main phase."; + +const FULL_THROTTLE: &str = "After this main phase, there are two additional combat phases.\nAt the beginning of each combat this turn, untap all creatures that attacked this turn."; + +const MORAUG: &str = "Each creature you control gets +1/+0 for each time it has attacked this turn.\nLandfall — Whenever a land you control enters, if it's your main phase, there's an additional combat phase after this phase. At the beginning of that combat, untap all creatures you control."; + +const AURELIA: &str = "Flying, vigilance, haste\nWhenever Aurelia attacks for the first time each turn, untap all creatures you control. After this phase, there is an additional combat phase."; + +const ALL_OUT_ASSAULT: &str = "Creatures you control get +1/+1 and have deathtouch.\nWhen this enchantment enters, if it's your main phase, there is an additional combat phase after this phase followed by an additional main phase. When you next attack this turn, untap each creature you control."; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/// CR 506.1: the five steps of one combat phase, in order. +fn combat() -> [Phase; 5] { + [ + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + ] +} + +/// Walk the production phase machine and record every phase entered, stopping +/// at the end step (or after `limit` hops). +fn walk_phases(runner: &mut GameRunner, limit: usize) -> Vec { + let mut seq = Vec::new(); + let mut events = Vec::new(); + for _ in 0..limit { + engine::game::turns::advance_phase(runner.state_mut(), &mut events); + seq.push(runner.state().phase); + if runner.state().phase == Phase::End { + break; + } + } + seq +} + +/// Cast a 0-cost copy of `oracle` from `player`'s hand in `phase` and return the +/// runner positioned at that phase with the spell resolved. +fn cast_in_phase(name: &str, oracle: &str, phase: Phase) -> GameRunner { + let mut scenario = GameScenario::new(); + scenario.at_phase(phase); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, name, false, oracle) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + runner.cast(spell).resolve(); + runner.advance_until_stack_empty(); + assert!( + !runner.state().extra_phases.is_empty(), + "reach guard: {name} must schedule at least one extra phase; \ + extra_phases={:?}", + runner.state().extra_phases + ); + runner +} + +// --------------------------------------------------------------------------- +// The "after this MAIN phase" precondition (Group A). +// --------------------------------------------------------------------------- + +const FURY_OF_THE_HORDE: &str = "You may exile two red cards from your hand rather than pay this spell's mana cost.\nUntap all creatures that attacked this turn. After this main phase, there is an additional combat phase followed by an additional main phase."; + +/// CR 608.2c + CR 500.8: the "after this **main** phase" wording names a phase +/// TYPE, so outside a main phase there is no specified phase to add anything +/// after and NOTHING is created — while the rest of the instruction still +/// happens. Gatherer, verbatim: +/// +/// * Fury of the Horde / Waves of Aggression — "If it's somehow not a main +/// phase when [this] 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**." +/// * Full Throttle — "If you somehow cast this spell when it's not a main +/// phase, the second ability still takes effect, but **there are no +/// additional combat phases this turn**." +/// +/// Reachable in production: Vedalken Orrery and Leyline of Anticipation give +/// these flash. The fixture models that by casting them as instants. +/// +/// This is the discriminating test for +/// `oracle_effect::imperative::this_phase_anchor_gate` — returning `None` from +/// its `Main` arm makes every row schedule a real extra combat phase and turns +/// this test red. +#[test] +fn main_qualified_anchor_creates_no_phases_outside_a_main_phase() { + for (name, oracle) in [ + ("Relentless Assault", RELENTLESS_ASSAULT), + ("Fury of the Horde", FURY_OF_THE_HORDE), + ("Full Throttle", FULL_THROTTLE), + ] { + for phase in [Phase::DeclareBlockers, Phase::End] { + let mut scenario = GameScenario::new(); + scenario.at_phase(phase); + // A creature that attacked this turn and is still tapped — the + // observable for "all it does is untap". + let attacker = scenario.add_creature(P0, "Bear", 2, 2).id(); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, name, true, oracle) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + runner.state_mut().objects[&attacker].tapped = true; + runner + .state_mut() + .creatures_attacked_this_turn + .insert(attacker); + + runner.cast(spell).resolve(); + runner.advance_until_stack_empty(); + + assert!( + runner.state().extra_phases.is_empty(), + "{name} resolving in {phase:?} must create NO extra phases; got {:?}", + runner.state().extra_phases, + ); + + // The paired positive reach-guard: the spell really resolved and its + // ungated sibling clause still executed. Full Throttle's other + // ability is a delayed trigger with no resolution-time observable, + // so it is exempt. + if name != "Full Throttle" { + assert!( + !runner.state().objects[&attacker].tapped, + "{name} resolving in {phase:?} must still untap the creature that \ + attacked this turn — the gate is scoped to the additional-phase \ + clause, not to the whole spell", + ); + } + } + } +} + +/// The gate-true half of the pair above: the SAME cards resolving inside a main +/// phase still schedule their phases (CR 500.8). Without this row the test above +/// would pass just as well if the gate were `AbilityCondition::Never`. +#[test] +fn main_qualified_anchor_still_schedules_inside_a_main_phase() { + for (name, oracle, expected_entries) in [ + ("Relentless Assault", RELENTLESS_ASSAULT, 2), + ("Fury of the Horde", FURY_OF_THE_HORDE, 2), + ("Full Throttle", FULL_THROTTLE, 2), + ] { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PostCombatMain); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, name, false, oracle) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + runner.cast(spell).resolve(); + runner.advance_until_stack_empty(); + + assert_eq!( + runner.state().extra_phases.len(), + expected_entries, + "{name} resolving in a main phase must still schedule its phases; got {:?}", + runner.state().extra_phases, + ); + assert!( + runner + .state() + .extra_phases + .iter() + .all(|ep| ep.anchor == Phase::PostCombatMain), + "{name}: every entry anchors at the main phase it resolved in; got {:?}", + runner.state().extra_phases, + ); + } +} + +// --------------------------------------------------------------------------- +// Overpowering Attack — the reported card. +// --------------------------------------------------------------------------- + +/// CR 500.8: cast in the postcombat main phase, Overpowering Attack +/// grants exactly ONE extra combat phase and ONE extra main phase, then the turn +/// ends. +/// +/// Reverting STEP 1/2 (the parser sentinel + the resolver remap) leaves the +/// anchor at `EndCombat`, which the postcombat main phase never returns to — no +/// extra combat at all, the reported bug. Reverting the resume frame gives a +/// SECOND postcombat main phase (the turn resumed at +/// `next_phase(PostCombatMain)` of the *inserted* main instead of at the +/// anchor's successor). +#[test] +fn overpowering_attack_in_postcombat_main_grants_one_combat_and_one_main() { + let mut runner = cast_in_phase( + "Overpowering Attack", + OVERPOWERING_ATTACK, + Phase::PostCombatMain, + ); + + let seq = walk_phases(&mut runner, 24); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); + expected.push(Phase::PostCombatMain); // the additional main phase + expected.push(Phase::End); // …and then the turn ends + assert_eq!(seq, expected); + assert_eq!( + seq.iter().filter(|p| **p == Phase::PostCombatMain).count(), + 1, + "CR 500.8: exactly one additional main phase — not two", + ); + assert!(runner.state().extra_phase_resume.is_empty()); +} + +/// CR 500.8: cast in the PREcombat main phase, the inserted combat phase runs +/// directly after that main phase and the turn's OWN combat phase still follows. +/// Reverting the resume frame collapses this to a single combat phase — the +/// inserted one eats the natural one's slot. +#[test] +fn overpowering_attack_in_precombat_main_inserts_before_the_natural_combat() { + let mut runner = cast_in_phase( + "Overpowering Attack", + OVERPOWERING_ATTACK, + Phase::PreCombatMain, + ); + + let seq = walk_phases(&mut runner, 32); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // inserted combat phase + expected.push(Phase::PostCombatMain); // inserted main phase + expected.extend(combat()); // the turn's own combat phase + expected.push(Phase::PostCombatMain); // the turn's own postcombat main + expected.push(Phase::End); + assert_eq!(seq, expected); + assert!(runner.state().extra_phase_resume.is_empty()); +} + +/// CR 500.8 + CR 506.1 (B4.3): because the inserted combat phase now runs FIRST, +/// it is the turn's first combat phase. `combat_phases_started_this_turn` is the +/// sole input of the `FirstCombatPhaseOfTurn` gate (`game/triggers.rs` and +/// `game/effects/mod.rs` both compare it to 1), so cards gated on "the first +/// combat phase of the turn" (Genji Glove, Hexplate Wallbreaker, Karlach, +/// Finest Hour, Raiyuu, Raph & Leo, Balthier and Fran) now fire in the inserted +/// combat rather than the natural one. This is CR-correct but a real behavior +/// change, so it is pinned here. +#[test] +fn precombat_insert_shifts_which_combat_is_the_turns_first() { + let mut runner = cast_in_phase( + "Overpowering Attack", + OVERPOWERING_ATTACK, + Phase::PreCombatMain, + ); + assert_eq!( + runner.state().combat_phases_started_this_turn, + 0, + "no combat phase has begun yet this turn", + ); + + let mut events = Vec::new(); + // For each combat phase entered: (ordinal the gate reads, is this an + // INSERTED combat?). A live resume frame means the turn is inside an + // insertion and still owes a return point, so it identifies the inserted + // combat without reaching into the parser or the AST. + let mut combats: Vec<(u32, bool)> = Vec::new(); + for _ in 0..32 { + engine::game::turns::advance_phase(runner.state_mut(), &mut events); + if runner.state().phase == Phase::BeginCombat { + combats.push(( + runner.state().combat_phases_started_this_turn, + !runner.state().extra_phase_resume.is_empty(), + )); + } + if runner.state().phase == Phase::End { + break; + } + } + + assert_eq!( + combats, + vec![(1, true), (2, false)], + "the INSERTED combat is the turn's FIRST combat phase and the turn's own \ + combat phase is its second; before the fix the order was reversed (the \ + insertion was anchored at a combat phase that had not happened yet)", + ); +} + +// --------------------------------------------------------------------------- +// Relentless Assault — the follow-up main double count (D4). +// --------------------------------------------------------------------------- + +/// CR 500.8: one extra combat phase and one extra main phase. Before +/// the fix the "followed by an additional main phase" entry was anchored at +/// `PostCombatMain` while the turn resumed at `next_phase(EndCombat)`, which is +/// also `PostCombatMain` — three main phases for a card that grants one. +#[test] +fn relentless_assault_in_postcombat_main_does_not_double_the_follow_up_main() { + let mut runner = cast_in_phase( + "Relentless Assault", + RELENTLESS_ASSAULT, + Phase::PostCombatMain, + ); + + let seq = walk_phases(&mut runner, 24); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(seq, expected); + assert_eq!( + seq.iter().filter(|p| **p == Phase::PostCombatMain).count(), + 1, + "CR 500.8: the follow-up main phase must be entered exactly once", + ); +} + +/// CR 500.8 — a SECOND bundle created inside the additional main phase the first +/// one created. This is Aggravated Assault's primary line (`{3}{R}{R}: Untap all +/// creatures you control. After this main phase, there is an additional combat +/// phase followed by an additional main phase. Activate only as a sorcery.` — +/// re-activated in the extra main it just made); the fixture drives the same +/// clause through the cast pipeline with two Relentless Assaults, whose printed +/// text carries that sentence verbatim, so no mana or "activate only as a +/// sorcery" plumbing sits between the test and the phase machine. +/// +/// CR 500.8 is positional: each bundle is added "directly after the specified +/// phase". The second cast's specified phase is the ADDITIONAL main phase it +/// resolves in, so its combat and follow-up main run back to back there, ahead +/// of the turn's own combat phase — which the FIRST bundle's anchor still owes. +/// +/// Regression: the second bundle's follow-up main was deferred past the natural +/// combat phase and only picked up by the natural postcombat main's own anchor +/// scan, giving `… extra combat, extra main, extra combat, NATURAL combat, +/// natural main, extra main, end`. Both stacks still drained, so the defect was +/// pure ordering — a count-only assertion cannot see it, which is why the whole +/// phase sequence is asserted here. +#[test] +fn a_second_bundle_created_in_an_additional_main_precedes_the_natural_combat() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let first = scenario + .add_spell_to_hand_from_oracle(P0, "Relentless Assault", false, RELENTLESS_ASSAULT) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let second = scenario + .add_spell_to_hand_from_oracle(P0, "Relentless Assault", false, RELENTLESS_ASSAULT) + .with_mana_cost(ManaCost::generic(0)) + .id(); + let mut runner = scenario.build(); + + runner.cast(first).resolve(); + runner.advance_until_stack_empty(); + assert_eq!( + runner.state().extra_phases.len(), + 2, + "reach guard: the first cast schedules a combat + main bundle; got {:?}", + runner.state().extra_phases, + ); + + let mut events = Vec::new(); + let mut seq = Vec::new(); + let mut recast = false; + for _ in 0..40 { + engine::game::turns::advance_phase(runner.state_mut(), &mut events); + seq.push(runner.state().phase); + // Re-cast in the FIRST additional main phase — the phase the first + // bundle created — so the second bundle's anchor is that phase. + if !recast && runner.state().phase == Phase::PostCombatMain { + recast = true; + runner.cast(second).resolve(); + runner.advance_until_stack_empty(); + assert_eq!( + runner.state().phase, + Phase::PostCombatMain, + "reach guard: the second cast resolves in the additional main phase", + ); + assert_eq!( + runner.state().extra_phases.len(), + 2, + "reach guard: the second cast schedules its own bundle; got {:?}", + runner.state().extra_phases, + ); + } + if runner.state().phase == Phase::End { + break; + } + } + assert!(recast, "reach guard: an additional main phase was entered"); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // first bundle's combat + expected.push(Phase::PostCombatMain); // first bundle's main — re-cast here + expected.extend(combat()); // second bundle's combat + expected.push(Phase::PostCombatMain); // second bundle's main + expected.extend(combat()); // the turn's OWN combat phase — still owed + expected.push(Phase::PostCombatMain); // the turn's own postcombat main + expected.push(Phase::End); + assert_eq!( + seq, expected, + "CR 500.8: the second bundle is inserted directly after the main phase it \ + resolved in, not after the first bundle's anchor", + ); + assert!( + runner.state().extra_phases.is_empty(), + "no entry may be stranded: {:?}", + runner.state().extra_phases, + ); + assert!( + runner.state().extra_phase_resume.is_empty(), + "no return point may be stranded: {:?}", + runner.state().extra_phase_resume, + ); +} + +// --------------------------------------------------------------------------- +// Full Throttle — count > 1 (H2). +// --------------------------------------------------------------------------- + +/// CR 500.8: "there are two additional combat phases" means two +/// combats ADDITIONAL to the turn's own — three in total when cast precombat. +/// The old `EndCombat` re-anchor for the second bundle made it reachable only by +/// consuming the natural combat's slot, so the turn ran two. +#[test] +fn full_throttle_precombat_grants_three_combat_phases() { + let mut runner = cast_in_phase("Full Throttle", FULL_THROTTLE, Phase::PreCombatMain); + assert_eq!(runner.state().extra_phases.len(), 2); + + let seq = walk_phases(&mut runner, 40); + + assert_eq!( + seq.iter().filter(|p| **p == Phase::BeginCombat).count(), + 3, + "two additional combat phases plus the turn's own; got {seq:?}", + ); + let mut expected: Vec = Vec::new(); + expected.extend(combat()); + expected.extend(combat()); + expected.extend(combat()); + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(seq, expected); + assert!(runner.state().extra_phase_resume.is_empty()); +} + +// --------------------------------------------------------------------------- +// Moraug — the trigger path, precombat (H3: ordered sequence, not a count). +// --------------------------------------------------------------------------- + +/// Build Moraug on the battlefield with a land in hand, play the land in the +/// precombat main phase, and let the landfall trigger resolve. +fn moraug_landfall_in_precombat_main() -> GameRunner { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_creature_from_oracle(P0, "Moraug, Fury of Akoum", 6, 6, MORAUG); + let land = scenario.add_land_to_hand(P0, "Mountain").id(); + + let mut runner = scenario.build(); + let card_id = runner.state().objects[&land].card_id; + runner + .act(GameAction::PlayLand { + object_id: land, + card_id, + }) + .expect("play the land"); + runner.advance_until_stack_empty(); + + // Reach guard: the landfall trigger really resolved and scheduled a phase + // anchored at the main phase it resolved in (CR 500.8 + CR 608.2c). + assert_eq!( + runner + .state() + .extra_phases + .iter() + .map(|ep| (ep.anchor, ep.phase)) + .collect::>(), + vec![(Phase::PreCombatMain, Phase::BeginCombat)], + "Moraug's landfall trigger must schedule one combat phase after THIS main phase", + ); + runner +} + +/// CR 500.8 (H3): assert the ORDERED sequence, not the combat count — a count +/// assertion passes both before and after the fix (the pre-fix engine also +/// reached one combat phase, just the wrong one). The discriminating property is +/// that the inserted combat comes BEFORE the turn's own combat phase and both +/// occur. +#[test] +fn moraug_landfall_in_precombat_main_inserts_before_the_natural_combat() { + let mut runner = moraug_landfall_in_precombat_main(); + + let seq = walk_phases(&mut runner, 32); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // the landfall combat, directly after this main phase + expected.extend(combat()); // the turn's own combat phase + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(seq, expected); + assert!(runner.state().extra_phase_resume.is_empty()); +} + +/// CR 500.8 (B2 — the nested-insertion guard): a combat-triggered extra combat +/// (Port Razer / Combat Celebrant / Scourge of the Throne, all of which schedule +/// `anchor: EndCombat, phase: BeginCombat` when they resolve during combat — +/// proven by `current_phase_sentinel_resolves_to_end_combat_from_every_combat_step`) +/// firing INSIDE Moraug's inserted combat creates a nested return point. Both +/// return points end at the same `EndCombat`, so the resume must unwind to the +/// OUTERMOST one; inspecting only the innermost frame orphans Moraug's and the +/// turn loses its natural combat phase. +#[test] +fn moraug_precombat_with_a_repeatable_combat_trigger_keeps_the_natural_combat() { + let mut runner = moraug_landfall_in_precombat_main(); + + let mut events = Vec::new(); + let mut seq = Vec::new(); + let mut injected = false; + for _ in 0..40 { + engine::game::turns::advance_phase(runner.state_mut(), &mut events); + seq.push(runner.state().phase); + // The Port Razer-shaped trigger connects during the FIRST (inserted) + // combat and schedules another combat after "this phase". + if !injected && runner.state().phase == Phase::CombatDamage { + injected = true; + runner.state_mut().extra_phases.push(ExtraPhase { + anchor: Phase::EndCombat, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + } + if runner.state().phase == Phase::End { + break; + } + } + assert!(injected, "the nested extra combat must have been scheduled"); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); // Moraug's inserted combat + expected.extend(combat()); // the nested trigger's combat + expected.extend(combat()); // the turn's OWN combat phase — still owed + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(seq, expected); + assert!( + runner.state().extra_phase_resume.is_empty(), + "every return point is repaid by the end of the turn", + ); +} + +// --------------------------------------------------------------------------- +// Behavior preservation for the 30+ combat-resolved cards (Group B). +// --------------------------------------------------------------------------- + +/// CR 500.8 + CR 506.1: Aurelia's "after this phase" resolves during combat, so +/// the sentinel remaps to `EndCombat` — byte-identical scheduling to the +/// pre-change hard-coded default, and an unchanged phase sequence. Green before +/// AND after; this is the regression guard for the whole class. +#[test] +fn aurelia_attack_trigger_extra_combat_sequence_unchanged() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::DeclareAttackers); + let aurelia = scenario + .add_creature_from_oracle(P0, "Aurelia, the Warleader", 3, 4, AURELIA) + .id(); + let mut runner = scenario.build(); + + // Reach guard: the card parsed into a real additional-combat trigger, not an + // `Unimplemented` gap. + let triggers = format!("{:?}", runner.state().objects[&aurelia].trigger_definitions); + assert!( + triggers.contains("AdditionalPhase"), + "Aurelia must carry a parsed AdditionalPhase trigger, got {triggers}", + ); + assert!( + !triggers.contains("Unimplemented"), + "Aurelia's triggers must contain no Unimplemented gap", + ); + + // Schedule the extra combat exactly as the trigger's resolver does when it + // resolves during the declare-attackers step (`last_step_of_phase` → + // `EndCombat`). + runner.state_mut().extra_phases.push(ExtraPhase { + anchor: Phase::EndCombat, + phase: Phase::BeginCombat, + attacker_restriction: None, + attacker_restriction_source: None, + }); + + let seq = walk_phases(&mut runner, 24); + + assert_eq!( + seq, + vec![ + // the rest of the natural combat + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + // the extra combat, directly after it + Phase::BeginCombat, + Phase::DeclareAttackers, + Phase::DeclareBlockers, + Phase::CombatDamage, + Phase::EndCombat, + // then the turn resumes at the anchor's natural successor + Phase::PostCombatMain, + Phase::End, + ] + ); + assert!(runner.state().extra_phase_resume.is_empty()); +} + +// --------------------------------------------------------------------------- +// All-Out Assault — the ETB trigger path (Group C). +// --------------------------------------------------------------------------- + +/// CR 500.8: the same shape as Overpowering Attack, reached through +/// an enters-the-battlefield trigger with an "if it's your main phase" gate +/// rather than through a spell's resolution. +#[test] +fn all_out_assault_etb_in_postcombat_main() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PostCombatMain); + let assault = scenario + .add_spell_to_hand_from_oracle(P0, "All-Out Assault", false, ALL_OUT_ASSAULT) + .with_mana_cost(ManaCost::generic(0)) + .as_enchantment() + .id(); + let mut runner = scenario.build(); + runner.cast(assault).resolve(); + runner.advance_until_stack_empty(); + + assert_eq!( + runner + .state() + .extra_phases + .iter() + .map(|ep| (ep.anchor, ep.phase)) + .collect::>(), + vec![ + (Phase::PostCombatMain, Phase::PostCombatMain), + (Phase::PostCombatMain, Phase::BeginCombat), + ], + "the ETB trigger anchors both entries at the main phase it resolved in", + ); + + let seq = walk_phases(&mut runner, 24); + + let mut expected: Vec = Vec::new(); + expected.extend(combat()); + expected.push(Phase::PostCombatMain); + expected.push(Phase::End); + assert_eq!(seq, expected); +} diff --git a/crates/engine/tests/integration/issue_828_full_throttle.rs b/crates/engine/tests/integration/issue_828_full_throttle.rs index 534c4dd7dc..ffd4791746 100644 --- a/crates/engine/tests/integration/issue_828_full_throttle.rs +++ b/crates/engine/tests/integration/issue_828_full_throttle.rs @@ -41,12 +41,14 @@ fn full_throttle_schedules_two_extra_combats_after_main_phase() { assert_eq!( runner.state().extra_phases[1], engine::types::game_state::ExtraPhase { - anchor: Phase::EndCombat, + anchor: Phase::PreCombatMain, phase: Phase::BeginCombat, attacker_restriction: None, attacker_restriction_source: None, }, - "the second extra combat must chain after the first combat ends" + "CR 500.8: both extra combats are inserted directly after THIS main phase; \ + the turn chains them through the resume frame, so the second must not be \ + re-anchored to the first combat's end (that consumed the natural combat)" ); } @@ -71,8 +73,9 @@ fn full_throttle_postcombat_main_anchors_to_postcombat_main() { ); assert_eq!( runner.state().extra_phases[1].anchor, - Phase::EndCombat, - "the second extra combat must chain after end of combat" + Phase::PostCombatMain, + "CR 500.8: every bundle anchors at the same insertion point — the main \ + phase the spell resolved in" ); } @@ -133,7 +136,10 @@ fn full_throttle_turn_advances_through_two_extra_combats() { runner.state().extra_phases ); assert_eq!( - declare_attackers_rounds, 2, - "Full Throttle must produce two reachable extra combat phases" + declare_attackers_rounds, 3, + "CR 500.8: Full Throttle grants two combat phases ADDITIONAL \ + to the turn's own combat phase, so a precombat-main cast yields three \ + reachable combats (the old expectation of two encoded the natural combat \ + being consumed by the second extra one)" ); } diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 45b26ca6da..2e48687e77 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -747,6 +747,7 @@ mod issue_7212_recruit_sibling_trigger; mod issue_7221_forage_trigger; mod issue_7232_expend_auto_land_payment; mod issue_7234_cumulative_upkeep_effect_cost; +mod issue_7240_additional_phase_anchor; mod issue_735_amalia_power_threshold; mod issue_735_cost_paid_object_non_regression; mod issue_735_lily_bowen_power_double;